feat(minifier): remove unused import specifiers#16797
Conversation
github-actions
bot
added
A-minifier
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. |
camc314
force-pushed
the
c/12-13-feat_minifier_remove_unused_import_specifiers
branch
2 times, most recently
from
3b08078 to
d174888
Compare
CodSpeed Performance ReportMerging #16797 will not alter performanceComparing Summary
Footnotes
|
camc314
force-pushed
the
c/12-13-feat_minifier_remove_unused_import_specifiers
branch
from
d174888 to
4bceec6
Compare
![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-specifier removal is directionally correct and well-tested, but there are a couple of semantics/compatibility risks around converting to side-effect-only imports in the presence of import attributes/assertions, especially for import {} from 'x' with { ... }. The implementation’s ctx.state.changed signaling is a bit brittle due to scattered mutation paths. Finally, the same transformation is wired into both peephole and DCE traversals, which increases the chance of order-dependent behavior and redundant work.
Summary of changes
What changed
- Added unused-import-specifier removal pass to the minifier peephole pipeline:
PeepholeOptimizationsnow handlesStatement::ImportDeclaration(_)viaremove_unused_import_specifiers.DeadCodeEliminationalso invokes the same logic forImportDeclarationstatements.
- Implemented
remove_unused_import_specifiersincrates/oxc_minifier/src/peephole/remove_unused_declaration.rs:- Skips when
unusedoption isKeep, when parsing as script, or when root scope contains directeval. - Removes specifiers whose local symbols are marked unused.
- If all specifiers are removed, converts to a side-effect-only import (
import 'x'), preserving potential module side effects.
- Skips when
- Added extensive tests covering default/named/namespace/mixed imports, partial retention, dead-code scenario from #16742, import attributes, renames, and re-export usage.
- Updated allocation snapshot (
allocs_minifier.snap) reflecting changed allocation counts.
There was a problem hiding this comment.
Pull request overview
This PR implements the removal of unused import specifiers as part of the minifier's dead code elimination. The feature converts imports with all unused specifiers into side-effect-only imports (e.g., import a from 'a' → import 'a') to preserve potential side effects while removing unused bindings.
Key changes:
- Adds
remove_unused_import_specifiers()function that processes import declarations and removes unused specifiers - Integrates the new function into both
PeepholeOptimizationsandDeadCodeEliminationtraversal passes - Includes comprehensive test coverage for various import patterns (default, named, namespace, mixed, partial removal)
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
crates/oxc_minifier/src/peephole/remove_unused_declaration.rs |
Adds new remove_unused_import_specifiers() function with documentation and comprehensive test suite covering various import scenarios |
crates/oxc_minifier/src/peephole/mod.rs |
Integrates the new function into both PeepholeOptimizations::exit_statement() and DeadCodeElimination::exit_statement() to process import declarations |
tasks/track_memory_allocations/allocs_minifier.snap |
Updates memory allocation snapshot showing reduced allocations (84 vs 85 sys allocs) for RadixUIAdoptionSection.jsx due to removed import specifiers |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
camc314
force-pushed
the
c/12-13-feat_minifier_remove_unused_import_specifiers
branch
3 times, most recently
from
db72ac2 to
56940c0
Compare
There was a problem hiding this comment.
Main concerns are maintainability and determinism: the same import-specifier removal runs in both peephole and DCE, which can lead to redundant work and order-dependent behavior. Additionally, ctx.state.changed signaling in remove_unused_import_specifiers is still somewhat coupled to vector-length checks rather than explicit before/after structural state, making it fragile to future edits. The semantic/compatibility risk around import {} ... with {} appears covered by the added test, but the implementation would benefit from more robust change tracking.
Additional notes (1)
- Maintainability |
crates/oxc_minifier/src/peephole/mod.rs:169-169
remove_unused_import_specifiersis invoked from bothPeepholeOptimizationsandDeadCodeElimination. That creates a risk of order-dependent behavior (one pass mutates the import, the other sees a different shape) and redundant work. It also makes reasoning aboutctx.state.changedand later optimizations harder because the same transformation can happen in multiple phases.
If the intent is “always do this”, consider making it a single, well-defined stage (either peephole or DCE), or ensure one of the callers is gated (e.g., only run in DCE where symbol-use info is guaranteed complete).
Summary of changes
Summary of changes
- Minifier now removes unused import specifiers by introducing
PeepholeOptimizations::remove_unused_import_specifiers. - Wired the new optimization into two traversals:
PeepholeOptimizations::exit_statement()handlesStatement::ImportDeclaration(_).DeadCodeElimination::exit_statement()also invokes the same import-specifier removal.
- Implementation details (
crates/oxc_minifier/src/peephole/remove_unused_declaration.rs):- Skips when root scope contains direct
eval. - For
ImportDeclarationwithspecifiers: Some(...), retains only specifiers whose local symbols are not marked unused. - If all specifiers are removed, converts to a side-effect-only import by setting
import_decl.specifiers = None.
- Skips when root scope contains direct
- Added comprehensive tests for default/named/namespace/mixed imports, partial removals,
import {}handling,with { type: 'json' }attributes, renames, re-exports, and direct-eval safety. - Updated allocation snapshot showing fewer allocations for
RadixUIAdoptionSection.jsx.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Import
I've refactored this to be slightly cleaner. We now only change
I think this change is OK, since we follow the same pattern as the other code removal paths, e.g. @CharlieHelps re-review this PR please |
camc314
force-pushed
the
c/12-13-feat_minifier_remove_unused_import_specifiers
branch
from
56940c0 to
5ba4d67
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
camc314
requested review from
CharlieHelps
and removed request for
CharlieHelps
There was a problem hiding this comment.
The main issues are determinism/maintainability: remove_unused_import_specifiers is executed in both peephole and DCE, increasing redundant work and the risk of order-dependent behavior. Relying on debug_assert! for module-only invariants makes behavior differ between debug and release builds; use an explicit source_type().is_script() guard if the optimization is module-only. The phase.is_some() early return is overly broad and should be narrowed or explicitly justified. Finally, ctx.state.changed should be driven by a single before/after decision to reduce future fragility.
Additional notes (1)
- Maintainability |
crates/oxc_minifier/src/peephole/mod.rs:169-169
Runningremove_unused_import_specifiersin bothPeepholeOptimizationsandDeadCodeEliminationincreases the chance of order-dependent behavior and redundant work. After peephole runs, DCE will observe a mutated AST shape (e.g.,specifiers=None), which can subtly affect other downstream heuristics andctx.state.changedsemantics. If symbol-usage information is the prerequisite, this pass belongs in the stage where that information is authoritative, not duplicated across stages.
Summary of changes
Summary of changes
- Added a new minifier optimization
remove_unused_import_specifiersto remove unusedimportbindings and, when all bindings are removed, convert the statement to a side-effect-only import (import 'x'). - Wired the optimization into two traversal passes:
PeepholeOptimizations::exit_statementnow handlesStatement::ImportDeclaration(_).DeadCodeElimination::exit_statementnow also handlesStatement::ImportDeclaration(_).
- Implemented guards in
remove_unused_import_specifiers:- Skip when
options.unused == Keep. - Skip when the root scope contains direct
eval. - Skip when
import_decl.phase.is_some(). - Added a
debug_assert!(!ctx.source_type().is_script()).
- Skip when
- Added extensive unit tests covering default/named/namespace/mixed imports, partial removals,
import {}handling,with { type: 'json' }attributes, renames, re-export usage, and direct-eval safety. - Updated memory allocation snapshot (
allocs_minifier.snap) reflecting fewer allocations forRadixUIAdoptionSection.jsx.
Merge activity
|
Fixes #16742 🤖 generated with help from Claude Opus 4.5
graphite-app
bot
force-pushed
the
c/12-13-feat_minifier_remove_unused_import_specifiers
branch
from
5ba4d67 to
74eae13
Compare
graphite-app
bot
deleted the
c/12-13-feat_minifier_remove_unused_import_specifiers
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]>
Added a new option (`invalid_import_side_effects`) to disable #16797 as it requires a new assumptions. refs rolldown/rolldown#7512 (comment)
2 participants
Fixes #16742
🤖 generated with help from Claude Opus 4.5