When the imported module is through node_modules and symlink to folder that isnt node_modules (#37387)

* Add tests that fail because of symlink to non common directory node_modules

* When the imported module is through node_modules and symlink to folder that isnt node_modules
Most of the monorepo like scenarios are like this so looking at symlink to decide if file can be imported is essential
Fixes #28689

src/compiler/moduleSpecifiers.ts

@@ -178,40 +178,70 @@ namespace ts.moduleSpecifiers {
         );
     }
 
+    export function forEachFileNameOfModule<T>(
+        files: readonly SourceFile[],
+        importingFileName: string,
+        importedFileName: string,
+        getCanonicalFileName: GetCanonicalFileName,
+        host: ModuleSpecifierResolutionHost,
+        redirectTargetsMap: RedirectTargetsMap,
+        preferSymlinks: boolean,
+        cb: (fileName: string) => T | undefined
+    ): T | undefined {
+        const redirects = redirectTargetsMap.get(importedFileName);
+        const importedFileNames = redirects ? [...redirects, importedFileName] : [importedFileName];
+        const cwd = host.getCurrentDirectory ? host.getCurrentDirectory() : "";
+        const targets = importedFileNames.map(f => getNormalizedAbsolutePath(f, cwd));
+        if (!preferSymlinks) {
+            const result = forEach(targets, cb);
+            if (result) return result;
+        }
+        const links = host.getProbableSymlinks
+            ? host.getProbableSymlinks(files)
+            : discoverProbableSymlinks(files, getCanonicalFileName, cwd);
+
+        const compareStrings = (!host.useCaseSensitiveFileNames || host.useCaseSensitiveFileNames()) ? compareStringsCaseSensitive : compareStringsCaseInsensitive;
+        const result = forEachEntry(links, (resolved, path) => {
+            if (startsWithDirectory(importingFileName, resolved, getCanonicalFileName)) {
+                return undefined; // Don't want to a package to globally import from itself
+            }
+
+            const target = find(targets, t => compareStrings(t.slice(0, resolved.length + 1), resolved + "/") === Comparison.EqualTo);
+            if (target === undefined) return undefined;
+
+            const relative = getRelativePathFromDirectory(resolved, target, getCanonicalFileName);
+            const option = resolvePath(path, relative);
+            if (!host.fileExists || host.fileExists(option)) {
+                const result = cb(option);
+                if (result) return result;
+            }
+        });
+        return result ||
+            (preferSymlinks ? forEach(targets, cb) : undefined);
+    }
+
     /**
      * Looks for existing imports that use symlinks to this module.
      * Symlinks will be returned first so they are preferred over the real path.
      */
     function getAllModulePaths(files: readonly SourceFile[], importingFileName: string, importedFileName: string, getCanonicalFileName: GetCanonicalFileName, host: ModuleSpecifierResolutionHost, redirectTargetsMap: RedirectTargetsMap): readonly string[] {
-        const redirects = redirectTargetsMap.get(importedFileName);
-        const importedFileNames = redirects ? [...redirects, importedFileName] : [importedFileName];
         const cwd = host.getCurrentDirectory ? host.getCurrentDirectory() : "";
-        const targets = importedFileNames.map(f => getNormalizedAbsolutePath(f, cwd));
-        const links = host.getProbableSymlinks
-            ? host.getProbableSymlinks(files)
-            : 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 allFileNames = createMap<string>();
+        forEachFileNameOfModule(
+            files,
+            importingFileName,
+            importedFileName,
+            getCanonicalFileName,
+            host,
+            redirectTargetsMap,
+            /*preferSymlinks*/ true,
+            path => {
+                // dont return value, so we collect everything
+                allFileNames.set(path, getCanonicalFileName(path));
             }
-
-            const target = find(targets, 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);
-        if (result.length < 2) return result;
+        );
 
         // Sort by paths closest to importing file Name directory
-        const allFileNames = arrayToMap(result, identity, getCanonicalFileName);
         const sortedPaths: string[] = [];
         for (
             let directory = getDirectoryPath(toPath(importingFileName, cwd, getCanonicalFileName));

src/harness/fourslashImpl.ts

@@ -26,6 +26,8 @@ namespace FourSlash {
 
         files: FourSlashFile[];
 
+        symlinks: vfs.FileSet | undefined;
+
         // A mapping from marker names to name/position pairs
         markerPositions: ts.Map<Marker>;
 
@@ -342,6 +344,10 @@ namespace FourSlash {
                 });
             }
 
+            if (testData.symlinks) {
+                this.languageServiceAdapterHost.vfs.apply(testData.symlinks);
+            }
+
             this.formatCodeSettings = ts.testFormatSettings;
 
             // Open the first file by default
@@ -3767,6 +3773,7 @@ namespace FourSlash {
         const files: FourSlashFile[] = [];
         // Global options
         const globalOptions: { [s: string]: string; } = {};
+        let symlinks: vfs.FileSet | undefined;
         // Marker positions
 
         // Split up the input file by line
@@ -3815,32 +3822,38 @@ namespace FourSlash {
                 throw new Error("Three-slash line in the middle of four-slash region at line " + i);
             }
             else if (line.substr(0, 2) === "//") {
-                // Comment line, check for global/file @options and record them
-                const match = optionRegex.exec(line.substr(2));
-                if (match) {
-                    const key = match[1].toLowerCase();
-                    const value = match[2];
-                    if (!ts.contains(fileMetadataNames, key)) {
-                        // Check if the match is already existed in the global options
-                        if (globalOptions[key] !== undefined) {
-                            throw new Error(`Global option '${key}' already exists`);
+                const possiblySymlinks = Harness.TestCaseParser.parseSymlinkFromTest(line, symlinks);
+                if (possiblySymlinks) {
+                    symlinks = possiblySymlinks;
+                }
+                else {
+                    // Comment line, check for global/file @options and record them
+                    const match = optionRegex.exec(line.substr(2));
+                    if (match) {
+                        const key = match[1].toLowerCase();
+                        const value = match[2];
+                        if (!ts.contains(fileMetadataNames, key)) {
+                            // Check if the match is already existed in the global options
+                            if (globalOptions[key] !== undefined) {
+                                throw new Error(`Global option '${key}' already exists`);
+                            }
+                            globalOptions[key] = value;
                         }
-                        globalOptions[key] = value;
-                    }
-                    else {
-                        switch (key) {
-                            case MetadataOptionNames.fileName:
-                                // Found an @FileName directive, if this is not the first then create a new subfile
-                                nextFile();
-                                currentFileName = ts.isRootedDiskPath(value) ? value : basePath + "/" + value;
-                                currentFileOptions[key] = value;
-                                break;
-                            case MetadataOptionNames.symlink:
-                                currentFileSymlinks = ts.append(currentFileSymlinks, value);
-                                break;
-                            default:
-                                // Add other fileMetadata flag
-                                currentFileOptions[key] = value;
+                        else {
+                            switch (key) {
+                                case MetadataOptionNames.fileName:
+                                    // Found an @FileName directive, if this is not the first then create a new subfile
+                                    nextFile();
+                                    currentFileName = ts.isRootedDiskPath(value) ? value : basePath + "/" + value;
+                                    currentFileOptions[key] = value;
+                                    break;
+                                case MetadataOptionNames.symlink:
+                                    currentFileSymlinks = ts.append(currentFileSymlinks, value);
+                                    break;
+                                default:
+                                    // Add other fileMetadata flag
+                                    currentFileOptions[key] = value;
+                            }
                         }
                     }
                 }
@@ -3870,6 +3883,7 @@ namespace FourSlash {
             markers,
             globalOptions,
             files,
+            symlinks,
             ranges
         };
     }

src/harness/harnessIO.ts

@@ -1128,6 +1128,16 @@ namespace Harness {
         const optionRegex = /^[\/]{2}\s*@(\w+)\s*:\s*([^\r\n]*)/gm;  // multiple matches on multiple lines
         const linkRegex = /^[\/]{2}\s*@link\s*:\s*([^\r\n]*)\s*->\s*([^\r\n]*)/gm;  // multiple matches on multiple lines
 
+        export function parseSymlinkFromTest(line: string, symlinks: vfs.FileSet | undefined) {
+            const linkMetaData = linkRegex.exec(line);
+            linkRegex.lastIndex = 0;
+            if (!linkMetaData) return undefined;
+
+            if (!symlinks) symlinks = {};
+            symlinks[linkMetaData[2].trim()] = new vfs.Symlink(linkMetaData[1].trim());
+            return symlinks;
+        }
+
         export function extractCompilerSettings(content: string): CompilerSettings {
             const opts: CompilerSettings = {};
 
@@ -1163,11 +1173,9 @@ namespace Harness {
 
             for (const line of lines) {
                 let testMetaData: RegExpExecArray | null;
-                const linkMetaData = linkRegex.exec(line);
-                linkRegex.lastIndex = 0;
-                if (linkMetaData) {
-                    if (!symlinks) symlinks = {};
-                    symlinks[linkMetaData[2].trim()] = new vfs.Symlink(linkMetaData[1].trim());
+                const possiblySymlinks = parseSymlinkFromTest(line, symlinks);
+                if (possiblySymlinks) {
+                    symlinks = possiblySymlinks;
                 }
                 else if (testMetaData = optionRegex.exec(line)) {
                     // Comment line, check for global/file @options and record them

src/services/codefixes/importFixes.ts

@@ -786,9 +786,9 @@ namespace ts.codefix {
         cb: (module: Symbol) => void,
     ) {
         let filteredCount = 0;
-        const packageJson = filterByPackageJson && createAutoImportFilter(from, program, host);
+        const moduleSpecifierResolutionHost = createModuleSpecifierResolutionHost(program, host);
+        const packageJson = filterByPackageJson && createAutoImportFilter(from, program, host, moduleSpecifierResolutionHost);
         const allSourceFiles = program.getSourceFiles();
-        const globalTypingsCache = host.getGlobalTypingsCacheLocation && host.getGlobalTypingsCacheLocation();
         forEachExternalModule(program.getTypeChecker(), allSourceFiles, (module, sourceFile) => {
             if (sourceFile === undefined) {
                 if (!packageJson || packageJson.allowsImportingAmbientModule(module, allSourceFiles)) {
@@ -800,7 +800,7 @@ namespace ts.codefix {
             }
             else if (sourceFile &&
                 sourceFile !== from &&
-                isImportablePath(from.fileName, sourceFile.fileName, hostGetCanonicalFileName(host), globalTypingsCache)
+                isImportableFile(program, from, sourceFile, moduleSpecifierResolutionHost)
             ) {
                 if (!packageJson || packageJson.allowsImportingSourceFile(sourceFile, allSourceFiles)) {
                     cb(module);
@@ -826,6 +826,32 @@ namespace ts.codefix {
         }
     }
 
+    function isImportableFile(
+        program: Program,
+        from: SourceFile,
+        to: SourceFile,
+        moduleSpecifierResolutionHost: ModuleSpecifierResolutionHost
+    ) {
+        const getCanonicalFileName = hostGetCanonicalFileName(moduleSpecifierResolutionHost);
+        const globalTypingsCache = moduleSpecifierResolutionHost.getGlobalTypingsCacheLocation?.();
+        return !!moduleSpecifiers.forEachFileNameOfModule(
+            program.getSourceFiles(),
+            from.fileName,
+            to.fileName,
+            hostGetCanonicalFileName(moduleSpecifierResolutionHost),
+            moduleSpecifierResolutionHost,
+            program.redirectTargetsMap,
+            /*preferSymlinks*/ false,
+            toPath => {
+                const toFile = program.getSourceFile(toPath);
+                // Determine to import using toPath only if toPath is what we were looking at
+                // or there doesnt exist the file in the program by the symlink
+                return (toFile === to || !toFile) &&
+                    isImportablePath(from.fileName, toPath, getCanonicalFileName, globalTypingsCache);
+            }
+        );
+    }
+
     /**
      * Don't include something from a `node_modules` that isn't actually reachable by a global import.
      * A relative import to node_modules is usually a bad idea.
@@ -870,12 +896,10 @@ namespace ts.codefix {
         return !isStringANonContextualKeyword(res) ? res || "_" : `_${res}`;
     }
 
-    function createAutoImportFilter(fromFile: SourceFile, program: Program, host: LanguageServiceHost) {
-        const packageJsons = host.getPackageJsonsVisibleToFile && host.getPackageJsonsVisibleToFile(fromFile.fileName) || getPackageJsonsVisibleToFile(fromFile.fileName, host);
-        const dependencyGroups = PackageJsonDependencyGroup.Dependencies | PackageJsonDependencyGroup.DevDependencies | PackageJsonDependencyGroup.OptionalDependencies;
+    function createModuleSpecifierResolutionHost(program: Program, host: LanguageServiceHost): ModuleSpecifierResolutionHost {
         // Mix in `getProbablySymlinks` from Program when host doesn't have it
         // in order for non-Project hosts to have a symlinks cache.
-        const moduleSpecifierResolutionHost: ModuleSpecifierResolutionHost = {
+        return {
             directoryExists: maybeBind(host, host.directoryExists),
             fileExists: maybeBind(host, host.fileExists),
             getCurrentDirectory: maybeBind(host, host.getCurrentDirectory),
@@ -884,9 +908,14 @@ namespace ts.codefix {
             getProbableSymlinks: maybeBind(host, host.getProbableSymlinks) || program.getProbableSymlinks,
             getGlobalTypingsCacheLocation: maybeBind(host, host.getGlobalTypingsCacheLocation),
         };
+    }
+
+    function createAutoImportFilter(fromFile: SourceFile, program: Program, host: LanguageServiceHost, moduleSpecifierResolutionHost = createModuleSpecifierResolutionHost(program, host)) {
+        const packageJsons = host.getPackageJsonsVisibleToFile && host.getPackageJsonsVisibleToFile(fromFile.fileName) || getPackageJsonsVisibleToFile(fromFile.fileName, host);
+        const dependencyGroups = PackageJsonDependencyGroup.Dependencies | PackageJsonDependencyGroup.DevDependencies | PackageJsonDependencyGroup.OptionalDependencies;
 
         let usesNodeCoreModules: boolean | undefined;
-        return { allowsImportingAmbientModule, allowsImportingSourceFile, allowsImportingSpecifier };
+        return { allowsImportingAmbientModule, allowsImportingSourceFile, allowsImportingSpecifier, moduleSpecifierResolutionHost };
 
         function moduleSpecifierIsCoveredByPackageJson(specifier: string) {
             const packageName = getNodeModuleRootSpecifier(specifier);

tests/cases/fourslash/importFixesWithSymlinkInSiblingRushPnpm.ts

@@ -0,0 +1,49 @@
+/// <reference path="fourslash.ts" />
+// @experimentalDecorators: true
+
+// @Filename: /project/libraries/dtos/tsconfig.json
+// { }
+
+// @Filename: /project/libraries/dtos/src/book.entity.ts
+////@Entity()
+////export class BookEntity {
+////    id: number
+////}
+
+// @Filename: /project/libraries/dtos/src/user.entity.ts
+////import { Entity } from "mikro-orm"
+////@Entity()
+////export class UserEntity {
+////    id: number
+////}
+
+// @Filename: /project/common/temp/node_modules/.registry.npmjs.org/mikro-orm/3.4.1_pg@7.18.2/node_modules/mikro-orm/package.json
+////{ "name": "mikro-orm", "version": "3.4.1", "typings": "dist/index.d.ts" }
+
+// @Filename: /project/common/temp/node_modules/.registry.npmjs.org/mikro-orm/3.4.1_pg@7.18.2/node_modules/mikro-orm/dist/index.d.ts
+////export * from "./decorators";
+
+// @Filename: /project/common/temp/node_modules/.registry.npmjs.org/mikro-orm/3.4.1_pg@7.18.2/node_modules/mikro-orm/dist/decorators/index.d.ts
+////export * from "./entity";
+
+// @Filename: /project/common/temp/node_modules/.registry.npmjs.org/mikro-orm/3.4.1_pg@7.18.2/node_modules/mikro-orm/dist/decorators/entity.d.ts
+////export declare function Entity(): Function;
+
+// @link: /project/common/temp/node_modules/.registry.npmjs.org/mikro-orm/3.4.1_pg@7.18.2/node_modules/mikro-orm -> /project/libraries/dtos/node_modules/mikro-orm
+
+
+goTo.file("/project/libraries/dtos/src/book.entity.ts");
+verify.importFixAtPosition([
+    getImportFixContent("mikro-orm"),
+    getImportFixContent("mikro-orm/dist/decorators"),
+    getImportFixContent("mikro-orm/dist/decorators/entity"),
+]);
+
+function getImportFixContent(from: string) {
+    return `import { Entity } from "${from}";
+
+@Entity()
+export class BookEntity {
+    id: number
+}`;
+}

tests/cases/fourslash/importFixesWithSymlinkInSiblingRushYarn.ts

@@ -0,0 +1,49 @@
+/// <reference path="fourslash.ts" />
+// @experimentalDecorators: true
+
+// @Filename: /project/libraries/dtos/tsconfig.json
+// { }
+
+// @Filename: /project/libraries/dtos/src/book.entity.ts
+////@Entity()
+////export class BookEntity {
+////    id: number
+////}
+
+// @Filename: /project/libraries/dtos/src/user.entity.ts
+////import { Entity } from "mikro-orm"
+////@Entity()
+////export class UserEntity {
+////    id: number
+////}
+
+// @Filename: /project/common/temp/node_modules/mikro-orm/package.json
+////{ "name": "mikro-orm", "version": "3.4.1", "typings": "dist/index.d.ts" }
+
+// @Filename: /project/common/temp/node_modules/mikro-orm/dist/index.d.ts
+////export * from "./decorators";
+
+// @Filename: /project/common/temp/node_modules/mikro-orm/dist/decorators/index.d.ts
+////export * from "./entity";
+
+// @Filename: /project/common/temp/node_modules/mikro-orm/dist/decorators/entity.d.ts
+////export declare function Entity(): Function;
+
+// @link: /project/common/temp/node_modules/mikro-orm -> /project/libraries/dtos/node_modules/mikro-orm
+
+
+goTo.file("/project/libraries/dtos/src/book.entity.ts");
+verify.importFixAtPosition([
+    getImportFixContent("mikro-orm"),
+    getImportFixContent("mikro-orm/dist/decorators"),
+    getImportFixContent("mikro-orm/dist/decorators/entity"),
+]);
+
+function getImportFixContent(from: string) {
+    return `import { Entity } from "${from}";
+
+@Entity()
+export class BookEntity {
+    id: number
+}`;
+}