Avoid false symlink mappings for physical dependencies - #4813
Conversation
There was a problem hiding this comment.
🟢 Ready to approve
The targeted guard matches resolver semantics and the regression test validates the corrected declaration output.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Prevents false symlink mappings for physical dependencies, fixing invalid module specifiers during declaration emit.
Changes:
- Requires a non-empty
OriginalPathbefore recording package symlinks. - Adds a compiler regression test and expected declaration baseline.
File summaries
| File | Description |
|---|---|
internal/compiler/program.go |
Guards symlink-cache insertion for physical packages. |
testdata/tests/cases/compiler/jsDeclarationEmitDoesNotReuseUnrelatedJSDocImport.ts |
Reproduces the declaration-emit regression. |
testdata/baselines/reference/compiler/jsDeclarationEmitDoesNotReuseUnrelatedJSDocImport.js |
Verifies the correct external module specifier. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
|
@microsoft-github-policy-service agree company="Hyperparam" |
|
@typescript-bot test it |
|
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@jakebailey Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
Summary
Fixes declaration emit incorrectly reusing an unrelated JSDoc import when referencing a type from a physically installed dependency.
Related issue: microsoft/TypeScript#63705
Reproduction: https://github.com/platypii/typescript-7-jsdoc-private-alias-repro
Root cause
For a normal, non-symlinked package,
ResolvePackageDirectoryreturns a resolved package directory with an emptyOriginalPath.GetSymlinkCacheappendedpackage.jsonto that empty path before callingProcessResolution. This produced the relative pathpackage.json, which was then resolved against the project directory. As a result, the symlink cache incorrectly recorded the project root as a symlink to the dependency.The module specifier generator could consequently treat a project source file as an alternative location for the dependency's type and reuse an unrelated JSDoc module specifier. This produced declarations such as:
even though that module does not export
SchemaElement.Fix
Only add package-directory resolutions to the symlink cache when
OriginalPathis non-empty. An emptyOriginalPathmeans there is no symlink mapping to record.A regression test verifies that declaration emit instead references the type's public external source:
Reproduction note
The standalone reproduction contains
.npmrcwithinstall-links=trueonly because it models the dependency using a localfile:package, which npm would otherwise install as a symlink.Registry dependencies are normally installed as physical directories under
node_modules, so users do not need this npm setting to encounter the bug in a real project.Tests
go test ./internal/testrunner -run 'TestLocal/jsDeclarationEmitDoesNotReuseUnrelatedJSDocImport' -count=1go test ./internal/compiler ./internal/modulespecifiers ./internal/symlinks