fix(minifier): preserve classes with decorators#16878
Conversation
- Add check to prevent removal of classes with decorators - Decorators may have side effects even if class is unused - Add tests for class and class expression decorators Co-authored-by: sapphi-red <49056869+sapphi-red@users.noreply.github.com>
CodSpeed Performance ReportMerging #16878 will not alter performanceComparing Summary
Footnotes
|
There was a problem hiding this comment.
The change is functionally correct and improves side-effect safety by preserving decorated classes. The main risk is maintainability: decorator preservation logic is now split across class-level (c.decorators) and element-level (e.has_decorator()) checks, which could drift over time. Tests added for both declaration and expression cases are a good safeguard.
Additional notes (3)
-
Maintainability |
crates/oxc_minifier/src/peephole/remove_unused_expression.rs:687-696
The new early-return preserves decorated classes, which is correct for side-effect safety. However, this is a semantic guardrail and should ideally be centralized/consistent with other "keep" predicates used for classes (e.g., alongside the existinge.has_decorator()checks). Right now, decorator-related preservation is split between class-level (c.decorators) and element-level (e.has_decorator()), which increases the risk that future refactors update one but not the other. -
Readability |
crates/oxc_minifier/src/peephole/remove_unused_expression.rs:690-693
The new early return correctly preserves decorated classes, but the comment is slightly misleading given the existinge.has_decorator()check below (which only covers class elements). Consider clarifying that this new check is specifically for class-level decorators to avoid future regressions where someone assumes member decorators already cover it. -
Maintainability |
crates/oxc_minifier/src/peephole/remove_unused_declaration.rs:195-197
The tests added for decorated class declarations/expressions are good, but they currently only assert the input is preserved. Consider adding one more test that ensures an otherwise-removable decorated class remains even when it is syntactically unused in a larger program (e.g., surrounded by other removable statements), to guard against future changes in statement-list trimming logic.
Summary of changes
What changed
Minifier behavior
- Updated
remove_unused_class()incrates/oxc_minifier/src/peephole/remove_unused_expression.rsto preserve classes that have class-level decorators by returning early when!c.decorators.is_empty(). - This applies to the class removal logic that previously focused on superclass
TypeErrorcases and class-body element side effects.
Test coverage
- Added tests to ensure decorated class declarations are preserved in
crates/oxc_minifier/src/peephole/remove_unused_declaration.rs(test_same_options("@dec class C {}", ...)). - Added tests to ensure decorated class expressions are preserved in
crates/oxc_minifier/src/peephole/remove_unused_expression.rs(test_same_options("(@dec class {})", ...)).
Context alignment
- Brings class-level decorator handling in line with existing logic that already keeps class members with decorators (
e.has_decorator()), preventing incorrect dead-code elimination when decorators can produce side effects.
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where classes with decorators were being incorrectly removed as dead code during minification. Since decorators can have side effects (like registration or global state modification), classes with decorators must be preserved even if they appear unused.
Key Changes:
- Added early return in
remove_unused_class()when class-level decorators are present - Added test coverage for both class declarations (
@dec class C {}) and class expressions ((@dec class {}))
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
crates/oxc_minifier/src/peephole/remove_unused_expression.rs |
Added decorator check in remove_unused_class() to return None when !c.decorators.is_empty(), preventing removal; added test case for class expressions with decorators |
crates/oxc_minifier/src/peephole/remove_unused_declaration.rs |
Added test case for class declarations with decorators to ensure they are preserved |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Classes with decorators were being removed as dead code even when unused. Decorators may have side effects (registration, global state modification, etc.), so these classes must be preserved.
Changes
remove_unused_class()to return early when!c.decorators.is_empty()Example
This aligns with existing behavior for class members with decorators (line 697).
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.