Holding Cmd while hovering a symbol shows LocationLink's "targetSelectionRange" instead of "selectionRange" as preview

[DanTup]

When returning Location classes from a DefinitionProvider, VS code shows a preview of the code if you hold Cmd while hovering over a symbol:

Multiple lines are shown, leading me to believe this is intended to show a snippet of the code that defines this symbol.

However, if I switch to using LocationLink where targetRange points at the full code snippet and targetSelectionRange includes only the name of the function (the LSP docs say "e.g the name of a function" for targetSelectionRange) then the preview only shows the targetSelectionRange code, which means the preview shows only the function name (which seems a bit pointless):

It seems like maybe this should be using the targetRange so there is some preview of the code, as it did for Location?

[jrieken]

this came in via 459939b

[mjbvz]

@jrieken Do you know what about that commit is incorrect?

@DanTup Can you please provide steps to reproduce this so we can investigate?

[DanTup]

My guess is that this line should be using targetRange.

To reproduce, you just need to return a LocationLink that has a targetSelectionRange that covers a small range (eg. the name of a function, which is what the LSP docs give as an example) and a targetRange that is much greater (the whole function body), and then hold Cmd while hovering over a reference to the function. The preview will only show the name (as shown in the second screenshot above), but it's expected to show a preview of the code (as it does in the first screenshot, when not using LocationLink).

[mjbvz]

Hmm, I believe TS already does this though:

vscode/extensions/typescript-language-features/src/languageFeatures/definitions.ts

Line 43 in 6dd0a9a

return {

Do you have a self-contained example for this or a build of dart that uses definition links? I'd like to be able to debug what is going on.

result.range seems like the correct value to use at that point in the code as the other two properties are selection ranges:

[DanTup]

Ok, I'm a little confused :-)

The screenshot above shows code completion showing range but no targetRange - what type is that? LocationLink has no range but does have a targetRange (the other fields match).

I tried to reproduce this in a simple VS Code extension, but could not - it seemed to work as expected (despite the linked code above seemingly using targetSelectionRange). However, I do still see this in LSP.

Here's what my logged LSP traffic looks like:

==> {
	"jsonrpc": "2.0",
	"id": 6,
	"method": "textDocument/definition",
	"params": {
		"textDocument": {
			"uri": "file:///Users/danny/Desktop/TestProjects/test_2710/lib/main.dart"
		},
		"position": {
			"line": 57,
			"character": 22
		}
	}
}
<== {
	"id": 6,
	"jsonrpc": "2.0",
	"result": [
		{
			"originSelectionRange": {
				"start": {
					"line": 57,
					"character": 20
				},
				"end": {
					"line": 57,
					"character": 25
				}
			},
			"targetUri": "file:///Users/danny/Dev/Google/flutter/packages/flutter/lib/src/material/icons.dart",
			"targetRange": {
				"start": {
					"line": 8,
					"character": 0
				},
				"end": {
					"line": 17034,
					"character": 1
				}
			},
			"targetSelectionRange": {
				"start": {
					"line": 62,
					"character": 6
				},
				"end": {
					"line": 62,
					"character": 11
				}
			}
		}
	]
}

This generates the second screenshot from the original comment. It shows only Icons (which is what the targetSelectionRange points at) and not a preview of the class (which is what targetRange points at).

The LSP changes to use LocationLink aren't available in a pre-built Dart SDK yet so running it locally might be a bit clumsy (though they might be in tonights nightly build). I guess you don't have an equiv of MockDebug for LSP to make a small sample?

[DanTup]

I did find there's some different handling for long ranges:

vscode/src/vs/editor/contrib/gotoSymbol/link/goToDefinitionAtPosition.ts

Lines 220 to 222 in 5e4f1b0

	if (numberOfLinesInRange >= GotoDefinitionAtPositionEditorContribution.MAX_SOURCE_PREVIEW_LINES) {
	rangeToUse = this.getPreviewRangeBasedOnIndentation(textEditorModel, startLineNumber);
	}

I don't think it explains this, though I think that would be tripped up where the targetRange includes leading comments (since it'll only include the first two lines, as they both have the same indentation). The docs don't seem to be specific about whether leading comments should be included in the targetRange (if they should not, I think this should be explicitly documented).

Despite this, I still can't repro the behaviour seen using LSP without LSP. I did find a second definition of LocationLink that has range instead of targetRange, though I'm not sure how they're mapped (or whether it's relevant here).

[mjbvz]

@dbaeumer Seems like there is a bug in the LSP, although it seems like this logs was added to work around a previous issue: https://github.com/microsoft/vscode-languageserver-node/blob/51cf328886b9bddb4d5eb72cde983872c5564084/client/src/common/protocolConverter.ts#L583

@jrieken For VS Code itself, would it make sense to trim the passed in range rather than falling back to use getPreviewRangeBasedOnIndentation for long lines? What do you think?

[jrieken]

@jrieken For VS Code itself, would it make sense to trim the passed in range rather than falling back

The "how to trim" is the interesting part... We used to have a simple crop bit it would often look awkward when only seeing increasing indentation