Skip to content

Commit 15b8af7

Browse files
author
Andy Hanson
committed
Avoid a symlinked package globally importing itself (fixes another case of #26044)
1 parent d9b285c commit 15b8af7

File tree

4 files changed

+52
-8
lines changed

4 files changed

+52
-8
lines changed

src/compiler/checker.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3883,15 +3883,15 @@ namespace ts {
38833883
const links = getSymbolLinks(symbol);
38843884
let specifier = links.specifierCache && links.specifierCache.get(contextFile.path);
38853885
if (!specifier) {
3886-
specifier = flatten(moduleSpecifiers.getModuleSpecifiers(
3886+
specifier = moduleSpecifiers.getModuleSpecifierForDeclarationFile(
38873887
symbol,
38883888
compilerOptions,
38893889
contextFile,
38903890
context.tracker.moduleResolverHost,
38913891
context.tracker.moduleResolverHost.getSourceFiles!(), // TODO: GH#18217
38923892
{ importModuleSpecifierPreference: "non-relative" },
38933893
host.redirectTargetsMap,
3894-
))[0];
3894+
);
38953895
links.specifierCache = links.specifierCache || createMap();
38963896
links.specifierCache.set(contextFile.path, specifier);
38973897
}

src/compiler/moduleSpecifiers.ts

Lines changed: 27 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
// Used by importFixes to synthesize import module specifiers.
1+
// Used by importFixes, getEditsForFileRename, and declaration emit to synthesize import module specifiers.
22
/* @internal */
33
namespace ts.moduleSpecifiers {
44
export interface ModuleSpecifierPreferences {
@@ -17,10 +17,22 @@ namespace ts.moduleSpecifiers {
1717
redirectTargetsMap: RedirectTargetsMap,
1818
): string {
1919
const info = getInfo(compilerOptions, importingSourceFile, importingSourceFileName, host);
20-
const modulePaths = getAllModulePaths(files, toFileName, info.getCanonicalFileName, host, redirectTargetsMap);
20+
const modulePaths = getAllModulePaths(files, importingSourceFileName, toFileName, info.getCanonicalFileName, host, redirectTargetsMap);
2121
return firstDefined(modulePaths, moduleFileName => getGlobalModuleSpecifier(moduleFileName, info, host, compilerOptions)) ||
2222
first(getLocalModuleSpecifiers(toFileName, info, compilerOptions, preferences));
2323
}
24+
25+
export function getModuleSpecifierForDeclarationFile(
26+
moduleSymbol: Symbol,
27+
compilerOptions: CompilerOptions,
28+
importingSourceFile: SourceFile,
29+
host: ModuleSpecifierResolutionHost,
30+
files: ReadonlyArray<SourceFile>,
31+
preferences: ModuleSpecifierPreferences,
32+
redirectTargetsMap: RedirectTargetsMap,
33+
): string {
34+
return first(first(getModuleSpecifiers(moduleSymbol, compilerOptions, importingSourceFile, host, files, preferences, redirectTargetsMap)));
35+
}
2436

2537
// For each symlink/original for a module, returns a list of ways to import that file.
2638
export function getModuleSpecifiers(
@@ -40,7 +52,7 @@ namespace ts.moduleSpecifiers {
4052
return Debug.fail("Files list must be present to resolve symlinks in specifier resolution");
4153
}
4254
const moduleSourceFile = getSourceFileOfNode(moduleSymbol.valueDeclaration);
43-
const modulePaths = getAllModulePaths(files, moduleSourceFile.fileName, info.getCanonicalFileName, host, redirectTargetsMap);
55+
const modulePaths = getAllModulePaths(files, importingSourceFile.path, moduleSourceFile.fileName, info.getCanonicalFileName, host, redirectTargetsMap);
4456

4557
const global = mapDefined(modulePaths, moduleFileName => getGlobalModuleSpecifier(moduleFileName, info, host, compilerOptions));
4658
return global.length ? global.map(g => [g]) : modulePaths.map(moduleFileName =>
@@ -149,11 +161,16 @@ namespace ts.moduleSpecifiers {
149161
return getCanonicalFileName(a) === getCanonicalFileName(b);
150162
}
151163

164+
// KLUDGE: Don't assume one 'node_modules' links to another. More likely a single directory inside the node_modules is the symlink.
165+
// ALso, don't assume that an `@foo` directory is linked. More likely the contents of that are linked.
166+
function isNodeModulesOrScopedPackageDirectory(s: string): boolean {
167+
return s === "node_modules" || startsWith(s, "@");
168+
}
169+
152170
function guessDirectorySymlink(a: string, b: string, cwd: string, getCanonicalFileName: GetCanonicalFileName): [string, string] {
153171
const aParts = getPathComponents(toPath(a, cwd, getCanonicalFileName));
154172
const bParts = getPathComponents(toPath(b, cwd, getCanonicalFileName));
155-
// KLUDGE: Don't assume one 'node_modules' links to another. More likely a single directory inside the node_modules is the symlink.
156-
while ((aParts[aParts.length - 2]) !== "node_modules" && bParts[bParts.length - 2] !== "node_modules" && stringsEqual(aParts[aParts.length - 1], bParts[bParts.length - 1], getCanonicalFileName)) {
173+
while (!isNodeModulesOrScopedPackageDirectory(aParts[aParts.length - 2]) && !isNodeModulesOrScopedPackageDirectory(bParts[bParts.length - 2]) && stringsEqual(aParts[aParts.length - 1], bParts[bParts.length - 1], getCanonicalFileName)) {
157174
aParts.pop();
158175
bParts.pop();
159176
}
@@ -176,7 +193,7 @@ namespace ts.moduleSpecifiers {
176193
* Looks for existing imports that use symlinks to this module.
177194
* Symlinks will be returned first so they are preferred over the real path.
178195
*/
179-
function getAllModulePaths(files: ReadonlyArray<SourceFile>, importedFileName: string, getCanonicalFileName: (file: string) => string, host: ModuleSpecifierResolutionHost, redirectTargetsMap: RedirectTargetsMap): ReadonlyArray<string> {
196+
function getAllModulePaths(files: ReadonlyArray<SourceFile>, importingFileName: string, importedFileName: string, getCanonicalFileName: GetCanonicalFileName, host: ModuleSpecifierResolutionHost, redirectTargetsMap: RedirectTargetsMap): ReadonlyArray<string> {
180197
const redirects = redirectTargetsMap.get(importedFileName);
181198
const importedFileNames = redirects ? [...redirects, importedFileName] : [importedFileName];
182199
const cwd = host.getCurrentDirectory ? host.getCurrentDirectory() : "";
@@ -186,6 +203,10 @@ namespace ts.moduleSpecifiers {
186203
const result: string[] = [];
187204
const compareStrings = (!host.useCaseSensitiveFileNames || host.useCaseSensitiveFileNames()) ? compareStringsCaseSensitive : compareStringsCaseInsensitive;
188205
links.forEach((resolved, path) => {
206+
if (startsWithDirectory(importingFileName, resolved, getCanonicalFileName)) {
207+
return; // Don't want to a package to globally import from itself
208+
}
209+
189210
const target = targets.find(t => compareStrings(t.slice(0, resolved.length + 1), resolved + "/") === Comparison.EqualTo);
190211
if (target === undefined) return;
191212

src/compiler/utilities.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7157,6 +7157,12 @@ namespace ts {
71577157
return path.slice(0, Math.max(rootLength, path.lastIndexOf(directorySeparator)));
71587158
}
71597159

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+
71607166
export function isUrl(path: string) {
71617167
return getEncodedRootLength(path) < 0;
71627168
}
Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
/// <reference path="fourslash.ts" />
2+
3+
// @Filename: /packages/a/test.ts
4+
// @Symlink: /node_modules/a/test.ts
5+
////x;
6+
7+
// @Filename: /packages/a/utils.ts
8+
// @Symlink: /node_modules/a/utils.ts
9+
////import {} from "a/utils";
10+
////export const x = 0;
11+
12+
goTo.file("/packages/a/test.ts");
13+
verify.importFixAtPosition([
14+
`import { x } from "./utils";
15+
16+
x;`,
17+
]);

0 commit comments

Comments
 (0)