Fix duplicate completions from two different copies of a node_modules package (#47584)

* Fix duplicate completions from two different copies of a node_modules package * Fix logic for scoped packages * Fix errors from merge * Less gross way to reconcile these two conflicting PRs
2026-02-04 21:53:42 -06:00 · 2022-01-27 14:35:36 -08:00 · 2022-01-27 14:35:36 -08:00 · 4d298591db
commit 4d298591db
parent 61b7bbb026
8 changed files with 395 additions and 75 deletions
--- a/src/compiler/moduleSpecifiers.ts
+++ b/src/compiler/moduleSpecifiers.ts
@ -789,70 +789,6 @@ namespace ts.moduleSpecifiers {
        }
    }

-    interface NodeModulePathParts {
-        readonly topLevelNodeModulesIndex: number;
-        readonly topLevelPackageNameIndex: number;
-        readonly packageRootIndex: number;
-        readonly fileNameIndex: number;
-    }
-    function getNodeModulePathParts(fullPath: string): NodeModulePathParts | undefined {
-        // If fullPath can't be valid module file within node_modules, returns undefined.
-        // Example of expected pattern: /base/path/node_modules/[@scope/otherpackage/@otherscope/node_modules/]package/[subdirectory/]file.js
-        // Returns indices:                       ^            ^                                                      ^             ^
-
-        let topLevelNodeModulesIndex = 0;
-        let topLevelPackageNameIndex = 0;
-        let packageRootIndex = 0;
-        let fileNameIndex = 0;
-
-        const enum States {
-            BeforeNodeModules,
-            NodeModules,
-            Scope,
-            PackageContent
-        }
-
-        let partStart = 0;
-        let partEnd = 0;
-        let state = States.BeforeNodeModules;
-
-        while (partEnd >= 0) {
-            partStart = partEnd;
-            partEnd = fullPath.indexOf("/", partStart + 1);
-            switch (state) {
-                case States.BeforeNodeModules:
-                    if (fullPath.indexOf(nodeModulesPathPart, partStart) === partStart) {
-                        topLevelNodeModulesIndex = partStart;
-                        topLevelPackageNameIndex = partEnd;
-                        state = States.NodeModules;
-                    }
-                    break;
-                case States.NodeModules:
-                case States.Scope:
-                    if (state === States.NodeModules && fullPath.charAt(partStart + 1) === "@") {
-                        state = States.Scope;
-                    }
-                    else {
-                        packageRootIndex = partEnd;
-                        state = States.PackageContent;
-                    }
-                    break;
-                case States.PackageContent:
-                    if (fullPath.indexOf(nodeModulesPathPart, partStart) === partStart) {
-                        state = States.NodeModules;
-                    }
-                    else {
-                        state = States.PackageContent;
-                    }
-                    break;
-            }
-        }
-
-        fileNameIndex = partStart;
-
-        return state > States.NodeModules ? { topLevelNodeModulesIndex, topLevelPackageNameIndex, packageRootIndex, fileNameIndex } : undefined;
-    }
-
    function getPathRelativeToRootDirs(path: string, rootDirs: readonly string[], getCanonicalFileName: GetCanonicalFileName): string | undefined {
        return firstDefined(rootDirs, rootDir => {
            const relativePath = getRelativePathIfInDirectory(path, rootDir, getCanonicalFileName);
--- a/src/compiler/utilities.ts
+++ b/src/compiler/utilities.ts
@ -7501,4 +7501,68 @@ namespace ts {
    export function isThisTypeParameter(type: Type): boolean {
        return !!(type.flags & TypeFlags.TypeParameter && (type as TypeParameter).isThisType);
    }
+
+    export interface NodeModulePathParts {
+        readonly topLevelNodeModulesIndex: number;
+        readonly topLevelPackageNameIndex: number;
+        readonly packageRootIndex: number;
+        readonly fileNameIndex: number;
+    }
+    export function getNodeModulePathParts(fullPath: string): NodeModulePathParts | undefined {
+        // If fullPath can't be valid module file within node_modules, returns undefined.
+        // Example of expected pattern: /base/path/node_modules/[@scope/otherpackage/@otherscope/node_modules/]package/[subdirectory/]file.js
+        // Returns indices:                       ^            ^                                                      ^             ^
+
+        let topLevelNodeModulesIndex = 0;
+        let topLevelPackageNameIndex = 0;
+        let packageRootIndex = 0;
+        let fileNameIndex = 0;
+
+        const enum States {
+            BeforeNodeModules,
+            NodeModules,
+            Scope,
+            PackageContent
+        }
+
+        let partStart = 0;
+        let partEnd = 0;
+        let state = States.BeforeNodeModules;
+
+        while (partEnd >= 0) {
+            partStart = partEnd;
+            partEnd = fullPath.indexOf("/", partStart + 1);
+            switch (state) {
+                case States.BeforeNodeModules:
+                    if (fullPath.indexOf(nodeModulesPathPart, partStart) === partStart) {
+                        topLevelNodeModulesIndex = partStart;
+                        topLevelPackageNameIndex = partEnd;
+                        state = States.NodeModules;
+                    }
+                    break;
+                case States.NodeModules:
+                case States.Scope:
+                    if (state === States.NodeModules && fullPath.charAt(partStart + 1) === "@") {
+                        state = States.Scope;
+                    }
+                    else {
+                        packageRootIndex = partEnd;
+                        state = States.PackageContent;
+                    }
+                    break;
+                case States.PackageContent:
+                    if (fullPath.indexOf(nodeModulesPathPart, partStart) === partStart) {
+                        state = States.NodeModules;
+                    }
+                    else {
+                        state = States.PackageContent;
+                    }
+                    break;
+            }
+        }
+
+        fileNameIndex = partStart;
+
+        return state > States.NodeModules ? { topLevelNodeModulesIndex, topLevelPackageNameIndex, packageRootIndex, fileNameIndex } : undefined;
+    }
 }
--- a/src/services/exportInfoMap.ts
+++ b/src/services/exportInfoMap.ts
@ -32,6 +32,7 @@ namespace ts {
        symbolTableKey: __String;
        moduleName: string;
        moduleFile: SourceFile | undefined;
+        packageName: string | undefined;

        // SymbolExportInfo, but optional symbols
        readonly symbol: Symbol | undefined;
@ -63,6 +64,17 @@ namespace ts {
        let exportInfoId = 1;
        const exportInfo = createMultiMap<string, CachedSymbolExportInfo>();
        const symbols = new Map<number, [symbol: Symbol, moduleSymbol: Symbol]>();
+        /**
+         * Key: node_modules package name (no @types).
+         * Value: path to deepest node_modules folder seen that is
+         * both visible to `usableByFileName` and contains the package.
+         *
+         * Later, we can see if a given SymbolExportInfo is shadowed by
+         * a another installation of the same package in a deeper
+         * node_modules folder by seeing if its path starts with the
+         * value stored here.
+         */
+        const packages = new Map<string, string>();
        let usableByFileName: Path | undefined;
        const cache: ExportInfoMap = {
            isUsableByFile: importingFile => importingFile === usableByFileName,
@ -77,6 +89,29 @@ namespace ts {
                    cache.clear();
                    usableByFileName = importingFile;
                }
+
+                let packageName;
+                if (moduleFile) {
+                    const nodeModulesPathParts = getNodeModulePathParts(moduleFile.fileName);
+                    if (nodeModulesPathParts) {
+                        const { topLevelNodeModulesIndex, topLevelPackageNameIndex, packageRootIndex } = nodeModulesPathParts;
+                        packageName = unmangleScopedPackageName(getPackageNameFromTypesPackageName(moduleFile.fileName.substring(topLevelPackageNameIndex + 1, packageRootIndex)));
+                        if (startsWith(importingFile, moduleFile.path.substring(0, topLevelNodeModulesIndex))) {
+                            const prevDeepestNodeModulesPath = packages.get(packageName);
+                            const nodeModulesPath = moduleFile.fileName.substring(0, topLevelPackageNameIndex);
+                            if (prevDeepestNodeModulesPath) {
+                                const prevDeepestNodeModulesIndex = prevDeepestNodeModulesPath.indexOf(nodeModulesPathPart);
+                                if (topLevelNodeModulesIndex > prevDeepestNodeModulesIndex) {
+                                    packages.set(packageName, nodeModulesPath);
+                                }
+                            }
+                            else {
+                                packages.set(packageName, nodeModulesPath);
+                            }
+                        }
+                    }
+                }
+
                const isDefault = exportKind === ExportKind.Default;
                const namedSymbol = isDefault && getLocalSymbolForExportDefault(symbol) || symbol;
                // 1. A named export must be imported by its key in `moduleSymbol.exports` or `moduleSymbol.members`.
@ -104,6 +139,7 @@ namespace ts {
                    moduleName,
                    moduleFile,
                    moduleFileName: moduleFile?.fileName,
+                    packageName,
                    exportKind,
                    targetFlags: target.flags,
                    isFromPackageJson,
@ -121,17 +157,20 @@ namespace ts {
                exportInfo.forEach((info, key) => {
                    const { symbolName, ambientModuleName } = parseKey(key);
                    const rehydrated = info.map(rehydrateCachedInfo);
-                    action(
-                        rehydrated,
-                        preferCapitalized => {
-                            const { symbol, exportKind } = rehydrated[0];
-                            const namedSymbol = exportKind === ExportKind.Default && getLocalSymbolForExportDefault(symbol) || symbol;
-                            return preferCapitalized
-                                ? getNameForExportedSymbol(namedSymbol, /*scriptTarget*/ undefined, /*preferCapitalized*/ true)
-                                : symbolName;
-                        },
-                        !!ambientModuleName,
-                        key);
+                    const filtered = rehydrated.filter((r, i) => isNotShadowedByDeeperNodeModulesPackage(r, info[i].packageName));
+                    if (filtered.length) {
+                        action(
+                            filtered,
+                            preferCapitalized => {
+                                const { symbol, exportKind } = rehydrated[0];
+                                const namedSymbol = exportKind === ExportKind.Default && getLocalSymbolForExportDefault(symbol) || symbol;
+                                return preferCapitalized
+                                    ? getNameForExportedSymbol(namedSymbol, /*scriptTarget*/ undefined, /*preferCapitalized*/ true)
+                                    : symbolName;
+                            },
+                            !!ambientModuleName,
+                            key);
+                    }
                });
            },
            releaseSymbols: () => {
@ -232,6 +271,12 @@ namespace ts {
            }
            return true;
        }
+
+        function isNotShadowedByDeeperNodeModulesPackage(info: SymbolExportInfo, packageName: string | undefined) {
+            if (!packageName || !info.moduleFileName) return true;
+            const packageDeepestNodeModulesPath = packages.get(packageName);
+            return !packageDeepestNodeModulesPath || startsWith(info.moduleFileName, packageDeepestNodeModulesPath);
+        }
    }

    export function isImportableFile(
--- a/tests/cases/fourslash/completionsImport_duplicatePackages_scoped.ts
+++ b/tests/cases/fourslash/completionsImport_duplicatePackages_scoped.ts
@ -0,0 +1,56 @@
+/// <reference path="fourslash.ts" />
+
+// @module: commonjs
+// @esModuleInterop: true
+
+// @Filename: /node_modules/@scope/react-dom/package.json
+//// { "name": "react-dom", "version": "1.0.0", "types": "./index.d.ts" }
+
+// @Filename: /node_modules/@scope/react-dom/index.d.ts
+//// import * as React from "react";
+//// export function render(): void;
+
+// @Filename: /node_modules/@scope/react/package.json
+//// { "name": "react", "version": "1.0.0", "types": "./index.d.ts" }
+
+// @Filename: /node_modules/@scope/react/index.d.ts
+//// import "./other";
+//// export declare function useState(): void;
+
+// @Filename: /node_modules/@scope/react/other.d.ts
+//// export declare function useRef(): void;
+
+// @Filename: /packages/a/node_modules/@scope/react/package.json
+//// { "name": "react", "version": "1.0.1", "types": "./index.d.ts" }
+
+// @Filename: /packages/a/node_modules/@scope/react/index.d.ts
+//// export declare function useState(): void;
+
+// @Filename: /packages/a/index.ts
+//// import "@scope/react-dom";
+//// import "@scope/react";
+
+// @Filename: /packages/a/foo.ts
+//// /**/
+
+goTo.marker("");
+verify.completions({
+  marker: "",
+  exact: completion.globalsPlus([{
+    name: "render",
+    source: "@scope/react-dom",
+    sourceDisplay: "@scope/react-dom",
+    hasAction: true,
+    sortText: completion.SortText.AutoImportSuggestions,
+  }, {
+    name: "useState",
+    source: "@scope/react",
+    sourceDisplay: "@scope/react",
+    hasAction: true,
+    sortText: completion.SortText.AutoImportSuggestions,
+  }]),
+  preferences: {
+    includeCompletionsForModuleExports: true,
+    allowIncompleteCompletions: true,
+  },
+});
--- a/tests/cases/fourslash/completionsImport_duplicatePackages_scopedTypes.ts
+++ b/tests/cases/fourslash/completionsImport_duplicatePackages_scopedTypes.ts
@ -0,0 +1,56 @@
+/// <reference path="fourslash.ts" />
+
+// @module: commonjs
+// @esModuleInterop: true
+
+// @Filename: /node_modules/@types/scope__react-dom/package.json
+//// { "name": "react-dom", "version": "1.0.0", "types": "./index.d.ts" }
+
+// @Filename: /node_modules/@types/scope__react-dom/index.d.ts
+//// import * as React from "react";
+//// export function render(): void;
+
+// @Filename: /node_modules/@types/scope__react/package.json
+//// { "name": "react", "version": "1.0.0", "types": "./index.d.ts" }
+
+// @Filename: /node_modules/@types/scope__react/index.d.ts
+//// import "./other";
+//// export declare function useState(): void;
+
+// @Filename: /node_modules/@types/scope__react/other.d.ts
+//// export declare function useRef(): void;
+
+// @Filename: /packages/a/node_modules/@types/scope__react/package.json
+//// { "name": "react", "version": "1.0.1", "types": "./index.d.ts" }
+
+// @Filename: /packages/a/node_modules/@types/scope__react/index.d.ts
+//// export declare function useState(): void;
+
+// @Filename: /packages/a/index.ts
+//// import "@scope/react-dom";
+//// import "@scope/react";
+
+// @Filename: /packages/a/foo.ts
+//// /**/
+
+goTo.marker("");
+verify.completions({
+  marker: "",
+  exact: completion.globalsPlus([{
+    name: "render",
+    source: "@scope/react-dom",
+    sourceDisplay: "@scope/react-dom",
+    hasAction: true,
+    sortText: completion.SortText.AutoImportSuggestions,
+  }, {
+    name: "useState",
+    source: "@scope/react",
+    sourceDisplay: "@scope/react",
+    hasAction: true,
+    sortText: completion.SortText.AutoImportSuggestions,
+  }]),
+  preferences: {
+    includeCompletionsForModuleExports: true,
+    allowIncompleteCompletions: true,
+  },
+});
--- a/tests/cases/fourslash/completionsImport_duplicatePackages_scopedTypesAndNotTypes.ts
+++ b/tests/cases/fourslash/completionsImport_duplicatePackages_scopedTypesAndNotTypes.ts
@ -0,0 +1,56 @@
+/// <reference path="fourslash.ts" />
+
+// @module: commonjs
+// @esModuleInterop: true
+
+// @Filename: /node_modules/@types/scope__react-dom/package.json
+//// { "name": "react-dom", "version": "1.0.0", "types": "./index.d.ts" }
+
+// @Filename: /node_modules/@types/scope__react-dom/index.d.ts
+//// import * as React from "react";
+//// export function render(): void;
+
+// @Filename: /node_modules/@types/scope__react/package.json
+//// { "name": "react", "version": "1.0.0", "types": "./index.d.ts" }
+
+// @Filename: /node_modules/@types/scope__react/index.d.ts
+//// import "./other";
+//// export declare function useState(): void;
+
+// @Filename: /node_modules/@types/scope__react/other.d.ts
+//// export declare function useRef(): void;
+
+// @Filename: /packages/a/node_modules/@scope/react/package.json
+//// { "name": "react", "version": "1.0.1", "types": "./index.d.ts" }
+
+// @Filename: /packages/a/node_modules/@scope/react/index.d.ts
+//// export declare function useState(): void;
+
+// @Filename: /packages/a/index.ts
+//// import "react-dom";
+//// import "react";
+
+// @Filename: /packages/a/foo.ts
+//// /**/
+
+goTo.marker("");
+verify.completions({
+  marker: "",
+  exact: completion.globalsPlus([{
+    name: "render",
+    source: "@scope/react-dom",
+    sourceDisplay: "@scope/react-dom",
+    hasAction: true,
+    sortText: completion.SortText.AutoImportSuggestions,
+  }, {
+    name: "useState",
+    source: "@scope/react",
+    sourceDisplay: "@scope/react",
+    hasAction: true,
+    sortText: completion.SortText.AutoImportSuggestions,
+  }]),
+  preferences: {
+    includeCompletionsForModuleExports: true,
+    allowIncompleteCompletions: true,
+  },
+});
--- a/tests/cases/fourslash/completionsImport_duplicatePackages_types.ts
+++ b/tests/cases/fourslash/completionsImport_duplicatePackages_types.ts
@ -0,0 +1,57 @@
+/// <reference path="fourslash.ts" />
+
+// @module: commonjs
+// @esModuleInterop: true
+
+// @Filename: /node_modules/@types/react-dom/package.json
+//// { "name": "react-dom", "version": "1.0.0", "types": "./index.d.ts" }
+
+// @Filename: /node_modules/@types/react-dom/index.d.ts
+//// import * as React from "react";
+//// export function render(): void;
+
+// @Filename: /node_modules/@types/react/package.json
+//// { "name": "react", "version": "1.0.0", "types": "./index.d.ts" }
+
+// @Filename: /node_modules/@types/react/index.d.ts
+//// import "./other";
+//// export declare function useState(): void;
+
+// @Filename: /node_modules/@types/react/other.d.ts
+//// export declare function useRef(): void;
+
+// @Filename: /packages/a/node_modules/@types/react/package.json
+//// { "name": "react", "version": "1.0.1", "types": "./index.d.ts" }
+
+// @Filename: /packages/a/node_modules/@types/react/index.d.ts
+//// export declare function useState(): void;
+
+// @Filename: /packages/a/index.ts
+//// import "react-dom";
+//// import "react";
+
+// @Filename: /packages/a/foo.ts
+//// /**/
+
+goTo.marker("");
+verify.completions({
+  marker: "",
+  exact: completion.globalsPlus([{
+    name: "render",
+    source: "react-dom",
+    sourceDisplay: "react-dom",
+    hasAction: true,
+    sortText: completion.SortText.AutoImportSuggestions,
+  }, {
+    name: "useState",
+    source: "react",
+    sourceDisplay: "react",
+    hasAction: true,
+    sortText: completion.SortText.AutoImportSuggestions,
+  }]),
+  preferences: {
+    includeCompletionsForModuleExports: true,
+    allowIncompleteCompletions: true,
+  },
+});
+
--- a/tests/cases/fourslash/completionsImport_duplicatePackages_typesAndNotTypes.ts
+++ b/tests/cases/fourslash/completionsImport_duplicatePackages_typesAndNotTypes.ts
@ -0,0 +1,50 @@
+/// <reference path="fourslash.ts" />
+
+// @module: commonjs
+// @esModuleInterop: true
+
+// @Filename: /node_modules/@types/react-dom/package.json
+//// { "name": "react-dom", "version": "1.0.0", "types": "./index.d.ts" }
+
+// @Filename: /node_modules/@types/react-dom/index.d.ts
+//// import * as React from "react";
+//// export function render(): void;
+
+// @Filename: /node_modules/@types/react/package.json
+//// { "name": "react", "version": "1.0.0", "types": "./index.d.ts" }
+
+// @Filename: /node_modules/@types/react/index.d.ts
+//// import "./other";
+//// export declare function useState(): void;
+
+// @Filename: /node_modules/@types/react/other.d.ts
+//// export declare function useRef(): void;
+
+// @Filename: /packages/a/node_modules/react/package.json
+//// { "name": "react", "version": "1.0.1", "types": "./index.d.ts" }
+
+// @Filename: /packages/a/node_modules/react/index.d.ts
+//// export declare function useState(): void;
+
+// @Filename: /packages/a/index.ts
+//// import "react-dom";
+//// import "react";
+
+// @Filename: /packages/a/foo.ts
+//// useState/**/
+
+goTo.marker("");
+verify.completions({
+  marker: "",
+  exact: completion.globalsPlus([{
+    name: "useState",
+    source: "react",
+    sourceDisplay: "react",
+    hasAction: true,
+    sortText: completion.SortText.AutoImportSuggestions,
+  }]),
+  preferences: {
+    includeCompletionsForModuleExports: true,
+    allowIncompleteCompletions: true,
+  },
+});