Skip to content

Commit ade14d4

Browse files
DunqingDunqing
committed
fix(ecmascript): enhance side-effect detection for classes, TypedArrays, computed members, and spread (#20213)
## Summary Part of #19673 Several side-effect detection improvements: - **Computed member expressions**: When `property_read_side_effects` is `None`, non-literal keys (e.g. `obj[identRef]`) now check the key expression and object for their own side effects instead of always returning `true` - **ObjectExpression spread**: When `property_read_side_effects` is `None`, `{...expr}` delegates to the argument's own side effects - **Class MethodDefinition**: Now checks for parameter decorators (TypeScript `@foo` on params) - **Class AccessorProperty**: Now checks accessor property values for side effects - **TypedArray constructors**: Added `Int8Array`, `Uint8Array`, `Uint8ClampedArray`, `Int16Array`, `Uint16Array`, `Int32Array`, `Uint32Array`, `Float32Array`, `Float64Array`, `BigInt64Array`, `BigUint64Array` to pure constructors (`DataView` excluded as it requires an `ArrayBuffer` argument) ## Test plan - [x] Added tests for computed member non-literal keys with `property_read_side_effects` - [x] Added tests for ObjectExpression spread with `property_read_side_effects` - [x] Added tests for class method parameter decorators - [x] Added tests for class accessor property values - [x] Added tests for TypedArray constructors - [x] All existing tests pass 🤖 Generated with [Claude Code](https://claude.com/claude-code)
1 parent e7163b6 commit ade14d4

File tree

2 files changed

+84
-10
lines changed

2 files changed

+84
-10
lines changed

‎crates/oxc_ecmascript/src/side_effects/expressions.rs‎

Lines changed: 33 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -297,7 +297,10 @@ fn is_pure_call(name: &str) -> bool {
297297
fn is_pure_constructor(name: &str) -> bool {
298298
matches!(name, "Set" | "Map" | "WeakSet" | "WeakMap" | "ArrayBuffer" | "Date"
299299
| "Boolean" | "Error" | "EvalError" | "RangeError" | "ReferenceError"
300-
| "SyntaxError" | "TypeError" | "URIError" | "Number" | "Object" | "String")
300+
| "SyntaxError" | "TypeError" | "URIError" | "Number" | "Object" | "String"
301+
| "Int8Array" | "Uint8Array" | "Uint8ClampedArray" | "Int16Array" | "Uint16Array"
302+
| "Int32Array" | "Uint32Array" | "Float32Array" | "Float64Array"
303+
| "BigInt64Array" | "BigUint64Array")
301304
}
302305

303306
/// Whether the name matches any known global constructors.
@@ -641,12 +644,18 @@ impl<'a> MayHaveSideEffects<'a> for ObjectPropertyKind<'a> {
641644
fn may_have_side_effects(&self, ctx: &impl MayHaveSideEffectsContext<'a>) -> bool {
642645
match self {
643646
ObjectPropertyKind::ObjectProperty(o) => o.may_have_side_effects(ctx),
644-
ObjectPropertyKind::SpreadProperty(e) => match &e.argument {
645-
Expression::ArrayExpression(arr) => arr.may_have_side_effects(ctx),
646-
Expression::StringLiteral(_) => false,
647-
Expression::TemplateLiteral(t) => t.may_have_side_effects(ctx),
648-
_ => true,
649-
},
647+
ObjectPropertyKind::SpreadProperty(e) => {
648+
if ctx.property_read_side_effects() == PropertyReadSideEffects::None {
649+
e.argument.may_have_side_effects(ctx)
650+
} else {
651+
match &e.argument {
652+
Expression::ArrayExpression(arr) => arr.may_have_side_effects(ctx),
653+
Expression::StringLiteral(_) => false,
654+
Expression::TemplateLiteral(t) => t.may_have_side_effects(ctx),
655+
_ => true,
656+
}
657+
}
658+
}
650659
}
651660
}
652661
}
@@ -701,7 +710,9 @@ impl<'a> MayHaveSideEffects<'a> for ClassElement<'a> {
701710
block.body.iter().any(|stmt| stmt.may_have_side_effects(ctx))
702711
}
703712
ClassElement::MethodDefinition(e) => {
704-
!e.decorators.is_empty() || e.key.may_have_side_effects(ctx)
713+
!e.decorators.is_empty()
714+
|| e.key.may_have_side_effects(ctx)
715+
|| e.value.params.items.iter().any(|item| !item.decorators.is_empty())
705716
}
706717
ClassElement::PropertyDefinition(e) => {
707718
!e.decorators.is_empty()
@@ -710,7 +721,9 @@ impl<'a> MayHaveSideEffects<'a> for ClassElement<'a> {
710721
&& e.value.as_ref().is_some_and(|v| v.may_have_side_effects(ctx)))
711722
}
712723
ClassElement::AccessorProperty(e) => {
713-
!e.decorators.is_empty() || e.key.may_have_side_effects(ctx)
724+
!e.decorators.is_empty()
725+
|| e.key.may_have_side_effects(ctx)
726+
|| e.value.as_ref().is_some_and(|init| init.may_have_side_effects(ctx))
714727
}
715728
ClassElement::TSIndexSignature(_) => false,
716729
}
@@ -767,7 +780,17 @@ impl<'a> MayHaveSideEffects<'a> for ComputedMemberExpression<'a> {
767780
!integer_index_property_access_may_have_side_effects(&self.object, b, ctx)
768781
})
769782
}
770-
_ => true,
783+
_ => {
784+
// Non-literal keys (e.g. `obj[expr]`) may trigger toString/valueOf on the key,
785+
// which is a side effect. But if property read side effects are disabled,
786+
// only check the key expression and object for their own side effects.
787+
if ctx.property_read_side_effects() == PropertyReadSideEffects::None {
788+
self.expression.may_have_side_effects(ctx)
789+
|| self.object.may_have_side_effects(ctx)
790+
} else {
791+
true
792+
}
793+
}
771794
}
772795
}
773796
}

‎crates/oxc_minifier/tests/ecmascript/may_have_side_effects.rs‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,20 @@ fn test(source_text: &str, expected: bool) {
7373
test_with_ctx(source_text, &ctx, expected);
7474
}
7575

76+
#[track_caller]
77+
fn test_ts(source_text: &str, expected: bool) {
78+
let ctx = Ctx::default();
79+
let allocator = Allocator::default();
80+
let ret = Parser::new(&allocator, source_text, SourceType::tsx()).parse();
81+
assert!(!ret.panicked, "{source_text}");
82+
assert!(ret.errors.is_empty(), "{source_text}");
83+
84+
let Some(Statement::ExpressionStatement(stmt)) = &ret.program.body.first() else {
85+
panic!("should have a expression statement body: {source_text}");
86+
};
87+
assert_eq!(stmt.expression.may_have_side_effects(&ctx), expected, "{source_text}");
88+
}
89+
7690
#[track_caller]
7791
fn test_with_global_variables(
7892
source_text: &str,
@@ -751,8 +765,17 @@ fn test_class_expression() {
751765
test("(class { static a = foo() })", true);
752766
test("(class { accessor [foo()]; })", true);
753767
test("(class { static accessor [foo()]; })", true);
768+
// AccessorProperty with value
769+
test("(class { accessor a = 1; })", false);
770+
test("(class { accessor a = foo(); })", true);
771+
test("(class { static accessor a = 1; })", false);
772+
test("(class { static accessor a = foo(); })", true);
754773
test("(class { #x; static { #x in {}; } })", false);
755774
test("(class { #x; static { #x in foo(); } })", true);
775+
// MethodDefinition with parameter decorators (TypeScript feature)
776+
test_ts("(class { a(@foo x) {} })", true);
777+
test_ts("(class { a(@foo x, @bar y) {} })", true);
778+
test_ts("(class { a(x) {} })", false);
756779
}
757780

758781
#[test]
@@ -913,6 +936,21 @@ fn test_new_expressions() {
913936
test("new Number", false);
914937
test("new Object", false);
915938
test("new String", false);
939+
940+
// TypedArray constructors
941+
test("new Int8Array", false);
942+
test("new Uint8Array", false);
943+
test("new Uint8ClampedArray", false);
944+
test("new Int16Array", false);
945+
test("new Uint16Array", false);
946+
test("new Int32Array", false);
947+
test("new Uint32Array", false);
948+
test("new Float32Array", false);
949+
test("new Float64Array", false);
950+
test("new BigInt64Array", false);
951+
test("new BigUint64Array", false);
952+
// DataView requires an ArrayBuffer argument; calling without one throws
953+
test("new DataView", true);
916954
}
917955

918956
// `PF` in <https://github.com/rollup/rollup/blob/master/src/ast/nodes/shared/knownGlobals.ts>
@@ -1149,10 +1187,23 @@ fn test_property_read_side_effects_support() {
11491187
test_with_ctx("foo[0]", &none_ctx, false);
11501188
test_with_ctx("foo[0n]", &none_ctx, false);
11511189
test_with_ctx("foo[bar()]", &none_ctx, true);
1190+
// Non-literal keys: when property_read_side_effects is None,
1191+
// only the key expression and object are checked for side effects
1192+
test_with_ctx("foo[bar]", &all_ctx, true);
1193+
test_with_ctx("foo[bar]", &none_ctx, false);
1194+
test_with_ctx("foo[bar()]", &all_ctx, true);
1195+
test_with_ctx("foo[bar()]", &none_ctx, true); // bar() itself has side effects
11521196
test_with_ctx("foo.#bar", &all_ctx, true);
11531197
test_with_ctx("foo.#bar", &none_ctx, false);
11541198
test_with_ctx("({ bar } = foo)", &all_ctx, true);
11551199
// test_with_ctx("({ bar } = foo)", &none_ctx, false);
1200+
1201+
// ObjectExpression spread: when property_read_side_effects is None,
1202+
// spread delegates to the argument's own side effects
1203+
test_with_ctx("({...foo})", &all_ctx, true);
1204+
test_with_ctx("({...foo})", &none_ctx, false);
1205+
test_with_ctx("({...foo()})", &all_ctx, true);
1206+
test_with_ctx("({...foo()})", &none_ctx, true); // foo() itself has side effects
11561207
}
11571208

11581209
#[test]

0 commit comments

Comments
 (0)