fix(transformer/typescript): remove unused import equals declaration#16776
Conversation
github-actions
bot
added
A-transformer
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
CodSpeed Performance ReportMerging #16776 will not alter performanceComparing Summary
Footnotes
|
There was a problem hiding this comment.
Pull request overview
This PR attempts to fix the removal of unused TypeScript import equals declarations by marking their references as type references before transformation. The implementation moves the reference marking logic from the declaration removal phase to an earlier enter_program phase and processes import chains in reverse order to handle dependencies correctly.
Key changes:
- Adds
mark_unused_import_equals_references_as_typemethod that processes import equals declarations before transformation - Processes imports in reverse order to correctly handle chains where one import references another
- Removes inline reference marking code from
transform_ts_import_equals
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
crates/oxc_transformer/src/typescript/module.rs |
Adds enter_program hook and two new helper methods for marking unused import equals references; removes inline reference marking from transformation logic |
crates/oxc_transformer/src/typescript/mod.rs |
Calls module.enter_program to mark unused import references before transformation |
tasks/transform_conformance/tests/.../remove-unused-import-equals/input.ts |
Test case with used/unused import chains and namespace with unused imports |
tasks/transform_conformance/tests/.../remove-unused-import-equals/output.js |
Expected output showing only used imports transformed and unused ones removed |
tasks/transform_conformance/snapshots/oxc.snap.md |
Test snapshot showing semantic mismatches indicating the test doesn't fully pass yet |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...e/tests/babel-plugin-transform-typescript/test/fixtures/remove-unused-import-equals/input.ts
Show resolved
Hide resolved
...e/tests/babel-plugin-transform-typescript/test/fixtures/remove-unused-import-equals/input.ts
Show resolved
Hide resolved
...e/tests/babel-plugin-transform-typescript/test/fixtures/remove-unused-import-equals/input.ts
Show resolved
Hide resolved
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
There was a problem hiding this comment.
The new unused import = pre-pass is directionally good, but it currently only scans top-level statements and misses nested import = declarations inside namespaces/modules, which is a correctness issue (and likely related to the new snapshot mismatches). Additionally, moving reference-flag mutation to enter_program introduces ordering assumptions about when semantic references are finalized. Finally, the strict debug_assert_eq!(..., ReferenceFlags::Read) makes the transformer fragile to future semantic-flag changes.
Additional notes (1)
- Maintainability |
crates/oxc_transformer/src/typescript/mod.rs:81-85
The new pre-pass marks module-reference identifiers asTypeonly when the binding has all-type references. This assumes thatget_resolved_references(decl.id.symbol_id())is fully populated and stable atenter_programtime.
If any later traversal phase can create additional value references to that symbol (e.g., via desugaring, namespace transforms, or other TS passes that synthesize identifiers), this pre-pass can incorrectly flip a Read to Type too early, potentially causing an actually-used value import to be treated as type-only downstream.
Given you now call this at enter_program, it’s worth asserting/ensuring that no subsequent pass introduces new value references to TSImportEqualsDeclaration bindings (or that such passes run before this marking).
Summary of changes
What changed
- TypeScript transformer traversal order:
TypeScriptModule::enter_programis now invoked fromTypeScript::enter_program(crates/oxc_transformer/src/typescript/mod.rs). - Refactor of unused
import =handling: moved the logic that re-flags module-reference identifiers fromRead→Typeout oftransform_ts_import_equalsand into a dedicated pre-pass:- Added
TypeScriptModule::enter_programwhich callsmark_unused_import_equals_references_as_type. - Implemented reverse-order scanning of top-level statements to correctly handle chained
import x = y.zrelationships.
- Added
- Conformance coverage: added a new fixture
remove-unused-import-equalsand updated snapshot counts/results.
Files touched:
crates/oxc_transformer/src/typescript/mod.rscrates/oxc_transformer/src/typescript/module.rstasks/transform_conformance/snapshots/oxc.snap.mdtasks/transform_conformance/tests/.../remove-unused-import-equals/{input.ts,output.js}
overlookmotel
left a comment
overlookmotel
left a comment
There was a problem hiding this comment.
I don't really understand this weird import X = ... syntax, but the test seems to show the desired behavior. LGTM!
overlookmotel
force-pushed
the
12-12-fix_transformer_typescript_remove_unused_import_equals_declaration
branch
from
42b4e46 to
f20876b
Compare
|
Rebased on main to fix merge conflicts. |
Merge activity
|
graphite-app
bot
force-pushed
the
12-12-fix_transformer_typescript_remove_unused_import_equals_declaration
branch
from
f20876b to
3002649
Compare
graphite-app
bot
deleted the
12-12-fix_transformer_typescript_remove_unused_import_equals_declaration
branch
### 🚀 Features - d209c21 allocator: Add cap to FixedSizeAllocatorPool and block when exhausted (#17023) (Cameron) - fb2af91 allocator: Add bitset utils (#17042) (zhaoting zhou) - c16082c tasks/compat_data: Integrate `node-compat-table` (#16831) (Boshen) - 5586823 span: Extract TS declaration file check to its own function (#17037) (camchenry) - 3d2b492 minifier: Fold iife arrow functions in call expressions (#16477) (Armano) - 67e9f9e codegen: Keep comments on the export specifiers (#16943) (夕舞八弦) - cb515fa parser: Improve error message for `yield` as identifier usage (#16950) (sapphi-red) - dcc856b parser: Add help for `new_dynamic_import` error (#16949) (sapphi-red) - c3c79f8 parser: Improve import attribute value error message (#16948) (sapphi-red) - 291b57b ast_tools: Generate TS declaration files for deserializer and walk files (#16912) (camc314) - 74eae13 minifier: Remove unused import specifiers (#16797) (camc314) ### 🐛 Bug Fixes - fb9e193 linter: OOM problems with custom plugins (#17082) (overlookmotel) - e59132b parser/napi: Fix lazy deser (#17069) (overlookmotel) - a92faf0 ast_tools: Support `u128` in `assert_layouts` generator (#17050) (overlookmotel) - 47b4c2f minifier/docs: Correct hyperlink path in OPTIMIZATIONS.md (#16986) (GRK) - 3002649 transformer/typescript: Remove unused import equals declaration (#16776) (Dunqing) - 5a2af88 regular_expression: Correct named capture group reference error (#16952) (sapphi-red) ### ⚡ Performance - b657bb6 allocator: Reduce time `Mutex` lock is held in `FixedSizeAllocatorPool::get` (#17079) (overlookmotel) - 1f3b19b ast: `#[ast]` macro use `#[repr(transparent)]` for single-field structs (#17052) (overlookmotel) - 225f229 parser: Use SmallVec for duplicate default export detection (#16801) (camc314) ### 📚 Documentation - a9c419f traverse: Update safety comments (#16944) (overlookmotel) Co-authored-by: overlookmotel <[email protected]>
2 participants
close: #16628