Add editor.occurrencesHighlight.asymmetric setting for asymmetric document highlights#294658
Add editor.occurrencesHighlight.asymmetric setting for asymmetric document highlights#294658
Conversation
The optimization that skips re-fetching highlights now only applies when the cursor stays within the original query's word range, not when it moves to any highlighted range. This correctly handles languages like Dart and TypeScript where document highlights can be asymmetric. Co-authored-by: Yoyokrazy <12552271+Yoyokrazy@users.noreply.github.com>
|
@copilot can vscode extensions contribute overrides to vscode core settings? For example I'm thinking about contributing a setting that is like "allow asymmetric highlights" but I think certain language packs would need to enable this |
Yes — extensions can use // extensions/javascript/package.json
"configurationDefaults": {
"[javascript]": {
"editor.maxTokenizationLineLength": 2500
}
}So if a core setting like "configurationDefaults": {
"[dart]": {
"editor.occurrencesHighlightAsymmetric": true
}
}That said, the current approach in this PR (narrowing the optimization to only skip re-fetch within the original query word range) avoids needing a per-language setting entirely. As @DanTup noted in comment 5, the extra requests when moving between highlights are minimal since requests already fire on every cursor move to a non-highlighted position. This fix is correct for all languages by default without any extension opt-in. |
|
@copilot Please go ahead and add a setting then that allows for asymmetric highlights, and have it be turned off by default. This should be a language overridable setting called editor.occurrencesHighlight.asymmetric and have it be a boolean. Make the code changes in this PR respect that when it comes to the recomputing highlight ranges. That way dart can override it, but not everyone will want it since it does inherently create more computations |
… highlights Adds a new language-overridable boolean setting (default false) that controls whether the word highlighter supports asymmetric highlights. When enabled, moving between highlight ranges re-queries the language server. When disabled, the original optimization is preserved. Co-authored-by: Yoyokrazy <12552271+Yoyokrazy@users.noreply.github.com>
Done in 82240c1. Added "configurationDefaults": {
"[dart]": {
"editor.occurrencesHighlight.asymmetric": true
}
} |
|
I don't understand the reason for a setting - the extra computation is almost zero (and already occurs any time you move the cursor to a location that's not within a range). It's just something that languages won't do and won't notice this bug (for example TypeScript is producing asymmetric ranges and presumably nobody else ever noticed it wasn't working correctly). The setting also makes it a bit weirder for LSP servers that are editor-agnostic. |
|
@DanTup Fair enough on LSP servers + computation weight (and the fact that we already expect these to be kicked off for nearly every cursor movement), just exploring some alternate solutions on the side. Unfortunately don't have time to be giving this full focus right now, so working on some options in the background to have before deciding on the one to ship. |
|
It also feels slightly odd to use a setting for something that describes behaviour of an extension/language server and not a user preference. It never makes sense for a user to change this, it would make more sense as a field on the provider (and a capabilitiy in LSP)? I think that's a lot of complexity for such an incredibly tiny performance gain though (especially as we don't know that it's only Dart and TypeScript that do this - maybe every language does anyway? a lot of languages support the kind of code I used in my TypeScript example).
Understood, and I appreciate you looking at this. There are a few places in the Dart server where I think asymmetric groups might make sense (in addition to the one we already have), but I'm reluctant to implement them without knowing that this will be fixed. Thanks! |
|
|
||
| // track the word range that triggered this query, for the optimization check on future cursor movements | ||
| const pos = this.editor.getPosition(); | ||
| const queryWord = this.model.getWordAtPosition(pos); |
There was a problem hiding this comment.
I think this should use the actual range from the server, not the current "word"? It's possible these two things don't match up (because I think word is a basic regex and not from the real servers parser), so if the server range is larger than the "word", then moves within the range but out of the word would still fire unnecessarily?
|
@Yoyokrazy is this still on your radar? If not, would you accept a PR that just restricts the optimisation to the current range and doesn't have any setting? Since TS is also doing this, I feel like it's just expected behaviour and the additional optimisation is not worth the overhead of a setting. |
The word highlighter skips re-fetching highlights when the cursor moves to any existing highlight range, assuming highlights are symmetric. Languages like Dart and TypeScript can produce asymmetric highlights — e.g., a symbol with dual meaning highlights both the field and the variable, but cursoring onto just the field highlights only field references. Moving between these ranges should re-query the language server but doesn't, causing stale highlights.
Change: Added a new language-overridable boolean setting
editor.occurrencesHighlight.asymmetric(defaultfalse) that controls whether the word highlighter supports asymmetric highlights. When enabled, the optimization only skips re-fetching when the cursor remains within the original query word range, not when it lands on any highlighted range. When disabled, the original optimization behavior is preserved.editor.occurrencesHighlight.asymmetricas a language-overridableEditorBooleanOption(defaultfalse)lastQueryWordRangefield to track the word range that produced the current decorationslastQueryWordRangelastQueryWordRangewhen initiating a new highlight querylastQueryWordRangein_stopAlland_stopSingularLanguage extensions like Dart can opt in via
configurationDefaultsin theirpackage.json:Original prompt
This section details on the original issue you should resolve
<issue_title>Highliting optimisation is not always correct for asymmetric highlits</issue_title>
<issue_description>Labels: feature-request, editor-highlight
Assignee:
@YoyokrazyMilestone: Backlog
Author:
@FMorschelState: Open
Created: 2025-01-20T15:15:16Z
Does this issue occur when all extensions are disabled?: NA
Steps to Reproduce:
myValueon line 2 (notice highlight)myValueon line 8 (notice highlight)In Dart, we have the pattern on line 8 that means to deconstruct a variable. In that context (writing it that way),
myValuehas both the meaning of the getter for the class and also the new variablemyValueinside thatif(you can also writemyValue: var myValueand this will disambiguate). So this means that we have one case where the highlighting is not symmetric.myValuevariable on line 2 again, you'll see that the highlighting on line 9 and afterwhenon line 8 all show still, even though on step 1, that wasn't true.I'm asking for a way for the Dart language to make this highlighting valid. Reducing this optimisation (eg. only skip the request if you're still in the same region of the original request) or allowing it to be disabled. I'm unsure if other languages have similar cases but if not currently maybe in the future.
CC
@DanTupComments
Comment 1 —
@DanTup(2025-01-20)This might be clearer with a picture.
In the following code, the code underlined red on line 2 defines a field
myValue. The code underlined both orange and red on line 7 is both a reference to that field, and binds a variable with the same name. When you put your cursor on there, we will therefore return both the field (red) and the variable (orange) as the document highlights.However, if you put your cursor in just the field (line 2), we will only return references to the field, and when you put your cursor in the variable (eg. line 8), we will only highlight the variable.
So the document highlights are not symmetrical. However, VS Code assumes they are, and therefore skips fetching document highlights when the cursor is moved to a location that was in the previous set of results. I think it would be better it VS Code only performed that optimisation if the cursor moves within the original range, and not to any range within the results. It will result in some additional requests to the language server, but that seems very minor and it would support asymmetrical highlights like this.
Comment 2 —
@Yoyokrazy(2025-02-13)Thanks for filing this, I understand the ask here. You're right that the optimization skips subsequent requests if the cursor movement is within the range of the existing highlights. I don't think that it would be too difficult to introduce a setting that unblocks these sorts of asymmetrical highlights, though I want to do a little digging and see if there would be any measurable perf impact or visual flickering due to this case.
To clarify, Dart already returns highlights that would be correct based on the cursor being on the symbol in locations either on line 2, 7 (first occurrence), or 8? If that's the case, I think that I should be able to get this done and test against the example case you've listed out here.
Comment 3 —
@FMorschel(2025-02-13)Yes, it does. You get the matching line colours
@DanTupadded to the screenshot.For this code, the only place where all are highlighted, is on the first reference at line 7 where it has two meanings. You got it all right.
Thanks!
Comment 4 —
@Yoyokrazy(2025-02-13)Ok, that sounds good. I need to do a bit more planning with this, as the more I think about it, the more I'm hesitant to introduce a setting to the wordhighlighter that is effectively language specific. I appreciate you guys bringing this, I think that it's a reasonable ask -- I just need to spend some time to find the best way to addr...
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.