Skip to content
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

noDts project resolutions verification and updates for incremental behaviour #56016

Merged
merged 13 commits into from
Oct 10, 2023
Merged

noDts project resolutions verification and updates for incremental behaviour #56016

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
d552636
Add test where different specifier than whats in the file is resolved…
sheetalkamat Oct 4, 2023
098e3ef
Refactoring to ensure project files are added and removed through exi…
sheetalkamat Oct 6, 2023
43720b9
Some tests where file is added later to the project
sheetalkamat Oct 6, 2023
0efe4ee
Fix issue with js file having to be already present in project service
sheetalkamat Oct 6, 2023
5ef80a9
Add test when repeated goToSource shouldnt change the project
sheetalkamat Oct 6, 2023
2d40a69
Fix issue with unnecessary project update even with same source info …
sheetalkamat Oct 6, 2023
fe7f068
Some more refactoring for handling background projects
sheetalkamat Oct 6, 2023
8538100
Add test for resolution reuse from different folder
sheetalkamat Oct 6, 2023
a49de0e
Verify resolution cache after resolveSingleModuleNameWithoutWatching
sheetalkamat Oct 6, 2023
1b6c9a8
Mark the caches as readonly after program construction so there are n…
sheetalkamat Oct 9, 2023
e74da81
Mark project::getModuleResolutionCache as internal as it should have …
sheetalkamat Oct 9, 2023
d770127
Merge branch 'main' into sourceDefChanges
sheetalkamat Oct 9, 2023
5ae49dd
Merge branch 'main' into sourceDefChanges
sheetalkamat Oct 10, 2023
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
58 changes: 45 additions & 13 deletions src/compiler/moduleNameResolver.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,7 @@ function createResolvedModuleWithFailedLookupLocationsHandlingSymlink(
affectingLocations: string[],
diagnostics: Diagnostic[],
state: ModuleResolutionState,
cache: ModuleResolutionCache | NonRelativeModuleNameResolutionCache | undefined,
legacyResult?: string,
): ResolvedModuleWithFailedLookupLocations {
// If this is from node_modules for non relative name, always respect preserveSymlinks
Expand All @@ -246,6 +247,7 @@ function createResolvedModuleWithFailedLookupLocationsHandlingSymlink(
affectingLocations,
diagnostics,
state.resultFromCache,
cache,
legacyResult,
);
}
Expand All @@ -257,13 +259,24 @@ function createResolvedModuleWithFailedLookupLocations(
affectingLocations: string[],
diagnostics: Diagnostic[],
resultFromCache: ResolvedModuleWithFailedLookupLocations | undefined,
cache: ModuleResolutionCache | NonRelativeModuleNameResolutionCache | undefined,
legacyResult?: string,
): ResolvedModuleWithFailedLookupLocations {
if (resultFromCache) {
resultFromCache.failedLookupLocations = updateResolutionField(resultFromCache.failedLookupLocations, failedLookupLocations);
resultFromCache.affectingLocations = updateResolutionField(resultFromCache.affectingLocations, affectingLocations);
resultFromCache.resolutionDiagnostics = updateResolutionField(resultFromCache.resolutionDiagnostics, diagnostics);
return resultFromCache;
if (!cache?.isReadonly) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My only concern is it seems somewhat easy to miss this check if someone were adding new code that interacts with the cache, and it seems like there still are no compile-time or runtime checks preventing you from mutating the contents at a time when it could cause problems. Would it be worth changing ResolvedModuleWithFailedLookupLocations etc. to have readonly properties that can only be changed by a method on the cache, which could check isReadonly to ensure it’s not being violated?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i can do that, but frankly I dont see point in that since I think we want this method to be that one. Which ensures the resolutionWithFailedLookupLocation is created correctly.

resultFromCache.failedLookupLocations = updateResolutionField(resultFromCache.failedLookupLocations, failedLookupLocations);
resultFromCache.affectingLocations = updateResolutionField(resultFromCache.affectingLocations, affectingLocations);
resultFromCache.resolutionDiagnostics = updateResolutionField(resultFromCache.resolutionDiagnostics, diagnostics);
return resultFromCache;
}
else {
return {
...resultFromCache,
failedLookupLocations: initializeResolutionFieldForReadonlyCache(resultFromCache.failedLookupLocations, failedLookupLocations),
affectingLocations: initializeResolutionFieldForReadonlyCache(resultFromCache.affectingLocations, affectingLocations),
resolutionDiagnostics: initializeResolutionFieldForReadonlyCache(resultFromCache.resolutionDiagnostics, diagnostics),
};
}
}
return {
resolvedModule: resolved && {
Expand Down Expand Up @@ -291,6 +304,12 @@ export function updateResolutionField<T>(to: T[] | undefined, value: T[] | undef
return to;
}

function initializeResolutionFieldForReadonlyCache<T>(fromCache: T[] | undefined, value: T[]): T[] | undefined {
if (!fromCache?.length) return initializeResolutionField(value);
if (!value.length) return fromCache.slice();
return [...fromCache, ...value];
}

/** @internal */
export interface ModuleResolutionState {
host: ModuleResolutionHost;
Expand Down Expand Up @@ -612,10 +631,10 @@ export function resolveTypeReferenceDirective(typeReferenceDirectiveName: string
affectingLocations: initializeResolutionField(affectingLocations),
resolutionDiagnostics: initializeResolutionField(diagnostics),
};
if (containingDirectory) {
cache?.getOrCreateCacheForDirectory(containingDirectory, redirectedReference).set(typeReferenceDirectiveName, /*mode*/ resolutionMode, result);
if (containingDirectory && cache && !cache.isReadonly) {
cache.getOrCreateCacheForDirectory(containingDirectory, redirectedReference).set(typeReferenceDirectiveName, /*mode*/ resolutionMode, result);
if (!isExternalModuleNameRelative(typeReferenceDirectiveName)) {
cache?.getOrCreateCacheForNonRelativeName(typeReferenceDirectiveName, resolutionMode, redirectedReference).set(containingDirectory, result);
cache.getOrCreateCacheForNonRelativeName(typeReferenceDirectiveName, resolutionMode, redirectedReference).set(containingDirectory, result);
}
}
if (traceEnabled) traceResult(result);
Expand Down Expand Up @@ -850,6 +869,8 @@ export interface PerDirectoryResolutionCache<T> {
* This updates the redirects map as well if needed so module resolutions are cached if they can across the projects
*/
update(options: CompilerOptions): void;
/** @internal */ directoryToModuleNameMap: CacheWithRedirects<Path, ModeAwareCache<T>>;
/** @internal */ isReadonly?: boolean;
}

export interface NonRelativeNameResolutionCache<T> {
Expand All @@ -861,11 +882,13 @@ export interface NonRelativeNameResolutionCache<T> {
* This updates the redirects map as well if needed so module resolutions are cached if they can across the projects
*/
update(options: CompilerOptions): void;
/** @internal */ isReadonly?: boolean;
}

export interface PerNonRelativeNameCache<T> {
get(directory: string): T | undefined;
set(directory: string, result: T): void;
/** @internal */ isReadonly?: boolean;
}

export interface ModuleResolutionCache extends PerDirectoryResolutionCache<ResolvedModuleWithFailedLookupLocations>, NonRelativeModuleNameResolutionCache, PackageJsonInfoCache {
Expand All @@ -889,6 +912,7 @@ export interface PackageJsonInfoCache {
/** @internal */ entries(): [Path, PackageJsonInfo | boolean][];
/** @internal */ getInternalMap(): Map<Path, PackageJsonInfo | boolean> | undefined;
clear(): void;
/** @internal */ isReadonly?: boolean;
}

export type PerModuleNameCache = PerNonRelativeNameCache<ResolvedModuleWithFailedLookupLocations>;
Expand Down Expand Up @@ -920,6 +944,7 @@ export interface CacheWithRedirects<K, V> {
getOrCreateMapOfCacheRedirects(redirectedReference: ResolvedProjectReference | undefined): Map<K, V>;
update(newOptions: CompilerOptions): void;
clear(): void;
getOwnMap(): Map<K, V>;
}

/** @internal */
Expand All @@ -936,6 +961,7 @@ export function createCacheWithRedirects<K, V>(ownOptions: CompilerOptions | und
getOrCreateMapOfCacheRedirects,
update,
clear,
getOwnMap: () => ownMap,
};

function getMapOfCacheRedirects(redirectedReference: ResolvedProjectReference | undefined): Map<K, V> | undefined {
Expand Down Expand Up @@ -1042,6 +1068,7 @@ function createPerDirectoryResolutionCache<T>(
getOrCreateCacheForDirectory,
clear,
update,
directoryToModuleNameMap,
};

function clear() {
Expand Down Expand Up @@ -1426,10 +1453,12 @@ export function resolveModuleName(moduleName: string, containingFile: string, co
if (result && result.resolvedModule) perfLogger?.logInfoEvent(`Module "${moduleName}" resolved to "${result.resolvedModule.resolvedFileName}"`);
perfLogger?.logStopResolveModule((result && result.resolvedModule) ? "" + result.resolvedModule.resolvedFileName : "null");

cache?.getOrCreateCacheForDirectory(containingDirectory, redirectedReference).set(moduleName, resolutionMode, result);
if (!isExternalModuleNameRelative(moduleName)) {
// put result in per-module name cache
cache?.getOrCreateCacheForNonRelativeName(moduleName, resolutionMode, redirectedReference).set(containingDirectory, result);
if (cache && !cache.isReadonly) {
cache.getOrCreateCacheForDirectory(containingDirectory, redirectedReference).set(moduleName, resolutionMode, result);
if (!isExternalModuleNameRelative(moduleName)) {
// put result in per-module name cache
cache.getOrCreateCacheForNonRelativeName(moduleName, resolutionMode, redirectedReference).set(containingDirectory, result);
}
}
}

Expand Down Expand Up @@ -1850,6 +1879,7 @@ function nodeModuleNameResolverWorker(
affectingLocations,
diagnostics,
state,
cache,
legacyResult,
);

Expand Down Expand Up @@ -2386,15 +2416,15 @@ export function getPackageJsonInfo(packageDirectory: string, onlyRecordFailures:
trace(host, Diagnostics.Found_package_json_at_0, packageJsonPath);
}
const result: PackageJsonInfo = { packageDirectory, contents: { packageJsonContent, versionPaths: undefined, resolvedEntrypoints: undefined } };
state.packageJsonInfoCache?.setPackageJsonInfo(packageJsonPath, result);
if (state.packageJsonInfoCache && !state.packageJsonInfoCache.isReadonly) state.packageJsonInfoCache.setPackageJsonInfo(packageJsonPath, result);
state.affectingLocations?.push(packageJsonPath);
return result;
}
else {
if (directoryExists && traceEnabled) {
trace(host, Diagnostics.File_0_does_not_exist, packageJsonPath);
}
state.packageJsonInfoCache?.setPackageJsonInfo(packageJsonPath, directoryExists);
if (state.packageJsonInfoCache && !state.packageJsonInfoCache.isReadonly) state.packageJsonInfoCache.setPackageJsonInfo(packageJsonPath, directoryExists);
// record package json as one of failed lookup locations - in the future if this file will appear it will invalidate resolution results
state.failedLookupLocations?.push(packageJsonPath);
}
Expand Down Expand Up @@ -3178,6 +3208,7 @@ export function classicNameResolver(moduleName: string, containingFile: string,
affectingLocations,
diagnostics,
state,
cache,
);

function tryResolve(extensions: Extensions): SearchResult<Resolved> {
Expand Down Expand Up @@ -3273,6 +3304,7 @@ export function loadModuleFromGlobalCache(moduleName: string, projectName: strin
affectingLocations,
diagnostics,
state.resultFromCache,
/*cache*/ undefined,
);
}

Expand Down
45 changes: 40 additions & 5 deletions src/compiler/resolutionCache.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,18 @@ export interface ResolutionCacheHost extends MinimalResolutionCacheHost {
getCurrentProgram(): Program | undefined;
fileIsOpen(filePath: Path): boolean;
onDiscoveredSymlink?(): void;

// For incremental testing
beforeResolveSingleModuleNameWithoutWatching?(
moduleResolutionCache: ModuleResolutionCache,
): any;
afterResolveSingleModuleNameWithoutWatching?(
moduleResolutionCache: ModuleResolutionCache,
moduleName: string,
containingFile: string,
result: ResolvedModuleWithFailedLookupLocations,
data: any,
): any;
}

/** @internal */
Expand Down Expand Up @@ -439,6 +451,10 @@ export function getRootPathSplitLength(rootPath: Path) {
return rootPath.split(directorySeparator).length - (hasTrailingDirectorySeparator(rootPath) ? 1 : 0);
}

function getModuleResolutionHost(resolutionHost: ResolutionCacheHost) {
return resolutionHost.getCompilerHost?.() || resolutionHost;
}

/** @internal */
export function createModuleResolutionLoaderUsingGlobalCache(
containingFile: string,
Expand Down Expand Up @@ -471,7 +487,7 @@ function resolveModuleNameUsingGlobalCache(
redirectedReference?: ResolvedProjectReference,
mode?: ResolutionMode,
): ResolvedModuleWithFailedLookupLocations {
const host = resolutionHost.getCompilerHost?.() || resolutionHost;
const host = getModuleResolutionHost(resolutionHost);
const primaryResult = ts_resolveModuleName(moduleName, containingFile, compilerOptions, host, moduleResolutionCache, redirectedReference, mode);
// return result immediately only if global cache support is not enabled or if it is .ts, .tsx or .d.ts
if (!resolutionHost.getGlobalCache) {
Expand Down Expand Up @@ -686,6 +702,10 @@ export function createResolutionCache(resolutionHost: ResolutionCacheHost, rootD
}

function startCachingPerDirectoryResolution() {
moduleResolutionCache.isReadonly = undefined;
typeReferenceDirectiveResolutionCache.isReadonly = undefined;
libraryResolutionCache.isReadonly = undefined;
moduleResolutionCache.getPackageJsonInfoCache().isReadonly = undefined;
moduleResolutionCache.clearAllExceptPackageJsonInfoCache();
typeReferenceDirectiveResolutionCache.clearAllExceptPackageJsonInfoCache();
libraryResolutionCache.clearAllExceptPackageJsonInfoCache();
Expand Down Expand Up @@ -740,6 +760,10 @@ export function createResolutionCache(resolutionHost: ResolutionCacheHost, rootD
directoryWatchesOfFailedLookups.forEach(closeDirectoryWatchesOfFailedLookup);
fileWatchesOfAffectingLocations.forEach(closeFileWatcherOfAffectingLocation);
hasChangedAutomaticTypeDirectiveNames = false;
moduleResolutionCache.isReadonly = true;
typeReferenceDirectiveResolutionCache.isReadonly = true;
libraryResolutionCache.isReadonly = true;
moduleResolutionCache.getPackageJsonInfoCache().isReadonly = true;
}

function closeDirectoryWatchesOfFailedLookup(watcher: DirectoryWatchesOfFailedLookup, path: Path) {
Expand Down Expand Up @@ -828,7 +852,7 @@ export function createResolutionCache(resolutionHost: ResolutionCacheHost, rootD
}
}
else {
const host = resolutionHost.getCompilerHost?.() || resolutionHost;
const host = getModuleResolutionHost(resolutionHost);
if (isTraceEnabled(options, host) && !seenNamesInFile.has(name, mode)) {
const resolved = getResolutionWithResolvedFileName(resolution!);
trace(
Expand Down Expand Up @@ -912,7 +936,7 @@ export function createResolutionCache(resolutionHost: ResolutionCacheHost, rootD
containingFile,
redirectedReference,
options,
resolutionHost.getCompilerHost?.() || resolutionHost,
getModuleResolutionHost(resolutionHost),
typeReferenceDirectiveResolutionCache,
),
getResolutionWithResolvedFileName: getResolvedTypeReferenceDirective,
Expand Down Expand Up @@ -957,7 +981,7 @@ export function createResolutionCache(resolutionHost: ResolutionCacheHost, rootD
options: CompilerOptions,
libFileName: string,
) {
const host = resolutionHost.getCompilerHost?.() || resolutionHost;
const host = getModuleResolutionHost(resolutionHost);
let resolution = resolvedLibraries?.get(libFileName);
if (!resolution || resolution.isInvalidated) {
const existingResolution = resolution;
Expand Down Expand Up @@ -994,7 +1018,18 @@ export function createResolutionCache(resolutionHost: ResolutionCacheHost, rootD
const resolutionsInFile = resolvedModuleNames.get(path);
const resolution = resolutionsInFile?.get(moduleName, /*mode*/ undefined);
if (resolution && !resolution.isInvalidated) return resolution;
return resolveModuleNameUsingGlobalCache(resolutionHost, moduleResolutionCache, moduleName, containingFile, resolutionHost.getCompilationSettings());
const data = resolutionHost.beforeResolveSingleModuleNameWithoutWatching?.(moduleResolutionCache);
const host = getModuleResolutionHost(resolutionHost);
// We are not resolving d.ts so just normal resolution instead of doing resolution pass to global cache
const result = ts_resolveModuleName(
moduleName,
containingFile,
resolutionHost.getCompilationSettings(),
host,
moduleResolutionCache,
);
resolutionHost.afterResolveSingleModuleNameWithoutWatching?.(moduleResolutionCache, moduleName, containingFile, result, data);
return result;
}

function isNodeModulesAtTypesDirectory(dirPath: Path) {
Expand Down
Loading