Skip to content

fix(ecmascript): enhance side-effect detection for classes, TypedArrays, computed members, and spread #20213

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
graphite-app merged 1 commit into from
Mar 12, 2026
+84 −10
Merged

fix(ecmascript): enhance side-effect detection for classes, TypedArrays, computed members, and spread #20213

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 33 additions & 10 deletions crates/oxc_ecmascript/src/side_effects/expressions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,7 +297,10 @@ fn is_pure_call(name: &str) -> bool {
fn is_pure_constructor(name: &str) -> bool {
matches!(name, "Set" | "Map" | "WeakSet" | "WeakMap" | "ArrayBuffer" | "Date"
| "Boolean" | "Error" | "EvalError" | "RangeError" | "ReferenceError"
| "SyntaxError" | "TypeError" | "URIError" | "Number" | "Object" | "String")
| "SyntaxError" | "TypeError" | "URIError" | "Number" | "Object" | "String"
| "Int8Array" | "Uint8Array" | "Uint8ClampedArray" | "Int16Array" | "Uint16Array"
| "Int32Array" | "Uint32Array" | "Float32Array" | "Float64Array"
| "BigInt64Array" | "BigUint64Array")
}

/// Whether the name matches any known global constructors.
Expand Down Expand Up @@ -641,12 +644,18 @@ impl<'a> MayHaveSideEffects<'a> for ObjectPropertyKind<'a> {
fn may_have_side_effects(&self, ctx: &impl MayHaveSideEffectsContext<'a>) -> bool {
match self {
ObjectPropertyKind::ObjectProperty(o) => o.may_have_side_effects(ctx),
ObjectPropertyKind::SpreadProperty(e) => match &e.argument {
Expression::ArrayExpression(arr) => arr.may_have_side_effects(ctx),
Expression::StringLiteral(_) => false,
Expression::TemplateLiteral(t) => t.may_have_side_effects(ctx),
_ => true,
},
ObjectPropertyKind::SpreadProperty(e) => {
if ctx.property_read_side_effects() == PropertyReadSideEffects::None {
e.argument.may_have_side_effects(ctx)
} else {
match &e.argument {
Expression::ArrayExpression(arr) => arr.may_have_side_effects(ctx),
Expression::StringLiteral(_) => false,
Expression::TemplateLiteral(t) => t.may_have_side_effects(ctx),
_ => true,
}
}
}
}
}
}
Expand Down Expand Up @@ -701,7 +710,9 @@ impl<'a> MayHaveSideEffects<'a> for ClassElement<'a> {
block.body.iter().any(|stmt| stmt.may_have_side_effects(ctx))
}
ClassElement::MethodDefinition(e) => {
!e.decorators.is_empty() || e.key.may_have_side_effects(ctx)
!e.decorators.is_empty()
|| e.key.may_have_side_effects(ctx)
|| e.value.params.items.iter().any(|item| !item.decorators.is_empty())
}
ClassElement::PropertyDefinition(e) => {
!e.decorators.is_empty()
Expand All @@ -710,7 +721,9 @@ impl<'a> MayHaveSideEffects<'a> for ClassElement<'a> {
&& e.value.as_ref().is_some_and(|v| v.may_have_side_effects(ctx)))
}
ClassElement::AccessorProperty(e) => {
!e.decorators.is_empty() || e.key.may_have_side_effects(ctx)
!e.decorators.is_empty()
|| e.key.may_have_side_effects(ctx)
|| e.value.as_ref().is_some_and(|init| init.may_have_side_effects(ctx))
}
ClassElement::TSIndexSignature(_) => false,
}
Expand Down Expand Up @@ -767,7 +780,17 @@ impl<'a> MayHaveSideEffects<'a> for ComputedMemberExpression<'a> {
!integer_index_property_access_may_have_side_effects(&self.object, b, ctx)
})
}
_ => true,
_ => {
// Non-literal keys (e.g. `obj[expr]`) may trigger toString/valueOf on the key,
// which is a side effect. But if property read side effects are disabled,
// only check the key expression and object for their own side effects.
if ctx.property_read_side_effects() == PropertyReadSideEffects::None {
self.expression.may_have_side_effects(ctx)
|| self.object.may_have_side_effects(ctx)
} else {
true
}
}
}
}
}
Expand Down
51 changes: 51 additions & 0 deletions crates/oxc_minifier/tests/ecmascript/may_have_side_effects.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,20 @@ fn test(source_text: &str, expected: bool) {
test_with_ctx(source_text, &ctx, expected);
}

#[track_caller]
fn test_ts(source_text: &str, expected: bool) {
let ctx = Ctx::default();
let allocator = Allocator::default();
let ret = Parser::new(&allocator, source_text, SourceType::tsx()).parse();
assert!(!ret.panicked, "{source_text}");
assert!(ret.errors.is_empty(), "{source_text}");

let Some(Statement::ExpressionStatement(stmt)) = &ret.program.body.first() else {
panic!("should have a expression statement body: {source_text}");
};
assert_eq!(stmt.expression.may_have_side_effects(&ctx), expected, "{source_text}");
}

#[track_caller]
fn test_with_global_variables(
source_text: &str,
Expand Down Expand Up @@ -751,8 +765,17 @@ fn test_class_expression() {
test("(class { static a = foo() })", true);
test("(class { accessor [foo()]; })", true);
test("(class { static accessor [foo()]; })", true);
// AccessorProperty with value
test("(class { accessor a = 1; })", false);
test("(class { accessor a = foo(); })", true);
test("(class { static accessor a = 1; })", false);
test("(class { static accessor a = foo(); })", true);
test("(class { #x; static { #x in {}; } })", false);
test("(class { #x; static { #x in foo(); } })", true);
// MethodDefinition with parameter decorators (TypeScript feature)
test_ts("(class { a(@foo x) {} })", true);
test_ts("(class { a(@foo x, @bar y) {} })", true);
test_ts("(class { a(x) {} })", false);
}

#[test]
Expand Down Expand Up @@ -913,6 +936,21 @@ fn test_new_expressions() {
test("new Number", false);
test("new Object", false);
test("new String", false);

// TypedArray constructors
test("new Int8Array", false);
test("new Uint8Array", false);
test("new Uint8ClampedArray", false);
test("new Int16Array", false);
test("new Uint16Array", false);
test("new Int32Array", false);
test("new Uint32Array", false);
test("new Float32Array", false);
test("new Float64Array", false);
test("new BigInt64Array", false);
test("new BigUint64Array", false);
// DataView requires an ArrayBuffer argument; calling without one throws
test("new DataView", true);
}

// `PF` in <https://github.com/rollup/rollup/blob/master/src/ast/nodes/shared/knownGlobals.ts>
Expand Down Expand Up @@ -1149,10 +1187,23 @@ fn test_property_read_side_effects_support() {
test_with_ctx("foo[0]", &none_ctx, false);
test_with_ctx("foo[0n]", &none_ctx, false);
test_with_ctx("foo[bar()]", &none_ctx, true);
// Non-literal keys: when property_read_side_effects is None,
// only the key expression and object are checked for side effects
test_with_ctx("foo[bar]", &all_ctx, true);
test_with_ctx("foo[bar]", &none_ctx, false);
test_with_ctx("foo[bar()]", &all_ctx, true);
test_with_ctx("foo[bar()]", &none_ctx, true); // bar() itself has side effects
test_with_ctx("foo.#bar", &all_ctx, true);
test_with_ctx("foo.#bar", &none_ctx, false);
test_with_ctx("({ bar } = foo)", &all_ctx, true);
// test_with_ctx("({ bar } = foo)", &none_ctx, false);

// ObjectExpression spread: when property_read_side_effects is None,
// spread delegates to the argument's own side effects
test_with_ctx("({...foo})", &all_ctx, true);
test_with_ctx("({...foo})", &none_ctx, false);
test_with_ctx("({...foo()})", &all_ctx, true);
test_with_ctx("({...foo()})", &none_ctx, true); // foo() itself has side effects
}

#[test]
Expand Down
Loading