DefinitionLink targetSelectionRange not working as expected

[KamasamaK]

Issue Type: Bug

Scenario 1:

1. Use Peek Definition on a symbol that has a DefinitionLink provided with targetSelectionRange defined.
2. The range provided by DefinitionLink.targetRange is used for highlighting.

Scenario 2:

1. Use Go to Definition on a symbol that has a DefinitionLink provided with targetSelectionRange defined.
2. Navigates to beginning of DefinitionLink.targetRange

I am assuming that in at least one of the above scenarios, DefinitionLink.targetSelectionRange should be used instead of DefinitionLink.targetRange. It is not clear based on the documentation, so I am not sure in what case DefinitionLink.targetSelectionRange is supposed to be used, but I figure it must be used for something. I am expecting it to work similarly to DocumentSymbol.selectionRange.

VS Code version: Code 1.27.2 (f46c4c4, 2018-09-12T16:17:45.060Z)
OS version: Windows_NT x64 10.0.17134

[vscodebot]

(Experimental duplicate detection)
Thanks for submitting this issue. Please also check if it is already covered by an existing one, like:

• Finalize DefinitionLink api (#54101)

[jrieken]

I believe this happened with 0a1b3a5

[mjbvz]

Not a regression; we just never hooked anything up to use targetSelectionRange after adding this api

I believe targetSelectionRange should be the span we reveal when you run go to definition or click in the peek definition view. The targetRange should be the highlighted span

[KamasamaK]

Even though that makes sense, I'm not sure I like the idea of using the targetRange for highlighting in the Peek Definition view. It's mostly because of the styling, which at least in the default Dark+ theme is very overwhelming and harms the visibility of the syntax highlighting.

I would ask that after making this change, you implement DefinitionLink usage with proper ranges for TypeScript and see if you still like it. The range that the TypeScript extension currently uses for definitions is understandably what I would describe as just the targetSelectionRange.

[dbaeumer]

@mjbvz any plans when this will be addressed. I want to hook this up in LSP and document this correctly. So I will for now convert targetSelectionRange to targetRange on the API side to have a decent behaviour.

The other question is why targetSelectionRange is optional. Shouldn't it be mandatory in this example as it is with DocumentSymbol

[aeschli]

@jrieken Trying to validate that, I now see that the targetSelectionRange is what is highlighted in the peek view and is used as the range to navigate to.
Not clear to me what the targetRange does or can bee seen.

Is that the expected behaviour? Can the spec be clarified?

[jrieken]

Not clear to me what the targetRange does or can bee seen.

Yeah, that's actually not yet used. The point is to use target selection range when given. The idea of the target range is that we can use it for the preview hover when cmd+hovering over a declaration link.

[jrieken]

Adding verified as @aeschli did all that there can be done