Invalidate the unresolved import resolutions when typing files are set

sheetalkamat · sheetalkamat · commit 60b19f5782b8 · 2018-04-13T15:17:13.000-07:00
This has 3 changes:
1. In updateGraph when enqueue the typing installation request (depending on unresolved imports)
2. When ActionSet event is received, invalidate only files with unresolved imports and resolve those.
3. When ActionInvalidate event is received, typing installer has detected some change in global typing cache location, so just enqueue a new typing installation request. This will repeat the cycle of setting correct typings and pickiing unresolved imports
diff --git a/src/compiler/resolutionCache.ts b/src/compiler/resolutionCache.ts
@@ -10,6 +10,7 @@ namespace ts {
 
         invalidateResolutionOfFile(filePath: Path): void;
         removeResolutionsOfFile(filePath: Path): void;
+        setFilesWithInvalidatedNonRelativeUnresolvedImports(filesWithUnresolvedImports: Map<any>): void;
         createHasInvalidatedResolution(forceAllFilesAsInvalidated?: boolean): HasInvalidatedResolution;
 
         startCachingPerDirectoryResolution(): void;
@@ -74,6 +75,7 @@ namespace ts {
     export function createResolutionCache(resolutionHost: ResolutionCacheHost, rootDirForResolution: string, logChangesWhenResolvingModule: boolean): ResolutionCache {
         let filesWithChangedSetOfUnresolvedImports: Path[] | undefined;
         let filesWithInvalidatedResolutions: Map<true> | undefined;
+        let filesWithInvalidatedNonRelativeUnresolvedImports: Map<any> | undefined;
         let allFilesHaveInvalidatedResolution = false;
 
         const getCurrentDirectory = memoize(() => resolutionHost.getCurrentDirectory());
@@ -122,6 +124,7 @@ namespace ts {
             resolveTypeReferenceDirectives,
             removeResolutionsOfFile,
             invalidateResolutionOfFile,
+            setFilesWithInvalidatedNonRelativeUnresolvedImports,
             createHasInvalidatedResolution,
             updateTypeRootsWatch,
             closeTypeRootsWatch,
@@ -173,7 +176,8 @@ namespace ts {
             }
             const collected = filesWithInvalidatedResolutions;
             filesWithInvalidatedResolutions = undefined;
-            return path => collected && collected.has(path);
+            return path => (collected && collected.has(path)) ||
+                (filesWithInvalidatedNonRelativeUnresolvedImports && filesWithInvalidatedNonRelativeUnresolvedImports.has(path));
         }
 
         function clearPerDirectoryResolutions() {
@@ -184,6 +188,7 @@ namespace ts {
 
         function finishCachingPerDirectoryResolution() {
             allFilesHaveInvalidatedResolution = false;
+            filesWithInvalidatedNonRelativeUnresolvedImports = undefined;
             directoryWatchesOfFailedLookups.forEach((watcher, path) => {
                 if (watcher.refCount === 0) {
                     directoryWatchesOfFailedLookups.delete(path);
@@ -237,13 +242,15 @@ namespace ts {
 
             const resolvedModules: R[] = [];
             const compilerOptions = resolutionHost.getCompilationSettings();
-
+            const hasInvalidatedNonRelativeUnresolvedImport = logChanges && filesWithInvalidatedNonRelativeUnresolvedImports && filesWithInvalidatedNonRelativeUnresolvedImports.has(path);
             const seenNamesInFile = createMap<true>();
             for (const name of names) {
                 let resolution = resolutionsInFile.get(name);
                 // Resolution is valid if it is present and not invalidated
                 if (!seenNamesInFile.has(name) &&
-                    allFilesHaveInvalidatedResolution || !resolution || resolution.isInvalidated) {
+                    allFilesHaveInvalidatedResolution || !resolution || resolution.isInvalidated ||
+                    // If the name is unresolved import that was invalidated, recalculate
+                    (hasInvalidatedNonRelativeUnresolvedImport && !isExternalModuleNameRelative(name) && !getResolutionWithResolvedFileName(resolution))) {
                     const existingResolution = resolution;
                     const resolutionInDirectory = perDirectoryResolution.get(name);
                     if (resolutionInDirectory) {
@@ -284,7 +291,7 @@ namespace ts {
                 if (oldResolution === newResolution) {
                     return true;
                 }
-                if (!oldResolution || !newResolution || oldResolution.isInvalidated) {
+                if (!oldResolution || !newResolution) {
                     return false;
                 }
                 const oldResult = getResolutionWithResolvedFileName(oldResolution);
@@ -577,6 +584,11 @@ namespace ts {
             );
         }
 
+        function setFilesWithInvalidatedNonRelativeUnresolvedImports(filesMap: Map<any>) {
+            Debug.assert(filesWithInvalidatedNonRelativeUnresolvedImports === filesMap || filesWithInvalidatedNonRelativeUnresolvedImports === undefined);
+            filesWithInvalidatedNonRelativeUnresolvedImports = filesMap;
+        }
+
         function invalidateResolutionOfFailedLookupLocation(fileOrDirectoryPath: Path, isCreatingWatchedDirectory: boolean) {
             let isChangedFailedLookupLocation: (location: string) => boolean;
             if (isCreatingWatchedDirectory) {
diff --git a/src/harness/unittests/tsserverProjectSystem.ts b/src/harness/unittests/tsserverProjectSystem.ts
@@ -7294,7 +7294,6 @@ namespace ts.projectSystem {
             const host = createServerHost(files);
             const session = createSession(host);
             const projectService = session.getProjectService();
-            debugger;
             session.executeCommandSeq<protocol.OpenRequest>({
                 command: protocol.CommandTypes.Open,
                 arguments: {
diff --git a/src/harness/unittests/typingsInstaller.ts b/src/harness/unittests/typingsInstaller.ts
@@ -1006,7 +1006,7 @@ namespace ts.projectSystem {
             installer.installAll(/*expectedCount*/ 1);
         });
 
-        it("should recompute resolutions after typings are installed", () => {
+        it("cached unresolved typings are not recomputed if program structure did not change", () => {
             const host = createServerHost([]);
             const session = createSession(host);
             const f = {
@@ -1048,7 +1048,7 @@ namespace ts.projectSystem {
             session.executeCommand(changeRequest);
             host.checkTimeoutQueueLengthAndRun(2); // This enqueues the updategraph and refresh inferred projects
             const version2 = proj.lastCachedUnresolvedImportsList;
-            assert.notEqual(version1, version2, "set of unresolved imports should change");
+            assert.strictEqual(version1, version2, "set of unresolved imports should change");
         });
 
         it("expired cache entry (inferred project, should install typings)", () => {
@@ -1621,4 +1621,75 @@ namespace ts.projectSystem {
             assert.deepEqual(commands, expectedCommands, "commands");
         });
     });
+
+    describe("recomputing resolutions of unresolved imports", () => {
+        const globalTypingsCacheLocation = "/tmp";
+        const appPath = "/a/b/app.js" as Path;
+        const foooPath = "/a/b/node_modules/fooo/index.d.ts";
+        function verifyResolvedModuleOfFooo(project: server.Project) {
+            const foooResolution = project.getLanguageService().getProgram().getSourceFileByPath(appPath).resolvedModules.get("fooo");
+            assert.equal(foooResolution.resolvedFileName, foooPath);
+            return foooResolution;
+        }
+
+        function verifyUnresolvedImportResolutions(appContents: string, typingNames: string[], typingFiles: FileOrFolder[]) {
+            const app: FileOrFolder = {
+                path: appPath,
+                content: `${appContents}import * as x from "fooo";`
+            };
+            const fooo: FileOrFolder = {
+                path: foooPath,
+                content: `export var x: string;`
+            };
+            const host = createServerHost([app, fooo]);
+            const installer = new (class extends Installer {
+                constructor() {
+                    super(host, { globalTypingsCacheLocation, typesRegistry: createTypesRegistry("foo") });
+                }
+                installWorker(_requestId: number, _args: string[], _cwd: string, cb: TI.RequestCompletedAction) {
+                    executeCommand(this, host, typingNames, typingFiles, cb);
+                }
+            })();
+            const projectService = createProjectService(host, { typingsInstaller: installer });
+            projectService.openClientFile(app.path);
+            projectService.checkNumberOfProjects({ inferredProjects: 1 });
+
+            const proj = projectService.inferredProjects[0];
+            checkProjectActualFiles(proj, [app.path, fooo.path]);
+            const foooResolution1 = verifyResolvedModuleOfFooo(proj);
+
+            installer.installAll(/*expectedCount*/ 1);
+            host.checkTimeoutQueueLengthAndRun(2);
+            checkProjectActualFiles(proj, typingFiles.map(f => f.path).concat(app.path, fooo.path));
+            const foooResolution2 = verifyResolvedModuleOfFooo(proj);
+            assert.strictEqual(foooResolution1, foooResolution2);
+        }
+
+        it("correctly invalidate the resolutions with typing names", () => {
+            verifyUnresolvedImportResolutions('import * as a from "foo";', ["foo"], [{
+                path: `${globalTypingsCacheLocation}/node_modules/foo/index.d.ts`,
+                content: "export function a(): void;"
+            }]);
+        });
+
+        it("correctly invalidate the resolutions with typing names that are trimmed", () => {
+            const fooAA: FileOrFolder = {
+                path: `${globalTypingsCacheLocation}/node_modules/foo/a/a.d.ts`,
+                content: "export function a (): void;"
+            };
+            const fooAB: FileOrFolder = {
+                path: `${globalTypingsCacheLocation}/node_modules/foo/a/b.d.ts`,
+                content: "export function b (): void;"
+            };
+            const fooAC: FileOrFolder = {
+                path: `${globalTypingsCacheLocation}/node_modules/foo/a/c.d.ts`,
+                content: "export function c (): void;"
+            };
+            verifyUnresolvedImportResolutions(`
+                    import * as a from "foo/a/a";
+                    import * as b from "foo/a/b";
+                    import * as c from "foo/a/c";
+            `, ["foo"], [fooAA, fooAB, fooAC]);
+        });
+    });
 }
diff --git a/src/server/editorServices.ts b/src/server/editorServices.ts
@@ -524,13 +524,13 @@ namespace ts.server {
             }
             switch (response.kind) {
                 case ActionSet:
-                    project.resolutionCache.clear();
-                    this.typingsCache.updateTypingsForProject(response.projectName, response.compilerOptions, response.typeAcquisition, response.unresolvedImports, response.typings);
+                    // Update the typing files and update the project
+                    project.updateTypingFiles(this.typingsCache.updateTypingsForProject(response.projectName, response.compilerOptions, response.typeAcquisition, response.unresolvedImports, response.typings));
                     break;
                 case ActionInvalidate:
-                    project.resolutionCache.clear();
-                    this.typingsCache.deleteTypingsForProject(response.projectName);
-                    break;
+                    // Do not clear resolution cache, there was changes detected in typings, so enque typing request and let it get us correct results
+                    this.typingsCache.enqueueInstallTypingsForProject(project, project.lastCachedUnresolvedImportsList, /*forceRefresh*/ true);
+                    return;
             }
             this.delayUpdateProjectGraphAndEnsureProjectStructureForOpenFiles(project);
         }
diff --git a/src/server/project.ts b/src/server/project.ts
@@ -89,7 +89,18 @@ namespace ts.server {
         private plugins: PluginModule[] = [];
 
         /*@internal*/
+        /**
+         * This is map from files to unresolved imports in it
+         * Maop does not contain entries for files that do not have unresolved imports
+         * This helps in containing the set of files to invalidate
+         */
         cachedUnresolvedImportsPerFile = createMap<ReadonlyArray<string>>();
+
+        /**
+         * This is the set that has entry to true if file doesnt contain any unresolved import
+         */
+        private filesWithNoUnresolvedImports = createMap<true>();
+
         /*@internal*/
         lastCachedUnresolvedImportsList: SortedReadonlyArray<string>;
         /*@internal*/
@@ -143,7 +154,8 @@ namespace ts.server {
         /*@internal*/
         hasChangedAutomaticTypeDirectiveNames = false;
 
-        private typingFiles: SortedReadonlyArray<string>;
+        /*@internal*/
+        typingFiles: SortedReadonlyArray<string> = emptyArray;
 
         private readonly cancellationToken: ThrottledCancellationToken;
 
@@ -554,6 +566,7 @@ namespace ts.server {
             this.resolutionCache.clear();
             this.resolutionCache = undefined;
             this.cachedUnresolvedImportsPerFile = undefined;
+            this.filesWithNoUnresolvedImports = undefined;
             this.directoryStructureHost = undefined;
 
             // Clean up file watchers waiting for missing files
@@ -714,6 +727,7 @@ namespace ts.server {
             else {
                 this.resolutionCache.invalidateResolutionOfFile(info.path);
             }
+            this.filesWithNoUnresolvedImports.delete(info.path);
             this.cachedUnresolvedImportsPerFile.delete(info.path);
 
             if (detachFromProject) {
@@ -735,16 +749,21 @@ namespace ts.server {
         }
 
         /* @internal */
-        private extractUnresolvedImportsFromSourceFile(file: SourceFile, result: Push<string>, ambientModules: string[]) {
+        private extractUnresolvedImportsFromSourceFile(file: SourceFile, result: string[] | undefined, ambientModules: string[]): string[] | undefined {
+            // No unresolve imports in this file
+            if (this.filesWithNoUnresolvedImports.has(file.path)) {
+                return result;
+            }
+
             const cached = this.cachedUnresolvedImportsPerFile.get(file.path);
             if (cached) {
                 // found cached result - use it and return
                 for (const f of cached) {
-                    result.push(f);
+                    (result || (result = [])).push(f);
                 }
-                return;
+                return result;
             }
-            let unresolvedImports: string[];
+            let unresolvedImports: string[] | undefined;
             if (file.resolvedModules) {
                 file.resolvedModules.forEach((resolvedModule, name) => {
                     // pick unresolved non-relative names
@@ -760,11 +779,17 @@ namespace ts.server {
                             trimmed = trimmed.substr(0, i);
                         }
                         (unresolvedImports || (unresolvedImports = [])).push(trimmed);
-                        result.push(trimmed);
+                        (result || (result = [])).push(trimmed);
                     }
                 });
             }
-            this.cachedUnresolvedImportsPerFile.set(file.path, unresolvedImports || emptyArray);
+            if (unresolvedImports) {
+                this.cachedUnresolvedImportsPerFile.set(file.path, unresolvedImports);
+            }
+            else {
+                this.filesWithNoUnresolvedImports.set(file.path, true);
+            }
+            return result;
 
             function isAmbientlyDeclaredModule(name: string) {
                 return ambientModules.some(m => m === name);
@@ -778,7 +803,7 @@ namespace ts.server {
         updateGraph(): boolean {
             this.resolutionCache.startRecordingFilesWithChangedResolutions();
 
-            let hasChanges = this.updateGraphWorker();
+            const hasChanges = this.updateGraphWorker();
             const hasMoreOrLessScriptInfos = this.hasMoreOrLessScriptInfos;
             this.hasMoreOrLessScriptInfos = false;
 
@@ -787,6 +812,7 @@ namespace ts.server {
             for (const file of changedFiles) {
                 // delete cached information for changed files
                 this.cachedUnresolvedImportsPerFile.delete(file);
+                this.filesWithNoUnresolvedImports.delete(file);
             }
 
             // update builder only if language service is enabled
@@ -799,20 +825,15 @@ namespace ts.server {
                 // (can reuse cached imports for files that were not changed)
                 // 4. compilation settings were changed in the way that might affect module resolution - drop all caches and collect all data from the scratch
                 if (hasChanges || changedFiles.length) {
-                    const result: string[] = [];
+                    let result: string[] | undefined;
                     const ambientModules = this.program.getTypeChecker().getAmbientModules().map(mod => stripQuotes(mod.getName()));
                     for (const sourceFile of this.program.getSourceFiles()) {
-                        this.extractUnresolvedImportsFromSourceFile(sourceFile, result, ambientModules);
+                        result = this.extractUnresolvedImportsFromSourceFile(sourceFile, result, ambientModules);
                     }
-                    this.lastCachedUnresolvedImportsList = toDeduplicatedSortedArray(result);
+                    this.lastCachedUnresolvedImportsList = result ? toDeduplicatedSortedArray(result) : emptyArray;
                 }
 
-                const cachedTypings = this.projectService.typingsCache.getTypingsForProject(this, this.lastCachedUnresolvedImportsList, hasMoreOrLessScriptInfos);
-                if (!arrayIsEqualTo(this.typingFiles, cachedTypings)) {
-                    this.typingFiles = cachedTypings;
-                    this.markAsDirty();
-                    hasChanges = this.updateGraphWorker() || hasChanges;
-                }
+                this.projectService.typingsCache.enqueueInstallTypingsForProject(this, this.lastCachedUnresolvedImportsList, hasMoreOrLessScriptInfos);
             }
             else {
                 this.lastCachedUnresolvedImportsList = undefined;
@@ -824,6 +845,13 @@ namespace ts.server {
             return !hasChanges;
         }
 
+        /*@internal*/
+        updateTypingFiles(typingFiles: SortedReadonlyArray<string>) {
+            this.typingFiles = typingFiles;
+            // Invalidate files with unresolved imports
+            this.resolutionCache.setFilesWithInvalidatedNonRelativeUnresolvedImports(this.cachedUnresolvedImportsPerFile);
+        }
+
         /* @internal */
         getCurrentProgram() {
             return this.program;
@@ -959,15 +987,14 @@ namespace ts.server {
         setCompilerOptions(compilerOptions: CompilerOptions) {
             if (compilerOptions) {
                 compilerOptions.allowNonTsExtensions = true;
-                if (changesAffectModuleResolution(this.compilerOptions, compilerOptions)) {
-                    // reset cached unresolved imports if changes in compiler options affected module resolution
-                    this.cachedUnresolvedImportsPerFile.clear();
-                    this.lastCachedUnresolvedImportsList = undefined;
-                }
                 const oldOptions = this.compilerOptions;
                 this.compilerOptions = compilerOptions;
                 this.setInternalCompilerOptionsForEmittingJsFiles();
                 if (changesAffectModuleResolution(oldOptions, compilerOptions)) {
+                    // reset cached unresolved imports if changes in compiler options affected module resolution
+                    this.cachedUnresolvedImportsPerFile.clear();
+                    this.filesWithNoUnresolvedImports.clear();
+                    this.lastCachedUnresolvedImportsList = undefined;
                     this.resolutionCache.clear();
                 }
                 this.markAsDirty();
diff --git a/src/server/typingsCache.ts b/src/server/typingsCache.ts
diff --git a/tests/baselines/reference/api/tsserverlibrary.d.ts b/tests/baselines/reference/api/tsserverlibrary.d.ts

Original file line number	Diff line number	Diff line change
`@@ -524,13 +524,13 @@ namespace ts.server {`
`524`	`524`	`}`
`525`	`525`	`switch (response.kind) {`
`526`	`526`	`case ActionSet:`
`527`		`- project.resolutionCache.clear();`
`528`		`- this.typingsCache.updateTypingsForProject(response.projectName, response.compilerOptions, response.typeAcquisition, response.unresolvedImports, response.typings);`
	`527`	`+ // Update the typing files and update the project`
	`528`	`+ project.updateTypingFiles(this.typingsCache.updateTypingsForProject(response.projectName, response.compilerOptions, response.typeAcquisition, response.unresolvedImports, response.typings));`
`529`	`529`	`break;`
`530`	`530`	`case ActionInvalidate:`
`531`		`- project.resolutionCache.clear();`
`532`		`- this.typingsCache.deleteTypingsForProject(response.projectName);`
`533`		`- break;`
	`531`	`+ // Do not clear resolution cache, there was changes detected in typings, so enque typing request and let it get us correct results`
	`532`	`+ this.typingsCache.enqueueInstallTypingsForProject(project, project.lastCachedUnresolvedImportsList, /forceRefresh/ true);`
	`533`	`+ return;`
`534`	`534`	`}`
`535`	`535`	`this.delayUpdateProjectGraphAndEnsureProjectStructureForOpenFiles(project);`
`536`	`536`	`}`