fix(auto-import): don't suggest module specifiers under shadowed export/import conditions - #64183
Open
erantianantha wants to merge 1 commit into
Open
Conversation
…rt/import conditions Fixes microsoft#64171
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Critical fallback-reachability logic and invalid-target capture behavior remain incorrect.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Prevents auto-import suggestions for module specifiers shadowed by earlier active package conditions.
Changes:
- Tracks runtime condition capture during specifier generation.
- Adds condition exclusions when enumerating package entrypoints.
- Adds unit and fourslash regression coverage.
File summaries
| File | Review |
|---|---|
tsc/internal/modulespecifiers/specifiers.go |
Moderate: Invalid string targets must capture resolution because Node throws rather than falling through to later conditions. |
tsc/internal/modulespecifiers/specifiers_test.go |
Tests target-capture behavior; expectations must reflect invalid string targets. |
tsc/internal/module/resolver.go |
Critical: Excluding preceding active conditions unconditionally breaks reachable nested-condition fallbacks; preserve the nested target predicate. |
tsc/internal/fourslash/tests/autoImportPackageJsonImportsShadowedCondition4_test.go |
Tests null imports targets. |
tsc/internal/fourslash/tests/autoImportPackageJsonImportsShadowedCondition3_test.go |
Tests custom conditions. |
tsc/internal/fourslash/tests/autoImportPackageJsonImportsShadowedCondition2_test.go |
Tests active imports targets. |
tsc/internal/fourslash/tests/autoImportPackageJsonImportsShadowedCondition1_test.go |
Tests shadowed imports fallbacks. |
tsc/internal/fourslash/tests/autoImportPackageJsonExportsShadowedCondition3_test.go |
Tests ESM type fallbacks. |
tsc/internal/fourslash/tests/autoImportPackageJsonExportsShadowedCondition2_test.go |
Tests type-condition transparency. |
tsc/internal/fourslash/tests/autoImportPackageJsonExportsShadowedCondition1_test.go |
Tests shadowed exports targets. |
Review details
Suppressed comments (3)
tsc/internal/modulespecifiers/specifiers.go:1371
- Empty arrays also capture the enclosing condition: package-target resolution returns
nullfor an empty array (and for an array whose elements all returnundefined), so the conditional-object algorithm does not inspect a later key.core.Somereturnsfalsefor[], causing{ "node": [], "default": "./valid.js" }to incorrectly produce a specifier fromdefault. Arrays should be treated as capturing, and the empty-array test should expecttrue.
case packagejson.JSONValueTypeArray:
return core.Some(target.AsArray(), func(elem packagejson.ExportsOrImports) bool {
return targetCapturesResolution(elem, conditions, isImports)
})
tsc/internal/modulespecifiers/specifiers.go:1370
- The new array capture logic is not applied while traversing arrays: the array branch at lines 1302–1309 still tries every later element until one produces a module name. Consequently, targets such as
[null, "./src/*.ts"](and a valid-but-missing first runtime target) can still produce an alias from the second element even though resolution has already been captured. Stop traversing an array after an element for whichtargetCapturesResolutionis true.
case packagejson.JSONValueTypeArray:
return core.Some(target.AsArray(), func(elem packagejson.ExportsOrImports) bool {
return targetCapturesResolution(elem, conditions, isImports)
tsc/internal/modulespecifiers/specifiers.go:1387
- This accepts every
./target, but the resolver rejects targets containing..,., ornode_modulespath segments (module/resolver.go:782-793). For an invalid earlier condition such as"./dist/../other.js", this helper therefore marks resolution as captured and suppresses a later fallback that the resolver would actually inspect.
if strings.HasPrefix(target, "./") {
return true
}
- Files reviewed: 10/10 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
2334
to
+2335
| for _, prevCondition := range prevConditions { | ||
| if excludeConditions == nil { | ||
| excludeConditions = &collections.Set[string]{} | ||
| } | ||
| excludeConditions.Add(prevCondition) | ||
| newExcludeConditions.Add(prevCondition) |
Comment on lines
+1363
to
+1365
| case packagejson.JSONValueTypeString: | ||
| str, _ := target.Value.(string) | ||
| return isValidRuntimeTarget(str, isImports) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #64171
Problem
When generating auto-import suggestions from
package.jsonexportsorimports, the compiler walks all matching conditional keys. In runtime resolution (Node.js ESM /nodenext), the resolver commits to the first matching active condition and never falls through to later conditions even if the file target does not exist.Previously,
tryGetModuleNameFromExportsOrImportswould continue iterating through subsequent runtime conditions (such as"default"), offering auto-import specifier aliases that fail at runtime withERR_MODULE_NOT_FOUND.Solution
tryGetModuleNameFromExportsOrImports, track when an active runtime condition has committed to a valid target usingtargetCapturesResolution. Once captured, subsequent runtime conditions in that mapping are skipped.typesandtypes@...) are kept transparent since they are invisible to runtime resolvers and should neither shadow nor be shadowed by runtime conditions.loadEntrypointsFromExportMap(resolver.go), properly record preceding runtime conditions innewExcludeConditionsso fallback targets are not treated as reachable entrypoints when earlier conditions apply.Testing
specifiers_test.goverifyingtargetCapturesResolutionacross relative paths, bare specifiers, null targets, and nested objects.exportsand#importswith shadowed condition branches.