Support go-to-definition for imports of arbitrary files

[andrewbranch]

Fixes#41861

The first commit adds support for jumping to arbitrary files from import/require module specifiers, whether those are scripts or non-source files like CSS (thanks @DanielRosenwasser for the idea that this should work).

The second commit goes further by allowing us to return a successful response for files that don’t exist. I was curious what would happen, so I removed the host.fileExists check and made sure we don’t try to read a file’s text to convert { position: 0 } to { line: 0, column: 0 }. The result in VS Code was this:

Having the quick option to create the file seemed like a win to me; better than doing nothing. But I’m open to debate on whether we want that.

Also, if anyone knows how this will work in Visual Studio or has a quick way to test it, that would be appreciated!

[DanielRosenwasser]

@typescript-bot pack this

[typescript-bot]

Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at 5f256f1. You can monitor the build here.

[DanielRosenwasser]

Heads up @mjbvz and @minestarks.

[DanielRosenwasser]

How does this work with partial semantic mode (including in the browser)?

What will happen if we try go-to-definition on thing or the module specifier in this example?

import{thing}from"./vandelay.js"/*1*/;thing/*2*/();

[typescript-bot]

Hey @DanielRosenwasser, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
"devDependencies": {
"typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/94419/artifacts?artifactName=tgz&fileId=5DF50A9CF42BC37629C1481AFE23876083889D0471511DD2357A7CB0CDC5F1A602&fileName=/typescript-4.2.0-insiders.20210128.tgz"
}
}

and then running npm install.

---

There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/pr-build@4.2.0-pr-42539-4".;

[andrewbranch]

What will happen if we try go-to-definition on thing

Whatever the current behavior for go-to-definition on the identifier reference, that will be unchanged.

or the module specifier

Don’t we already resolve one level of direct imports from your current source file? That should be unchanged as well. If we don’t, we would return a response saying the definition is at position 0 of ./thing.js, resolved relative to whatever we said the path of the current source file is. If we were to re-add the host.fileExists check and we haven’t already successfully resolved and loaded ./thing.js for normal module resolution reasons, we would not offer a definition there.

[DanielRosenwasser]

I mostly ask because #39037 is a related issue I've been interested in unlocking as well, but I get why this doesn't automatically unlock that.

[sandersn]

Answering a question that nobody has asked, I tested this with emacs. This gives a picture of how a simply written language plugin will behave and is usually similar to how VS works too.

Everything works fine. For the non-existent file, the editor raises an error inside tide-get-file-buffer and since the error's not handled, it's displayed to the user. That's a fine user experience, and I would be surprised if it's worse in other editors.

[andrewbranch]

we dont resolve imports currently in partial semantic mode, so adding a test that it works for that would be good too

@sheetalkamat I’m looking at adding a case in partialSemanticServer.ts now (just pushed a new commit), and it seems imports are resolved... the source file’s resolvedModules contains correct resolutions.

If we hadn’t resolved them, this case would be a problem, because the module specifier is extensionless. Without doing at least some basic module resolution, we wouldn’t know that the target file has a .ts extension. I guess in that situation, we would either want to turn this feature off or potentially do real module resolution on-demand. But at the moment it seems that module resolution did already happen so it’s not a problem.

[sheetalkamat]

@andrewbranch interesting.. till you pointed out, I didn't realize we were doing module resolution as part of noResolve. PR #4368 added this. Not sure if this additional cost is really what we want with partial semantic server or not but that can be handled separately.

[andrewbranch]

In that case, this feature currently works exactly the same for partial semantic mode as full semantic mode, and the new test will ensure that we don’t break it if we change partial semantic mode later.

[andrewbranch]

New changes:

1. Whenever the file reference lookup does not yield an actual source file (this could be because the file doesn’t exist, or because it’s a file type like CSS that the compiler doesn’t care about), a new unverified: true property is added to the definition in the response. VS Code could use this to show a less-error-y prompt to create a new file if it finds that the file doesn’t exist.

2. Whenever (1) happens, instead of immediately returning that speculative file result, we continue other methods of definition lookup and combine the results. This specifically allows us to return two definitions for pattern ambient module matches: one for the actual file referenced, and one for the ambient module declaration. Screen cap:

   

   Notes: I’m not sure why the second definition is expanded by default instead of the first. Possibly because the first has a zero-length definition span? Also, the underlining on only part of the path seems to be unrelated to TypeScript’s response (the span we send there is the whole module specifier, quotes included).

[sheetalkamat]

@andrewbranch just realised that we are not passing this on in the through server even though we added this property to protocol.
tsserver37220.log

[andrewbranch]

That's correct, it was shipped in 4.3