Skip to content

Commit 8ffb7f0

Browse files
authored
Reprioritize cross-project module specifier suggestions for auto-import (#40253)
* Add test * Suggest `paths` module specifiers even when a node_modules path was available * Fix some tests * Fix remaining tests * Add comments
1 parent db5f519 commit 8ffb7f0

11 files changed

+444
-44
lines changed

src/compiler/moduleSpecifiers.ts

Lines changed: 94 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ namespace ts.moduleSpecifiers {
6969
const info = getInfo(importingSourceFileName, host);
7070
const modulePaths = getAllModulePaths(importingSourceFileName, nodeModulesFileName, host);
7171
return firstDefined(modulePaths,
72-
moduleFileName => tryGetModuleNameAsNodeModule(moduleFileName, info, host, compilerOptions, /*packageNameOnly*/ true));
72+
modulePath => tryGetModuleNameAsNodeModule(modulePath, info, host, compilerOptions, /*packageNameOnly*/ true));
7373
}
7474

7575
function getModuleSpecifierWorker(
@@ -81,7 +81,7 @@ namespace ts.moduleSpecifiers {
8181
): string {
8282
const info = getInfo(importingSourceFileName, host);
8383
const modulePaths = getAllModulePaths(importingSourceFileName, toFileName, host);
84-
return firstDefined(modulePaths, moduleFileName => tryGetModuleNameAsNodeModule(moduleFileName, info, host, compilerOptions)) ||
84+
return firstDefined(modulePaths, modulePath => tryGetModuleNameAsNodeModule(modulePath, info, host, compilerOptions)) ||
8585
getLocalModuleSpecifier(toFileName, info, compilerOptions, preferences);
8686
}
8787

@@ -101,8 +101,48 @@ namespace ts.moduleSpecifiers {
101101
const modulePaths = getAllModulePaths(importingSourceFile.path, moduleSourceFile.originalFileName, host);
102102

103103
const preferences = getPreferences(userPreferences, compilerOptions, importingSourceFile);
104-
const global = mapDefined(modulePaths, moduleFileName => tryGetModuleNameAsNodeModule(moduleFileName, info, host, compilerOptions));
105-
return global.length ? global : modulePaths.map(moduleFileName => getLocalModuleSpecifier(moduleFileName, info, compilerOptions, preferences));
104+
const importedFileIsInNodeModules = some(modulePaths, p => p.isInNodeModules);
105+
106+
// Module specifier priority:
107+
// 1. "Bare package specifiers" (e.g. "@foo/bar") resulting from a path through node_modules to a package.json's "types" entry
108+
// 2. Specifiers generated using "paths" from tsconfig
109+
// 3. Non-relative specfiers resulting from a path through node_modules (e.g. "@foo/bar/path/to/file")
110+
// 4. Relative paths
111+
let nodeModulesSpecifiers: string[] | undefined;
112+
let pathsSpecifiers: string[] | undefined;
113+
let relativeSpecifiers: string[] | undefined;
114+
for (const modulePath of modulePaths) {
115+
const specifier = tryGetModuleNameAsNodeModule(modulePath, info, host, compilerOptions);
116+
nodeModulesSpecifiers = append(nodeModulesSpecifiers, specifier);
117+
if (specifier && modulePath.isRedirect) {
118+
// If we got a specifier for a redirect, it was a bare package specifier (e.g. "@foo/bar",
119+
// not "@foo/bar/path/to/file"). No other specifier will be this good, so stop looking.
120+
return nodeModulesSpecifiers!;
121+
}
122+
123+
if (!specifier && !modulePath.isRedirect) {
124+
const local = getLocalModuleSpecifier(modulePath.path, info, compilerOptions, preferences);
125+
if (pathIsBareSpecifier(local)) {
126+
pathsSpecifiers = append(pathsSpecifiers, local);
127+
}
128+
else if (!importedFileIsInNodeModules || modulePath.isInNodeModules) {
129+
// Why this extra conditional, not just an `else`? If some path to the file contained
130+
// 'node_modules', but we can't create a non-relative specifier (e.g. "@foo/bar/path/to/file"),
131+
// that means we had to go through a *sibling's* node_modules, not one we can access directly.
132+
// If some path to the file was in node_modules but another was not, this likely indicates that
133+
// we have a monorepo structure with symlinks. In this case, the non-node_modules path is
134+
// probably the realpath, e.g. "../bar/path/to/file", but a relative path to another package
135+
// in a monorepo is probably not portable. So, the module specifier we actually go with will be
136+
// the relative path through node_modules, so that the declaration emitter can produce a
137+
// portability error. (See declarationEmitReexportedSymlinkReference3)
138+
relativeSpecifiers = append(relativeSpecifiers, local);
139+
}
140+
}
141+
}
142+
143+
return pathsSpecifiers?.length ? pathsSpecifiers :
144+
nodeModulesSpecifiers?.length ? nodeModulesSpecifiers :
145+
Debug.checkDefined(relativeSpecifiers);
106146
}
107147

108148
interface Info {
@@ -161,10 +201,10 @@ namespace ts.moduleSpecifiers {
161201
return match ? match.length : 0;
162202
}
163203

164-
function comparePathsByNumberOfDirectorySeparators(a: string, b: string) {
165-
return compareValues(
166-
numberOfDirectorySeparators(a),
167-
numberOfDirectorySeparators(b)
204+
function comparePathsByRedirectAndNumberOfDirectorySeparators(a: ModulePath, b: ModulePath) {
205+
return compareBooleans(b.isRedirect, a.isRedirect) || compareValues(
206+
numberOfDirectorySeparators(a.path),
207+
numberOfDirectorySeparators(b.path)
168208
);
169209
}
170210

@@ -173,7 +213,7 @@ namespace ts.moduleSpecifiers {
173213
importedFileName: string,
174214
host: ModuleSpecifierResolutionHost,
175215
preferSymlinks: boolean,
176-
cb: (fileName: string) => T | undefined
216+
cb: (fileName: string, isRedirect: boolean) => T | undefined
177217
): T | undefined {
178218
const getCanonicalFileName = hostGetCanonicalFileName(host);
179219
const cwd = host.getCurrentDirectory();
@@ -182,7 +222,7 @@ namespace ts.moduleSpecifiers {
182222
const importedFileNames = [...(referenceRedirect ? [referenceRedirect] : emptyArray), importedFileName, ...redirects];
183223
const targets = importedFileNames.map(f => getNormalizedAbsolutePath(f, cwd));
184224
if (!preferSymlinks) {
185-
const result = forEach(targets, cb);
225+
const result = forEach(targets, p => cb(p, referenceRedirect === p));
186226
if (result) return result;
187227
}
188228
const links = host.getSymlinkCache
@@ -197,61 +237,68 @@ namespace ts.moduleSpecifiers {
197237
return undefined; // Don't want to a package to globally import from itself
198238
}
199239

200-
const target = find(targets, t => compareStrings(t.slice(0, resolved.real.length), resolved.real) === Comparison.EqualTo);
201-
if (target === undefined) return undefined;
240+
return forEach(targets, target => {
241+
if (compareStrings(target.slice(0, resolved.real.length), resolved.real) !== Comparison.EqualTo) {
242+
return;
243+
}
202244

203-
const relative = getRelativePathFromDirectory(resolved.real, target, getCanonicalFileName);
204-
const option = resolvePath(path, relative);
205-
if (!host.fileExists || host.fileExists(option)) {
206-
const result = cb(option);
207-
if (result) return result;
208-
}
245+
const relative = getRelativePathFromDirectory(resolved.real, target, getCanonicalFileName);
246+
const option = resolvePath(path, relative);
247+
if (!host.fileExists || host.fileExists(option)) {
248+
const result = cb(option, target === referenceRedirect);
249+
if (result) return result;
250+
}
251+
});
209252
});
210253
return result ||
211-
(preferSymlinks ? forEach(targets, cb) : undefined);
254+
(preferSymlinks ? forEach(targets, p => cb(p, p === referenceRedirect)) : undefined);
255+
}
256+
257+
interface ModulePath {
258+
path: string;
259+
isInNodeModules: boolean;
260+
isRedirect: boolean;
212261
}
213262

214263
/**
215264
* Looks for existing imports that use symlinks to this module.
216265
* Symlinks will be returned first so they are preferred over the real path.
217266
*/
218-
function getAllModulePaths(importingFileName: string, importedFileName: string, host: ModuleSpecifierResolutionHost): readonly string[] {
267+
function getAllModulePaths(importingFileName: string, importedFileName: string, host: ModuleSpecifierResolutionHost): readonly ModulePath[] {
219268
const cwd = host.getCurrentDirectory();
220269
const getCanonicalFileName = hostGetCanonicalFileName(host);
221-
const allFileNames = new Map<string, string>();
270+
const allFileNames = new Map<string, { path: string, isRedirect: boolean, isInNodeModules: boolean }>();
222271
let importedFileFromNodeModules = false;
223272
forEachFileNameOfModule(
224273
importingFileName,
225274
importedFileName,
226275
host,
227276
/*preferSymlinks*/ true,
228-
path => {
277+
(path, isRedirect) => {
278+
const isInNodeModules = pathContainsNodeModules(path);
279+
allFileNames.set(path, { path: getCanonicalFileName(path), isRedirect, isInNodeModules });
280+
importedFileFromNodeModules = importedFileFromNodeModules || isInNodeModules;
229281
// dont return value, so we collect everything
230-
allFileNames.set(path, getCanonicalFileName(path));
231-
importedFileFromNodeModules = importedFileFromNodeModules || pathContainsNodeModules(path);
232282
}
233283
);
234284

235285
// Sort by paths closest to importing file Name directory
236-
const sortedPaths: string[] = [];
286+
const sortedPaths: ModulePath[] = [];
237287
for (
238288
let directory = getDirectoryPath(toPath(importingFileName, cwd, getCanonicalFileName));
239289
allFileNames.size !== 0;
240290
) {
241291
const directoryStart = ensureTrailingDirectorySeparator(directory);
242-
let pathsInDirectory: string[] | undefined;
243-
allFileNames.forEach((canonicalFileName, fileName) => {
244-
if (startsWith(canonicalFileName, directoryStart)) {
245-
// If the importedFile is from node modules, use only paths in node_modules folder as option
246-
if (!importedFileFromNodeModules || pathContainsNodeModules(fileName)) {
247-
(pathsInDirectory || (pathsInDirectory = [])).push(fileName);
248-
}
292+
let pathsInDirectory: ModulePath[] | undefined;
293+
allFileNames.forEach(({ path, isRedirect, isInNodeModules }, fileName) => {
294+
if (startsWith(path, directoryStart)) {
295+
(pathsInDirectory ||= []).push({ path: fileName, isRedirect, isInNodeModules });
249296
allFileNames.delete(fileName);
250297
}
251298
});
252299
if (pathsInDirectory) {
253300
if (pathsInDirectory.length > 1) {
254-
pathsInDirectory.sort(comparePathsByNumberOfDirectorySeparators);
301+
pathsInDirectory.sort(comparePathsByRedirectAndNumberOfDirectorySeparators);
255302
}
256303
sortedPaths.push(...pathsInDirectory);
257304
}
@@ -261,7 +308,7 @@ namespace ts.moduleSpecifiers {
261308
}
262309
if (allFileNames.size) {
263310
const remainingPaths = arrayFrom(allFileNames.values());
264-
if (remainingPaths.length > 1) remainingPaths.sort(comparePathsByNumberOfDirectorySeparators);
311+
if (remainingPaths.length > 1) remainingPaths.sort(comparePathsByRedirectAndNumberOfDirectorySeparators);
265312
sortedPaths.push(...remainingPaths);
266313
}
267314
return sortedPaths;
@@ -312,18 +359,19 @@ namespace ts.moduleSpecifiers {
312359
: removeFileExtension(relativePath);
313360
}
314361

315-
function tryGetModuleNameAsNodeModule(moduleFileName: string, { getCanonicalFileName, sourceDirectory }: Info, host: ModuleSpecifierResolutionHost, options: CompilerOptions, packageNameOnly?: boolean): string | undefined {
362+
function tryGetModuleNameAsNodeModule({ path, isRedirect }: ModulePath, { getCanonicalFileName, sourceDirectory }: Info, host: ModuleSpecifierResolutionHost, options: CompilerOptions, packageNameOnly?: boolean): string | undefined {
316363
if (!host.fileExists || !host.readFile) {
317364
return undefined;
318365
}
319-
const parts: NodeModulePathParts = getNodeModulePathParts(moduleFileName)!;
366+
const parts: NodeModulePathParts = getNodeModulePathParts(path)!;
320367
if (!parts) {
321368
return undefined;
322369
}
323370

324371
// Simplify the full file path to something that can be resolved by Node.
325372

326-
let moduleSpecifier = moduleFileName;
373+
let moduleSpecifier = path;
374+
let isPackageRootPath = false;
327375
if (!packageNameOnly) {
328376
let packageRootIndex = parts.packageRootIndex;
329377
let moduleFileNameForExtensionless: string | undefined;
@@ -332,19 +380,24 @@ namespace ts.moduleSpecifiers {
332380
const { moduleFileToTry, packageRootPath } = tryDirectoryWithPackageJson(packageRootIndex);
333381
if (packageRootPath) {
334382
moduleSpecifier = packageRootPath;
383+
isPackageRootPath = true;
335384
break;
336385
}
337386
if (!moduleFileNameForExtensionless) moduleFileNameForExtensionless = moduleFileToTry;
338387

339388
// try with next level of directory
340-
packageRootIndex = moduleFileName.indexOf(directorySeparator, packageRootIndex + 1);
389+
packageRootIndex = path.indexOf(directorySeparator, packageRootIndex + 1);
341390
if (packageRootIndex === -1) {
342391
moduleSpecifier = getExtensionlessFileName(moduleFileNameForExtensionless);
343392
break;
344393
}
345394
}
346395
}
347396

397+
if (isRedirect && !isPackageRootPath) {
398+
return undefined;
399+
}
400+
348401
const globalTypingsCacheLocation = host.getGlobalTypingsCacheLocation && host.getGlobalTypingsCacheLocation();
349402
// Get a path that's relative to node_modules or the importing file's path
350403
// if node_modules folder is in this folder or any of its parent folders, no need to keep it.
@@ -360,16 +413,16 @@ namespace ts.moduleSpecifiers {
360413
return getEmitModuleResolutionKind(options) !== ModuleResolutionKind.NodeJs && packageName === nodeModulesDirectoryName ? undefined : packageName;
361414

362415
function tryDirectoryWithPackageJson(packageRootIndex: number) {
363-
const packageRootPath = moduleFileName.substring(0, packageRootIndex);
416+
const packageRootPath = path.substring(0, packageRootIndex);
364417
const packageJsonPath = combinePaths(packageRootPath, "package.json");
365-
let moduleFileToTry = moduleFileName;
418+
let moduleFileToTry = path;
366419
if (host.fileExists(packageJsonPath)) {
367420
const packageJsonContent = JSON.parse(host.readFile!(packageJsonPath)!);
368421
const versionPaths = packageJsonContent.typesVersions
369422
? getPackageJsonTypesVersionsPaths(packageJsonContent.typesVersions)
370423
: undefined;
371424
if (versionPaths) {
372-
const subModuleName = moduleFileName.slice(packageRootPath.length + 1);
425+
const subModuleName = path.slice(packageRootPath.length + 1);
373426
const fromPaths = tryGetModuleNameFromPaths(
374427
removeFileExtension(subModuleName),
375428
removeExtensionAndIndexPostFix(subModuleName, Ending.Minimal, options),

src/compiler/path.ts

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,14 @@ namespace ts {
6868
return /^\.\.?($|[\\/])/.test(path);
6969
}
7070

71+
/**
72+
* Determines whether a path is neither relative nor absolute, e.g. "path/to/file".
73+
* Also known misleadingly as "non-relative".
74+
*/
75+
export function pathIsBareSpecifier(path: string): boolean {
76+
return !pathIsAbsolute(path) && !pathIsRelative(path);
77+
}
78+
7179
export function hasExtension(fileName: string): boolean {
7280
return stringContains(getBaseFileName(fileName), ".");
7381
}

src/harness/fourslashImpl.ts

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2867,9 +2867,14 @@ namespace FourSlash {
28672867
const change = ts.first(codeFix.changes);
28682868
ts.Debug.assert(change.fileName === fileName);
28692869
this.applyEdits(change.fileName, change.textChanges);
2870-
const text = range ? this.rangeText(range) : this.getFileContent(this.activeFile.fileName);
2870+
const text = range ? this.rangeText(range) : this.getFileContent(fileName);
28712871
actualTextArray.push(text);
2872-
scriptInfo.updateContent(originalContent);
2872+
2873+
// Undo changes to perform next fix
2874+
const span = change.textChanges[0].span;
2875+
const deletedText = originalContent.substr(span.start, change.textChanges[0].span.length);
2876+
const insertedText = change.textChanges[0].newText;
2877+
this.editScriptAndUpdateMarkers(fileName, span.start, span.start + insertedText.length, deletedText);
28732878
}
28742879
if (expectedTextArray.length !== actualTextArray.length) {
28752880
this.raiseError(`Expected ${expectedTextArray.length} import fixes, got ${actualTextArray.length}`);

src/testRunner/unittests/tsserver/projectReferenceErrors.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -149,7 +149,7 @@ fnErr();
149149
{ line: 4, offset: 5 },
150150
{ line: 4, offset: 10 },
151151
Diagnostics.Module_0_has_no_exported_member_1,
152-
[`"../decls/fns"`, "fnErr"],
152+
[`"../dependency/fns"`, "fnErr"],
153153
"error",
154154
)
155155
],
Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
/// <reference path="../fourslash.ts" />
2+
3+
// @Filename: /tsconfig.base.json
4+
//// {
5+
//// "compilerOptions": {
6+
//// "module": "commonjs",
7+
//// "baseUrl": ".",
8+
//// "paths": {
9+
//// "packages/*": ["./packages/*"]
10+
//// }
11+
//// }
12+
//// }
13+
14+
// @Filename: /packages/app/tsconfig.json
15+
//// {
16+
//// "extends": "../../tsconfig.base.json",
17+
//// "compilerOptions": { "outDir": "../../dist/packages/app" },
18+
//// "references": [{ "path": "../dep" }]
19+
//// }
20+
21+
// @Filename: /packages/app/index.ts
22+
//// dep/**/
23+
24+
// @Filename: /packages/app/utils.ts
25+
//// import "packages/dep";
26+
27+
28+
// @Filename: /packages/dep/tsconfig.json
29+
//// {
30+
//// "extends": "../../tsconfig.base.json",
31+
//// "compilerOptions": { "outDir": "../../dist/packages/dep" }
32+
//// }
33+
34+
// @Filename: /packages/dep/index.ts
35+
//// import "./sub/folder";
36+
37+
// @Filename: /packages/dep/sub/folder/index.ts
38+
//// export const dep = 0;
39+
40+
goTo.marker("");
41+
verify.importFixAtPosition([`import { dep } from "packages/dep/sub/folder";\r
42+
\r
43+
dep`]);

0 commit comments

Comments
 (0)