importFixes: When one file redirects to another, consider both for global import specifiers (#25834)

Andy · web-flow · commit 9c9f3e3cf999 · 2018-07-31T17:28:56.000-07:00
* importFixes: When one file redirects to another, consider both for global import specifiers * Add test for #26044 * Avoid a symlinked package globally importing itself (fixes another case of #26044) * Compare to node_modules with getCanonicalFileName
diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts
@@ -3888,14 +3888,15 @@ namespace ts {
                     const links = getSymbolLinks(symbol);
                     let specifier = links.specifierCache && links.specifierCache.get(contextFile.path);
                     if (!specifier) {
-                        specifier = flatten(moduleSpecifiers.getModuleSpecifiers(
+                        specifier = moduleSpecifiers.getModuleSpecifierForDeclarationFile(
                             symbol,
                             compilerOptions,
                             contextFile,
                             context.tracker.moduleResolverHost,
                             context.tracker.moduleResolverHost.getSourceFiles!(), // TODO: GH#18217
-                            { importModuleSpecifierPreference: "non-relative" }
-                        ))[0];
+                            { importModuleSpecifierPreference: "non-relative" },
+                            host.redirectTargetsMap,
+                        );
                         links.specifierCache = links.specifierCache || createMap();
                         links.specifierCache.set(contextFile.path, specifier);
                     }
diff --git a/src/compiler/moduleSpecifiers.ts b/src/compiler/moduleSpecifiers.ts
@@ -1,4 +1,4 @@
-// Used by importFixes to synthesize import module specifiers.
+// Used by importFixes, getEditsForFileRename, and declaration emit to synthesize import module specifiers.
 /* @internal */
 namespace ts.moduleSpecifiers {
     export interface ModuleSpecifierPreferences {
@@ -14,13 +14,26 @@ namespace ts.moduleSpecifiers {
         host: ModuleSpecifierResolutionHost,
         files: ReadonlyArray<SourceFile>,
         preferences: ModuleSpecifierPreferences = {},
+        redirectTargetsMap: RedirectTargetsMap,
     ): string {
         const info = getInfo(compilerOptions, importingSourceFile, importingSourceFileName, host);
-        const modulePaths = getAllModulePaths(files, toFileName, info.getCanonicalFileName, host);
+        const modulePaths = getAllModulePaths(files, importingSourceFileName, toFileName, info.getCanonicalFileName, host, redirectTargetsMap);
         return firstDefined(modulePaths, moduleFileName => getGlobalModuleSpecifier(moduleFileName, info, host, compilerOptions)) ||
             first(getLocalModuleSpecifiers(toFileName, info, compilerOptions, preferences));
     }
 
+    export function getModuleSpecifierForDeclarationFile(
+        moduleSymbol: Symbol,
+        compilerOptions: CompilerOptions,
+        importingSourceFile: SourceFile,
+        host: ModuleSpecifierResolutionHost,
+        files: ReadonlyArray<SourceFile>,
+        preferences: ModuleSpecifierPreferences,
+        redirectTargetsMap: RedirectTargetsMap,
+    ): string {
+        return first(first(getModuleSpecifiers(moduleSymbol, compilerOptions, importingSourceFile, host, files, preferences, redirectTargetsMap)));
+    }
+
     // For each symlink/original for a module, returns a list of ways to import that file.
     export function getModuleSpecifiers(
         moduleSymbol: Symbol,
@@ -29,6 +42,7 @@ namespace ts.moduleSpecifiers {
         host: ModuleSpecifierResolutionHost,
         files: ReadonlyArray<SourceFile>,
         preferences: ModuleSpecifierPreferences,
+        redirectTargetsMap: RedirectTargetsMap,
     ): ReadonlyArray<ReadonlyArray<string>> {
         const ambient = tryGetModuleNameFromAmbientModule(moduleSymbol);
         if (ambient) return [[ambient]];
@@ -37,7 +51,8 @@ namespace ts.moduleSpecifiers {
         if (!files) {
             return Debug.fail("Files list must be present to resolve symlinks in specifier resolution");
         }
-        const modulePaths = getAllModulePaths(files, getSourceFileOfNode(moduleSymbol.valueDeclaration).fileName, info.getCanonicalFileName, host);
+        const moduleSourceFile = getSourceFileOfNode(moduleSymbol.valueDeclaration);
+        const modulePaths = getAllModulePaths(files, importingSourceFile.path, moduleSourceFile.fileName, info.getCanonicalFileName, host, redirectTargetsMap);
 
         const global = mapDefined(modulePaths, moduleFileName => getGlobalModuleSpecifier(moduleFileName, info, host, compilerOptions));
         return global.length ? global.map(g => [g]) : modulePaths.map(moduleFileName =>
@@ -142,64 +157,69 @@ namespace ts.moduleSpecifiers {
         return firstDefined(imports, ({ text }) => pathIsRelative(text) ? fileExtensionIs(text, Extension.Js) : undefined) || false;
     }
 
-    function discoverProbableSymlinks(files: ReadonlyArray<SourceFile>, getCanonicalFileName: (file: string) => string, host: ModuleSpecifierResolutionHost) {
+    function stringsEqual(a: string, b: string, getCanonicalFileName: GetCanonicalFileName): boolean {
+        return getCanonicalFileName(a) === getCanonicalFileName(b);
+    }
+
+    // KLUDGE: Don't assume one 'node_modules' links to another. More likely a single directory inside the node_modules is the symlink.
+    // ALso, don't assume that an `@foo` directory is linked. More likely the contents of that are linked.
+    function isNodeModulesOrScopedPackageDirectory(s: string, getCanonicalFileName: GetCanonicalFileName): boolean {
+        return getCanonicalFileName(s) === "node_modules" || startsWith(s, "@");
+    }
+
+    function guessDirectorySymlink(a: string, b: string, cwd: string, getCanonicalFileName: GetCanonicalFileName): [string, string] {
+        const aParts = getPathComponents(toPath(a, cwd, getCanonicalFileName));
+        const bParts = getPathComponents(toPath(b, cwd, getCanonicalFileName));
+        while (!isNodeModulesOrScopedPackageDirectory(aParts[aParts.length - 2], getCanonicalFileName) &&
+            !isNodeModulesOrScopedPackageDirectory(bParts[bParts.length - 2], getCanonicalFileName) &&
+            stringsEqual(aParts[aParts.length - 1], bParts[bParts.length - 1], getCanonicalFileName)) {
+            aParts.pop();
+            bParts.pop();
+        }
+        return [getPathFromPathComponents(aParts), getPathFromPathComponents(bParts)];
+    }
+
+    function discoverProbableSymlinks(files: ReadonlyArray<SourceFile>, getCanonicalFileName: GetCanonicalFileName, cwd: string): ReadonlyMap<string> {
+        const result = createMap<string>();
         const symlinks = mapDefined(files, sf =>
             sf.resolvedModules && firstDefinedIterator(sf.resolvedModules.values(), res =>
                 res && res.originalPath && res.resolvedFileName !== res.originalPath ? [res.resolvedFileName, res.originalPath] : undefined));
-        const result = createMap<string>();
-        if (symlinks) {
-            const currentDirectory = host.getCurrentDirectory ? host.getCurrentDirectory() : "";
-            const compareStrings = (!host.useCaseSensitiveFileNames || host.useCaseSensitiveFileNames()) ? compareStringsCaseSensitive : compareStringsCaseInsensitive;
-            for (const [resolvedPath, originalPath] of symlinks) {
-                const resolvedParts = getPathComponents(toPath(resolvedPath, currentDirectory, getCanonicalFileName));
-                const originalParts = getPathComponents(toPath(originalPath, currentDirectory, getCanonicalFileName));
-                while (compareStrings(resolvedParts[resolvedParts.length - 1], originalParts[originalParts.length - 1]) === Comparison.EqualTo) {
-                    resolvedParts.pop();
-                    originalParts.pop();
-                }
-                result.set(getPathFromPathComponents(originalParts), getPathFromPathComponents(resolvedParts));
-            }
+        for (const [resolvedPath, originalPath] of symlinks) {
+            const [commonResolved, commonOriginal] = guessDirectorySymlink(resolvedPath, originalPath, cwd, getCanonicalFileName);
+            result.set(commonOriginal, commonResolved);
         }
         return result;
     }
 
-    function getAllModulePathsUsingIndirectSymlinks(files: ReadonlyArray<SourceFile>, target: string, getCanonicalFileName: (file: string) => string, host: ModuleSpecifierResolutionHost) {
-        const links = discoverProbableSymlinks(files, getCanonicalFileName, host);
-        const paths = arrayFrom(links.keys());
-        let options: string[] | undefined;
-        const compareStrings = (!host.useCaseSensitiveFileNames || host.useCaseSensitiveFileNames()) ? compareStringsCaseSensitive : compareStringsCaseInsensitive;
-        for (const path of paths) {
-            const resolved = links.get(path)!;
-            if (compareStrings(target.slice(0, resolved.length + 1), resolved + "/") === Comparison.EqualTo) {
-                const relative = getRelativePathFromDirectory(resolved, target, getCanonicalFileName);
-                const option = resolvePath(path, relative);
-                if (!host.fileExists || host.fileExists(option)) {
-                    if (!options) options = [];
-                    options.push(option);
-                }
-            }
-        }
-        if (options) {
-            options.push(target); // Since these are speculative, we also include the original resolved name as a possibility
-            return options;
-        }
-        return [target];
-    }
-
     /**
      * Looks for existing imports that use symlinks to this module.
-     * Only if no symlink is available, the real path will be used.
+     * Symlinks will be returned first so they are preferred over the real path.
      */
-    function getAllModulePaths(files: ReadonlyArray<SourceFile>, importedFileName: string, getCanonicalFileName: (file: string) => string, host: ModuleSpecifierResolutionHost): ReadonlyArray<string> {
-        const symlinks = mapDefined(files, sf =>
-            sf.resolvedModules && firstDefinedIterator(sf.resolvedModules.values(), res =>
-                res && res.resolvedFileName === importedFileName ? res.originalPath : undefined));
+    function getAllModulePaths(files: ReadonlyArray<SourceFile>, importingFileName: string, importedFileName: string, getCanonicalFileName: GetCanonicalFileName, host: ModuleSpecifierResolutionHost, redirectTargetsMap: RedirectTargetsMap): ReadonlyArray<string> {
+        const redirects = redirectTargetsMap.get(importedFileName);
+        const importedFileNames = redirects ? [...redirects, importedFileName] : [importedFileName];
         const cwd = host.getCurrentDirectory ? host.getCurrentDirectory() : "";
-        const baseOptions = getAllModulePathsUsingIndirectSymlinks(files, getNormalizedAbsolutePath(importedFileName, cwd), getCanonicalFileName, host);
-        if (symlinks.length === 0) {
-            return baseOptions;
-        }
-        return deduplicate(concatenate(baseOptions, flatMap(symlinks, importedFileName => getAllModulePathsUsingIndirectSymlinks(files, getNormalizedAbsolutePath(importedFileName, cwd), getCanonicalFileName, host))));
+        const targets = importedFileNames.map(f => getNormalizedAbsolutePath(f, cwd));
+        const links = discoverProbableSymlinks(files, getCanonicalFileName, cwd);
+
+        const result: string[] = [];
+        const compareStrings = (!host.useCaseSensitiveFileNames || host.useCaseSensitiveFileNames()) ? compareStringsCaseSensitive : compareStringsCaseInsensitive;
+        links.forEach((resolved, path) => {
+            if (startsWithDirectory(importingFileName, resolved, getCanonicalFileName)) {
+                return; // Don't want to a package to globally import from itself
+            }
+
+            const target = targets.find(t => compareStrings(t.slice(0, resolved.length + 1), resolved + "/") === Comparison.EqualTo);
+            if (target === undefined) return;
+
+            const relative = getRelativePathFromDirectory(resolved, target, getCanonicalFileName);
+            const option = resolvePath(path, relative);
+            if (!host.fileExists || host.fileExists(option)) {
+                result.push(option);
+            }
+        });
+        result.push(...targets);
+        return result;
     }
 
     function getRelativePathNParents(relativePath: string): number {
diff --git a/src/compiler/program.ts b/src/compiler/program.ts
@@ -634,7 +634,8 @@ namespace ts {
         const packageIdToSourceFile = createMap<SourceFile>();
         // Maps from a SourceFile's `.path` to the name of the package it was imported with.
         let sourceFileToPackageName = createMap<string>();
-        let redirectTargetsSet = createMap<true>();
+        // Key is a file name. Value is the (non-empty, or undefined) list of files that redirect to it.
+        let redirectTargetsMap = createMultiMap<string>();
 
         const filesByName = createMap<SourceFile | undefined>();
         let missingFilePaths: ReadonlyArray<Path> | undefined;
@@ -752,7 +753,7 @@ namespace ts {
             getSourceFileFromReference,
             getLibFileFromReference,
             sourceFileToPackageName,
-            redirectTargetsSet,
+            redirectTargetsMap,
             isEmittedFile,
             getConfigFileParsingDiagnostics,
             getResolvedModuleWithFailedLookupLocationsFromCache,
@@ -1073,7 +1074,7 @@ namespace ts {
                     fileChanged = false;
                     newSourceFile = oldSourceFile; // Use the redirect.
                 }
-                else if (oldProgram.redirectTargetsSet.has(oldSourceFile.path)) {
+                else if (oldProgram.redirectTargetsMap.has(oldSourceFile.path)) {
                     // If a redirected-to source file changes, the redirect may be broken.
                     if (newSourceFile !== oldSourceFile) {
                         return oldProgram.structureIsReused = StructureIsReused.Not;
@@ -1220,7 +1221,7 @@ namespace ts {
             resolvedProjectReferences = oldProgram.getProjectReferences();
 
             sourceFileToPackageName = oldProgram.sourceFileToPackageName;
-            redirectTargetsSet = oldProgram.redirectTargetsSet;
+            redirectTargetsMap = oldProgram.redirectTargetsMap;
 
             return oldProgram.structureIsReused = StructureIsReused.Completely;
         }
@@ -2079,7 +2080,7 @@ namespace ts {
                     // Some other SourceFile already exists with this package name and version.
                     // Instead of creating a duplicate, just redirect to the existing one.
                     const dupFile = createRedirectSourceFile(fileFromPackageId, file!, fileName, path); // TODO: GH#18217
-                    redirectTargetsSet.set(fileFromPackageId.path, true);
+                    redirectTargetsMap.add(fileFromPackageId.path, fileName);
                     filesByName.set(path, dupFile);
                     sourceFileToPackageName.set(path, packageId.name);
                     processingOtherFiles!.push(dupFile);
diff --git a/src/compiler/types.ts b/src/compiler/types.ts
@@ -2551,7 +2551,7 @@ namespace ts {
         /**
          * If two source files are for the same version of the same package, one will redirect to the other.
          * (See `createRedirectSourceFile` in program.ts.)
-         * The redirect will have this set. The redirected-to source file will be in `redirectTargetsSet`.
+         * The redirect will have this set. The redirected-to source file will be in `redirectTargetsMap`.
          */
         /* @internal */ redirectInfo?: RedirectInfo;
 
@@ -2798,7 +2798,7 @@ namespace ts {
         /** Given a source file, get the name of the package it was imported from. */
         /* @internal */ sourceFileToPackageName: Map<string>;
         /** Set of all source files that some other source file redirects to. */
-        /* @internal */ redirectTargetsSet: Map<true>;
+        /* @internal */ redirectTargetsMap: MultiMap<string>;
         /** Is the file emitted file */
         /* @internal */ isEmittedFile(file: string): boolean;
 
@@ -2807,6 +2807,9 @@ namespace ts {
         getProjectReferences(): (ResolvedProjectReference | undefined)[] | undefined;
     }
 
+    /* @internal */
+    export type RedirectTargetsMap = ReadonlyMap<ReadonlyArray<string>>;
+
     export interface ResolvedProjectReference {
         commandLine: ParsedCommandLine;
         sourceFile: SourceFile;
@@ -2884,6 +2887,8 @@ namespace ts {
         getSourceFiles(): ReadonlyArray<SourceFile>;
         getSourceFile(fileName: string): SourceFile | undefined;
         getResolvedTypeReferenceDirectives(): ReadonlyMap<ResolvedTypeReferenceDirective>;
+
+        readonly redirectTargetsMap: RedirectTargetsMap;
     }
 
     export interface TypeChecker {
diff --git a/src/compiler/utilities.ts b/src/compiler/utilities.ts
@@ -7157,6 +7157,12 @@ namespace ts {
         return path.slice(0, Math.max(rootLength, path.lastIndexOf(directorySeparator)));
     }
 
+    export function startsWithDirectory(fileName: string, directoryName: string, getCanonicalFileName: GetCanonicalFileName): boolean {
+        const canonicalFileName = getCanonicalFileName(fileName);
+        const canonicalDirectoryName = getCanonicalFileName(directoryName);
+        return startsWith(canonicalFileName, canonicalDirectoryName + "/") || startsWith(canonicalFileName, canonicalDirectoryName + "\\");
+    }
+
     export function isUrl(path: string) {
         return getEncodedRootLength(path) < 0;
     }
diff --git a/src/services/codefixes/importFixes.ts b/src/services/codefixes/importFixes.ts
@@ -275,7 +275,7 @@ namespace ts.codefix {
     ): ReadonlyArray<FixAddNewImport | FixUseImportType> {
         const isJs = isSourceFileJavaScript(sourceFile);
         const choicesForEachExportingModule = flatMap<SymbolExportInfo, ReadonlyArray<FixAddNewImport | FixUseImportType>>(moduleSymbols, ({ moduleSymbol, importKind, exportedSymbolIsTypeOnly }) => {
-            const modulePathsGroups = moduleSpecifiers.getModuleSpecifiers(moduleSymbol, program.getCompilerOptions(), sourceFile, host, program.getSourceFiles(), preferences);
+            const modulePathsGroups = moduleSpecifiers.getModuleSpecifiers(moduleSymbol, program.getCompilerOptions(), sourceFile, host, program.getSourceFiles(), preferences, program.redirectTargetsMap);
             return modulePathsGroups.map(group => group.map((moduleSpecifier): FixAddNewImport | FixUseImportType =>
                 // `position` should only be undefined at a missing jsx namespace, in which case we shouldn't be looking for pure types.
                 exportedSymbolIsTypeOnly && isJs ? { kind: ImportFixKind.ImportType, moduleSpecifier, position: Debug.assertDefined(position) } : { kind: ImportFixKind.AddNew, moduleSpecifier, importKind }));
diff --git a/src/services/documentHighlights.ts b/src/services/documentHighlights.ts
@@ -28,7 +28,7 @@ namespace ts.DocumentHighlights {
         const map = arrayToMultiMap(referenceEntries.map(FindAllReferences.toHighlightSpan), e => e.fileName, e => e.span);
         return arrayFrom(map.entries(), ([fileName, highlightSpans]) => {
             if (!sourceFilesSet.has(fileName)) {
-                Debug.assert(program.redirectTargetsSet.has(fileName));
+                Debug.assert(program.redirectTargetsMap.has(fileName));
                 const redirectTarget = program.getSourceFile(fileName);
                 const redirect = find(sourceFilesToSearch, f => !!f.redirectInfo && f.redirectInfo.redirectTarget === redirectTarget)!;
                 fileName = redirect.fileName;
diff --git a/src/services/getEditsForFileRename.ts b/src/services/getEditsForFileRename.ts
@@ -156,7 +156,7 @@ namespace ts {
 
                     // Need an update if the imported file moved, or the importing file moved and was using a relative path.
                     return toImport !== undefined && (toImport.updated || (importingSourceFileMoved && pathIsRelative(importLiteral.text)))
-                        ? moduleSpecifiers.getModuleSpecifier(program.getCompilerOptions(), sourceFile, newImportFromPath, toImport.newFileName, host, allFiles, preferences)
+                        ? moduleSpecifiers.getModuleSpecifier(program.getCompilerOptions(), sourceFile, newImportFromPath, toImport.newFileName, host, allFiles, preferences, program.redirectTargetsMap)
                         : undefined;
                 });
         }
diff --git a/src/testRunner/unittests/tsserverProjectSystem.ts b/src/testRunner/unittests/tsserverProjectSystem.ts
diff --git a/tests/cases/fourslash/importNameCodeFix_symlink_own_package.ts b/tests/cases/fourslash/importNameCodeFix_symlink_own_package.ts
diff --git a/tests/cases/fourslash/importNameCodeFix_symlink_own_package_2.ts b/tests/cases/fourslash/importNameCodeFix_symlink_own_package_2.ts

Original file line number	Diff line number	Diff line change
`@@ -7157,6 +7157,12 @@ namespace ts {`
`7157`	`7157`	`return path.slice(0, Math.max(rootLength, path.lastIndexOf(directorySeparator)));`
`7158`	`7158`	`}`
`7159`	`7159`
	`7160`	`+ export function startsWithDirectory(fileName: string, directoryName: string, getCanonicalFileName: GetCanonicalFileName): boolean {`
	`7161`	`+ const canonicalFileName = getCanonicalFileName(fileName);`
	`7162`	`+ const canonicalDirectoryName = getCanonicalFileName(directoryName);`
	`7163`	`+ return startsWith(canonicalFileName, canonicalDirectoryName + "/") \|\| startsWith(canonicalFileName, canonicalDirectoryName + "\\");`
	`7164`	`+ }`
	`7165`	`+`
`7160`	`7166`	`export function isUrl(path: string) {`
`7161`	`7167`	`return getEncodedRootLength(path) < 0;`
`7162`	`7168`	`}`
Original file line number	Diff line number	Diff line change
`@@ -156,7 +156,7 @@ namespace ts {`
`156`	`156`
`157`	`157`	`// Need an update if the imported file moved, or the importing file moved and was using a relative path.`
`158`	`158`	`return toImport !== undefined && (toImport.updated \|\| (importingSourceFileMoved && pathIsRelative(importLiteral.text)))`
`159`		`- ? moduleSpecifiers.getModuleSpecifier(program.getCompilerOptions(), sourceFile, newImportFromPath, toImport.newFileName, host, allFiles, preferences)`
	`159`	`+ ? moduleSpecifiers.getModuleSpecifier(program.getCompilerOptions(), sourceFile, newImportFromPath, toImport.newFileName, host, allFiles, preferences, program.redirectTargetsMap)`
`160`	`160`	`: undefined;`
`161`	`161`	`});`
`162`	`162`	`}`