fix(es/resolver): Merge re-opened TypeScript namespace scopes#11872
fix(es/resolver): Merge re-opened TypeScript namespace scopes#11872MavenRain wants to merge 6 commits into
Conversation
|
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".
Merging this PR will not alter performance
Comparing Footnotes
|
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".
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>
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".
…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".
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>
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".
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>
a9c8700 to
7aa055b
Compare
|
@magic-akari Can you take a look? |
|
After reviewing the test snapshots and generated outputs, no issues have been found so far. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7aa055b10c
ℹ️ 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".
| /// bindings, violating TypeScript's namespace-merge semantics. | ||
| fn pre_scan_namespace_exports(body: &TsNamespaceBody) -> NamespaceExportNames { | ||
| let mut scan = NamespaceExportNames::default(); | ||
| if let TsNamespaceBody::TsModuleBlock(block) = body { |
There was a problem hiding this comment.
Handle dotted namespace reopens
When the reopened namespace is written in dotted form (namespace Outer.Inner { ... }), the Inner body is represented as TsNamespaceBody::TsNamespaceDecl, so this branch never pre-scans its exports and the existing visit_mut_ts_namespace_decl path never installs a shared namespace export scope for Inner. A pair such as namespace Outer.Inner { export const a = 1 } followed by namespace Outer.Inner { export const b = a } still leaves a unresolved/outer-scoped instead of resolving to Outer.Inner.a, even though TypeScript emits Inner.a for that case.
Useful? React with 👍 / 👎.
| scan.values.insert(f.ident.sym.clone()); | ||
| } | ||
| Decl::Class(c) => { | ||
| scan.values.insert(c.ident.sym.clone()); |
There was a problem hiding this comment.
Qualify reopened class and function exports
When a later namespace reopen references an exported class or function from an earlier reopen, adding these names to the shared value scope gives the reference the exported declaration's context, but the TypeScript transform still does not add ExportDecl classes/functions to semantic.exported_binding (unlike vars/enums/modules/import aliases), so ExportQuery never rewrites the later-body reference to N.C/N.f. For namespace N { export class C {} } namespace N { new C() }, the generated code keeps a free C inside the second IIFE instead of reading N.C, causing a runtime ReferenceError.
Useful? React with 👍 / 👎.
| if exp.declared_symbols.contains_key(sym) { | ||
| return Some(exp.mark); |
There was a problem hiding this comment.
Predeclare later namespace exports before lookups
This lookup only sees names already accumulated in the shared scope, so references in an earlier namespace body cannot resolve to exports declared by a later reopen. Valid var cases like namespace Test { export var b = a + 1 } namespace Test { export var a = 1 } should emit the first initializer as Test.a + 1; with the current one-pass cache, a is unresolved/outer-scoped while visiting the first body, producing a free a instead of the namespace property.
Useful? React with 👍 / 👎.
magic-akari
left a comment
There was a problem hiding this comment.
I added additional test cases that should pass according to the expected behavior, but they are failing on my machine.
This suggests the current implementation may not fully match the expected behavior and may require a fix.
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.