[wildcards] Semantic Highlighting - consider styling wildcards

[pq]

Consider highlighting wildcards specially (see discussion).

@scheglov: as someone who enjoys semantic highlighting, I'd be especially curious to get your perspective. Would this be valuable? Too much noise? Too difficult to discern?

[scheglov]

Probably not very useful.

First of all, I don't see wildcard variables used widely, but correct me if I'm wrong.

Then, which wildcard "variables" do we want to highlight differently? Only local variables? Or also type parameters, and formal parameters?

Then, theoretically semantic highlighting allows us to quickly identify something as a method, local variables, etc without reading the whole code around it. And the identifier _ itself is probably enough signal for this purpose.

FWIW, RustRover does not have it.
Although it does not have highlighting for everything that I'd like to :-)

[pq]

Thanks Konstantin!

[bwilkerson]

For LSP, we can add a 'modifier' to a highlight range and users can choose whether or not to color the range differently based on the modifiers.

IntelliJ is a little harder because we'd have to add a new value to the protocol in order to pass the information across. I can remember what happens if the user hasn't set a highlight for one of the provided values, so we might want to investigate that.

[pq]

Starting w/ VS Code sounds like a great idea. I'll look into it. Thanks, Brian.

[pq]

/fyi @DanTup for thoughts.

[DanTup]

For VS Code/LSP I think it would be good to have semantic token modifiers on them for users to style them. I'm not yet sure if I'd prefer them greyed or not, but if we apply a modifier it'd be easy to try both out for a while and if we decide in future we want some default styling, we can add that in Dart-Code (by applying a style for the modifier).

There are two ways we usually apply custom modifiers/types for highlighting - one is if there is an existing specific HighlightRegionType we can add it to the LSP mapping (this might work if we add a new token type for IntelliJs benefit, although I don't know if that's breaking):

sdk/pkg/analysis_server/lib/src/lsp/semantic_tokens/mapping.dart

Lines 27 to 30 in 56230ca

	HighlightRegionType.INSTANCE_FIELD_DECLARATION: {
	SemanticTokenModifiers.declaration,
	CustomSemanticTokenModifiers.instance
	},

Otherwise, if the token type is the same as for other things and the custom modifier should be applied to only a subset, we can include it in the highlight computer:

sdk/pkg/analysis_server/lib/src/computer/computer_highlights.dart

Lines 226 to 233 in 56230ca

	// For semantic tokens, constructor names are coloured like methods but
	// have a modifier applied.
	semanticTokenType: SemanticTokenTypes.method,
	semanticTokenModifiers: {
	CustomSemanticTokenModifiers.constructor,
	if (_isAnnotationIdentifier(parent))
	CustomSemanticTokenModifiers.annotation,
	},

I don't know which of these is best - it may depend on how IntelliJ will be handled. If for now we want to leave IntelliJ as-is, the second option probably makes sense (and we can always simplify it if we switch to a different HighlightRegionType in future).

[bwilkerson]

I don't see any SemanticTokenTypes values that appear to be appropriate, so I was assuming that it would be a custom non-binding modifier.

@jwren For thoughts about IntelliJ.

[DanTup]

I don't see any SemanticTokenTypes values that appear to be appropriate, so I was assuming that it would be a custom non-binding modifier.

Yep, that's what I had in mind too. The two options above are both still valid - if we had HighlightRegionType.wildcard (for example), we could map that in mappings to a standard token type, but with a custom modifier. If we didn't have a specific HighlightRegionType, we can detect the wildcard in the computer and just pass an additional custom modifier to addRegion (similar to the second quoted code above, which conditionally passes annotation).

[pq]

My gut is to start with a custom modifier and live IntelliJ as-is. If the feature gets a lot of traction (or @jwren or folks at Jetbrains have interest) we can revisit.

Thanks for the pointers @DanTup!

[DanTup]

@pq is this something you're looking at? If not, I'm happy to add it.

[pq]

Thanks for the nudge! I'm not actively looking at it and wouldn't have real time to throw at it until after the summit so it's up for grabs.

[DanTup]

I started looking at this and have a new custom modifier "wildcard" applied to variable declarations that are wildcards. However while writing tests I found that existing wildcard assignments like:

f() {
  int a;
  (_, a) = (1, 2);
}

Do not actually produce any highlights/semantic tokens for the _ (this leaves them uncoloured and means we have nowhere to apply a modifier). Should we be assigning a token here? If so, what should it be? (the a ends up as a local variable reference, but that might be odd for a variable with no declaration).

Any opinions?

[pq]

Hmmmm, in principle I guess I would like consistent highlighting here but I'm not sure how involved fixing that would be (and if assigning a token is a desirable approach).

@bwilkerson?

[bwilkerson]

Yeah, this one is interesting. The _ in the pattern isn't a declaration, and it isn't a reference to a variable.

The inconsistency of not highlighting it bothers me, but I can't think of a good alternative at the moment.

[lrhn]

The _ in a pattern is a placeholder where a name is omitted, at least when you write var _. A _ by itself just omits the var too.
I would treat it the same as var _ in a pattern.
Just like I'd treat void foo(_){} and void foo(var _){} the same.

[DanTup]

The inconsistency of not highlighting it bothers me, but I can't think of a good alternative at the moment.

The current options I can come up with are:

1. Create a CustomSemanticTokenType.wildcard instead of a custom modifier and use it for all wildcards (both existing wildcards in patterns, and new wildcard variables)
   • This makes them all consistent
   • They will all be uncoloured in generic LSP clients
   • For VS Code, we can apply default styling
2. Categorise wildcards in patterns as variables (declarations? 🤔) and use the custom modifier
3. Add a new token type for pattern wildcards, but keep existing variables as variables with a wildcard modifier
   • This keeps the current behaviour (wildcard variables are coloured, wildcards in patterns are not) but still allows client colouring of each (albeit individually because one is a type and one is a modifier)
4. Do nothing for existing wildcards in patterns
   • This prevents users from colouring wildcards in patterns
   • This leaves the two kinds of wildcards coloured differently by default in VS Code (although we could provide custom styling to uncolour wildcard variables to match patterns)

I feel like the top two options are probably the better ones but I'm interested in others opinions (or other suggestions!).

[lrhn]

I don't think treating a _ in a pattern as a "trivial, non-binding declaration" is wrong. It occurs in the same places as declarations, it's just a "null value" for declarations that can be used anywhere a variable declaration could, but does nothing and introduces no actual variable(-name).

[bwilkerson]

... treating a _ in a pattern as a "trivial, non-binding declaration" ...

I believe that's option 2. That would be the least invasive change, and I'd be fine with it. I'd also be fine with option 1, but it requires more work from users that like the current behavior.

[DanTup]

Ok, I'll go with 2. It at gets things consistent and we can always change in future if we have better ideas. Thanks!