Placing the cursor on self in a function without self argument highlights the entire file

[jonas-schievink]

[matklad]

Annoying enough to be a broken window :-)

this happens because self is then considered to refer to the current module.

[Veykril]

So basically we want to just highlight the identifier instead of the module? If yes should this be for modules in general or just self?

[jonas-schievink]

I think a good middle-ground would be to highlight only the module's name in its declaration, instead of the whole module.

[Veykril]

Well we already do that for inline modules,

the problem with out of line modules is that we can't really do that there since the declaration lies in another file. In this case we never set a focus range(since it doesn't exist for that file) which causes the entire module to be lit up. See
https://github.com/rust-analyzer/rust-analyzer/blob/4e2ec914f4b9609d162c3fd1776e8d293428fe5a/crates/ide/src/display/navigation_target.rs#L269-L283

I think a good solution would be to just filter the delcaration if it doesnt have a focus range in the document_highlight handler.

[matklad]

We might want to move the highlighting logic from rust-analyzer/handlers.rs to a method of Analysis. Re-using Analysis methods to implement different features in handlers is a bit of an anti-pattern.

[matklad]

is a bit of an anti-pattern.

To expand of this, it is yet another example of my favorite story -- each abstraction has two sides, implementor's side, and user's side. It's very easy to use part of the user's side when implementing an abstraction, and that leads to some non-trivial misery.

In this case, Analysis::find_all_refs is a user-facing thing, which was used to implement another user-facing thing -- document highlights. The problem here that it sort-of works, but breaks around the edges, and, whats worse, immediately sends you to thinking about how to change NavigationTarget here, while really NavigationTargets have nothing to do with hightlighting identifiers.

[Veykril]

Ye, I rebuild this on top of the search module now but there is still the problem of getting the identifier range of the definition. Currently we only have NavigationTarget for that from what I know so we probably would need something new for this that basically just queries for the focus_range-ish part? #9375