FAR cleanup and isDefinition definition #48211

amcasey · 2022-03-10T22:46:07Z

Benefits:

Uninverted callbacks
Intelligible meaning for isDefinition
Should do fewer redundant searches

Costs:

Does more tsconfig searches (probably negligible cost)
Probably does more work to compute isDefinition

Open Questions:

The FAR API and the FAR server request now have different interpretations for isDefinition - does it matter?
Can I add a new, internal property to the FAR result type?
Does it matter that isDefinition may be wrong in a way that has no effect on CodeLens, the only consumer? [Edit: I can no longer remember how it was wrong, but I strongly suspect it was because we had to guess when the starting point was not a definition, which it always will be for CodeLens] [Edit 2: It's also wrong-ish for jsdoc comments because there's no node to retrieve the symbol for]

Rough explanation of the new isDefinition:

Start at the place the user invoked FAR (this is surprisingly hard to find)
Record that location in a set of spans known to refer to the "real" symbol
Find a project with a result in the known set
Find the symbol at that position in that project
Use that symbol to compute isDefinition for each declaration in the results for that project (edit: and add those that refer to the symbol to the set)
Goto (3) until no [edit: no such project is found]
Clear isDefinition in all remaining projects

The test updates were accepted uncritically as the change is very simple and the updates aren't final. TODO: restore semantic functionality in code merging results from multiple projects to un-break VS Code's CodeLens.

This change had two goals: 1. Make the code easier to understand, primarily by simplifying the callback structure and minimizing side-effects 2. Improve performance by reducing repeated work, both FAR searches of individual projects and default tsconfig searches This implementation attempts to preserve the merging order found in the original code (someone less relevant in the present state of using syntactic isDefinition).

...in preparation for implementing isDefinition explicitly.

There are some extra tsconfig searches, but the lookups are cached, so the cost is negligible. I suspect the change is because we now drop redundant searches when dequeuing from the work queue, rather than enqueuing.

...to allow use of the checker when computing isDefinition across projects.

sandersn · 2022-03-16T22:01:32Z

Is this ready to review? It's still marked draft.

amcasey · 2022-03-16T22:17:19Z

@sandersn Sorry, I should have been clearer about what the status meant. Yes, I would like feedback on the approach now, so I can skip the remaining work if we decide to kill it.

sandersn · 2022-03-21T23:29:07Z

Before I read the code, re step (4) of isDefinition: is there a separate symbol for each project?
Also I don't understand what progress is or how it's made. I assumed that progress was the addition of locations known to refer to the real symbol, but I don't see steps (3), (4) or (5) adding locations.

I'll see if reading the code clears things up.

sandersn · 2022-03-22T22:02:17Z

I looked over the first big test and the results seem better for the reference count CodeLens: imports are now definitions and no longer count toward the reference count.

However, the definition is isDefinition was so tricky, this apparent improvement might not be desired. You should probably double-check the comment history of previous PRs if you haven't already.

sandersn · 2022-03-22T22:06:16Z

Also

Does it matter that isDefinition may be wrong in a way that has no effect on CodeLens, the only consumer?

When you find-all-refs on a declaration of a symbol that has only 1 non-declaration reference, both VS Code and emacs jump to that lone non-declaration. I think that code must rely on isDefinition too—at least the emacs code did when I first wrote it in 2016.

In this case, making imports have isDefinition: true would mean that find-all-ref would not jump to a single import, but display the usual find-all-refs UI. I think this is fine.

andrewbranch · 2022-03-22T22:56:18Z

I think that code must rely on isDefinition too—at least the emacs code did when I first wrote it in 2016.

I could be wrong, but I don’t think this is true for VS Code.

sandersn

I didn't review the session.ts changes too closely. The findAllReferences changes look OK as far as I understood them but they didn't clear up my original questions.

I like the test changes, so it's likely that this is the right direction.

src/services/findAllReferences.ts

sandersn · 2022-03-22T18:35:32Z

src/services/services.ts

@@ -1842,8 +1911,7 @@ namespace ts {

        function getFileReferences(fileName: string): ReferenceEntry[] {
            synchronizeHostData();
-            const moduleSymbol = program.getSourceFile(fileName)?.symbol;
-            return FindAllReferences.Core.getReferencesForFileName(fileName, program, program.getSourceFiles()).map(r => FindAllReferences.toReferenceEntry(r, moduleSymbol));
+            return FindAllReferences.Core.getReferencesForFileName(fileName, program, program.getSourceFiles()).map(r => FindAllReferences.toReferenceEntry(r));


Suggested change

return FindAllReferences.Core.getReferencesForFileName(fileName, program, program.getSourceFiles()).map(r => FindAllReferences.toReferenceEntry(r));

return FindAllReferences.Core.getReferencesForFileName(fileName, program, program.getSourceFiles()).map(FindAllReferences.toReferenceEntry);

JS this-binding scares me, so I err on the side of using a lambda. If you tell me it does the right thing without it, I'll take your word for it.

can't have undefined this if you never use this -tap-tap-

findAllReferences.ts only uses classes in class State, so only its methods need to be bound to an instance.

tests/baselines/reference/duplicatePackageServices.baseline.jsonc

amcasey · 2022-03-25T00:16:28Z

Before I read the code, re step (4) of isDefinition: is there a separate symbol for each project? Also I don't understand what progress is or how it's made. I assumed that progress was the addition of locations known to refer to the real symbol, but I don't see steps (3), (4) or (5) adding locations.

I'll see if reading the code clears things up.

Yes, each project has its own symbol IDs and I don't believe there's a way to map between them.

amcasey · 2022-03-25T00:18:53Z

I think that code must rely on isDefinition too—at least the emacs code did when I first wrote it in 2016.

I could be wrong, but I don’t think this is true for VS Code.

It does. It has a fallback where it makes a second FAR request, filtered to definitions, if the first returns exactly two results. If exactly one is a definition, it jumps between them. The current (and, I believe, preserved) behavior is that an import counts as a declaration of a separate, alias symbol, so an exported symbol that is exported in exactly one place will cause GTD to hop between the two (since neither is a definition from the other's perspective).

sandersn · 2022-03-25T15:49:22Z

Overall, I think this is a good direction. Let me know when you mark it Ready for Review and I'll take a harder look at the session.ts changes.

amcasey · 2022-03-28T23:15:34Z

@sandersn So you believe in this enough that it seems worthwhile to polish it (and probably update a lot of fourslash tests)? I still have some concerns around the costs and the open questions.

@andrewbranch Your thoughts? It feels risky to me, so I'd want to get it into the beta if we ship it at all.

sandersn · 2022-03-29T15:36:26Z

I liked the improvement in semantics quite a bit. Let me look again at the new cost of computing isDefinition.

amcasey added 10 commits March 7, 2022 14:08

First cut

b230065

Implement set with custom equals and getHashCode

cba96f3

Adopt custom set in session

cb1e800

Revert to a syntactic implementation of isDefinition

3649f68

The test updates were accepted uncritically as the change is very simple and the updates aren't final. TODO: restore semantic functionality in code merging results from multiple projects to un-break VS Code's CodeLens.

Stop enforcing search and aggregation order

6241c4f

...in preparation for implementing isDefinition explicitly.

Accept new baselines

d5df76a

There are some extra tsconfig searches, but the lookups are cached, so the cost is negligible. I suspect the change is because we now drop redundant searches when dequeuing from the work queue, rather than enqueuing.

Introduce LanguageService.updateIsDefinitionOfReferencedSymbols

46ce33e

...to allow use of the checker when computing isDefinition across projects.

Bound node search

c6cc433

More todos

a094f65

amcasey requested review from sandersn and sheetalkamat March 10, 2022 22:46

typescript-bot assigned amcasey Mar 10, 2022

typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Mar 10, 2022

amcasey requested a review from andrewbranch March 10, 2022 22:46

amcasey mentioned this pull request Mar 10, 2022

Clean up FAR and NavTo aggregation #47938

Closed

sandersn reviewed Mar 25, 2022

View reviewed changes

amcasey closed this Apr 9, 2022

amcasey mentioned this pull request Apr 9, 2022

Clean up FAR aggregation #48619

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FAR cleanup and isDefinition definition #48211

FAR cleanup and isDefinition definition #48211

amcasey commented Mar 10, 2022 •

edited

Loading

sandersn commented Mar 16, 2022

amcasey commented Mar 16, 2022

sandersn commented Mar 21, 2022

sandersn commented Mar 22, 2022

sandersn commented Mar 22, 2022

andrewbranch commented Mar 22, 2022

sandersn left a comment

sandersn Mar 22, 2022

amcasey Mar 25, 2022

sandersn Mar 25, 2022

amcasey commented Mar 25, 2022

amcasey commented Mar 25, 2022

sandersn commented Mar 25, 2022

amcasey commented Mar 28, 2022

sandersn commented Mar 29, 2022

	return FindAllReferences.Core.getReferencesForFileName(fileName, program, program.getSourceFiles()).map(r => FindAllReferences.toReferenceEntry(r));
	return FindAllReferences.Core.getReferencesForFileName(fileName, program, program.getSourceFiles()).map(FindAllReferences.toReferenceEntry);

FAR cleanup and isDefinition definition #48211

FAR cleanup and isDefinition definition #48211

Conversation

amcasey commented Mar 10, 2022 • edited Loading

sandersn commented Mar 16, 2022

amcasey commented Mar 16, 2022

sandersn commented Mar 21, 2022

sandersn commented Mar 22, 2022

sandersn commented Mar 22, 2022

andrewbranch commented Mar 22, 2022

sandersn left a comment

Choose a reason for hiding this comment

sandersn Mar 22, 2022

Choose a reason for hiding this comment

amcasey Mar 25, 2022

Choose a reason for hiding this comment

sandersn Mar 25, 2022

Choose a reason for hiding this comment

amcasey commented Mar 25, 2022

amcasey commented Mar 25, 2022

sandersn commented Mar 25, 2022

amcasey commented Mar 28, 2022

sandersn commented Mar 29, 2022

amcasey commented Mar 10, 2022 •

edited

Loading