Skip to content

Stop checking existing resolution now that we have concept of package ID #58528

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
sheetalkamat merged 2 commits into from
May 14, 2024
Merged

Stop checking existing resolution now that we have concept of package ID #58528

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a7b1b19
Stop checking existing resolution now that we have concept of package ID
sheetalkamat May 13, 2024
652c2db
Merge branch 'main' into stopCheckingConflictingDefs
sheetalkamat May 13, 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
4 changes: 0 additions & 4 deletions src/compiler/diagnosticMessages.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4200,10 +4200,6 @@
"category": "Error",
"code": 4085
},
"Conflicting definitions for '{0}' found at '{1}' and '{2}'. Consider installing a specific version of this library to resolve the conflict.": {
"category": "Error",
"code": 4090
},
"Parameter '{0}' of index signature from exported interface has or is using name '{1}' from private module '{2}'.": {
"category": "Error",
"code": 4091
Expand Down
43 changes: 2 additions & 41 deletions src/compiler/program.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1589,7 +1589,6 @@ export function createProgram(rootNamesOrOptions: readonly string[] | CreateProg
const cachedBindAndCheckDiagnosticsForFile: DiagnosticCache<Diagnostic> = {};
const cachedDeclarationDiagnosticsForFile: DiagnosticCache<DiagnosticWithLocation> = {};

let resolvedTypeReferenceDirectives = createModeAwareCache<ResolvedTypeReferenceDirectiveWithFailedLookupLocations>();
let fileProcessingDiagnostics: FilePreprocessingDiagnostics[] | undefined;
let automaticTypeDirectiveNames: string[] | undefined;
let automaticTypeDirectiveResolutions: ModeAwareCache<ResolvedTypeReferenceDirectiveWithFailedLookupLocations>;
Expand Down Expand Up @@ -1929,7 +1928,6 @@ export function createProgram(rootNamesOrOptions: readonly string[] | CreateProg
resolvedLibProcessing = undefined;
resolvedModulesProcessing = undefined;
resolvedTypeReferenceDirectiveNamesProcessing = undefined;
resolvedTypeReferenceDirectives = undefined!;

const program: Program = {
getRootFileNames: () => rootNames,
Expand Down Expand Up @@ -4022,55 +4020,18 @@ export function createProgram(rootNamesOrOptions: readonly string[] | CreateProg
reason: FileIncludeReason,
): void {
addResolutionDiagnostics(resolution);
// If we already found this library as a primary reference - nothing to do
const previousResolution = resolvedTypeReferenceDirectives.get(typeReferenceDirective, mode)?.resolvedTypeReferenceDirective;
if (previousResolution && previousResolution.primary) {
return;
}
let saveResolution = true;
const { resolvedTypeReferenceDirective } = resolution;
if (resolvedTypeReferenceDirective) {
if (resolvedTypeReferenceDirective.isExternalLibraryImport) currentNodeModulesDepth++;

if (resolvedTypeReferenceDirective.primary) {
// resolved from the primary path
processSourceFile(resolvedTypeReferenceDirective.resolvedFileName!, /*isDefaultLib*/ false, /*ignoreNoDefaultLib*/ false, resolvedTypeReferenceDirective.packageId, reason); // TODO: GH#18217
}
else {
// If we already resolved to this file, it must have been a secondary reference. Check file contents
// for sameness and possibly issue an error
if (previousResolution) {
// Don't bother reading the file again if it's the same file.
if (resolvedTypeReferenceDirective.resolvedFileName !== previousResolution.resolvedFileName) {
const otherFileText = host.readFile(resolvedTypeReferenceDirective.resolvedFileName!);
const existingFile = getSourceFile(previousResolution.resolvedFileName!)!;
if (otherFileText !== existingFile.text) {
addFilePreprocessingFileExplainingDiagnostic(
existingFile,
reason,
Diagnostics.Conflicting_definitions_for_0_found_at_1_and_2_Consider_installing_a_specific_version_of_this_library_to_resolve_the_conflict,
[typeReferenceDirective, resolvedTypeReferenceDirective.resolvedFileName!, previousResolution.resolvedFileName!],
);
}
}
// don't overwrite previous resolution result
saveResolution = false;
}
else {
// First resolution of this library
processSourceFile(resolvedTypeReferenceDirective.resolvedFileName!, /*isDefaultLib*/ false, /*ignoreNoDefaultLib*/ false, resolvedTypeReferenceDirective.packageId, reason);
}
}
// resolved from the primary path
processSourceFile(resolvedTypeReferenceDirective.resolvedFileName!, /*isDefaultLib*/ false, /*ignoreNoDefaultLib*/ false, resolvedTypeReferenceDirective.packageId, reason); // TODO: GH#18217

if (resolvedTypeReferenceDirective.isExternalLibraryImport) currentNodeModulesDepth--;
}
else {
addFilePreprocessingFileExplainingDiagnostic(/*file*/ undefined, reason, Diagnostics.Cannot_find_type_definition_file_for_0, [typeReferenceDirective]);
}

if (saveResolution) {
resolvedTypeReferenceDirectives.set(typeReferenceDirective, mode, resolution);
}
}

function pathForLibFile(libFileName: string): string {
Expand Down
6 changes: 5 additions & 1 deletion tests/baselines/reference/library-reference-4.symbols
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,14 @@ declare var foo: any;

=== /node_modules/foo/node_modules/alpha/index.d.ts ===
declare var alpha: any;
>alpha : Symbol(alpha, Decl(index.d.ts, 0, 11))
>alpha : Symbol(alpha, Decl(index.d.ts, 0, 11), Decl(index.d.ts, 0, 11))
Copy link
Member Author

@sheetalkamat sheetalkamat May 14, 2024

Choose a reason for hiding this comment

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

These didnt have package.json so didnt get deduped. But I added test case 4b and 5b where there is package.json


=== /node_modules/bar/index.d.ts ===
/// <reference types="alpha" />
declare var bar: any;
>bar : Symbol(bar, Decl(index.d.ts, 1, 11))

=== /node_modules/bar/node_modules/alpha/index.d.ts ===
declare var alpha: any;
>alpha : Symbol(alpha, Decl(index.d.ts, 0, 11), Decl(index.d.ts, 0, 11))

5 changes: 5 additions & 0 deletions tests/baselines/reference/library-reference-4.trace.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,11 @@
"File '/node_modules/bar/node_modules/alpha/index.d.ts' exists - use it as a name resolution result.",
"Resolving real path for '/node_modules/bar/node_modules/alpha/index.d.ts', result '/node_modules/bar/node_modules/alpha/index.d.ts'.",
"======== Type reference directive 'alpha' was successfully resolved to '/node_modules/bar/node_modules/alpha/index.d.ts', primary: false. ========",
"File '/node_modules/bar/node_modules/alpha/package.json' does not exist according to earlier cached lookups.",
"File '/node_modules/bar/node_modules/package.json' does not exist.",
"File '/node_modules/bar/package.json' does not exist according to earlier cached lookups.",
"File '/node_modules/package.json' does not exist according to earlier cached lookups.",
"File '/package.json' does not exist according to earlier cached lookups.",
"File '/.ts/package.json' does not exist.",
"File '/package.json' does not exist according to earlier cached lookups.",
"======== Resolving module '@typescript/lib-es5' from '/.src/test/__lib_node_modules_lookup_lib.es5.d.ts__.ts'. ========",
Expand Down
4 changes: 4 additions & 0 deletions tests/baselines/reference/library-reference-4.types
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,3 +19,7 @@ declare var alpha: any;
declare var bar: any;
>bar : any

=== /node_modules/bar/node_modules/alpha/index.d.ts ===
declare var alpha: any;
>alpha : any

18 changes: 6 additions & 12 deletions tests/baselines/reference/library-reference-5.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,4 @@
/node_modules/bar/index.d.ts(1,23): error TS4090: Conflicting definitions for 'alpha' found at '/node_modules/bar/node_modules/alpha/index.d.ts' and '/node_modules/foo/node_modules/alpha/index.d.ts'. Consider installing a specific version of this library to resolve the conflict.
The file is in the program because:
Type library referenced via 'alpha' from file '/node_modules/foo/index.d.ts'
Type library referenced via 'alpha' from file '/node_modules/bar/index.d.ts'
/node_modules/bar/node_modules/alpha/index.d.ts(1,13): error TS2403: Subsequent variable declarations must have the same type. Variable 'alpha' must be of type 'any', but here has type '{}'.


==== /src/root.ts (0 errors) ====
Expand All @@ -15,16 +12,13 @@
==== /node_modules/foo/node_modules/alpha/index.d.ts (0 errors) ====
declare var alpha: any;

==== /node_modules/bar/index.d.ts (1 errors) ====
==== /node_modules/bar/index.d.ts (0 errors) ====
/// <reference types="alpha" />
~~~~~
!!! error TS4090: Conflicting definitions for 'alpha' found at '/node_modules/bar/node_modules/alpha/index.d.ts' and '/node_modules/foo/node_modules/alpha/index.d.ts'. Consider installing a specific version of this library to resolve the conflict.
!!! error TS4090: The file is in the program because:
!!! error TS4090: Type library referenced via 'alpha' from file '/node_modules/foo/index.d.ts'
!!! error TS4090: Type library referenced via 'alpha' from file '/node_modules/bar/index.d.ts'
!!! related TS1404 /node_modules/foo/index.d.ts:1:23: File is included via type library reference here.
declare var bar: any;

==== /node_modules/bar/node_modules/alpha/index.d.ts (0 errors) ====
==== /node_modules/bar/node_modules/alpha/index.d.ts (1 errors) ====
declare var alpha: {};
~~~~~
!!! error TS2403: Subsequent variable declarations must have the same type. Variable 'alpha' must be of type 'any', but here has type '{}'.
!!! related TS6203 /node_modules/foo/node_modules/alpha/index.d.ts:1:13: 'alpha' was also declared here.
Comment on lines +19 to +23
Copy link
Member

@jakebailey jakebailey May 14, 2024

Choose a reason for hiding this comment

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

I was going to say "the old message seemed useful because it said where the dupe was", but I guess this related line would be enough. Though it's moderately a shame to lose a message that gives a potential solution.

Copy link
Member Author

@sheetalkamat sheetalkamat May 14, 2024

Choose a reason for hiding this comment

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

The things work much better where you could have two typings that dont add to global and used by two packages and hence two different versions. This handles that well. We have this issue with module resolution. The only message we gave was with typeRef and it wasnt correct checking either so i feel better removing this custom logic for typeRef only and allows packages to use typeRef of different versions if they dont add conflicting global info.


6 changes: 5 additions & 1 deletion tests/baselines/reference/library-reference-5.symbols
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,14 @@ declare var foo: any;

=== /node_modules/foo/node_modules/alpha/index.d.ts ===
declare var alpha: any;
>alpha : Symbol(alpha, Decl(index.d.ts, 0, 11))
>alpha : Symbol(alpha, Decl(index.d.ts, 0, 11), Decl(index.d.ts, 0, 11))

=== /node_modules/bar/index.d.ts ===
/// <reference types="alpha" />
declare var bar: any;
>bar : Symbol(bar, Decl(index.d.ts, 1, 11))

=== /node_modules/bar/node_modules/alpha/index.d.ts ===
declare var alpha: {};
>alpha : Symbol(alpha, Decl(index.d.ts, 0, 11), Decl(index.d.ts, 0, 11))

5 changes: 5 additions & 0 deletions tests/baselines/reference/library-reference-5.trace.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,11 @@
"File '/node_modules/bar/node_modules/alpha/index.d.ts' exists - use it as a name resolution result.",
"Resolving real path for '/node_modules/bar/node_modules/alpha/index.d.ts', result '/node_modules/bar/node_modules/alpha/index.d.ts'.",
"======== Type reference directive 'alpha' was successfully resolved to '/node_modules/bar/node_modules/alpha/index.d.ts', primary: false. ========",
"File '/node_modules/bar/node_modules/alpha/package.json' does not exist according to earlier cached lookups.",
"File '/node_modules/bar/node_modules/package.json' does not exist.",
"File '/node_modules/bar/package.json' does not exist according to earlier cached lookups.",
"File '/node_modules/package.json' does not exist according to earlier cached lookups.",
"File '/package.json' does not exist according to earlier cached lookups.",
"File '/.ts/package.json' does not exist.",
"File '/package.json' does not exist according to earlier cached lookups.",
"======== Resolving module '@typescript/lib-es5' from '/__lib_node_modules_lookup_lib.es5.d.ts__.ts'. ========",
Expand Down
5 changes: 5 additions & 0 deletions tests/baselines/reference/library-reference-5.types
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,3 +22,8 @@ declare var bar: any;
>bar : any
> : ^^^

=== /node_modules/bar/node_modules/alpha/index.d.ts ===
declare var alpha: {};
>alpha : any
> : ^^^

1 change: 1 addition & 0 deletions ...tsserver/typeReferenceDirectives/when-typeReferenceDirective-contains-UpperCasePackage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,7 @@ Info seq [hh:mm:ss:mss] Files (4)
Matched by default include pattern '**/*'
../lib/@types/UpperCasePackage/index.d.ts
Entry point for implicit type library 'UpperCasePackage'
Type library referenced via 'UpperCasePackage' from file '../lib/@app/lib/index.d.ts'
../lib/@app/lib/index.d.ts
Entry point for implicit type library 'lib'

Expand Down