fix: Corsa differences in export= module augmentation - #4446
fix: Corsa differences in export= module augmentation#4446pratheeknathani wants to merge 2 commits into
export= module augmentation#4446Conversation
Fixes microsoft/typescript-go#3481
There was a problem hiding this comment.
Pull request overview
Fixes a declaration-emit visibility edge case where an export = target (and its nested exported members) wasn’t treated as externally visible during .d.ts emit, causing a spurious TS4060 and preventing declaration output for a module augmentation scenario.
Changes:
- Treat declarations that are the target of an
export =in their containing external module as visible during declaration emit visibility checks. - Update submodule baselines to remove the erroneous TS4060 diagnostic and restore
a.d.tsemission for the affected test. - Remove now-unneeded triaged diff baselines for this case.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| internal/checker/emitresolver.go | Adds an export =-target visibility fast-path in determineIfDeclarationIsVisible. |
| testdata/baselines/reference/submoduleTriaged/compiler/exportAssignmentMembersVisibleInAugmentation.errors.txt.diff | Removes the triaged diff that captured the now-fixed TS4060 discrepancy. |
| testdata/baselines/reference/submodule/compiler/exportAssignmentMembersVisibleInAugmentation.errors.txt | Removes the error baseline now that TS4060 no longer occurs. |
| testdata/baselines/reference/submodule/compiler/exportAssignmentMembersVisibleInAugmentation.js | Updates baseline to include the restored a.d.ts output. |
| testdata/baselines/reference/submodule/compiler/exportAssignmentMembersVisibleInAugmentation.js.diff | Removes the diff baseline since the output now matches the reference expectation. |
| export function f(): T; // OK | ||
| ~ | ||
| !!! error TS4060: Return type of exported function has or is using private name 'T'. |
There was a problem hiding this comment.
Odd that this begins working
There was a problem hiding this comment.
Yeah, this lines up with what upstream TypeScript already does here (which is why there was a submoduleTriaged diff recording Corsa diverging from it).
TS marks foo/T visible via collectLinkedAliases(setVisibility=true) on the export = foo in index.d.ts, but Corsa moved that into PrecalculateDeclarationEmitVisibility, which only walks the file being emitted (a.ts), so the export = in the never-emitted index.d.ts was missed and the target fell through to isGlobalSourceFile and returned false. The fix just treats a declaration that's the export = target of its own module as visible, so it matches TS again.
There was a problem hiding this comment.
This sort of cross-file analysis is very spooky... It means emit can differ based on whether or not we use the checker for emit or not. Probably that is inevitable for this particular location, however
|
TypeScript Bot (@typescript-bot) perf test this faster |
|
Jake Bailey (@jakebailey) Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
The PR deleted submoduleTriaged/compiler/exportAssignmentMembersVisibleInAugmentation.errors.txt.diff (the diff is now resolved) but left its entry in testdata/submoduleTriaged.txt. TestSubmoduleTriagedFilesExist in internal/testutil/baseline asserts every listed file exists, so it failed across every go test ./... job (no-submodules, noembed, race, ubuntu, windows, concurrent, generate). Remove the stale line; the issue-3481 group keeps augmentExportEquals2.errors.txt.diff, which is still a tracked diff.
|
Pushed The earlier commit correctly deleted the now-resolved triaged baseline The follow-up removes that single stale line. The issue-3481 group still keeps |
| // isExportEqualsTargetDeclaration reports whether the given declaration's symbol is the | ||
| // entity referenced by an `export =` in its containing external module. Such a declaration | ||
| // is externally visible because the `export =` re-exports it. | ||
| func (r *EmitResolver) isExportEqualsTargetDeclaration(node *ast.Node) bool { |
There was a problem hiding this comment.
Not a huge fan of this - it reads like a kludgy workaround that doesn't actually fix the root cause. determineIfDeclarationIsVisible doesn't have any symbolic logic like this in it directly thus far - it's all just AST walking and recursion. markLinkedAliases in the emit resolver (which is analogous to the strada checker's collectLinkedAliases, not to be confused with markLinkedReferences which is for tracking import name usage for import elision) would seem to be the place where we should be updating our logic to handle cross-file NS merges better - likely by following imported names to their origin file and marking linked aliases on those, too.
Basically, what I'd recommend is having PrecalculateDeclarationEmitVisibility call file.AsNode().ForEachChild(r.aliasMarkingVisitor) not just on the input file, but on any files the input file references, so those namespace-relevant links get set (recursively - which'll require tweaking the locking and caching a bit, but should be totally doable). You can also remove the TODO from that, since this case pretty conclusively shows that cross-file NS merging (since we added it back in) requires this be an upfront walk.
|
Thank you for contributing to the TypeScript native port! Development has moved from this repository back to the main microsoft/TypeScript repository. GitHub does not have PR transfer functionality, so we're closing this PR here. If this change is still relevant, please reopen it as a new pull request in See microsoft/typescript-go#4918 for more information about the move. |
Summary
Test setup: // /node_modules/foo/index.d.ts export = foo; declare namespace foo { export type T = number; } // /a.ts import * as foo from "foo"; declare module "foo" { export function f(): T; // OK in TS } Corsa wrongly reported
TS4060: Return type of exported function has or is using private name 'T'when emitting...What changed
Test setup:
Corsa wrongly reported
TS4060: Return type of exported function has or is using private name 'T'when emittinga.d.ts. As a resulta.d.tswas never emitted.During declaration emit,
Tis resolved correctly, but its visibility is checked through its container, the namespacefoo.foois not markedexport; it is exported only throughexport = fooinindex.d.ts. TypeScript makes such anexport =target visible by callingcollectLinkedAliases(id, /*setVisibility*/ true)insidecheckExportAssignment, which runs while checking every file, including non-emitted ones likeindex.d.ts.Corsa moved that alias-visibility marking into the emit resolver (
PrecalculateDeclarationEmitVisibility), which only walks the file currently being emitted (a.ts/b.ts). Theexport = foolives inindex.d.ts, which is never emitted, sofoo(and thereforeT) was never marked visible.determineIfDeclarationIsVisiblethen fell through toisGlobalSourceFile(parent)and returned false, producing the spurious TS4060.Issue
Fixes microsoft/TypeScript#63814
Issue: microsoft/TypeScript#63814
Diffstat
Testing
Ran the relevant tests and linter for the changed files while developing.
Kept the change minimal and focused on this one issue.
AI assistance
I used GitHub Copilot to help write parts of this change. I've reviewed and tested it myself, I understand what it does, and I'll follow up on any review feedback.