refactor(ecmascript): align side-effect analysis with spec coercions#19308
refactor(ecmascript): align side-effect analysis with spec coercions#19308graphite-app[bot] merged 1 commit intomainfrom
Conversation
Merging this PR will not alter performance
Comparing 
Footnotes
|
Merge activity
|
…19308) ## Summary - align side-effect detection for conversion-heavy global calls with spec behavior - add explicit handling for `PrivateInExpression` throw behavior on non-object RHS - treat `import.meta.url` property reads as side-effect free - expand minifier may-have-side-effects tests for String/Number/Boolean/BigInt/Symbol coercion cases ## Testing - cargo test -p oxc_minifier may_have_side_effects -- --nocapture - cargo test -p oxc_ecmascript ## AI Usage - Drafted with AI assistance (Codex) and reviewed/tested by the author.
8402de4 to
4538098
Compare
There was a problem hiding this comment.
Pull request overview
This PR refines side-effect analysis for ECMAScript type coercion functions (String, Number, BigInt, Symbol) to align with spec behavior, adds explicit handling for PrivateInExpression throw semantics, and treats import.meta.url as side-effect-free. The changes improve minification accuracy by better detecting when these global functions can safely be eliminated.
Changes:
- Replaced generic "pure call" handling for String/Number/BigInt/Symbol with spec-aligned analysis that checks ToPrimitive behavior to detect user code execution via valueOf/toString
- Added PrivateInExpression handling that throws TypeError on non-object RHS
- Marked import.meta.url property reads as side-effect-free (with a critical bug - see comments)
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| crates/oxc_ecmascript/src/side_effects/expressions.rs | Implements refined side-effect analysis for type coercion functions, PrivateInExpression, and import.meta.url |
| crates/oxc_minifier/tests/ecmascript/may_have_side_effects.rs | Adds comprehensive test coverage for String/Number/BigInt/Symbol coercion edge cases and PrivateInExpression |
| crates/oxc_minifier/tests/peephole/substitute_alternate_syntax.rs | Updates BigInt(1) test to expect removal (no side effects) |
| crates/oxc_minifier/tests/peephole/obscure_edge_cases.rs | Updates import.meta.url test to expect removal (no side effects) |
| || object.value_type(ctx).is_string()) | ||
| } | ||
| // `import.meta.url` is spec-defined and side-effect free. | ||
| "url" if matches!(object.without_parentheses(), Expression::MetaProperty(_)) => false, |
There was a problem hiding this comment.
The check for side-effect-free property access should specifically verify that the MetaProperty is import.meta, not just any MetaProperty. Currently, this would also incorrectly treat new.target.url as side-effect-free. Consider checking if the MetaProperty has meta.name == "import" and property.name == "meta" before treating the url property access as side-effect-free. For example: if let Expression::MetaProperty(meta_prop) = object.without_parentheses() and meta_prop.meta.name == "import" and meta_prop.property.name == "meta".
| "url" if matches!(object.without_parentheses(), Expression::MetaProperty(_)) => false, | |
| "url" => { | |
| if let Expression::MetaProperty(meta_prop) = object.without_parentheses() { | |
| if meta_prop.meta.name == "import" && meta_prop.property.name == "meta" { | |
| return false; | |
| } | |
| } | |
| true | |
| } |
Summary
PrivateInExpressionthrow behavior on non-object RHSimport.meta.urlproperty reads as side-effect freeTesting
AI Usage