Clean up FAR and NavTo aggregation #47938

amcasey · 2022-02-17T23:10:15Z

We have library functions for searching within a project for symbols, references, rename locations, etc. All of that code is exactly as before - this change is about the layer that sits on top of that and aggregates the results across multiple projects. In the course of some performance investigations, I discovered that user-FAR was invoking per-project-FAR more times than I expected and that the code wasn't readable enough for me to confidently determine why or how to prevent it. This change attempts to address both issues.

As a bonus, while doing this, I also discovered that we repeat the search for the nearest tsconfig because the results are only cached for open files, which FAR and navto results tend not to be in. This change de-dups those as well. (If we decide this change is too risky, I believe this bit can be salvaged.)

I'm generally okay with the test change (seeing fewer results when a file is specified) both in principal, since not loading projects seems preferable when scoping to the active document, and in practice, since AFAIK only old version of VS Code pass a file with no project.

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 Most of the baseline changes just reflect the de-duping of tsconfig searches, a few show fewer and occasionally different FAR invocations, and a couple show response changes. I'm quite confident that the new FAR calls are better and moderately confident that the new isDefinition values are better (not to mention moderately skeptical that anyone will ever hit a case where the difference matters).

amcasey · 2022-02-17T23:12:44Z

src/testRunner/unittests/tsserver/declarationFileMaps.ts

@@ -308,20 +310,9 @@ namespace ts.projectSystem {
                    kind: ScriptElementKind.functionElement,
                    kindModifiers: "export",
                },
-                {


This tests mimics the request of an old VS Code client. A new client will not pass a file in the default allOpenProjects mode, but will in the opt-in currentProjectMode. I didn't think it made sense to return other-project results when the user selected currentProjectMode and scoping in this way improved consistency with other scenarios (mostly VS).

amcasey · 2022-02-17T23:14:23Z

...fd-proj-loading-is-disabled-and-proj-ref-redirects-are-disabled-and-a-decl-map-is-present.js

@@ -122,7 +122,5 @@ Finding references to /user/username/projects/myproject/a/index.ts position 40 i
 FileWatcher:: Added:: WatchInfo: /user/username/projects/myproject/b/lib/index.d.ts.map 500 undefined WatchType: Closed Script info
 Search path: /user/username/projects/myproject/b
 For info: /user/username/projects/myproject/b/index.ts :: Config file name: /user/username/projects/myproject/b/tsconfig.json
-Search path: /user/username/projects/myproject/b


A pair of lines like this indicates that we were walking up from (e.g.) /user/username/projects/myproject/b/index.ts to find the nearest (aka "default"?) tsconfig, (e.g.) /user/username/projects/myproject/b/tsconfig.json. If you scroll up, you will find an identical pair of lines indicating that this search was redundant.

amcasey · 2022-02-17T23:15:19Z

...baselines/reference/tsserver/projectReferences/project-is-directly-referenced-by-solution.js

The reason for looking here instead is nuanced. Breadcrumb for when someone asks me: this is the definition location of the symbol at the initial location.

amcasey · 2022-02-17T23:16:02Z

...baselines/reference/tsserver/projectReferences/project-is-directly-referenced-by-solution.js

+Finding references to /user/username/projects/myproject/src/helpers/functions.ts position 13 in project /user/username/projects/myproject/tsconfig-src.json
+response:{"response":{"refs":[{"file":"/user/username/projects/myproject/indirect3/main.ts","start":{"line":1,"offset":10},"end":{"line":1,"offset":13},"contextStart":{"line":1,"offset":1},"contextEnd":{"line":1,"offset":28},"lineText":"import { foo } from 'main';","isWriteAccess":true,"isDefinition":true},{"file":"/user/username/projects/myproject/indirect3/main.ts","start":{"line":2,"offset":1},"end":{"line":2,"offset":4},"lineText":"foo;","isWriteAccess":false,"isDefinition":false},{"file":"/user/username/projects/myproject/src/main.ts","start":{"line":1,"offset":10},"end":{"line":1,"offset":13},"contextStart":{"line":1,"offset":1},"contextEnd":{"line":1,"offset":41},"lineText":"import { foo } from 'helpers/functions';","isWriteAccess":true,"isDefinition":false},{"file":"/user/username/projects/myproject/src/main.ts","start":{"line":2,"offset":10},"end":{"line":2,"offset":13},"contextStart":{"line":2,"offset":1},"contextEnd":{"line":2,"offset":16},"lineText":"export { foo };","isWriteAccess":true,"isDefinition":false},{"file":"/user/username/projects/myproject/src/helpers/functions.ts","start":{"line":1,"offset":14},"end":{"line":1,"offset":17},"contextStart":{"line":1,"offset":1},"contextEnd":{"line":1,"offset":22},"lineText":"export const foo = 1;","isWriteAccess":true,"isDefinition":true}],"symbolName":"foo","symbolStartOffset":10,"symbolDisplayString":"(alias) const foo: 1\nimport foo"},"responseRequired":true}


isDefinition use to be true for the import and false for the const and now it is the opposite. Based on where the search started, I believe this is more correct.

amcasey · 2022-02-17T23:17:15Z

...selines/reference/tsserver/projectReferences/project-is-indirectly-referenced-by-solution.js

 Search path: /user/username/projects/myproject/src/helpers
 For info: /user/username/projects/myproject/src/helpers/functions.ts :: Config file name: /user/username/projects/myproject/tsconfig.json
+Finding references to /user/username/projects/myproject/src/helpers/functions.ts position 13 in project /user/username/projects/myproject/tsconfig-src.json


In a few places, a search happens earlier than it used to. This is part of trying to eliminate duplicate searches (pulling the high-quality search ahead of the low-quality search so the low-quality search can be skipped).

amcasey · 2022-02-17T23:17:29Z

...iles-and-project-found-is-not-solution-but-references-open-file-through-project-reference.js

+Finding references to /user/username/projects/myproject/src/helpers/functions.ts position 13 in project /user/username/projects/myproject/tsconfig-src.json
+response:{"response":{"refs":[{"file":"/user/username/projects/myproject/indirect3/main.ts","start":{"line":1,"offset":10},"end":{"line":1,"offset":13},"contextStart":{"line":1,"offset":1},"contextEnd":{"line":1,"offset":28},"lineText":"import { foo } from 'main';","isWriteAccess":true,"isDefinition":true},{"file":"/user/username/projects/myproject/indirect3/main.ts","start":{"line":2,"offset":1},"end":{"line":2,"offset":4},"lineText":"foo;","isWriteAccess":false,"isDefinition":false},{"file":"/user/username/projects/myproject/src/main.ts","start":{"line":1,"offset":10},"end":{"line":1,"offset":13},"contextStart":{"line":1,"offset":1},"contextEnd":{"line":1,"offset":41},"lineText":"import { foo } from 'helpers/functions';","isWriteAccess":true,"isDefinition":false},{"file":"/user/username/projects/myproject/src/main.ts","start":{"line":2,"offset":10},"end":{"line":2,"offset":13},"contextStart":{"line":2,"offset":1},"contextEnd":{"line":2,"offset":16},"lineText":"export { foo };","isWriteAccess":true,"isDefinition":false},{"file":"/user/username/projects/myproject/src/helpers/functions.ts","start":{"line":1,"offset":14},"end":{"line":1,"offset":17},"contextStart":{"line":1,"offset":1},"contextEnd":{"line":1,"offset":22},"lineText":"export const foo = 1;","isWriteAccess":true,"isDefinition":true},{"file":"/user/username/projects/myproject/own/main.ts","start":{"line":1,"offset":10},"end":{"line":1,"offset":13},"contextStart":{"line":1,"offset":1},"contextEnd":{"line":1,"offset":28},"lineText":"import { foo } from 'main';","isWriteAccess":true,"isDefinition":false},{"file":"/user/username/projects/myproject/own/main.ts","start":{"line":2,"offset":1},"end":{"line":2,"offset":4},"lineText":"foo;","isWriteAccess":false,"isDefinition":false}],"symbolName":"foo","symbolStartOffset":10,"symbolDisplayString":"(alias) const foo: 1\nimport foo"},"responseRequired":true}


isDefinition use to be true for the import and false for the const and now it is the opposite. Based on where the search started, I believe this is more correct.

amcasey · 2022-02-17T23:18:27Z

src/server/session.ts

@@ -294,71 +294,47 @@ namespace ts.server {
        return deduplicate(outputs, equateValues);
    }

-    type CombineOutputResult<T> = { project: Project; result: readonly T[]; }[];


Most of these helpers were inlined. The survivors were renamed and probably nested.

amcasey · 2022-02-17T23:18:50Z

src/server/session.ts

+
+        const results: RenameLocation[] = [];
+
+        for (const project of perProjectResults.projects) {


The aggregation is now here, rather than in the callback.

amcasey · 2022-02-17T23:19:12Z

src/server/session.ts

        );

-        return outputs;
+        // No filtering or dedup'ing is required if there's exactly one project


Special-casing the single-project case is new and should avoid a bunch of linear-search de-duping.

amcasey · 2022-02-17T23:19:40Z

src/server/session.ts

        projects: Projects,
        defaultProject: Project,
-        initialLocation: DocumentPosition,
+        initialPosition: DocumentPosition,


I could be persuaded to rename these back to location, since that seems to be a convention, but it doesn't make sense to me.

amcasey · 2022-02-17T23:21:21Z

src/server/session.ts

+     * @returns In the common case where there's only one project, returns an array of results from {@link getResultsForPosition}.
+     * If multiple projects were searched - even if they didn't return results - the result will be a {@link PerProjectResults}.
+     */
+    function getPerProjectReferences<TResult>(


This is the meat of the FAR logic. I'd read it as standalone code and not try to map back to the old implementation. If the comments aren't sufficient, I'd like to improve them.

amcasey · 2022-02-17T23:22:18Z

src/server/session.ts

@@ -1610,11 +1689,22 @@ namespace ts.server {

        private getFileReferences(args: protocol.FileRequestArgs, simplifiedResult: boolean): protocol.FileReferencesResponseBody | readonly ReferenceEntry[] {


This no longer uses a shared helper, since it behaves differently.

amcasey · 2022-02-17T23:22:57Z

src/server/session.ts

@@ -2121,26 +2211,73 @@ namespace ts.server {
                );
        }

-        private getFullNavigateToItems(args: protocol.NavtoRequestArgs): CombineOutputResult<NavigateToItem> {
+        private getFullNavigateToItems(args: protocol.NavtoRequestArgs): ProjectNavigateToItems[] {


This no longer uses a shared helper and there are behavioral changes (see baseline). I also attempted to get rid of some of the linear searching, but JS wasn't very cooperative.

amcasey · 2022-02-17T23:23:48Z

src/server/session.ts

+                // VS's `Go to symbol` sends requests with just a project and doesn't want cascading since it will
+                // send a separate request for each project of interest
+
+                // TODO (https://github.com/microsoft/TypeScript/issues/47839)


I basically need Sheetal's input to resolve these, but I think the current behavior is acceptably similar to the current behavior.

amcasey · 2022-02-17T23:26:31Z

I'm basically happy with this PR, but I've marked it as draft while I gather perf numbers and try dogfooding it.

amcasey · 2022-02-18T21:15:08Z

src/server/session.ts

+                    referencedSymbol.definition :
+                    {
+                        ...referencedSymbol.definition,
+                        textSpan: createTextSpan(mappedDefinitionFile.pos, referencedSymbol.definition.textSpan.length), // Why would the length be the same in the original?


These are probably always tokens.

amcasey · 2022-02-18T21:17:48Z

src/server/session.ts

+    interface PerProjectResults<TResult> {
+        // The projects that were searched in the order in which they were searched.
+        // (The order may be slightly fudged to prioritize "authoritative" projects.)
+        projects: readonly Project[];


Talk to @sandersn about isDefinition

amcasey · 2022-02-22T23:45:55Z

Consumers of isDefinition:

VS Code - if regular FAR returns exactly two results, "compact" results are requested, and exactly one of the results is not a definition, then the single non-definition will be returned. (Unfortunately, this is accomplished using two separate tsserver requests.)
VS Code - CodeLens always drops isDefinition results from the references count.
VS - The old implementation ignores it.
VS - The new implementation appears to (incorrectly?) assume that there will be exactly one definition in the result and uses it (or, in practice, the last occurrence) to determine whether the definition is read or written.

Edit: I filed a bug suggesting VS ignore it entirely.
Edit: The command name for (1) is "Go to References" or editor.action.goToReferences
Edit: Using isDefinition for (1) will produce slightly wrong behavior if both "references" are declarations (e.g. an exported class and an import of that class) - the editor will jump to the other declaration, rather than showing the peek window. This doesn't seem worth fixing.

amcasey · 2022-03-03T18:10:37Z

I'm going to split out the NavTo cleanup and the tsconfig search caching. Then I'll separately revisit how to correctly implement isDefinition.

amcasey · 2022-03-03T18:14:01Z

Split out #48106.

amcasey · 2022-03-04T00:25:37Z

Split out and abandoned #48113.

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

Split out the remainder of this change as #48211

amcasey · 2022-04-09T00:15:52Z

Remaining work is in #48619.

rbadapanda · 2022-05-12T23:34:30Z

with typescript 3.7, refactor stopped working. Is it a known issue?

amcasey · 2022-05-19T00:20:56Z

@rbadapanda Can you please file a new issue, specifying which refactoring(s) you are not seeing? Fair warning: you are likely to be asked to try to repro in the most recent release (4.6) or the nightly build.

amcasey added 3 commits February 17, 2022 15:00

Start inlining helpers and renaming functions related to FAR/NavTo

14afc16

amcasey requested review from sandersn and andrewbranch February 17, 2022 23:10

typescript-bot assigned amcasey Feb 17, 2022

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

amcasey commented Feb 17, 2022

View reviewed changes

amcasey marked this pull request as ready for review February 18, 2022 21:01

amcasey commented Feb 18, 2022

View reviewed changes

amcasey mentioned this pull request Feb 19, 2022

Scratch: How much can we save by not tracking or enforcing search order? #47963

Closed

DanielRosenwasser mentioned this pull request Feb 24, 2022

TypeScript 4.7 Iteration Plan #48027

Closed

amcasey marked this pull request as draft March 3, 2022 18:09

amcasey mentioned this pull request Apr 5, 2022

Only report isDefinition when FAR is triggered on a definition #48566

Merged

amcasey closed this Apr 9, 2022

amcasey deleted the FarCleanup branch May 19, 2022 00:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up FAR and NavTo aggregation #47938

Clean up FAR and NavTo aggregation #47938

amcasey commented Feb 17, 2022

amcasey Feb 17, 2022

amcasey Feb 17, 2022

amcasey Feb 17, 2022

amcasey Feb 17, 2022

amcasey Feb 17, 2022

amcasey Feb 17, 2022

amcasey Feb 17, 2022

amcasey Feb 17, 2022

amcasey Feb 17, 2022

amcasey Feb 17, 2022

amcasey Feb 17, 2022

amcasey Feb 17, 2022

amcasey Feb 17, 2022

amcasey Feb 17, 2022

amcasey commented Feb 17, 2022

amcasey Feb 18, 2022

amcasey Feb 18, 2022

amcasey commented Feb 22, 2022 •

edited

Loading

amcasey commented Mar 3, 2022

amcasey commented Mar 3, 2022

amcasey commented Mar 4, 2022

amcasey commented Mar 10, 2022

amcasey commented Apr 9, 2022

rbadapanda commented May 12, 2022

amcasey commented May 19, 2022

		Finding references to /user/username/projects/myproject/src/helpers/functions.ts position 13 in project /user/username/projects/myproject/tsconfig-src.json
		response:{"response":{"refs":[{"file":"/user/username/projects/myproject/indirect3/main.ts","start":{"line":1,"offset":10},"end":{"line":1,"offset":13},"contextStart":{"line":1,"offset":1},"contextEnd":{"line":1,"offset":28},"lineText":"import { foo } from 'main';","isWriteAccess":true,"isDefinition":true},{"file":"/user/username/projects/myproject/indirect3/main.ts","start":{"line":2,"offset":1},"end":{"line":2,"offset":4},"lineText":"foo;","isWriteAccess":false,"isDefinition":false},{"file":"/user/username/projects/myproject/src/main.ts","start":{"line":1,"offset":10},"end":{"line":1,"offset":13},"contextStart":{"line":1,"offset":1},"contextEnd":{"line":1,"offset":41},"lineText":"import { foo } from 'helpers/functions';","isWriteAccess":true,"isDefinition":false},{"file":"/user/username/projects/myproject/src/main.ts","start":{"line":2,"offset":10},"end":{"line":2,"offset":13},"contextStart":{"line":2,"offset":1},"contextEnd":{"line":2,"offset":16},"lineText":"export { foo };","isWriteAccess":true,"isDefinition":false},{"file":"/user/username/projects/myproject/src/helpers/functions.ts","start":{"line":1,"offset":14},"end":{"line":1,"offset":17},"contextStart":{"line":1,"offset":1},"contextEnd":{"line":1,"offset":22},"lineText":"export const foo = 1;","isWriteAccess":true,"isDefinition":true}],"symbolName":"foo","symbolStartOffset":10,"symbolDisplayString":"(alias) const foo: 1\nimport foo"},"responseRequired":true}


		const results: RenameLocation[] = [];

		for (const project of perProjectResults.projects) {

		@@ -1610,11 +1689,22 @@ namespace ts.server {

		private getFileReferences(args: protocol.FileRequestArgs, simplifiedResult: boolean): protocol.FileReferencesResponseBody \| readonly ReferenceEntry[] {

Clean up FAR and NavTo aggregation #47938

Clean up FAR and NavTo aggregation #47938

Conversation

amcasey commented Feb 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amcasey commented Feb 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amcasey commented Feb 22, 2022 • edited Loading

amcasey commented Mar 3, 2022

amcasey commented Mar 3, 2022

amcasey commented Mar 4, 2022

amcasey commented Mar 10, 2022

amcasey commented Apr 9, 2022

rbadapanda commented May 12, 2022

amcasey commented May 19, 2022

amcasey commented Feb 22, 2022 •

edited

Loading