Skip to content

Commit

Permalink
fix(ecmascript): correct may_have_side_effects for classes (oxc-proje…
Browse files Browse the repository at this point in the history
  • Loading branch information
sapphi-red committed Feb 26, 2025
1 parent faf966f commit f5c8698
Show file tree
Hide file tree
Showing 3 changed files with 43 additions and 14 deletions.
29 changes: 24 additions & 5 deletions crates/oxc_ecmascript/src/side_effects/may_have_side_effects.rs
Original file line number Diff line number Diff line change
Expand Up @@ -340,7 +340,25 @@ impl MayHaveSideEffects for PropertyKey<'_> {
}

impl MayHaveSideEffects for Class<'_> {
/// Based on <https://github.com/evanw/esbuild/blob/v0.25.0/internal/js_ast/js_ast_helpers.go#L2320>
fn may_have_side_effects(&self, is_global_reference: &impl IsGlobalReference) -> bool {
if !self.decorators.is_empty() {
return true;
}

// NOTE: extending a value that is neither constructors nor null, throws an error
// but that error is ignored here (it is included in the assumption)
// Example cases: `class A extends 0 {}`, `class A extends (async function() {}) {}`
// Considering these cases is difficult and requires to de-opt most classes with a super class.
// To allow classes with a super class to be removed, we ignore this side effect.
if self
.super_class
.as_ref()
.is_some_and(|sup| sup.may_have_side_effects(is_global_reference))
{
return true;
}

self.body.body.iter().any(|element| element.may_have_side_effects(is_global_reference))
}
}
Expand All @@ -351,17 +369,18 @@ impl MayHaveSideEffects for ClassElement<'_> {
// TODO: check side effects inside the block
ClassElement::StaticBlock(block) => !block.body.is_empty(),
ClassElement::MethodDefinition(e) => {
e.r#static && e.key.may_have_side_effects(is_global_reference)
!e.decorators.is_empty() || e.key.may_have_side_effects(is_global_reference)
}
ClassElement::PropertyDefinition(e) => {
e.r#static
&& (e.key.may_have_side_effects(is_global_reference)
|| e.value
!e.decorators.is_empty()
|| e.key.may_have_side_effects(is_global_reference)
|| (e.r#static
&& e.value
.as_ref()
.is_some_and(|v| v.may_have_side_effects(is_global_reference)))
}
ClassElement::AccessorProperty(e) => {
e.r#static && e.key.may_have_side_effects(is_global_reference)
!e.decorators.is_empty() || e.key.may_have_side_effects(is_global_reference)
}
ClassElement::TSIndexSignature(_) => false,
}
Expand Down
2 changes: 2 additions & 0 deletions crates/oxc_minifier/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ The compressor is responsible for rewriting statements and expressions for minim
- Examples that breaks this assumption: `{ toString() { console.log('sideeffect') } }`
- Errors thrown when creating a String or an Array that exceeds the maximum length can disappear or moved
- Examples that breaks this assumption: `try { new Array(Number(2n**53n)) } catch { console.log('log') }`
- Invalid super class error does not happen
- Examples that breaks this assumption: `const v = []; class A extends v {}`

## Terser Tests

Expand Down
26 changes: 17 additions & 9 deletions crates/oxc_minifier/tests/ecmascript/may_have_side_effects.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,10 +71,10 @@ fn closure_compiler_tests() {
test("[function a(){}]", false);
test("(class { })", false);
test("(class { method() { i++ } })", false);
test("(class { [computedName()]() {} })", false); // computedName is called when constructed
test("(class { [computedName()]() {} })", true);
test("(class { [computedName]() {} })", false);
test("(class Foo extends Bar { })", false);
test("(class extends foo() { })", false); // foo() is called when constructed
test("(class extends foo() { })", true);
test("a", false);
test("a.b", true);
test("a.b.c", true);
Expand Down Expand Up @@ -223,14 +223,14 @@ fn closure_compiler_tests() {

// COMPUTED_PROP - CLASS
test("(class C { [a]() {} })", false);
test("(class C { [a()]() {} })", false); // a is called when constructed
test("(class C { [a()]() {} })", true);

// computed property getters and setters are modeled as COMPUTED_PROP with an
// annotation to indicate getter or setter.
test("(class C { get [a]() {} })", false);
test("(class C { get [a()]() {} })", false); // a is called when constructed
test("(class C { get [a()]() {} })", true);
test("(class C { set [a](x) {} })", false);
test("(class C { set [a()](x) {} })", false); // a is called when constructed
test("(class C { set [a()](x) {} })", true);

// GETTER_DEF
test("({ get a() {} })", false);
Expand Down Expand Up @@ -575,16 +575,24 @@ fn test_array_expression() {
#[test]
fn test_class_expression() {
test("(class {})", false);
test("(class extends a {})", false);
test("(class extends foo() {})", false); // foo() is called when constructed
test("(@foo class {})", true);
test("(class extends a {})", false); // this may have a side effect, but ignored by the assumption
test("(class extends foo() {})", true);
test("(class { static {} })", false);
test("(class { static { foo(); } })", true);
test("(class { a() {} })", false);
test("(class { [1]() {} })", false);
test("(class { [1n]() {} })", false);
test("(class { #a() {} })", false);
test("(class { [foo()]() {} })", true);
test("(class { @foo a() {} })", true);
test("(class { a; })", false);
test("(class { 1; })", false);
test("(class { [1]; })", false);
test("(class { [1n]; })", false);
test("(class { #a; })", false);
test("(class { [foo()] = 1 })", false); // foo() is called when constructed
test("(class { @foo a; })", true);
test("(class { [foo()] = 1 })", true);
test("(class { a = foo() })", false); // foo() is called when constructed
test("(class { static a; })", false);
test("(class { static 1; })", false);
Expand All @@ -593,7 +601,7 @@ fn test_class_expression() {
test("(class { static #a; })", false);
test("(class { static [foo()] = 1 })", true);
test("(class { static a = foo() })", true);
test("(class { accessor [foo()]; })", false);
test("(class { accessor [foo()]; })", true);
test("(class { static accessor [foo()]; })", true);
}

Expand Down

0 comments on commit f5c8698

Please sign in to comment.