Clean up code for nonrelative path completions (#23150)

* Clean up code for nonrelative path completions

* Remove unnecessary test and simplify based on that

* More code review

* Call getCompletionEntriesFromTypings unconditionally

Author: Andy

src/compiler/moduleNameResolver.ts

@@ -117,7 +117,8 @@ namespace ts {
         }
     }
 
-    function readJson(path: string, host: ModuleResolutionHost): PackageJson {
+    /* @internal */
+    export function readJson(path: string, host: { readFile(fileName: string): string | undefined }): object {
         try {
             const jsonText = host.readFile(path);
             return jsonText ? JSON.parse(jsonText) : {};
@@ -300,7 +301,7 @@ namespace ts {
                             // `types-publisher` sometimes creates packages with `"typings": null` for packages that don't provide their own types.
                             // See `createNotNeededPackageJSON` in the types-publisher` repo.
                             // tslint:disable-next-line:no-null-keyword
-                            const isNotNeededPackage = host.fileExists(packageJsonPath) && readJson(packageJsonPath, host).typings === null;
+                            const isNotNeededPackage = host.fileExists(packageJsonPath) && (readJson(packageJsonPath, host) as PackageJson).typings === null;
                             if (!isNotNeededPackage) {
                                 // Return just the type directive names
                                 result.push(getBaseFileName(normalized));
@@ -983,7 +984,7 @@ namespace ts {
         const directoryExists = !onlyRecordFailures && directoryProbablyExists(nodeModuleDirectory, host);
         const packageJsonPath = pathToPackageJson(nodeModuleDirectory);
         if (directoryExists && host.fileExists(packageJsonPath)) {
-            const packageJsonContent = readJson(packageJsonPath, host);
+            const packageJsonContent = readJson(packageJsonPath, host) as PackageJson;
             if (subModuleName === "") { // looking up the root - need to handle types/typings/main redirects for subModuleName
                 const path = tryReadPackageJsonFields(/*readTypes*/ true, packageJsonContent, nodeModuleDirectory, state);
                 if (typeof path === "string") {

src/services/pathCompletions.ts

@@ -137,8 +137,9 @@ namespace ts.Completions.PathCompletions {
             if (directories) {
                 for (const directory of directories) {
                     const directoryName = getBaseFileName(normalizePath(directory));
-
-                    result.push(nameAndKind(directoryName, ScriptElementKind.directory));
+                    if (directoryName !== "@types") {
+                        result.push(nameAndKind(directoryName, ScriptElementKind.directory));
+                    }
                 }
             }
         }
@@ -177,19 +178,33 @@ namespace ts.Completions.PathCompletions {
             }
         }
 
-        if (compilerOptions.moduleResolution === ModuleResolutionKind.NodeJs) {
-            forEachAncestorDirectory(scriptPath, ancestor => {
-                const nodeModules = combinePaths(ancestor, "node_modules");
-                if (host.directoryExists(nodeModules)) {
-                    getCompletionEntriesForDirectoryFragment(fragment, nodeModules, fileExtensions, /*includeExtensions*/ false, host, /*exclude*/ undefined, result);
-                }
-            });
+        const fragmentDirectory = containsSlash(fragment) ? getDirectoryPath(fragment) : undefined;
+        for (const ambientName of getAmbientModuleCompletions(fragment, fragmentDirectory, typeChecker)) {
+            result.push(nameAndKind(ambientName, ScriptElementKind.externalModuleName));
         }
 
         getCompletionEntriesFromTypings(host, compilerOptions, scriptPath, result);
 
-        for (const moduleName of enumeratePotentialNonRelativeModules(fragment, scriptPath, compilerOptions, typeChecker, host)) {
-            result.push(nameAndKind(moduleName, ScriptElementKind.externalModuleName));
+        if (getEmitModuleResolutionKind(compilerOptions) === ModuleResolutionKind.NodeJs) {
+            // If looking for a global package name, don't just include everything in `node_modules` because that includes dependencies' own dependencies.
+            // (But do if we didn't find anything, e.g. 'package.json' missing.)
+            let foundGlobal = false;
+            if (fragmentDirectory === undefined) {
+                for (const moduleName of enumerateNodeModulesVisibleToScript(host, scriptPath)) {
+                    if (!result.some(entry => entry.name === moduleName)) {
+                        foundGlobal = true;
+                        result.push(nameAndKind(moduleName, ScriptElementKind.externalModuleName));
+                    }
+                }
+            }
+            if (!foundGlobal) {
+                forEachAncestorDirectory(scriptPath, ancestor => {
+                    const nodeModules = combinePaths(ancestor, "node_modules");
+                    if (tryDirectoryExists(host, nodeModules)) {
+                        getCompletionEntriesForDirectoryFragment(fragment, nodeModules, fileExtensions, /*includeExtensions*/ false, host, /*exclude*/ undefined, result);
+                    }
+                });
+            }
         }
 
         return result;
@@ -228,7 +243,7 @@ namespace ts.Completions.PathCompletions {
         const normalizedPrefixDirectory = getDirectoryPath(normalizedPrefix);
         const normalizedPrefixBase = getBaseFileName(normalizedPrefix);
 
-        const fragmentHasPath = stringContains(fragment, directorySeparator);
+        const fragmentHasPath = containsSlash(fragment);
 
         // Try and expand the prefix to include any path from the fragment so that we can limit the readDirectory call
         const expandedPrefixDirectory = fragmentHasPath ? combinePaths(normalizedPrefixDirectory, normalizedPrefixBase + getDirectoryPath(fragment)) : normalizedPrefixDirectory;
@@ -262,45 +277,19 @@ namespace ts.Completions.PathCompletions {
         return path[0] === directorySeparator ? path.slice(1) : path;
     }
 
-    function enumeratePotentialNonRelativeModules(fragment: string, scriptPath: string, options: CompilerOptions, typeChecker: TypeChecker, host: LanguageServiceHost): string[] {
-        // Check If this is a nested module
-        const isNestedModule = stringContains(fragment, directorySeparator);
-        const moduleNameFragment = isNestedModule ? fragment.substr(0, fragment.lastIndexOf(directorySeparator)) : undefined;
-
+    function getAmbientModuleCompletions(fragment: string, fragmentDirectory: string | undefined, checker: TypeChecker): ReadonlyArray<string> {
         // Get modules that the type checker picked up
-        const ambientModules = map(typeChecker.getAmbientModules(), sym => stripQuotes(sym.name));
-        let nonRelativeModuleNames = filter(ambientModules, moduleName => startsWith(moduleName, fragment));
+        const ambientModules = checker.getAmbientModules().map(sym => stripQuotes(sym.name));
+        const nonRelativeModuleNames = ambientModules.filter(moduleName => startsWith(moduleName, fragment));
 
         // Nested modules of the form "module-name/sub" need to be adjusted to only return the string
         // after the last '/' that appears in the fragment because that's where the replacement span
         // starts
-        if (isNestedModule) {
-            const moduleNameWithSeperator = ensureTrailingDirectorySeparator(moduleNameFragment);
-            nonRelativeModuleNames = map(nonRelativeModuleNames, nonRelativeModuleName => {
-                return removePrefix(nonRelativeModuleName, moduleNameWithSeperator);
-            });
-        }
-
-
-        if (!options.moduleResolution || options.moduleResolution === ModuleResolutionKind.NodeJs) {
-            for (const visibleModule of enumerateNodeModulesVisibleToScript(host, scriptPath)) {
-                if (!isNestedModule) {
-                    nonRelativeModuleNames.push(visibleModule.moduleName);
-                }
-                else if (startsWith(visibleModule.moduleName, moduleNameFragment)) {
-                    const nestedFiles = tryReadDirectory(host, visibleModule.moduleDir, supportedTypeScriptExtensions, /*exclude*/ undefined, /*include*/ ["./*"]);
-                    if (nestedFiles) {
-                        for (let f of nestedFiles) {
-                            f = normalizePath(f);
-                            const nestedModule = removeFileExtension(getBaseFileName(f));
-                            nonRelativeModuleNames.push(nestedModule);
-                        }
-                    }
-                }
-            }
+        if (fragmentDirectory !== undefined) {
+            const moduleNameWithSeperator = ensureTrailingDirectorySeparator(fragmentDirectory);
+            return nonRelativeModuleNames.map(nonRelativeModuleName => removePrefix(nonRelativeModuleName, moduleNameWithSeperator));
         }
-
-        return deduplicate(nonRelativeModuleNames, equateStringsCaseSensitive, compareStringsCaseSensitive);
+        return nonRelativeModuleNames;
     }
 
     export function getTripleSlashReferenceCompletion(sourceFile: SourceFile, position: number, compilerOptions: CompilerOptions, host: LanguageServiceHost): ReadonlyArray<PathCompletion> | undefined {
@@ -390,55 +379,24 @@ namespace ts.Completions.PathCompletions {
         return paths;
     }
 
-    function enumerateNodeModulesVisibleToScript(host: LanguageServiceHost, scriptPath: string) {
-        const result: VisibleModuleInfo[] = [];
-
-        if (host.readFile && host.fileExists) {
-            for (const packageJson of findPackageJsons(scriptPath, host)) {
-                const contents = tryReadingPackageJson(packageJson);
-                if (!contents) {
-                    return;
-                }
+    function enumerateNodeModulesVisibleToScript(host: LanguageServiceHost, scriptPath: string): ReadonlyArray<string> {
+        if (!host.readFile || !host.fileExists) return emptyArray;
 
-                const nodeModulesDir = combinePaths(getDirectoryPath(packageJson), "node_modules");
-                const foundModuleNames: string[] = [];
-
-                // Provide completions for all non @types dependencies
-                for (const key of nodeModulesDependencyKeys) {
-                    addPotentialPackageNames(contents[key], foundModuleNames);
-                }
-
-                for (const moduleName of foundModuleNames) {
-                    const moduleDir = combinePaths(nodeModulesDir, moduleName);
-                    result.push({
-                        moduleName,
-                        moduleDir
-                    });
-                }
-            }
-        }
-
-        return result;
-
-        function tryReadingPackageJson(filePath: string) {
-            try {
-                const fileText = tryReadFile(host, filePath);
-                return fileText ? JSON.parse(fileText) : undefined;
-            }
-            catch (e) {
-                return undefined;
-            }
-        }
-
-        function addPotentialPackageNames(dependencies: any, result: string[]) {
-            if (dependencies) {
+        const result: string[] = [];
+        for (const packageJson of findPackageJsons(scriptPath, host)) {
+            const contents = readJson(packageJson, host as { readFile: (filename: string) => string | undefined }); // Cast to assert that readFile is defined
+            // Provide completions for all non @types dependencies
+            for (const key of nodeModulesDependencyKeys) {
+                const dependencies: object | undefined = (contents as any)[key];
+                if (!dependencies) continue;
                 for (const dep in dependencies) {
                     if (dependencies.hasOwnProperty(dep) && !startsWith(dep, "@types/")) {
                         result.push(dep);
                     }
                 }
             }
         }
+        return result;
     }
 
     // Replace everything after the last directory seperator that appears
@@ -484,11 +442,6 @@ namespace ts.Completions.PathCompletions {
      */
     const tripleSlashDirectiveFragmentRegex = /^(\/\/\/\s*<reference\s+(path|types)\s*=\s*(?:'|"))([^\3"]*)$/;
 
-    interface VisibleModuleInfo {
-        moduleName: string;
-        moduleDir: string;
-    }
-
     const nodeModulesDependencyKeys = ["dependencies", "devDependencies", "peerDependencies", "optionalDependencies"];
 
     function tryGetDirectories(host: LanguageServiceHost, directoryName: string): string[] {
@@ -499,10 +452,6 @@ namespace ts.Completions.PathCompletions {
         return tryIOAndConsumeErrors(host, host.readDirectory, path, extensions, exclude, include) || emptyArray;
     }
 
-    function tryReadFile(host: LanguageServiceHost, path: string): string | undefined {
-        return tryIOAndConsumeErrors(host, host.readFile, path);
-    }
-
     function tryFileExists(host: LanguageServiceHost, path: string): boolean {
         return tryIOAndConsumeErrors(host, host.fileExists, path);
     }
@@ -522,4 +471,8 @@ namespace ts.Completions.PathCompletions {
         catch { /*ignore*/ }
         return undefined;
     }
+
+    function containsSlash(fragment: string) {
+        return stringContains(fragment, directorySeparator);
+    }
 }

tests/cases/fourslash/completionForStringLiteralNonrelativeImport2.ts

@@ -27,11 +27,4 @@
 // @Filename: ambient.ts
 //// declare module "fake-module/other"
 
-const kinds = ["import_as", "import_equals", "require"];
-
-for (const kind of kinds) {
-    goTo.marker(kind + "0");
-    verify.completionListContains("repeated");
-    verify.completionListContains("other");
-    verify.not.completionListItemsCountIsGreaterThan(2);
-}
+verify.completionsAt(["import_as0", "import_equals0", "require0"], ["other", "repeated"], { isNewIdentifierLocation: true })

tests/cases/fourslash/completionForStringLiteralNonrelativeImport3.ts

@@ -27,12 +27,4 @@
 // @Filename: node_modules/fake-module/repeated.jsx
 //// /*repeatedjsx*/
 
-const kinds = ["import_as", "import_equals", "require"];
-
-for (const kind of kinds) {
-    goTo.marker(kind + "0");
-    verify.completionListContains("ts");
-    verify.completionListContains("tsx");
-    verify.completionListContains("dts");
-    verify.not.completionListItemsCountIsGreaterThan(3);
-}
+verify.completionsAt(["import_as0", "import_equals0", "require0"], ["dts", "js", "jsx", "repeated", "ts", "tsx"], { isNewIdentifierLocation: true });

tests/cases/fourslash/completionForStringLiteralNonrelativeImportTypings3.ts

@@ -18,11 +18,4 @@
 // @Filename: package.json
 //// { "dependencies": { "@types/module-y": "latest" } }
 
-const kinds = ["types_ref", "import_as", "import_equals", "require"];
-
-for (const kind of kinds) {
-    goTo.marker(kind + "0");
-    verify.completionListContains("module-x");
-    verify.completionListContains("module-y");
-    verify.not.completionListItemsCountIsGreaterThan(2);
-}
+verify.completionsAt(["types_ref0", "import_as0", "import_equals0", "require0"], ["module-x", "module-y"], { isNewIdentifierLocation: true });

tests/cases/fourslash/completionListInImportClause05.ts

@@ -12,4 +12,4 @@
 // NOTE: The node_modules folder is in "/", rather than ".", because it requires
 // less scaffolding to mock.  In particular, "/" is where we look for type roots.
 
-verify.completionsAt("1", ["@a/b", "@c/d", "@e/f"], { isNewIdentifierLocation: true });
+verify.completionsAt("1", ["@e/f", "@a/b", "@c/d"], { isNewIdentifierLocation: true });

tests/cases/fourslash/completionsPaths.ts

@@ -23,6 +23,5 @@
 // @Filename: /src/folder/4.ts
 ////const foo = require(`x//*4*/`);
 
-const [r0, r1, r2, r3] = test.ranges();
 verify.completionsAt("1", ["y", "x"], { isNewIdentifierLocation: true });
 verify.completionsAt(["2", "3", "4"], ["bar", "foo"], { isNewIdentifierLocation: true });