Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Widen Highlights root range to covering element #7505

Merged
merged 1 commit into from
Feb 5, 2021

Conversation

Veykril
Copy link
Member

@Veykril Veykril commented Feb 1, 2021

There have been a few issues about/containing spurious syntax highlighting panics, which all seem to come from the rust_analyzer::handlers::handle_semantic_tokens_range request, which I believe this to be the cause of as the text range we want to highlight here is currently potentially smaller than that of the covering element, so we might highlight something that is inside the covering element, but outside of the text range we wish to highlight causing the assert to fail.
Unfortunately this isn't really easy to test since I have yet to find a reproducible cause(#7504 doesn't work for me cause I can't seem to checkout the given commit).

See #7504, #7298, #7299 and #7416, all of those contain an assertion failure in syntax highlighting, but only in the range request.

@Veykril
Copy link
Member Author

Veykril commented Feb 1, 2021

Hm, actually this is probably the wrong approach since we might highlight the entire file again now if the covering_element is solely the source file. I think this https://github.com/rust-analyzer/rust-analyzer/blob/1a59f75cdaa730c16a694a4294eccf6dfe6fe0ad/crates/ide/src/syntax_highlighting.rs#L101-L104 might be our problem actually. This of course can let ranges pass that aren't fully contained in the root text range which then potentially fails the assertion in https://github.com/rust-analyzer/rust-analyzer/blob/1a59f75cdaa730c16a694a4294eccf6dfe6fe0ad/crates/ide/src/syntax_highlighting/highlights.rs#L41-L42 later on.

@matklad
Copy link
Member

matklad commented Feb 4, 2021

Hm, actually this is probably the wrong approach since we might highlight the entire file again now if the covering_element is solely the source file.

hehe, I remember tracing the same trail of thought a while back )

@matklad
Copy link
Member

matklad commented Feb 4, 2021

I think we might want to use two different ranges here? The Highligts should use the rang of encompassing element, but we should use original range for element filtering.

@Veykril
Copy link
Member Author

Veykril commented Feb 4, 2021

Good point, that should work 👍

@Veykril Veykril changed the title Highlight the entire covering_element's text range Widen Highlights root range to covering element Feb 5, 2021
@Veykril
Copy link
Member Author

Veykril commented Feb 5, 2021

bors r+

@bors
Copy link
Contributor

bors bot commented Feb 5, 2021

@bors bors bot merged commit 2a75594 into rust-lang:master Feb 5, 2021
@Veykril Veykril deleted the hl-fix branch February 5, 2021 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants