Goto-definition combined with .d.ts.map file produces location past the end of the file

[ghost]

TypeScript Version: master

Code

See the test "tsserverProjectSystem project references" "goToDefinition" added in #24867.

CC @weswigham

[weswigham]

Is the problem that the generated sourcemaps is wrong or that we are capable of returning a position beyond the file's bounds when an invalid sourcemap is present?

[ghost]

That's the sourcemap that tsc generated when I tried it out. Would be nice if there were a way to programatically generate a.d.ts and a.d.ts.map inside the test instead of hardcoding.

[weswigham]

There is! Some of the declaration map tests make use of it.

[weswigham]

I'm going to hazard a guess that #24886 fixed this.

[ghost]

Since we're using the feature now, you can reproduce the error by clicking on any skipTrivia in textChanges.ts.

[weswigham]

I'm going to hazard a guess that #24886 fixed this.

And we haven't done an lkg but only use light to build, so the change can't be seen in our own project yet.

[ghost]

Hi @weswigham, could you provide the location of the test helper that will generate a map?

[weswigham]

This fourslash test does it

[ghost]

@weswigham Updated #24867 but it looks like the declaration emit hasn't changed in the master branch. Also, while working on that I've noticed that goto-definition doesn't always go to the right file either -- for example, in tsserverProjectSystem.ts, clicking on ProjectService in server.ProjectService takes me to scriptVersionCache.ts instead of editorServices.ts.

[weswigham]

@andy-ms I'm seeing it go the the right file, but what looks like at least one declaration too far:

Although what you're seeing is what happens when the language server is vscode's builtin 2.9.1, instead of a more recent build with #24886 in.

[weswigham]

@andy-ms So it's worth noting that while we "fixed" your test, it's still entirely possible for us to return a position outside the bounds of a file for the end of a span we return (though probably much less likely now, since every identifier is actually sourcemapped). Do we need to guard against that anywhere?

[ghost]

We do call scriptInfo.positionToLineOffset on the result, but I don't see any asserts in there -- so we'll just return an out-of-bounds location to the editor, which should be able to handle it without crashing.