-
Notifications
You must be signed in to change notification settings - Fork 767
Add "package deduplication", --disablePackageDeduplication #2361
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
9b349c1
dc9841e
d81c18e
358bd71
ed7017c
f98b377
ac7cfc5
43f6e08
b99c0ff
e25f2bc
044eee2
d47b3a7
d7e8232
af6f313
03a3d8f
27d78af
cf0bc18
fb1e84f
614dafd
cae2fba
34155c9
68627e3
2bb7300
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -283,6 +283,17 @@ func (w *filesParser) getProcessedFiles(loader *fileLoader) processedFiles { | |||||||||||||||||||||||||||||||||
| var sourceFilesFoundSearchingNodeModules collections.Set[tspath.Path] | ||||||||||||||||||||||||||||||||||
| libFilesMap := make(map[tspath.Path]*LibFile, libFileCount) | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| var sourceFileToPackageName map[tspath.Path]string | ||||||||||||||||||||||||||||||||||
| var redirectTargetsMap map[tspath.Path][]string | ||||||||||||||||||||||||||||||||||
| var deduplicatedPathMap map[tspath.Path]tspath.Path | ||||||||||||||||||||||||||||||||||
| var packageIdToCanonicalPath map[module.PackageId]tspath.Path | ||||||||||||||||||||||||||||||||||
| if !loader.opts.Config.CompilerOptions().DisablePackageDeduplication.IsTrue() { | ||||||||||||||||||||||||||||||||||
| sourceFileToPackageName = make(map[tspath.Path]string, totalFileCount) | ||||||||||||||||||||||||||||||||||
| redirectTargetsMap = make(map[tspath.Path][]string) | ||||||||||||||||||||||||||||||||||
| deduplicatedPathMap = make(map[tspath.Path]tspath.Path) | ||||||||||||||||||||||||||||||||||
| packageIdToCanonicalPath = make(map[module.PackageId]tspath.Path) | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| var collectFiles func(tasks []*parseTask, seen map[*parseTaskData]string) | ||||||||||||||||||||||||||||||||||
| collectFiles = func(tasks []*parseTask, seen map[*parseTaskData]string) { | ||||||||||||||||||||||||||||||||||
| for _, task := range tasks { | ||||||||||||||||||||||||||||||||||
|
|
@@ -331,6 +342,36 @@ func (w *filesParser) getProcessedFiles(loader *fileLoader) processedFiles { | |||||||||||||||||||||||||||||||||
| for _, trace := range task.resolutionsTrace { | ||||||||||||||||||||||||||||||||||
| loader.opts.Host.Trace(trace.Message, trace.Args...) | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| if packageIdToCanonicalPath != nil { | ||||||||||||||||||||||||||||||||||
| for _, resolution := range task.resolutionsInFile { | ||||||||||||||||||||||||||||||||||
| if !resolution.IsResolved() { | ||||||||||||||||||||||||||||||||||
| continue | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| pkgId := resolution.PackageId | ||||||||||||||||||||||||||||||||||
| if pkgId.Name == "" { | ||||||||||||||||||||||||||||||||||
| continue | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| resolvedPath := loader.toPath(resolution.ResolvedFileName) | ||||||||||||||||||||||||||||||||||
| packageName := pkgId.PackageName() | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| if canonical, exists := packageIdToCanonicalPath[pkgId]; exists { | ||||||||||||||||||||||||||||||||||
| if _, alreadyRecorded := sourceFileToPackageName[resolvedPath]; !alreadyRecorded { | ||||||||||||||||||||||||||||||||||
| sourceFileToPackageName[resolvedPath] = packageName | ||||||||||||||||||||||||||||||||||
| if resolvedPath != canonical { | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| deduplicatedPathMap[resolvedPath] = canonical | ||||||||||||||||||||||||||||||||||
| redirectTargetsMap[canonical] = append(redirectTargetsMap[canonical], resolution.ResolvedFileName) | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||
| packageIdToCanonicalPath[pkgId] = resolvedPath | ||||||||||||||||||||||||||||||||||
| sourceFileToPackageName[resolvedPath] = packageName | ||||||||||||||||||||||||||||||||||
| deduplicatedPathMap[resolvedPath] = resolvedPath | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| if subTasks := task.subTasks; len(subTasks) > 0 { | ||||||||||||||||||||||||||||||||||
| collectFiles(subTasks, seen) | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
@@ -408,6 +449,22 @@ func (w *filesParser) getProcessedFiles(loader *fileLoader) processedFiles { | |||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| if deduplicatedPathMap != nil { | ||||||||||||||||||||||||||||||||||
| for duplicatePath, canonicalPath := range deduplicatedPathMap { | ||||||||||||||||||||||||||||||||||
| if duplicatePath != canonicalPath { | ||||||||||||||||||||||||||||||||||
| if canonicalFile, ok := filesByPath[canonicalPath]; ok { | ||||||||||||||||||||||||||||||||||
| filesByPath[duplicatePath] = canonicalFile | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| allFiles = slices.DeleteFunc(allFiles, func(f *ast.SourceFile) bool { | ||||||||||||||||||||||||||||||||||
| if canonicalPath, ok := deduplicatedPathMap[f.Path()]; ok { | ||||||||||||||||||||||||||||||||||
| return f.Path() != canonicalPath | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| return false | ||||||||||||||||||||||||||||||||||
|
Comment on lines
+461
to
+464
|
||||||||||||||||||||||||||||||||||
| if canonicalPath, ok := deduplicatedPathMap[f.Path()]; ok { | |
| return f.Path() != canonicalPath | |
| } | |
| return false | |
| canonicalPath, ok := deduplicatedPathMap[f.Path()] | |
| if !ok { | |
| return false | |
| } | |
| // Only delete a duplicate when the canonical file actually exists in filesByPath. | |
| if f.Path() == canonicalPath { | |
| return false | |
| } | |
| if _, exists := filesByPath[canonicalPath]; !exists { | |
| return false | |
| } | |
| return true |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,68 @@ | ||
| package fourslash_test | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/microsoft/typescript-go/internal/fourslash" | ||
| "github.com/microsoft/typescript-go/internal/testutil" | ||
| ) | ||
|
|
||
| func TestDuplicatePackageServices_fileChanges(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| defer testutil.RecoverAndFail(t, "Panic on fourslash test") | ||
| const content = `// @noImplicitReferences: true | ||
| // @Filename: /node_modules/a/index.d.ts | ||
| import X from "x"; | ||
| export function a(x: X): void; | ||
| // @Filename: /node_modules/a/node_modules/x/index.d.ts | ||
| export default class /*defAX*/X { | ||
| private x: number; | ||
| } | ||
| // @Filename: /node_modules/a/node_modules/x/package.json | ||
| { "name": "x", "version": "1.2./*aVersionPatch*/3" } | ||
| // @Filename: /node_modules/b/index.d.ts | ||
| import X from "x"; | ||
| export const b: X; | ||
| // @Filename: /node_modules/b/node_modules/x/index.d.ts | ||
| export default class /*defBX*/X { | ||
| private x: number; | ||
| } | ||
| // @Filename: /node_modules/b/node_modules/x/package.json | ||
| { "name": "x", "version": "1.2./*bVersionPatch*/3" } | ||
| // @Filename: /src/a.ts | ||
| import { a } from "a"; | ||
| import { b } from "b"; | ||
| a(/*error*/b);` | ||
| f, done := fourslash.NewFourslash(t, nil /*capabilities*/, content) | ||
| defer done() | ||
|
|
||
| f.GoToFile(t, "/src/a.ts") | ||
| f.VerifyNumberOfErrorsInCurrentFile(t, 0) | ||
|
|
||
| testChangeAndChangeBack := func(versionPatch string, def string) { | ||
| // Insert "4" after the version patch marker, changing version from 1.2.3 to 1.2.43 | ||
| f.GoToMarker(t, versionPatch) | ||
| f.Insert(t, "4") | ||
|
|
||
| // Insert a space after the definition marker to trigger a recheck | ||
| f.GoToMarker(t, def) | ||
| f.Insert(t, " ") | ||
|
|
||
| // No longer have identical packageId, so we get errors. | ||
| f.VerifyErrorExistsAfterMarker(t, "error") | ||
|
|
||
| // Undo the changes | ||
| f.GoToMarker(t, versionPatch) | ||
| f.DeleteAtCaret(t, 1) | ||
| f.GoToMarker(t, def) | ||
| f.DeleteAtCaret(t, 1) | ||
|
|
||
| // Back to being identical. | ||
| f.GoToFile(t, "/src/a.ts") | ||
| f.VerifyNumberOfErrorsInCurrentFile(t, 0) | ||
| } | ||
|
|
||
| testChangeAndChangeBack("aVersionPatch", "defAX") | ||
| testChangeAndChangeBack("bVersionPatch", "defBX") | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| /src/a.ts(5,3): error TS2345: Argument of type 'import("/node_modules/c/node_modules/x/index").default' is not assignable to parameter of type 'import("/node_modules/a/node_modules/x/index").default'. | ||
| Types have separate declarations of a private property 'x'. | ||
|
|
||
|
|
||
| ==== /src/a.ts (1 errors) ==== | ||
| import { a } from "a"; | ||
| import { b } from "b"; | ||
| import { c } from "c"; | ||
| a(b); // Works | ||
| a(c); // Error, these are from different versions of the library. | ||
| ~ | ||
| !!! error TS2345: Argument of type 'import("/node_modules/c/node_modules/x/index").default' is not assignable to parameter of type 'import("/node_modules/a/node_modules/x/index").default'. | ||
| !!! error TS2345: Types have separate declarations of a private property 'x'. | ||
|
|
||
| ==== /node_modules/a/index.d.ts (0 errors) ==== | ||
| import X from "x"; | ||
| export function a(x: X): void; | ||
|
|
||
| ==== /node_modules/a/node_modules/x/index.d.ts (0 errors) ==== | ||
| export default class X { | ||
| private x: number; | ||
| } | ||
|
|
||
| ==== /node_modules/a/node_modules/x/package.json (0 errors) ==== | ||
| { "name": "x", "version": "1.2.3" } | ||
|
|
||
| ==== /node_modules/b/index.d.ts (0 errors) ==== | ||
| import X from "x"; | ||
| export const b: X; | ||
|
|
||
| ==== /node_modules/b/node_modules/x/index.d.ts (0 errors) ==== | ||
| content not parsed | ||
|
|
||
| ==== /node_modules/b/node_modules/x/package.json (0 errors) ==== | ||
| { "name": "x", "version": "1.2.3" } | ||
|
|
||
| ==== /node_modules/c/index.d.ts (0 errors) ==== | ||
| import X from "x"; | ||
| export const c: X; | ||
|
|
||
| ==== /node_modules/c/node_modules/x/index.d.ts (0 errors) ==== | ||
| export default class X { | ||
| private x: number; | ||
| } | ||
|
|
||
| ==== /node_modules/c/node_modules/x/package.json (0 errors) ==== | ||
| { "name": "x", "version": "1.2.4" } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic for deduplication relies on recording canonical paths during file collection, but canonical files are chosen in a non-deterministic manner based on the order resolutions are encountered. Consider adding documentation or a comment explaining which file becomes canonical (first encountered), since this ordering might matter for debugging or when understanding the behavior. Also consider whether sorting package IDs or using a deterministic ordering would improve consistency across runs.