fix(es/resolver): Merge re-opened TypeScript namespace scopes#11872
fix(es/resolver): Merge re-opened TypeScript namespace scopes#11872MavenRain wants to merge 5 commits into
Conversation
|
|
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b94ef6823a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| child.current.declared_symbols = body.declared_symbols; | ||
| child.current.declared_types = body.declared_types; |
There was a problem hiding this comment.
Copying declared_symbols/declared_types wholesale into later namespace declarations makes non-exported members from an earlier declaration visible in a re-opened one, which violates TypeScript namespace-merge semantics and can change runtime binding resolution. For example, namespace N { const a = 1 } namespace N { const b = a } should not bind a from the first body, but this cache import does exactly that by restoring all prior locals into child.current. Please merge only exported namespace members (and their corresponding contexts), not every declaration in the previous body.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The Codex comment is correct. When multiple namespace declarations share the same name, only exported names are shared across declarations; non-exported names stay local to each declaration body.
For example:
namespace Foo {
export var a;
var b;
}
var a, b, c;
namespace Foo {
var b;
foo(a);
foo(b);
foo(c);
}This should resolve like this:
namespace Foo#1 {
export var a#2;
var b#3;
}
var a#1, b#1, c#1;
namespace Foo#1 {
var b#4;
foo(a#2);
foo(b#4);
foo(c#1);
}+------------------------------------------------------------------+
| outer scope |
| a#1, b#1, c#1 |
| |
| +----------------------------------------------------------+ |
| | namespace Foo exported names | |
| | a#2 | |
| | | |
| | +----------------------+ +----------------------+ | |
| | | namespace Foo body 1 | | namespace Foo body 2 | | |
| | | local only | | local only | | |
| | | b#3 | | b#4 | | |
| | +----------------------+ +----------------------+ | |
| +----------------------------------------------------------+ |
+------------------------------------------------------------------+
The exact internal representation is an implementation detail. The only requirement is to preserve the visibility behavior shown above.
Merging this PR will not alter performance
Comparing 
Footnotes
|
| child.current.declared_symbols = body.declared_symbols; | ||
| child.current.declared_types = body.declared_types; |
There was a problem hiding this comment.
The Codex comment is correct. When multiple namespace declarations share the same name, only exported names are shared across declarations; non-exported names stay local to each declaration body.
For example:
namespace Foo {
export var a;
var b;
}
var a, b, c;
namespace Foo {
var b;
foo(a);
foo(b);
foo(c);
}This should resolve like this:
namespace Foo#1 {
export var a#2;
var b#3;
}
var a#1, b#1, c#1;
namespace Foo#1 {
var b#4;
foo(a#2);
foo(b#4);
foo(c#1);
}+------------------------------------------------------------------+
| outer scope |
| a#1, b#1, c#1 |
| |
| +----------------------------------------------------------+ |
| | namespace Foo exported names | |
| | a#2 | |
| | | |
| | +----------------------+ +----------------------+ | |
| | | namespace Foo body 1 | | namespace Foo body 2 | | |
| | | local only | | local only | | |
| | | b#3 | | b#4 | | |
| | +----------------------+ +----------------------+ | |
| +----------------------------------------------------------+ |
+------------------------------------------------------------------+
The exact internal representation is an implementation detail. The only requirement is to preserve the visibility behavior shown above.
MavenRain
force-pushed
the
fix/resolver-namespace-merge
branch
from
b94ef68 to
cee5448
Compare
|
I updated 18 fixture files (16 crates/swc/tests/tsc-references/* files, plus swc_ecma_transforms_typescript's namespace_004 snapshot and namespace-and-enum/output.js) to reflect the merged-namespace resolver |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cee5448477
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| child.current.declared_symbols = body.declared_symbols; | ||
| child.current.declared_types = body.declared_types; |
There was a problem hiding this comment.
Rehydrating child.current.declared_symbols/declared_types with the full cached scope makes non-exported members from earlier namespace declarations visible in later re-openings, which breaks TypeScript namespace-merge rules (only exported members are shared). This causes incorrect binding and runtime/codegen changes, e.g. a later local declaration can overwrite earlier exported enum semantics; fresh evidence is in this commit’s updated crates/swc_ecma_transforms_typescript/tests/__swc_snapshots__/tests/strip.rs/namespace_004.js, where MyNamespace.MyEnum.A shifts from 1 to 2 after a non-export enum MyEnum appears in a later reopen. The cache should carry only exported namespace members (and corresponding type entries), not every declaration in the prior body.
Useful? React with 👍 / 👎.
| child.in_ts_module = true; | ||
|
|
||
| if let Some(body) = cached { | ||
| child.current.mark = body.mark; |
There was a problem hiding this comment.
Reusing the previous declaration’s scope mark for every reopened namespace body conflates namespace-private declarations across bodies, even though TypeScript keeps non-exported names local per declaration. With a shared mark, later private bindings are treated as redeclarations of earlier ones and can alter downstream emit; fresh evidence is this commit’s crates/swc/tests/tsc-references/TwoInternalModulesThatMergeEachWithExportedAndNonExportedClassesOfTheSameName.1.normal.js (new duplicate-name error) and crates/swc_ecma_transforms_typescript/tests/__swc_snapshots__/tests/strip.rs/namespace_004.js (the later non-export enum now mutates the merged enum path). Mark reuse should be limited so private declarations in each reopen keep distinct contexts.
Useful? React with 👍 / 👎.
Address review feedback from @magic-akari / Codex on swc-project#11872: only `export …` declarations should be shared across sibling re-opens of a TypeScript namespace, not every declaration. Each re-open now gets a fresh per-body scope for its private bindings; only exported names live in the cache-backed `NamespaceExportScope`, which the body scope exposes via a `shared` link so reference lookups still see sibling-declared exports with a stable `SyntaxContext`. A per-body pre-scan of `export` declarations drives the routing in `modify`, making the decision order-independent: TypeScript's var/function same-body merge still holds (`export var a = 1; for (var a; …)`) while a non-exported re-open of a sibling-exported name stays isolated — fixing the `namespace_004` `MyEnum.A` `1`→`2` regression Codex flagged. Added tests/ts-resolver/namespace_reopen_private covering @magic-akari's example; updated fixture snapshots accordingly (re-opened non-exported classes / enums no longer trigger the "defined multiple times" diagnostic). Signed-off-by: Onyeka Obi <softwareengineerasaservant@isurvivable.cv>
MavenRain
force-pushed
the
fix/resolver-namespace-merge
branch
from
cee5448 to
d9ffafb
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d9ffafbeba
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let cache_parent_mark = self | ||
| .namespace_export | ||
| .as_ref() | ||
| .map(|exp| exp.borrow().mark) | ||
| .unwrap_or(self.current.mark); | ||
| let cache_key = namespace_name.map(|name| (cache_parent_mark, name)); |
There was a problem hiding this comment.
The cache key for TsModuleDecl always uses self.namespace_export when present, so nested namespaces are merged across outer namespace reopens even when the nested declaration is not exported from the outer body. In namespace Outer { namespace Inner { export const a = 1 } } namespace Outer { namespace Inner { export const b = a } }, the second Inner incorrectly sees a from the first body, but TypeScript keeps non-exported members local to each namespace body. Fresh evidence is that cache_parent_mark is derived from self.namespace_export unconditionally here, which makes both non-exported Inner declarations share one cache entry.
Useful? React with 👍 / 👎.
…pens
Address Codex P1 on swc-project#11872: the cache key for `TsModuleDecl` previously
used `self.namespace_export.mark` whenever it was present, so a nested
namespace declared inside an outer namespace body was always keyed to
the outer's stable export mark. That made the two `Inner` declarations
in
namespace Outer { namespace Inner { export const a = 1 } }
namespace Outer { namespace Inner { export const b = a } }
collide on a single cache entry and merge, even though TypeScript keeps
non-exported members local to each declaration body.
The cache-parent mark now falls back to `self.current.mark` (the outer
re-open's per-body mark) unless the nested namespace name is in the
enclosing body's pre-scanned `namespace_export_names`. Two outer
re-opens hold distinct body marks, so their non-exported `Inner`
children land in disjoint cache entries and stay isolated; exported
nested namespaces still key on the outer's stable export mark and
merge across re-opens.
Added tests/ts-resolver/namespace_reopen_nested_private exercising the
Codex case (second `Inner`'s reference to `a` is unresolved,
confirming isolation).
nested namespaces still key on the outer's stable export mark and
merge across re-opens.
Added tests/ts-resolver/namespace_reopen_nested_private exercising the
Codex case (second `Inner`'s reference to `a` is unresolved,
confirming isolation).
Signed-off-by: Onyeka Obi <softwareengineerasaservant@isurvivable.cv>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf4ec2a736
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if let ModuleItem::ModuleDecl(ModuleDecl::ExportDecl(export)) = item { | ||
| add_decl_export_names(&export.decl, &mut scan); | ||
| } |
There was a problem hiding this comment.
pre_scan_namespace_exports currently records only ModuleDecl::ExportDecl, so export import A = ... (represented as TsImportEqualsDecl { is_export: true, .. }) never enters namespace_export_names. In a re-opened namespace, that makes A get treated as body-local instead of merged-exported, so later bodies cannot resolve A from earlier declarations and references can fall through to unresolved/outer bindings. This breaks namespace-merge behavior for exported import aliases.
Useful? React with 👍 / 👎.
| Decl::TsEnum(e) => { | ||
| scan.values.insert(e.id.sym.clone()); | ||
| scan.types.insert(e.id.sym.clone()); | ||
| } |
There was a problem hiding this comment.
Treating every exported TsEnum as a shared value declaration causes re-opened namespaces to assume a runtime enum object exists even when the earlier declaration is non-emitted (e.g. const enum or declare enum). In namespace N { export const enum E { A } } namespace N { export enum E { B } }, the first body contributes E to the merged value cache, so later emit can skip initialization (N.E instead of N.E || (N.E = {})) and then write into undefined at runtime.
Useful? React with 👍 / 👎.
| self.namespace_export_names | ||
| .as_ref() | ||
| .is_some_and(|names| names.values.contains(name) || names.types.contains(name)) |
There was a problem hiding this comment.
nested_is_exported treats a nested namespace as exported when its name appears in either names.values or names.types, but names.types can be populated by unrelated type-only exports (for example export interface Inner {}) in the same outer body. That misclassifies a non-exported namespace Inner { ... } as exported, making reopen cache keys share the enclosing export mark and leaking private members across outer namespace reopens. This breaks valid cases where non-exported nested namespaces should stay body-local despite same-named exported type declarations.
Useful? React with 👍 / 👎.
Address review feedback from @magic-akari / Codex on swc-project#11872: only `export …` declarations should be shared across sibling re-opens of a TypeScript namespace, not every declaration. Each re-open now gets a fresh per-body scope for its private bindings; only exported names live in the cache-backed `NamespaceExportScope`, which the body scope exposes via a `shared` link so reference lookups still see sibling-declared exports with a stable `SyntaxContext`. A per-body pre-scan of `export` declarations drives the routing in `modify`, making the decision order-independent: TypeScript's var/function same-body merge still holds (`export var a = 1; for (var a; …)`) while a non-exported re-open of a sibling-exported name stays isolated — fixing the `namespace_004` `MyEnum.A` `1`→`2` regression Codex flagged. Added tests/ts-resolver/namespace_reopen_private covering @magic-akari's example; updated fixture snapshots accordingly (re-opened non-exported classes / enums no longer trigger the "defined multiple times" diagnostic). Signed-off-by: Onyeka Obi <softwareengineerasaservant@isurvivable.cv>
…pens
Address Codex P1 on swc-project#11872: the cache key for `TsModuleDecl` previously
used `self.namespace_export.mark` whenever it was present, so a nested
namespace declared inside an outer namespace body was always keyed to
the outer's stable export mark. That made the two `Inner` declarations
in
namespace Outer { namespace Inner { export const a = 1 } }
namespace Outer { namespace Inner { export const b = a } }
collide on a single cache entry and merge, even though TypeScript keeps
non-exported members local to each declaration body.
The cache-parent mark now falls back to `self.current.mark` (the outer
re-open's per-body mark) unless the nested namespace name is in the
enclosing body's pre-scanned `namespace_export_names`. Two outer
re-opens hold distinct body marks, so their non-exported `Inner`
children land in disjoint cache entries and stay isolated; exported
nested namespaces still key on the outer's stable export mark and
merge across re-opens.
Added tests/ts-resolver/namespace_reopen_nested_private exercising the
Codex case (second `Inner`'s reference to `a` is unresolved,
confirming isolation).
nested namespaces still key on the outer's stable export mark and
merge across re-opens.
Added tests/ts-resolver/namespace_reopen_nested_private exercising the
Codex case (second `Inner`'s reference to `a` is unresolved,
confirming isolation).
Signed-off-by: Onyeka Obi <softwareengineerasaservant@isurvivable.cv>
MavenRain
force-pushed
the
fix/resolver-namespace-merge
branch
from
bf4ec2a to
450fd85
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 450fd85e81
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if let ModuleItem::ModuleDecl(ModuleDecl::ExportDecl(export)) = item { | ||
| add_decl_export_names(&export.decl, &mut scan); | ||
| } |
There was a problem hiding this comment.
Extend the namespace export pre-scan to include ModuleDecl::TsImportEquals entries with is_export = true; currently only ExportDecl is recorded, so export import A = ... never enters namespace_export_names. In a re-opened namespace, that makes A look body-local instead of shared-exported, so later declarations can no longer resolve the alias through the merged namespace scope and may bind to unresolved/outer symbols instead.
Useful? React with 👍 / 👎.
MavenRain
force-pushed
the
fix/resolver-namespace-merge
branch
from
78b7d08 to
a9c8700
Compare
Address review feedback from @magic-akari / Codex on swc-project#11872: only `export …` declarations should be shared across sibling re-opens of a TypeScript namespace, not every declaration. Each re-open now gets a fresh per-body scope for its private bindings; only exported names live in the cache-backed `NamespaceExportScope`, which the body scope exposes via a `shared` link so reference lookups still see sibling-declared exports with a stable `SyntaxContext`. A per-body pre-scan of `export` declarations drives the routing in `modify`, making the decision order-independent: TypeScript's var/function same-body merge still holds (`export var a = 1; for (var a; …)`) while a non-exported re-open of a sibling-exported name stays isolated — fixing the `namespace_004` `MyEnum.A` `1`→`2` regression Codex flagged. Added tests/ts-resolver/namespace_reopen_private covering @magic-akari's example; updated fixture snapshots accordingly (re-opened non-exported classes / enums no longer trigger the "defined multiple times" diagnostic). Signed-off-by: Onyeka Obi <softwareengineerasaservant@isurvivable.cv>
…pens
Address Codex P1 on swc-project#11872: the cache key for `TsModuleDecl` previously
used `self.namespace_export.mark` whenever it was present, so a nested
namespace declared inside an outer namespace body was always keyed to
the outer's stable export mark. That made the two `Inner` declarations
in
namespace Outer { namespace Inner { export const a = 1 } }
namespace Outer { namespace Inner { export const b = a } }
collide on a single cache entry and merge, even though TypeScript keeps
non-exported members local to each declaration body.
The cache-parent mark now falls back to `self.current.mark` (the outer
re-open's per-body mark) unless the nested namespace name is in the
enclosing body's pre-scanned `namespace_export_names`. Two outer
re-opens hold distinct body marks, so their non-exported `Inner`
children land in disjoint cache entries and stay isolated; exported
nested namespaces still key on the outer's stable export mark and
merge across re-opens.
Added tests/ts-resolver/namespace_reopen_nested_private exercising the
Codex case (second `Inner`'s reference to `a` is unresolved,
confirming isolation).
nested namespaces still key on the outer's stable export mark and
merge across re-opens.
Added tests/ts-resolver/namespace_reopen_nested_private exercising the
Codex case (second `Inner`'s reference to `a` is unresolved,
confirming isolation).
Signed-off-by: Onyeka Obi <softwareengineerasaservant@isurvivable.cv>
…wc-project#11607) Signed-off-by: Onyeka Obi <softwareengineerasaservant@isurvivable.cv>
…pe resolver output Signed-off-by: Onyeka Obi <softwareengineerasaservant@isurvivable.cv>
Address review feedback from @magic-akari / Codex on swc-project#11872: only `export …` declarations should be shared across sibling re-opens of a TypeScript namespace, not every declaration. Each re-open now gets a fresh per-body scope for its private bindings; only exported names live in the cache-backed `NamespaceExportScope`, which the body scope exposes via a `shared` link so reference lookups still see sibling-declared exports with a stable `SyntaxContext`. A per-body pre-scan of `export` declarations drives the routing in `modify`, making the decision order-independent: TypeScript's var/function same-body merge still holds (`export var a = 1; for (var a; …)`) while a non-exported re-open of a sibling-exported name stays isolated — fixing the `namespace_004` `MyEnum.A` `1`→`2` regression Codex flagged. Added tests/ts-resolver/namespace_reopen_private covering @magic-akari's example; updated fixture snapshots accordingly (re-opened non-exported classes / enums no longer trigger the "defined multiple times" diagnostic). Signed-off-by: Onyeka Obi <softwareengineerasaservant@isurvivable.cv>
…pens
Address Codex P1 on swc-project#11872: the cache key for `TsModuleDecl` previously
used `self.namespace_export.mark` whenever it was present, so a nested
namespace declared inside an outer namespace body was always keyed to
the outer's stable export mark. That made the two `Inner` declarations
in
namespace Outer { namespace Inner { export const a = 1 } }
namespace Outer { namespace Inner { export const b = a } }
collide on a single cache entry and merge, even though TypeScript keeps
non-exported members local to each declaration body.
The cache-parent mark now falls back to `self.current.mark` (the outer
re-open's per-body mark) unless the nested namespace name is in the
enclosing body's pre-scanned `namespace_export_names`. Two outer
re-opens hold distinct body marks, so their non-exported `Inner`
children land in disjoint cache entries and stay isolated; exported
nested namespaces still key on the outer's stable export mark and
merge across re-opens.
Added tests/ts-resolver/namespace_reopen_nested_private exercising the
Codex case (second `Inner`'s reference to `a` is unresolved,
confirming isolation).
nested namespaces still key on the outer's stable export mark and
merge across re-opens.
Added tests/ts-resolver/namespace_reopen_nested_private exercising the
Codex case (second `Inner`'s reference to `a` is unresolved,
confirming isolation).
Signed-off-by: Onyeka Obi <softwareengineerasaservant@isurvivable.cv>
`pre_scan_namespace_exports` only inspected `ModuleDecl::ExportDecl`, so
`export import A = ...` (a `TsImportEqualsDecl { is_export: true, .. }`)
was never added to `namespace_export_names`. The alias binding was then
routed to the body-local scope instead of the namespace's merged export
scope, and references from sibling re-opens of the same namespace fell
through to outer/unresolved bindings, violating TypeScript's
namespace-merge semantics.
Recognise the exported import-equals form in the pre-scan, adding the
alias to `values` (and to `types` when not `is_type_only`), mirroring
how `export namespace`/`export class`/`export enum` are tracked. The
existing `visit_mut_ts_import_equals_decl` `modify` call now routes the
binding to the shared export scope, and type-position lookups in
sibling bodies resolve through `mark_for_ref_inner`'s
`declared_types -> declared_symbols` fall-through.
Signed-off-by: Onyeka Obi <softwareengineerasaservant@isurvivable.cv>
MavenRain
force-pushed
the
fix/resolver-namespace-merge
branch
from
a9c8700 to
7aa055b
Compare
3 participants
Description
Closes #11607. Unblocks #11514.
In TypeScript, re-declaring a namespace with the same name merges it;
identifiers in any declaration's body see bindings declared in earlier
declarations. SWC's resolver was creating a fresh block scope (with
Mark::fresh) for every namespace X { ... } body, leaving sibling
re-declarations with disjoint scopes. Lookups in the second body walked their
own (empty) scope, then fell through to the outer scope and resolved to
whatever happened to be declared there.
Repro (also added as tests/ts-resolver/namespace_reopen)
Before: a resolved to outer const a (a__2).
After: a resolves to Test.a from the first body (a__3).
Fix
A Resolver now carries a Rc<RefCell<FxHashMap<(Mark, Atom), NamespaceBody>>>
cache keyed by (parent_scope.mark, namespace_name). On
visit_mut_ts_module_decl:
mark, declared symbols, and declared types before visiting the body. All
earlier-declaration bindings are visible to lookups, and the merged scope's
mark is preserved so identifiers across declarations share a SyntaxContext.
cache, so a third (or fourth) re-declaration sees the union.
Cache keying on the parent scope's mark makes nested namespace re-opens behave
correctly: namespace Outer { namespace Inner {} } namespace Outer { namespace
Inner {} } merges the outer pair, then the inner pair (whose parent mark is
now the outer's merged mark) also merges.
Test coverage
The new fixture under tests/ts-resolver/namespace_reopen exercises the issue's
exact repro. The full swc_ecma_transforms_base test suite (~7500 tests,
including the 5182-fixture ts_resolver corpus) passes unchanged.