Try to resolve name refs during highlighting

[lnicola]

Preview:

This is probably not the cleanest implementation, but it's not clear to me what parts of reference_definition we don't want to run at this point. Also, is the SourceAnalyzer cheap enough to construct for each NameRef? Not like there's any alternative at this point, though.

[matklad]

The end-result looks good to me! However, I think we should probably start refactoring goto_definition so as to avoid duplication.

On the high level, the situation is as follows:

• in the IDE, we have a syntax tree and a NameRef
• this name reference can resolve to things of very different "shapes" it can be a field, or it can be a method
• these "shapes" are processed by IDE and dumbded down to a uniform representation, like NavigationTarget, for various specific features.

I think both goto definition and highlighting want the same set of "shapes", but want to process them differently.

Let's extract a helper function, with roughly the following signature

enum NameRefKind {
    MethodCall(hir::Function),
    MacroCall(hir::MacroBYExampleDef),
    FieldAcess(hir::StructField)
    ...
}

fn classify_name_ref(analyzer: &SourceAnalyzer, name_ref: ast::NameRef) -> NameRefKind {
   ...
}

We should then be able to result this function in both highlighting and goto def

[lnicola]

Thanks for the suggestion. I'll try, but it's still not clear to me:

• what's the duplication between highlighting and go to definition than you mention
• how would classify_name_ref differ from the existing reference_definition besides not doing the NavigationTarget stuff
  • and maybe not hitting the index?

[lnicola]

Or maybe you want to avoid future duplication? That would make sense.

[matklad]

I think if we add all bells&whistles to syntax highlihting, we'd have to re-implement all of these logic:

https://github.com/rust-analyzer/rust-analyzer/blob/366ad6f03ba2296f09cb79ea139fa53a132fa651/crates/ra_ide_api/src/goto_definition.rs#L56-L131

besides not doing the NavigationTarget stuff

That and the lack of index would indeed be the only differences. But not using NavigationTarget is a key. The philosophical problem with NavTarget is that it is an public API type, which is consumed more or less directly by the LSP. So it has API which is good for LSP, but which is a bad fit for rust-analyzer.

To demonstrate the concrete problem, suppose we do use reference_definition verbatim in highlighting. And let's say we want to highlight calls to unsafe functions specially. Than, we'd need to figure out if a function is unsafe from it's NavigationTarget, and that's hard because NavigationTarget API is not really Rust-specific. It talks about text editor concepts like ranges.

This "using public API types to build an internal implementation" problem is pretty bad long term. We already suffer from this in hover, which currently relies on reference_definition, but should be using classify_reference as well.

[lnicola]

Updated on top of #1311. I'll leave the SourceAnalyzer caching for some other PR.

[matklad]

Looks awesome!

bors r+

[lnicola]

I think this breaks highlighting of function definitions.

[matklad]

cc #1314

[bors]

Build failed

• continuous-integration/travis-ci/push

[matklad]

I guess, just restoring NAME => "function", should fix the test?

I've also realized that we don't have anything better than a smoke test here, which seems sub optimal :-)

Could add a more flavorful test-case here? I think just a bigger example would work.

OTOH, it would be cool to make the test itself more readable, by, for example, generating html output instead of just debug dump of highlightings. Though, this seems like a lot of effort for relatively little gain, unless we can make html-dumping code really simple.

[matklad]

OTOH, it would be cool to make the test itself more readable, by, for example, generating html output instead of just debug dump of highlightings.

Hm, actually, "dump code as a sytax-highlighted HTML" is a cool feature to have regardless, so perhaps it makes sense to just do it? Doesn't seem too hard: I think one must basically wrap highlighted ranges into spans with appropriate classes

[lnicola]

Fixed.

[matklad]

bors r+

[bors]

Build succeeded

• continuous-integration/travis-ci/push

[matklad]

Hm, actually, "dump code as a sytax-highlighted HTML" is a cool feature to have regardless, so perhaps it makes sense to just do it?

I'll work on it unless anybody else has already started

[lnicola]

That's fine by me.

[matklad]

finally got the chance to try it out, and this looks perfect! I especially like how it is now possible to know what is being imported:

[lnicola]

How do we do the same thing for names?

[matklad]

You mean, how do we color enums differently from, say, structs?

I think that should be relatively easier to do: one needs to look at the ast parent of the corresponding ast::Name

[lnicola]

I was thinking of variables and function definitions, but the same approach should work.

[bjorn3]

@matklad what are those three dots underneath some letters in your screenshot? (just curious)

[matklad]

@bjorn3 that's the spellchecker. By default, VS Code renderns "info" level diagnostics using short strip of dots