Skip to content

refactor(es/typescript): Precompute namespace import-equals usage in semantic pass#11534

Merged
kdy1 merged 1 commit intomainfrom
kdy1/merge-ts-more
Feb 7, 2026
Merged

refactor(es/typescript): Precompute namespace import-equals usage in semantic pass#11534
kdy1 merged 1 commit intomainfrom
kdy1/merge-ts-more

Conversation

@kdy1
Copy link
Member

@kdy1 kdy1 commented Feb 6, 2026

Summary

  • move namespace import-equals usage resolution into semantic analysis
  • remove transform-time namespace usage collector visitor and consume semantic lookup instead
  • keep namespace pre-strip timing and add semantic unit tests for import-chain propagation

Testing

  • cargo test -p swc_ecma_transforms_typescript -p swc_ecma_transforms_react -p swc_ecma_transforms && cargo test -p swc --test projects --test tsc
  • cargo fmt --all

@changeset-bot
Copy link

changeset-bot bot commented Feb 6, 2026

⚠️ No Changeset found

Latest commit: ebcdb57

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions
Copy link
Contributor

github-actions bot commented Feb 6, 2026

Binary Sizes

File Size
swc.linux-x64-gnu.node 28M (28577352 bytes)

Commit: 846b437

@codspeed-hq
Copy link

codspeed-hq bot commented Feb 6, 2026

CodSpeed Performance Report

Merging this PR will not alter performance

Comparing kdy1/merge-ts-more (ebcdb57) with main (b069558)

Summary

✅ 184 untouched benchmarks

@kdy1 kdy1 marked this pull request as ready for review February 7, 2026 01:13
@kdy1 kdy1 requested a review from a team as a code owner February 7, 2026 01:13
Copilot AI review requested due to automatic review settings February 7, 2026 01:13
@kdy1 kdy1 enabled auto-merge (squash) February 7, 2026 01:13
@kdy1 kdy1 changed the title refactor(ts): precompute namespace import-equals usage in semantic pass refactor(es/typescript): Precompute namespace import-equals usage in semantic pass Feb 7, 2026
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Refactors the TypeScript namespace import = alias usage resolution so it is precomputed during semantic analysis and then consumed by the transform pass to strip unused aliases inside namespace/module blocks.

Changes:

  • Compute per-namespace-block import = usage (including import-chain propagation) during semantic analysis.
  • Remove the transform-time namespace usage collector and switch stripping logic to consult SemanticInfo.
  • Add unit tests covering namespace-block import-chain propagation/merge behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
crates/swc_ecma_transforms_typescript/src/transform.rs Removes the AST-visitor-based namespace import-equals usage collector and uses semantic lookup to retain/strip TsImportEqualsDecl in namespace/module blocks.
crates/swc_ecma_transforms_typescript/src/semantic.rs Adds tracking for namespace-block import = usage keyed by declaration Span, implements per-block import-chain propagation with parent/child merging, and introduces unit tests.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@kdy1 kdy1 merged commit b7e87c7 into main Feb 7, 2026
194 checks passed
@kdy1 kdy1 deleted the kdy1/merge-ts-more branch February 7, 2026 01:17
@claude
Copy link
Contributor

claude bot commented Feb 7, 2026

Code Review: refactor(es/typescript): Precompute namespace import-equals usage in semantic pass

Good refactoring overall — moving the namespace import-equals usage resolution from a per-block transform-time visitor into the single semantic analysis pass is a clean structural improvement. It eliminates the separate NamespaceUsageCollect visitor and its duplication of logic already present in SemanticAnalyzer. CI is all green.

Below are observations and a few items worth considering:


1. Behavioral difference: noop_visit_type!() and type-only references

The old NamespaceUsageCollect did not use noop_visit_type!(), so it traversed into type annotations and collected identifiers found there. The new code piggybacks on SemanticAnalyzer, which does use noop_visit_type!() (semantic.rs:275), meaning type annotations are skipped entirely.

This means an import-equals that is only referenced inside a type annotation will now be stripped, whereas previously it was retained:

namespace Foo {
    import Bar = SomeModule.Bar;
    const x: Bar = getValue(); // Bar only referenced in type position
}
  • Old: Bar visited inside type annotation → import-equals retained
  • New: Type annotation skipped → import-equals stripped

This is arguably more correct since types are erased, but it is a behavioral change. If this is intentional, it might warrant a brief note in the PR description. If not, it's worth verifying that the existing test suite covers this case (all CI passes, so it may already be fine).

2. visit_export_named_specifier visits exported field now

The old NamespaceUsageCollect explicitly only visited node.orig:

node.orig.visit_with(self);

The new code in SemanticAnalyzer calls node.visit_children_with(self) (semantic.rs:430), which visits both orig and exported. For export { foo as bar }, the alias bar is now also recorded in the namespace block's usage set.

In practice this is unlikely to cause issues because the export alias name typically has a different SyntaxContext than local bindings, but it is a slight over-collection compared to the old code. Low risk, but worth being aware of.

3. Span-based lookup correctness

The new approach identifies import-equals declarations by Span rather than Id:

pub namespace_import_equals_usage: FxHashSet<Span>,

This is sound as long as spans are unique per declaration (which they are in a single parse). It's actually more precise than Id-based lookup since two import-equals in different namespace blocks could theoretically share the same Id but would have distinct spans. Good choice.

4. analyze_import_chain uses remove — iteration order dependency

In NamespaceBlock::analyze_import_chain (semantic.rs:92-112), the loop iterates over self.usage and calls self.import_chain.remove(id). Since FxHashSet has non-deterministic iteration order, the traversal path through the import chain depends on iteration order. However, since remove only consumes each chain entry once and the final result is the union of all transitively reachable ids, the output is order-independent. This is correct but worth noting for future maintainers — the algorithm is correct despite the non-deterministic iteration.

5. Child-to-parent block merging looks correct

The propagation in visit_ts_module_block (semantic.rs:517-522) correctly extends the parent's usage and import_chain with the child's sets after the child's own analyze_import_chain has run. The unit test namespace_block_merge_from_child_keeps_parent_alias_used verifies the transitive case. Good coverage.

6. Minor: code duplication between NamespaceBlock::analyze_import_chain and SemanticAnalyzer::analyze_import_chain

NamespaceBlock::analyze_import_chain (semantic.rs:92-112) and SemanticAnalyzer::analyze_import_chain (semantic.rs:121-141) are nearly identical — they differ only in whether they operate on self.usage/self.import_chain vs self.info.usage/self.import_chain. This is a pre-existing pattern (the old NamespaceUsageCollect also had its own copy), but now that both live in the same file, it might be worth extracting a shared helper function, e.g.:

fn resolve_import_chain(usage: &FxHashSet<Id>, import_chain: &mut FxHashMap<Id, Id>) -> FxHashSet<Id> { ... }

Not a blocker, just a suggestion.

7. Test coverage

The two new unit tests cover the core analyze_import_chain logic and child-to-parent merging well. The full integration is covered by the existing test suite (all CI green). Consider adding a test case for the type-annotation-only reference scenario mentioned in point 1 if it isn't already covered.


Summary

This is a well-structured refactoring that correctly consolidates namespace usage analysis into the semantic pass. The main thing to verify is whether the noop_visit_type!() behavioral difference (point 1) is intentional. Everything else looks good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant