From c15470595f7651fc962e47c6f10572d8833a539e Mon Sep 17 00:00:00 2001 From: Ryan Cavanaugh Date: Wed, 6 Dec 2017 13:17:21 -0800 Subject: [PATCH] Dedupe local types from ATA and reuse old programs correctly --- src/compiler/program.ts | 17 +++++----- .../unittests/reuseProgramStructure.ts | 13 ++++++++ src/harness/unittests/typingsInstaller.ts | 31 +++++++++++++++++++ src/server/project.ts | 31 ++++++++++++++++--- .../reference/api/tsserverlibrary.d.ts | 3 +- 5 files changed, 82 insertions(+), 13 deletions(-) diff --git a/src/compiler/program.ts b/src/compiler/program.ts index 4088a6951a..de106bae2c 100755 --- a/src/compiler/program.ts +++ b/src/compiler/program.ts @@ -704,7 +704,7 @@ namespace ts { interface OldProgramState { program: Program | undefined; - file: SourceFile; + oldSourceFile: SourceFile | undefined; /** The collection of paths modified *since* the old program. */ modifiedFilePaths: Path[]; } @@ -754,7 +754,6 @@ namespace ts { /** A transient placeholder used to mark predicted resolution in the result list. */ const predictedToResolveToAmbientModuleMarker: ResolvedModuleFull = {}; - for (let i = 0; i < moduleNames.length; i++) { const moduleName = moduleNames[i]; // If the source file is unchanged and doesnt have invalidated resolution, reuse the module resolutions @@ -825,9 +824,13 @@ namespace ts { // If we change our policy of rechecking failed lookups on each program create, // we should adjust the value returned here. function moduleNameResolvesToAmbientModuleInNonModifiedFile(moduleName: string, oldProgramState: OldProgramState): boolean { - const resolutionToFile = getResolvedModule(oldProgramState.file, moduleName); - if (resolutionToFile) { - // module used to be resolved to file - ignore it + const resolutionToFile = getResolvedModule(oldProgramState.oldSourceFile, moduleName); + const resolvedFile = resolutionToFile && oldProgramState.program && oldProgramState.program.getSourceFile(resolutionToFile.resolvedFileName); + if (resolutionToFile && resolvedFile && !resolvedFile.externalModuleIndicator) { + // In the old program, we resolved to an ambient module that was in the same + // place as we expected to find an actual module file. + // We actually need to return 'false' here even though this seems like a 'true' case + // because the normal module resolution algorithm will find this anyway. return false; } const ambientModule = oldProgramState.program && oldProgramState.program.getTypeChecker().tryFindAmbientModuleWithoutAugmentations(moduleName); @@ -1001,7 +1004,7 @@ namespace ts { const newSourceFilePath = getNormalizedAbsolutePath(newSourceFile.fileName, currentDirectory); if (resolveModuleNamesWorker) { const moduleNames = getModuleNames(newSourceFile); - const oldProgramState = { program: oldProgram, file: oldSourceFile, modifiedFilePaths }; + const oldProgramState: OldProgramState = { program: oldProgram, oldSourceFile, modifiedFilePaths }; const resolutions = resolveModuleNamesReusingOldState(moduleNames, newSourceFilePath, newSourceFile, oldProgramState); // ensure that module resolution results are still correct const resolutionsChanged = hasChangesInResolutions(moduleNames, resolutions, oldSourceFile.resolvedModules, moduleResolutionIsEqualTo); @@ -1945,7 +1948,7 @@ namespace ts { if (file.imports.length || file.moduleAugmentations.length) { // Because global augmentation doesn't have string literal name, we can check for global augmentation as such. const moduleNames = getModuleNames(file); - const oldProgramState = { program: oldProgram, file, modifiedFilePaths }; + const oldProgramState: OldProgramState = { program: oldProgram, oldSourceFile: oldProgram && oldProgram.getSourceFile(file.fileName), modifiedFilePaths }; const resolutions = resolveModuleNamesReusingOldState(moduleNames, getNormalizedAbsolutePath(file.fileName, currentDirectory), file, oldProgramState); Debug.assert(resolutions.length === moduleNames.length); for (let i = 0; i < moduleNames.length; i++) { diff --git a/src/harness/unittests/reuseProgramStructure.ts b/src/harness/unittests/reuseProgramStructure.ts index 45fe7383c3..a7298c835c 100644 --- a/src/harness/unittests/reuseProgramStructure.ts +++ b/src/harness/unittests/reuseProgramStructure.ts @@ -389,6 +389,19 @@ namespace ts { checkResolvedModulesCache(program4, "a.ts", createMapFromTemplate({ b: createResolvedModule("b.ts"), c: undefined })); }); + it("set the resolvedImports after re-using an ambient external module declaration", () => { + const files = [ + { name: "/a.ts", text: SourceText.New("", "", 'import * as a from "a";') }, + { name: "/types/zzz/index.d.ts", text: SourceText.New("", "", 'declare module "a" { }') }, + ]; + const options: CompilerOptions = { target, typeRoots: ["/types"] }; + const program1 = newProgram(files, ["/a.ts"], options); + const program2 = updateProgram(program1, ["/a.ts"], options, files => { + files[0].text = files[0].text.updateProgram('import * as aa from "a";'); + }); + assert.isDefined(program2.getSourceFile("/a.ts").resolvedModules.get("a"), "'a' is not an unresolved module after re-use"); + }); + it("resolved type directives cache follows type directives", () => { const files = [ { name: "/a.ts", text: SourceText.New("/// ", "", "var x = $") }, diff --git a/src/harness/unittests/typingsInstaller.ts b/src/harness/unittests/typingsInstaller.ts index adf99094f7..cb2d350744 100644 --- a/src/harness/unittests/typingsInstaller.ts +++ b/src/harness/unittests/typingsInstaller.ts @@ -228,6 +228,37 @@ namespace ts.projectSystem { projectService.checkNumberOfProjects({ externalProjects: 1 }); }); + it("external project - deduplicate from local @types packages", () => { + const appJs = { + path: "/a/b/app.js", + content: "" + }; + const nodeDts = { + path: "/node_modules/@types/node/index.d.ts", + content: "declare var node;" + }; + const host = createServerHost([appJs, nodeDts]); + const installer = new (class extends Installer { + constructor() { + super(host, { typesRegistry: createTypesRegistry("node") }); + } + installWorker() { + assert(false, "nothing should get installed"); + } + })(); + + const projectFileName = "/a/app/test.csproj"; + const projectService = createProjectService(host, { typingsInstaller: installer }); + projectService.openExternalProject({ + projectFileName, + options: {}, + rootFiles: [toExternalFile(appJs.path)], + typeAcquisition: { enable: true, include: ["node"] } + }); + installer.checkPendingCommands(/*expectedCount*/ 0); + projectService.checkNumberOfProjects({ externalProjects: 1 }); + }); + it("external project - no auto in typing acquisition, no .d.ts/js files", () => { const file1 = { path: "/a/b/app.ts", diff --git a/src/server/project.ts b/src/server/project.ts index ce750c78e2..11c023415e 100644 --- a/src/server/project.ts +++ b/src/server/project.ts @@ -506,6 +506,14 @@ namespace ts.server { } abstract getTypeAcquisition(): TypeAcquisition; + protected removeLocalTypingsFromTypeAcquisition(newTypeAcquisition: TypeAcquisition): TypeAcquisition { + if (!newTypeAcquisition || !newTypeAcquisition.include) { + // Nothing to filter out, so just return as-is + return newTypeAcquisition; + } + return { ...newTypeAcquisition, include: this.removeExistingTypings(newTypeAcquisition.include) }; + } + getExternalFiles(): SortedReadonlyArray { return emptyArray as SortedReadonlyArray; } @@ -718,7 +726,8 @@ namespace ts.server { this.projectStateVersion++; } - private extractUnresolvedImportsFromSourceFile(file: SourceFile, result: Push) { + /* @internal */ + private extractUnresolvedImportsFromSourceFile(file: SourceFile, result: Push, ambientModules: string[]) { const cached = this.cachedUnresolvedImportsPerFile.get(file.path); if (cached) { // found cached result - use it and return @@ -731,7 +740,7 @@ namespace ts.server { if (file.resolvedModules) { file.resolvedModules.forEach((resolvedModule, name) => { // pick unresolved non-relative names - if (!resolvedModule && !isExternalModuleNameRelative(name)) { + if (!resolvedModule && !isExternalModuleNameRelative(name) && !isAmbientlyDeclaredModule(name)) { // for non-scoped names extract part up-to the first slash // for scoped names - extract up to the second slash let trimmed = name.trim(); @@ -748,6 +757,10 @@ namespace ts.server { }); } this.cachedUnresolvedImportsPerFile.set(file.path, unresolvedImports || emptyArray); + + function isAmbientlyDeclaredModule(name: string) { + return ambientModules.some(m => m === name); + } } /** @@ -777,8 +790,9 @@ namespace ts.server { // 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[] = []; + const ambientModules = this.program.getTypeChecker().getAmbientModules().map(mod => stripQuotes(mod.getName())); for (const sourceFile of this.program.getSourceFiles()) { - this.extractUnresolvedImportsFromSourceFile(sourceFile, result); + this.extractUnresolvedImportsFromSourceFile(sourceFile, result, ambientModules); } this.lastCachedUnresolvedImportsList = toDeduplicatedSortedArray(result); } @@ -804,6 +818,13 @@ namespace ts.server { return !hasChanges; } + + + protected removeExistingTypings(include: string[]): string[] { + const existing = ts.getAutomaticTypeDirectiveNames(this.getCompilerOptions(), this.directoryStructureHost); + return include.filter(i => existing.indexOf(i) < 0); + } + private setTypings(typings: SortedReadonlyArray): boolean { if (arrayIsEqualTo(this.typingFiles, typings)) { return false; @@ -1299,7 +1320,7 @@ namespace ts.server { } setTypeAcquisition(newTypeAcquisition: TypeAcquisition): void { - this.typeAcquisition = newTypeAcquisition; + this.typeAcquisition = this.removeLocalTypingsFromTypeAcquisition(newTypeAcquisition); } getTypeAcquisition() { @@ -1445,7 +1466,7 @@ namespace ts.server { Debug.assert(!!newTypeAcquisition.include, "newTypeAcquisition.include may not be null/undefined"); Debug.assert(!!newTypeAcquisition.exclude, "newTypeAcquisition.exclude may not be null/undefined"); Debug.assert(typeof newTypeAcquisition.enable === "boolean", "newTypeAcquisition.enable may not be null/undefined"); - this.typeAcquisition = newTypeAcquisition; + this.typeAcquisition = this.removeLocalTypingsFromTypeAcquisition(newTypeAcquisition); } } } diff --git a/tests/baselines/reference/api/tsserverlibrary.d.ts b/tests/baselines/reference/api/tsserverlibrary.d.ts index c127232c09..9b7b9ac35f 100644 --- a/tests/baselines/reference/api/tsserverlibrary.d.ts +++ b/tests/baselines/reference/api/tsserverlibrary.d.ts @@ -7294,6 +7294,7 @@ declare namespace ts.server { disableLanguageService(): void; getProjectName(): string; abstract getTypeAcquisition(): TypeAcquisition; + protected removeLocalTypingsFromTypeAcquisition(newTypeAcquisition: TypeAcquisition): TypeAcquisition; getExternalFiles(): SortedReadonlyArray; getSourceFile(path: Path): SourceFile; close(): void; @@ -7314,12 +7315,12 @@ declare namespace ts.server { removeFile(info: ScriptInfo, fileExists: boolean, detachFromProject: boolean): void; registerFileUpdate(fileName: string): void; markAsDirty(): void; - private extractUnresolvedImportsFromSourceFile(file, result); /** * Updates set of files that contribute to this project * @returns: true if set of files in the project stays the same and false - otherwise. */ updateGraph(): boolean; + protected removeExistingTypings(include: string[]): string[]; private setTypings(typings); private updateGraphWorker(); private detachScriptInfoFromProject(uncheckedFileName);