Skip to content

Respect importModuleSpecifierPreference in sort order between fixes for the same symbol from different files #58597

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
andrewbranch merged 4 commits into from
May 21, 2024
Merged

Respect importModuleSpecifierPreference in sort order between fixes for the same symbol from different files #58597

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cee4ea9
Add failing test
andrewbranch Apr 10, 2024
7c33e80
Respect `importModuleSpecifierPreference` in sort order between fixes…
andrewbranch May 20, 2024
05dcd34
Add similar test for package.json "imports"
andrewbranch May 20, 2024
d9fe26e
Update other tests/baselines
andrewbranch May 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 28 additions & 20 deletions src/compiler/moduleSpecifiers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ import {
removeTrailingDirectorySeparator,
replaceFirstStar,
ResolutionMode,
ResolvedModuleSpecifierInfo,
resolveModuleName,
resolvePath,
ScriptKind,
Expand Down Expand Up @@ -287,14 +288,15 @@ export function tryGetModuleSpecifiersFromCache(
host: ModuleSpecifierResolutionHost,
userPreferences: UserPreferences,
options: ModuleSpecifierOptions = {},
): readonly string[] | undefined {
return tryGetModuleSpecifiersFromCacheWorker(
): ModuleSpecifierResult | undefined {
const result = tryGetModuleSpecifiersFromCacheWorker(
moduleSymbol,
importingSourceFile,
host,
userPreferences,
options,
)[0];
);
return result[1] && { kind: result[0], moduleSpecifiers: result[1], computedWithoutCache: false };
}

function tryGetModuleSpecifiersFromCacheWorker(
Expand All @@ -303,15 +305,15 @@ function tryGetModuleSpecifiersFromCacheWorker(
host: ModuleSpecifierResolutionHost,
userPreferences: UserPreferences,
options: ModuleSpecifierOptions = {},
): readonly [specifiers?: readonly string[], moduleFile?: SourceFile, modulePaths?: readonly ModulePath[], cache?: ModuleSpecifierCache] {
): readonly [kind?: ModuleSpecifierResult["kind"], specifiers?: readonly string[], moduleFile?: SourceFile, modulePaths?: readonly ModulePath[], cache?: ModuleSpecifierCache] {
const moduleSourceFile = getSourceFileOfModule(moduleSymbol);
if (!moduleSourceFile) {
return emptyArray as [];
}

const cache = host.getModuleSpecifierCache?.();
const cached = cache?.get(importingSourceFile.path, moduleSourceFile.path, userPreferences, options);
return [cached?.moduleSpecifiers, moduleSourceFile, cached?.modulePaths, cache];
return [cached?.kind, cached?.moduleSpecifiers, moduleSourceFile, cached?.modulePaths, cache];
}

/**
Expand Down Expand Up @@ -340,6 +342,13 @@ export function getModuleSpecifiers(
).moduleSpecifiers;
}

/** @internal */
export interface ModuleSpecifierResult {
kind: ResolvedModuleSpecifierInfo["kind"];
moduleSpecifiers: readonly string[];
computedWithoutCache: boolean;
}

/** @internal */
export function getModuleSpecifiersWithCacheInfo(
moduleSymbol: Symbol,
Expand All @@ -350,21 +359,21 @@ export function getModuleSpecifiersWithCacheInfo(
userPreferences: UserPreferences,
options: ModuleSpecifierOptions = {},
forAutoImport: boolean,
): { moduleSpecifiers: readonly string[]; computedWithoutCache: boolean; } {
): ModuleSpecifierResult {
let computedWithoutCache = false;
const ambient = tryGetModuleNameFromAmbientModule(moduleSymbol, checker);
if (ambient) return { moduleSpecifiers: [ambient], computedWithoutCache };
if (ambient) return { kind: "ambient", moduleSpecifiers: [ambient], computedWithoutCache };

// eslint-disable-next-line prefer-const
let [specifiers, moduleSourceFile, modulePaths, cache] = tryGetModuleSpecifiersFromCacheWorker(
let [kind, specifiers, moduleSourceFile, modulePaths, cache] = tryGetModuleSpecifiersFromCacheWorker(
moduleSymbol,
importingSourceFile,
host,
userPreferences,
options,
);
if (specifiers) return { moduleSpecifiers: specifiers, computedWithoutCache };
if (!moduleSourceFile) return { moduleSpecifiers: emptyArray, computedWithoutCache };
if (specifiers) return { kind, moduleSpecifiers: specifiers, computedWithoutCache };
if (!moduleSourceFile) return { kind: undefined, moduleSpecifiers: emptyArray, computedWithoutCache };

computedWithoutCache = true;
modulePaths ||= getAllModulePathsWorker(getInfo(importingSourceFile.fileName, host), moduleSourceFile.originalFileName, host, compilerOptions, options);
Expand All @@ -377,8 +386,8 @@ export function getModuleSpecifiersWithCacheInfo(
options,
forAutoImport,
);
cache?.set(importingSourceFile.path, moduleSourceFile.path, userPreferences, options, modulePaths, result);
return { moduleSpecifiers: result, computedWithoutCache };
cache?.set(importingSourceFile.path, moduleSourceFile.path, userPreferences, options, result.kind, modulePaths, result.moduleSpecifiers);
return result;
}

/** @internal */
Expand Down Expand Up @@ -409,7 +418,7 @@ function computeModuleSpecifiers(
userPreferences: UserPreferences,
options: ModuleSpecifierOptions = {},
forAutoImport: boolean,
): readonly string[] {
): ModuleSpecifierResult {
const info = getInfo(importingSourceFile.fileName, host);
const preferences = getModuleSpecifierPreferences(userPreferences, host, compilerOptions, importingSourceFile);
const existingSpecifier = isFullSourceFile(importingSourceFile) && forEach(modulePaths, modulePath =>
Expand All @@ -432,8 +441,7 @@ function computeModuleSpecifiers(
},
));
if (existingSpecifier) {
const moduleSpecifiers = [existingSpecifier];
return moduleSpecifiers;
return { kind: undefined, moduleSpecifiers: [existingSpecifier], computedWithoutCache: true };
}

const importedFileIsInNodeModules = some(modulePaths, p => p.isInNodeModules);
Expand All @@ -455,7 +463,7 @@ function computeModuleSpecifiers(
if (specifier && modulePath.isRedirect) {
// If we got a specifier for a redirect, it was a bare package specifier (e.g. "@foo/bar",
// not "@foo/bar/path/to/file"). No other specifier will be this good, so stop looking.
return nodeModulesSpecifiers!;
return { kind: "node_modules", moduleSpecifiers: nodeModulesSpecifiers!, computedWithoutCache: true };
}

if (!specifier) {
Expand Down Expand Up @@ -501,10 +509,10 @@ function computeModuleSpecifiers(
}
}

return pathsSpecifiers?.length ? pathsSpecifiers :
redirectPathsSpecifiers?.length ? redirectPathsSpecifiers :
nodeModulesSpecifiers?.length ? nodeModulesSpecifiers :
Debug.checkDefined(relativeSpecifiers);
return pathsSpecifiers?.length ? { kind: "paths", moduleSpecifiers: pathsSpecifiers, computedWithoutCache: true } :
redirectPathsSpecifiers?.length ? { kind: "redirect", moduleSpecifiers: redirectPathsSpecifiers, computedWithoutCache: true } :
nodeModulesSpecifiers?.length ? { kind: "node_modules", moduleSpecifiers: nodeModulesSpecifiers, computedWithoutCache: true } :
{ kind: "relative", moduleSpecifiers: Debug.checkDefined(relativeSpecifiers), computedWithoutCache: true };
}

interface Info {
Expand Down
3 changes: 2 additions & 1 deletion src/compiler/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9774,6 +9774,7 @@ export interface ModulePath {

/** @internal */
export interface ResolvedModuleSpecifierInfo {
kind: "node_modules" | "paths" | "redirect" | "relative" | "ambient" | undefined;
modulePaths: readonly ModulePath[] | undefined;
moduleSpecifiers: readonly string[] | undefined;
isBlockedByPackageJsonDependencies: boolean | undefined;
Expand All @@ -9787,7 +9788,7 @@ export interface ModuleSpecifierOptions {
/** @internal */
export interface ModuleSpecifierCache {
get(fromFileName: Path, toFileName: Path, preferences: UserPreferences, options: ModuleSpecifierOptions): Readonly<ResolvedModuleSpecifierInfo> | undefined;
set(fromFileName: Path, toFileName: Path, preferences: UserPreferences, options: ModuleSpecifierOptions, modulePaths: readonly ModulePath[], moduleSpecifiers: readonly string[]): void;
set(fromFileName: Path, toFileName: Path, preferences: UserPreferences, options: ModuleSpecifierOptions, kind: ResolvedModuleSpecifierInfo["kind"], modulePaths: readonly ModulePath[], moduleSpecifiers: readonly string[]): void;
setBlockedByPackageJsonDependencies(fromFileName: Path, toFileName: Path, preferences: UserPreferences, options: ModuleSpecifierOptions, isBlockedByPackageJsonDependencies: boolean): void;
setModulePaths(fromFileName: Path, toFileName: Path, preferences: UserPreferences, options: ModuleSpecifierOptions, modulePaths: readonly ModulePath[]): void;
clear(): void;
Expand Down
11 changes: 6 additions & 5 deletions src/server/moduleSpecifierCache.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@ export function createModuleSpecifierCache(host: ModuleSpecifierResolutionCacheH
if (!cache || currentKey !== key(fromFileName, preferences, options)) return undefined;
return cache.get(toFileName);
},
set(fromFileName, toFileName, preferences, options, modulePaths, moduleSpecifiers) {
ensureCache(fromFileName, preferences, options).set(toFileName, createInfo(modulePaths, moduleSpecifiers, /*isBlockedByPackageJsonDependencies*/ false));
set(fromFileName, toFileName, preferences, options, kind, modulePaths, moduleSpecifiers) {
ensureCache(fromFileName, preferences, options).set(toFileName, createInfo(kind, modulePaths, moduleSpecifiers, /*isBlockedByPackageJsonDependencies*/ false));

// If any module specifiers were generated based off paths in node_modules,
// a package.json file in that package was read and is an input to the cached.
Expand Down Expand Up @@ -58,7 +58,7 @@ export function createModuleSpecifierCache(host: ModuleSpecifierResolutionCacheH
info.modulePaths = modulePaths;
}
else {
cache.set(toFileName, createInfo(modulePaths, /*moduleSpecifiers*/ undefined, /*isBlockedByPackageJsonDependencies*/ undefined));
cache.set(toFileName, createInfo(/*kind*/ undefined, modulePaths, /*moduleSpecifiers*/ undefined, /*isBlockedByPackageJsonDependencies*/ undefined));
}
},
setBlockedByPackageJsonDependencies(fromFileName, toFileName, preferences, options, isBlockedByPackageJsonDependencies) {
Expand All @@ -68,7 +68,7 @@ export function createModuleSpecifierCache(host: ModuleSpecifierResolutionCacheH
info.isBlockedByPackageJsonDependencies = isBlockedByPackageJsonDependencies;
}
else {
cache.set(toFileName, createInfo(/*modulePaths*/ undefined, /*moduleSpecifiers*/ undefined, isBlockedByPackageJsonDependencies));
cache.set(toFileName, createInfo(/*kind*/ undefined, /*modulePaths*/ undefined, /*moduleSpecifiers*/ undefined, isBlockedByPackageJsonDependencies));
}
},
clear() {
Expand Down Expand Up @@ -100,10 +100,11 @@ export function createModuleSpecifierCache(host: ModuleSpecifierResolutionCacheH
}

function createInfo(
kind: ResolvedModuleSpecifierInfo["kind"] | undefined,
modulePaths: readonly ModulePath[] | undefined,
moduleSpecifiers: readonly string[] | undefined,
isBlockedByPackageJsonDependencies: boolean | undefined,
): ResolvedModuleSpecifierInfo {
return { modulePaths, moduleSpecifiers, isBlockedByPackageJsonDependencies };
return { kind, modulePaths, moduleSpecifiers, isBlockedByPackageJsonDependencies };
}
}
Loading