Eagerly resolve module specifiers for auto-import completions in `--moduleResolution node12+` #48752

andrewbranch · 2022-04-18T22:01:30Z

For auto-import completions, we used to not resolve the module specifier until the subsequent completion details request, triggered for one completion entry at a time when you focus it in the list. Later, we added a cache for module specifiers and started resolving up to 100 module specifiers per completions request, and pulling up to an additional 1000 from the cache.

Under --moduleResolution node12 or nodenext, we have to resolve module specifiers for auto-import completions right away, in excess of 100 if needed, because respecting package.json exports means that there may be exported symbols for which we cannot generate a module specifier without writing a relative path into "../node_modules/". We would prefer to treat such exports as unreachable and filter them from the list, which means we need to know the module specifier when generating the list. This is unfortunate for performance reasons if the list is very big, but we have no choice if we want to avoid showing completions for unreachable exports.

The introduction of the PackageJsonInfoCache in module resolution (added in the initial support for node12 and nodenext), and sharing that cache with the auto-imports logic (#47388) should mitigate some of this cost. I currently have no realistic project to measure this change with, as real-world projects using these yet-experimental resolution modes are rare or possibly nonexistent. I’ve added some bit flags to CompletionInfo that indicate a number of perf-impacting things that happened while computing the response, including the scenario that wasn’t possible before this PR, resolving >100 module specifiers in a single request.

…res can be filtered

typescript-bot · 2022-04-18T22:01:33Z

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

andrewbranch · 2022-04-19T00:22:38Z

src/compiler/types.ts

@@ -8463,7 +8463,7 @@ namespace ts {
    export interface ResolvedModuleSpecifierInfo {
        modulePaths: readonly ModulePath[] | undefined;
        moduleSpecifiers: readonly string[] | undefined;
-        isAutoImportable: boolean | undefined;
+        isBlockedByPackageJsonDependencies: boolean | undefined;


This is not a behavior change, but the property became a misnomer because in nodenext, factors that don't contribute to this property can make the module specifiers not auto-importable.

"Blocked" sounds like it means "not auto-importable". Is that correct and, if so, does that mean that the meaning of the flag has flipped?

For other reviewers, yes, the meaning has flipped.

andrewbranch · 2022-04-19T00:23:08Z

src/services/codefixes/importFixes.ts

@@ -629,19 +630,19 @@ namespace ts.codefix {
            const importedSymbolHasValueMeaning = !!(exportInfo.targetFlags & SymbolFlags.Value);
            const addAsTypeOnly = getAddAsTypeOnly(isValidTypeOnlyUseSite, /*isForNewImportDeclaration*/ true, exportInfo.symbol, exportInfo.targetFlags, checker, compilerOptions);
            computedWithoutCacheCount += computedWithoutCache ? 1 : 0;
-            return moduleSpecifiers?.map((moduleSpecifier): FixAddNewImport | FixAddJsdocTypeImport =>
+            return mapDefined(moduleSpecifiers, (moduleSpecifier): FixAddNewImport | FixAddJsdocTypeImport | undefined =>
+                rejectNodeModulesRelativePaths && pathContainsNodeModules(moduleSpecifier) ? undefined :


This line is the most important change.

andrewbranch · 2022-04-19T00:24:03Z

src/services/completions.ts

            }
-            const shouldResolveModuleSpecifier = isForImportStatementCompletion || preferences.allowIncompleteCompletions && resolvedCount < moduleSpecifierResolutionLimit;
+            const shouldResolveModuleSpecifier = needsFullResolution || preferences.allowIncompleteCompletions && resolvedCount < moduleSpecifierResolutionLimit;


This line is the next most important change. Everything else is bookkeeping, telemetry, renaming for accuracy, etc.

amcasey

We're going to have to be a little careful that this doesn't get consumed by the telemetry system as a floating point number that might get rounded.

amcasey · 2022-04-22T00:12:57Z

src/compiler/types.ts

@@ -8463,7 +8463,7 @@ namespace ts {
    export interface ResolvedModuleSpecifierInfo {
        modulePaths: readonly ModulePath[] | undefined;
        moduleSpecifiers: readonly string[] | undefined;
-        isAutoImportable: boolean | undefined;
+        isBlockedByPackageJsonDependencies: boolean | undefined;


"Blocked" sounds like it means "not auto-importable". Is that correct and, if so, does that mean that the meaning of the flag has flipped?

amcasey · 2022-04-22T00:14:40Z

src/compiler/types.ts

@@ -8463,7 +8463,7 @@ namespace ts {
    export interface ResolvedModuleSpecifierInfo {
        modulePaths: readonly ModulePath[] | undefined;
        moduleSpecifiers: readonly string[] | undefined;
-        isAutoImportable: boolean | undefined;
+        isBlockedByPackageJsonDependencies: boolean | undefined;


For other reviewers, yes, the meaning has flipped.

amcasey · 2022-04-22T00:18:24Z

src/services/completions.ts

+        // package.json exports can mean we *can't* resolve a module specifier (that doesn't include a
+        // relative path into node_modules), and we want to filter those completions out entirely.
+        // Import statement completions always need specifier resolution because the module specifier is
+        // part of their `insertText`, not the `codeActions` creating edits away from the cursor.


"away from the cursor" => "elsewhere in the file"?

amcasey · 2022-04-22T00:30:50Z

tests/baselines/reference/api/typescript.d.ts

    interface CompletionInfo {
+        /** For performance telemetry. */
+        flags?: CompletionInfoFlags;


We had discussed the possibility that this might subsume one or more existing flags, but it looks like that wasn't necessary?

I think there is a property that we could delete in the future if we’re not worried about old clients using it.

amcasey · 2022-04-22T00:31:33Z

tests/baselines/reference/completionsStringMethods.baseline

@@ -6,6 +6,7 @@
      "name": "1"
    },
    "completionList": {
+      "flags": 0,


Skimming, I couldn't find one that wasn't 0?

It doesn't seem like any of the baselines have a value other than 0, no, but there is a unit test that has one of the flags set.

jakebailey

LGTM, but I'm not sure if you were wanting some tests that test all of the different flag code paths or not.

andrewbranch · 2022-04-27T19:07:02Z

I wasn’t particularly concerned with testing all the telemetry flag combinations, was satisfied to see one of them come through in that unit test.

andrewbranch added 4 commits April 15, 2022 16:19

Add failing test

8964677

Block auto-import module specifiers including node_modules path

b10892c

Eagerly resolve module specifiers in completions in nodenext so failu…

7298111

…res can be filtered

Add completion info flags for telemetry

f4cf71f

typescript-bot assigned andrewbranch Apr 18, 2022

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

andrewbranch requested review from weswigham, amcasey and gabritto April 18, 2022 22:01

andrewbranch added 4 commits April 18, 2022 15:04

Update API baseline

d95f7bf

Update completions baselines

06be70d

Fix missed boolean flip

0b2e920

Fix remaining tests

122bfa6

andrewbranch commented Apr 19, 2022

View reviewed changes

amcasey approved these changes Apr 22, 2022

View reviewed changes

Merge branch 'main' into auto-imports-nodenext

9e4f900

andrewbranch requested review from sandersn and jakebailey and removed request for gabritto April 27, 2022 17:30

jakebailey approved these changes Apr 27, 2022

View reviewed changes

andrewbranch merged commit 476fc62 into microsoft:main Apr 27, 2022

andrewbranch deleted the auto-imports-nodenext branch April 27, 2022 23:07

andrewbranch mentioned this pull request Apr 27, 2022

[typescript-language-features] Add flags to completions telemetry microsoft/vscode#148313

Merged

Eagerly resolve module specifiers for auto-import completions in --moduleResolution node12+ #48752

Eagerly resolve module specifiers for auto-import completions in --moduleResolution node12+ #48752

Uh oh!

Conversation

andrewbranch commented Apr 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented Apr 18, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amcasey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakebailey left a comment

Choose a reason for hiding this comment

Uh oh!

andrewbranch commented Apr 27, 2022

Uh oh!

Uh oh!

Eagerly resolve module specifiers for auto-import completions in `--moduleResolution node12+` #48752

Eagerly resolve module specifiers for auto-import completions in `--moduleResolution node12+` #48752

andrewbranch commented Apr 18, 2022 •

edited

Loading