Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Find Source Definition #48264

Merged
merged 49 commits into from
Apr 14, 2022
Merged

Find Source Definition #48264

merged 49 commits into from
Apr 14, 2022

Conversation

andrewbranch
Copy link
Member

@andrewbranch andrewbranch commented Mar 14, 2022

This PR adds a new command similar to Go To Definition, but includes locations in JS files that would normally not be considered, excludes all ambient locations, and potentially uses some heuristics to find likely results in JS files that cannot be analyzed with our binder and type checker due to complex module wrappers or other constructs we don’t understand. The process when invoked is:

  1. Run go-to-definition on something. If it returns only ambient definitions that are alias targets, or stops at import aliases that couldn’t resolve to anything, continue with next steps. Otherwise, return any non-ambient definitions found.
  2. Spin up a temporary project with a single root file, the one in the requesting location, using a new internal compiler setting that prevents module resolution from considering .d.ts files. This lets us resolve to JS files that would normally be “shadowed” by .d.ts declarations.
  3. With that project, try go-to-def again, this time skipping any non-alias work and returning best-effort file results for things like imports where the module specifier resolved, but the specific export wasn’t found, or the target file wasn’t even recognized as a module.
  4. For any results that were “unverified” (those best-effort file results), see if the requesting location gave us a strong indicator of what the declaration name we’re looking for is (i.e., can we locally resolve to a named import). If so, try a find-all-refs-like text search on the file returned, and from those results get the top-most declarations by that name. (This is obviously highly approximate and needs some vetting as to whether it’s worth doing at all—at the very least, it needs corresponding editor UI to indicate that these results are not much better than a text search. I attempted to filter these declarations further by symbol flags and/or basic type analysis, but that immediately proved unreliable in the face of exports created by factory functions with poor JSDoc annotations.)

I attempted to tack this onto go-to-implementation instead of its own command, but go-to-implementation is extremely weird and did not play nicely with this code. This is much more go-to-definition-like, but I don’t think we can simply augment or replace go-to-definition because we want go-to-def to be super fast and reliable, and I think we don’t want this command returning anything ambient. However, for purposes of gathering public feedback in the meantime, I think I’m going to make a build that replaces go-to-def with this logic. (Otherwise, users would have to use a special build of VS Code in order to send the new command.)

Fixes #6209
Fixes #38474

@typescript-bot
Copy link
Collaborator

Thanks for the PR! It looks like you've changed the TSServer protocol in some way. Please ensure that any changes here don't break consumers of the current TSServer API. For some extra review, we'll ping @sheetalkamat, @amcasey, @mjbvz, @minestarks for you. Feel free to loop in other consumers/maintainers if necessary

@amcasey
Copy link
Member

amcasey commented Mar 16, 2022

One case to consider as we think about how this functionality should be triggered is what ctrl-click should do.

@andrewbranch
Copy link
Member Author

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 16, 2022

Heya @andrewbranch, I've started to run the tarball bundle task on this PR at 34c6cfd. You can monitor the build here.

@andrewbranch andrewbranch requested review from amcasey and weswigham and removed request for amcasey April 7, 2022 22:42
src/server/project.ts Outdated Show resolved Hide resolved
src/server/project.ts Outdated Show resolved Hide resolved
src/server/project.ts Show resolved Hide resolved
PR Backlog automation moved this from Waiting on author to Needs merge Apr 13, 2022
Copy link
Member

@sheetalkamat sheetalkamat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just need to make some of the methods internal

src/server/project.ts Show resolved Hide resolved
src/server/project.ts Show resolved Hide resolved
@andrewbranch andrewbranch changed the title Go To Source Definition Apr 14, 2022
@andrewbranch
Copy link
Member Author

Latest commits

  • make _./*|*/add work when _ itself can only be resolved with heuristics
  • rename the command FindSourceDefinition, dropping the bound span part of the response as it’s not needed.
@andrewbranch andrewbranch merged commit 8bd7ce6 into microsoft:main Apr 14, 2022
PR Backlog automation moved this from Needs merge to Done Apr 14, 2022
Jack-Works pushed a commit to Jack-Works/TypeScript that referenced this pull request Apr 22, 2022
* Prototype resolving to JS when go-to-def aliases all resolve to ambient declarations

* Add test infrastructure

* Start fleshing out test coverage

* Fix some go-to-def stuff

* Finish lodash test case

* Make go-to-implementation never return ambient results

* Build new functionality into go-to-implementation

* Update baselines

* Two more test cases

* Refine definition searches for unresolved imports

* Revert "Build new functionality into go-to-implementation"

This reverts commit 381799d.

* Fix tests

* Revert go-to-implementation changes

* Wow a bunch of code was unnecessary

* Update baselines and go-to-def test

* Fix navigation on symbols that are not aliases but resolve through aliases in chain

* Temporarily replace go-to-def with new command implementation

* Revert "Temporarily replace go-to-def with new command implementation"

This reverts commit 34c6cfd.

* Revert "Wow a bunch of code was unnecessary"

This reverts commit 1cb2ba6.

* Bring back some deleted code needed for a new test case

* Clean up a little

* Rename more stuff

* Update test

* Update API baseline

* Temporarily replace go-to-def with new command implementation

* PR review fixes

* Fix getTopMostDeclarationNamesInFile

* Rename local

* Use hash set

* Remove option from commandLineParser

* Keep noDtsResolution project around

* Handle AuxiliaryProject kind in ScriptInfo getDefaultProject etc.

* Do not run updateGraph in the background for AuxiliaryProject

* Don’t create auxiliary project outside of semantic mode

* No-op on scheduled invalidation

* Add comments to unit test

* Sync compiler options to auxiliary project

* Fix case sensitivity

* Update extensionIsOk with new file extensions

* PR feedback

* Update API baseline

* Mark scheduleInvalidateResolutionsOfFailedLookupLocations internal

* Use same heuristics on property accesses of loosely-resolvable aliases as unresolvable named imports

* Rename command, and no need to return the bound span

* Update API baseline
@johnsoncodehk
Copy link

Could we expose findSourceDefinition for language service API to make it available for LSP?

@andrewbranch
Copy link
Member Author

Unfortunately not—if you look at the implementation here, Find Source Definition happens by orchestrating multiple different language services to call Go To Definition with different module resolution options. What are you hoping to integrate it into?

@andrewbranch andrewbranch deleted the go-to-js branch April 25, 2022 16:14
@johnsoncodehk
Copy link

@andrewbranch I hope it can use in embedded typescript language for Vue. (For example: https://github.com/johnsoncodehk/volar-starter/blob/e4f6d9808181f4533811c1aafc4b6afea1f68598/src/components/HelloWorld.vue#L26-L30)

For Go to Definition and Go to Type Definition features, I using getDefinitionAndBoundSpan and getTypeDefinitionAtPosition API of TS language service implemented.

I'm also expected a new getSourceDefinitionAndBoundSpan or getSourceDefinitionAtPosition API.

(btw Go to Source Definition is not specification of LSP, so I need to implement it bypass LSP.)

@andrewbranch
Copy link
Member Author

It’s simply not possible to put this functionality on the LanguageService object (or, it would be a significant violation of the LS’s scope of concerns). At minimum you would need to create a second language service (which creates a second Program and TypeChecker!). It would take a bit of work on our end to abstract what I put directly into TS Server into some kind of public API that could be used independently of the project system. This is an experimental API so it’s not going to happen right now, but if you want to open a new suggestion issue for doing this, we’ll keep an eye on it and could discuss some of the design challenges. Thanks!

@johnsoncodehk
Copy link

johnsoncodehk commented Apr 25, 2022

At minimum you would need to create a second language service (which creates a second Program and TypeChecker!).

Actually this is absolutely acceptable 😅, we already have multiple language server & language service instances in order to prevent auto-complete request from being blocked by unfinished diagnostics request. (vuejs/language-tools#393 (reply in thread))

Great to see it experimentally added to the language service API, this feature will be very welcome in the Vue community, it solves some common DX issues in Vue projects (For example: vuejs/language-tools#1226). It doesn't matter if it will be removed anytime in the future, I can adapt the code quickly.

if you want to open a new suggestion issue for doing this, we’ll keep an eye on it and could discuss some of the design challenges.

Of course, thanks in advance!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
6 participants