Skip to content

Commit ee283d1

Browse files
RyanCavanaughRyanCavanaugh
authored
Merge pull request microsoft#20464 from RyanCavanaugh/fix20402
Don't add ambiently declared modules to ATA's unresolvedModules list
2 parents 18a7c3f + c154705 commit ee283d1

5 files changed

Lines changed: 82 additions & 13 deletions

File tree

‎src/compiler/program.ts‎

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -704,7 +704,7 @@ namespace ts {
704704

705705
interface OldProgramState {
706706
program: Program | undefined;
707-
file: SourceFile;
707+
oldSourceFile: SourceFile | undefined;
708708
/** The collection of paths modified *since* the old program. */
709709
modifiedFilePaths: Path[];
710710
}
@@ -754,7 +754,6 @@ namespace ts {
754754
/** A transient placeholder used to mark predicted resolution in the result list. */
755755
const predictedToResolveToAmbientModuleMarker: ResolvedModuleFull = <any>{};
756756

757-
758757
for (let i = 0; i < moduleNames.length; i++) {
759758
const moduleName = moduleNames[i];
760759
// If the source file is unchanged and doesnt have invalidated resolution, reuse the module resolutions
@@ -825,9 +824,13 @@ namespace ts {
825824
// If we change our policy of rechecking failed lookups on each program create,
826825
// we should adjust the value returned here.
827826
function moduleNameResolvesToAmbientModuleInNonModifiedFile(moduleName: string, oldProgramState: OldProgramState): boolean {
828-
const resolutionToFile = getResolvedModule(oldProgramState.file, moduleName);
829-
if (resolutionToFile) {
830-
// module used to be resolved to file - ignore it
827+
const resolutionToFile = getResolvedModule(oldProgramState.oldSourceFile, moduleName);
828+
const resolvedFile = resolutionToFile && oldProgramState.program && oldProgramState.program.getSourceFile(resolutionToFile.resolvedFileName);
829+
if (resolutionToFile && resolvedFile && !resolvedFile.externalModuleIndicator) {
830+
// In the old program, we resolved to an ambient module that was in the same
831+
// place as we expected to find an actual module file.
832+
// We actually need to return 'false' here even though this seems like a 'true' case
833+
// because the normal module resolution algorithm will find this anyway.
831834
return false;
832835
}
833836
const ambientModule = oldProgramState.program && oldProgramState.program.getTypeChecker().tryFindAmbientModuleWithoutAugmentations(moduleName);
@@ -1001,7 +1004,7 @@ namespace ts {
10011004
const newSourceFilePath = getNormalizedAbsolutePath(newSourceFile.fileName, currentDirectory);
10021005
if (resolveModuleNamesWorker) {
10031006
const moduleNames = getModuleNames(newSourceFile);
1004-
const oldProgramState = { program: oldProgram, file: oldSourceFile, modifiedFilePaths };
1007+
const oldProgramState: OldProgramState = { program: oldProgram, oldSourceFile, modifiedFilePaths };
10051008
const resolutions = resolveModuleNamesReusingOldState(moduleNames, newSourceFilePath, newSourceFile, oldProgramState);
10061009
// ensure that module resolution results are still correct
10071010
const resolutionsChanged = hasChangesInResolutions(moduleNames, resolutions, oldSourceFile.resolvedModules, moduleResolutionIsEqualTo);
@@ -1945,7 +1948,7 @@ namespace ts {
19451948
if (file.imports.length || file.moduleAugmentations.length) {
19461949
// Because global augmentation doesn't have string literal name, we can check for global augmentation as such.
19471950
const moduleNames = getModuleNames(file);
1948-
const oldProgramState = { program: oldProgram, file, modifiedFilePaths };
1951+
const oldProgramState: OldProgramState = { program: oldProgram, oldSourceFile: oldProgram && oldProgram.getSourceFile(file.fileName), modifiedFilePaths };
19491952
const resolutions = resolveModuleNamesReusingOldState(moduleNames, getNormalizedAbsolutePath(file.fileName, currentDirectory), file, oldProgramState);
19501953
Debug.assert(resolutions.length === moduleNames.length);
19511954
for (let i = 0; i < moduleNames.length; i++) {

‎src/harness/unittests/reuseProgramStructure.ts‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -389,6 +389,19 @@ namespace ts {
389389
checkResolvedModulesCache(program4, "a.ts", createMapFromTemplate({ b: createResolvedModule("b.ts"), c: undefined }));
390390
});
391391

392+
it("set the resolvedImports after re-using an ambient external module declaration", () => {
393+
const files = [
394+
{ name: "/a.ts", text: SourceText.New("", "", 'import * as a from "a";') },
395+
{ name: "/types/zzz/index.d.ts", text: SourceText.New("", "", 'declare module "a" { }') },
396+
];
397+
const options: CompilerOptions = { target, typeRoots: ["/types"] };
398+
const program1 = newProgram(files, ["/a.ts"], options);
399+
const program2 = updateProgram(program1, ["/a.ts"], options, files => {
400+
files[0].text = files[0].text.updateProgram('import * as aa from "a";');
401+
});
402+
assert.isDefined(program2.getSourceFile("/a.ts").resolvedModules.get("a"), "'a' is not an unresolved module after re-use");
403+
});
404+
392405
it("resolved type directives cache follows type directives", () => {
393406
const files = [
394407
{ name: "/a.ts", text: SourceText.New("/// <reference types='typedefs'/>", "", "var x = $") },

‎src/harness/unittests/typingsInstaller.ts‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -228,6 +228,37 @@ namespace ts.projectSystem {
228228
projectService.checkNumberOfProjects({ externalProjects: 1 });
229229
});
230230

231+
it("external project - deduplicate from local @types packages", () => {
232+
const appJs = {
233+
path: "/a/b/app.js",
234+
content: ""
235+
};
236+
const nodeDts = {
237+
path: "/node_modules/@types/node/index.d.ts",
238+
content: "declare var node;"
239+
};
240+
const host = createServerHost([appJs, nodeDts]);
241+
const installer = new (class extends Installer {
242+
constructor() {
243+
super(host, { typesRegistry: createTypesRegistry("node") });
244+
}
245+
installWorker() {
246+
assert(false, "nothing should get installed");
247+
}
248+
})();
249+
250+
const projectFileName = "/a/app/test.csproj";
251+
const projectService = createProjectService(host, { typingsInstaller: installer });
252+
projectService.openExternalProject({
253+
projectFileName,
254+
options: {},
255+
rootFiles: [toExternalFile(appJs.path)],
256+
typeAcquisition: { enable: true, include: ["node"] }
257+
});
258+
installer.checkPendingCommands(/*expectedCount*/ 0);
259+
projectService.checkNumberOfProjects({ externalProjects: 1 });
260+
});
261+
231262
it("external project - no auto in typing acquisition, no .d.ts/js files", () => {
232263
const file1 = {
233264
path: "/a/b/app.ts",

‎src/server/project.ts‎

Lines changed: 26 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -506,6 +506,14 @@ namespace ts.server {
506506
}
507507
abstract getTypeAcquisition(): TypeAcquisition;
508508

509+
protected removeLocalTypingsFromTypeAcquisition(newTypeAcquisition: TypeAcquisition): TypeAcquisition {
510+
if (!newTypeAcquisition || !newTypeAcquisition.include) {
511+
// Nothing to filter out, so just return as-is
512+
return newTypeAcquisition;
513+
}
514+
return { ...newTypeAcquisition, include: this.removeExistingTypings(newTypeAcquisition.include) };
515+
}
516+
509517
getExternalFiles(): SortedReadonlyArray<string> {
510518
return emptyArray as SortedReadonlyArray<string>;
511519
}
@@ -718,7 +726,8 @@ namespace ts.server {
718726
this.projectStateVersion++;
719727
}
720728

721-
private extractUnresolvedImportsFromSourceFile(file: SourceFile, result: Push<string>) {
729+
/* @internal */
730+
private extractUnresolvedImportsFromSourceFile(file: SourceFile, result: Push<string>, ambientModules: string[]) {
722731
const cached = this.cachedUnresolvedImportsPerFile.get(file.path);
723732
if (cached) {
724733
// found cached result - use it and return
@@ -731,7 +740,7 @@ namespace ts.server {
731740
if (file.resolvedModules) {
732741
file.resolvedModules.forEach((resolvedModule, name) => {
733742
// pick unresolved non-relative names
734-
if (!resolvedModule && !isExternalModuleNameRelative(name)) {
743+
if (!resolvedModule && !isExternalModuleNameRelative(name) && !isAmbientlyDeclaredModule(name)) {
735744
// for non-scoped names extract part up-to the first slash
736745
// for scoped names - extract up to the second slash
737746
let trimmed = name.trim();
@@ -748,6 +757,10 @@ namespace ts.server {
748757
});
749758
}
750759
this.cachedUnresolvedImportsPerFile.set(file.path, unresolvedImports || emptyArray);
760+
761+
function isAmbientlyDeclaredModule(name: string) {
762+
return ambientModules.some(m => m === name);
763+
}
751764
}
752765

753766
/**
@@ -777,8 +790,9 @@ namespace ts.server {
777790
// 4. compilation settings were changed in the way that might affect module resolution - drop all caches and collect all data from the scratch
778791
if (hasChanges || changedFiles.length) {
779792
const result: string[] = [];
793+
const ambientModules = this.program.getTypeChecker().getAmbientModules().map(mod => stripQuotes(mod.getName()));
780794
for (const sourceFile of this.program.getSourceFiles()) {
781-
this.extractUnresolvedImportsFromSourceFile(sourceFile, result);
795+
this.extractUnresolvedImportsFromSourceFile(sourceFile, result, ambientModules);
782796
}
783797
this.lastCachedUnresolvedImportsList = toDeduplicatedSortedArray(result);
784798
}
@@ -804,6 +818,13 @@ namespace ts.server {
804818
return !hasChanges;
805819
}
806820

821+
822+
823+
protected removeExistingTypings(include: string[]): string[] {
824+
const existing = ts.getAutomaticTypeDirectiveNames(this.getCompilerOptions(), this.directoryStructureHost);
825+
return include.filter(i => existing.indexOf(i) < 0);
826+
}
827+
807828
private setTypings(typings: SortedReadonlyArray<string>): boolean {
808829
if (arrayIsEqualTo(this.typingFiles, typings)) {
809830
return false;
@@ -1299,7 +1320,7 @@ namespace ts.server {
12991320
}
13001321

13011322
setTypeAcquisition(newTypeAcquisition: TypeAcquisition): void {
1302-
this.typeAcquisition = newTypeAcquisition;
1323+
this.typeAcquisition = this.removeLocalTypingsFromTypeAcquisition(newTypeAcquisition);
13031324
}
13041325

13051326
getTypeAcquisition() {
@@ -1445,7 +1466,7 @@ namespace ts.server {
14451466
Debug.assert(!!newTypeAcquisition.include, "newTypeAcquisition.include may not be null/undefined");
14461467
Debug.assert(!!newTypeAcquisition.exclude, "newTypeAcquisition.exclude may not be null/undefined");
14471468
Debug.assert(typeof newTypeAcquisition.enable === "boolean", "newTypeAcquisition.enable may not be null/undefined");
1448-
this.typeAcquisition = newTypeAcquisition;
1469+
this.typeAcquisition = this.removeLocalTypingsFromTypeAcquisition(newTypeAcquisition);
14491470
}
14501471
}
14511472
}

‎tests/baselines/reference/api/tsserverlibrary.d.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7295,6 +7295,7 @@ declare namespace ts.server {
72957295
disableLanguageService(): void;
72967296
getProjectName(): string;
72977297
abstract getTypeAcquisition(): TypeAcquisition;
7298+
protected removeLocalTypingsFromTypeAcquisition(newTypeAcquisition: TypeAcquisition): TypeAcquisition;
72987299
getExternalFiles(): SortedReadonlyArray<string>;
72997300
getSourceFile(path: Path): SourceFile;
73007301
close(): void;
@@ -7315,12 +7316,12 @@ declare namespace ts.server {
73157316
removeFile(info: ScriptInfo, fileExists: boolean, detachFromProject: boolean): void;
73167317
registerFileUpdate(fileName: string): void;
73177318
markAsDirty(): void;
7318-
private extractUnresolvedImportsFromSourceFile(file, result);
73197319
/**
73207320
* Updates set of files that contribute to this project
73217321
* @returns: true if set of files in the project stays the same and false - otherwise.
73227322
*/
73237323
updateGraph(): boolean;
7324+
protected removeExistingTypings(include: string[]): string[];
73247325
private setTypings(typings);
73257326
private updateGraphWorker();
73267327
private detachScriptInfoFromProject(uncheckedFileName);

0 commit comments

Comments
 (0)