From c557854fe3f8bbf8500a3ace014f4fbfca0ccd44 Mon Sep 17 00:00:00 2001 From: sapphi-red <49056869+sapphi-red@users.noreply.github.com> Date: Sat, 30 Aug 2025 04:15:20 +0000 Subject: [PATCH] feat(ecmascript): support MayHaveSideEffects for Statements (#13402) Added MayHaveSideEffects for Statements. --- ...ay_have_side_effects.rs => expressions.rs} | 23 +-- crates/oxc_ecmascript/src/side_effects/mod.rs | 25 ++- .../src/side_effects/statements.rs | 141 ++++++++++++++ .../may_have_side_effects_statements.rs | 178 ++++++++++++++++++ crates/oxc_minifier/tests/ecmascript/mod.rs | 1 + 5 files changed, 344 insertions(+), 24 deletions(-) rename crates/oxc_ecmascript/src/side_effects/{may_have_side_effects.rs => expressions.rs} (96%) create mode 100644 crates/oxc_ecmascript/src/side_effects/statements.rs create mode 100644 crates/oxc_minifier/tests/ecmascript/may_have_side_effects_statements.rs diff --git a/crates/oxc_ecmascript/src/side_effects/may_have_side_effects.rs b/crates/oxc_ecmascript/src/side_effects/expressions.rs similarity index 96% rename from crates/oxc_ecmascript/src/side_effects/may_have_side_effects.rs rename to crates/oxc_ecmascript/src/side_effects/expressions.rs index 97b1c60a577a5..066d4bff72d39 100644 --- a/crates/oxc_ecmascript/src/side_effects/may_have_side_effects.rs +++ b/crates/oxc_ecmascript/src/side_effects/expressions.rs @@ -5,28 +5,7 @@ use crate::{ to_primitive::ToPrimitive, }; -use super::{PropertyReadSideEffects, context::MayHaveSideEffectsContext}; - -/// Returns true if subtree changes application state. -/// -/// This trait assumes the following: -/// - `.toString()`, `.valueOf()`, and `[Symbol.toPrimitive]()` are side-effect free. -/// - This is mainly to assume `ToPrimitive` is side-effect free. -/// - Note that the builtin `Array::toString` has a side-effect when a value contains a Symbol as `ToString(Symbol)` throws an error. Maybe we should revisit this assumption and remove it. -/// - For example, `"" == [Symbol()]` returns an error, but this trait returns `false`. -/// - Errors thrown when creating a String or an Array that exceeds the maximum length does not happen. -/// - TDZ errors does not happen. -/// -/// Ported from [closure-compiler](https://github.com/google/closure-compiler/blob/f3ce5ed8b630428e311fe9aa2e20d36560d975e2/src/com/google/javascript/jscomp/AstAnalyzer.java#L94) -pub trait MayHaveSideEffects<'a> { - fn may_have_side_effects(&self, ctx: &impl MayHaveSideEffectsContext<'a>) -> bool; -} - -impl<'a, T: MayHaveSideEffects<'a>> MayHaveSideEffects<'a> for Option { - fn may_have_side_effects(&self, ctx: &impl MayHaveSideEffectsContext<'a>) -> bool { - self.as_ref().is_some_and(|t| t.may_have_side_effects(ctx)) - } -} +use super::{MayHaveSideEffects, PropertyReadSideEffects, context::MayHaveSideEffectsContext}; impl<'a> MayHaveSideEffects<'a> for Expression<'a> { fn may_have_side_effects(&self, ctx: &impl MayHaveSideEffectsContext<'a>) -> bool { diff --git a/crates/oxc_ecmascript/src/side_effects/mod.rs b/crates/oxc_ecmascript/src/side_effects/mod.rs index 5c54fd8895a89..0ef20bb469810 100644 --- a/crates/oxc_ecmascript/src/side_effects/mod.rs +++ b/crates/oxc_ecmascript/src/side_effects/mod.rs @@ -1,5 +1,26 @@ mod context; -mod may_have_side_effects; +mod expressions; +mod statements; pub use context::{MayHaveSideEffectsContext, PropertyReadSideEffects}; -pub use may_have_side_effects::MayHaveSideEffects; + +/// Returns true if subtree changes application state. +/// +/// This trait assumes the following: +/// - `.toString()`, `.valueOf()`, and `[Symbol.toPrimitive]()` are side-effect free. +/// - This is mainly to assume `ToPrimitive` is side-effect free. +/// - Note that the builtin `Array::toString` has a side-effect when a value contains a Symbol as `ToString(Symbol)` throws an error. Maybe we should revisit this assumption and remove it. +/// - For example, `"" == [Symbol()]` returns an error, but this trait returns `false`. +/// - Errors thrown when creating a String or an Array that exceeds the maximum length does not happen. +/// - TDZ errors does not happen. +/// +/// Ported from [closure-compiler](https://github.com/google/closure-compiler/blob/f3ce5ed8b630428e311fe9aa2e20d36560d975e2/src/com/google/javascript/jscomp/AstAnalyzer.java#L94) +pub trait MayHaveSideEffects<'a> { + fn may_have_side_effects(&self, ctx: &impl MayHaveSideEffectsContext<'a>) -> bool; +} + +impl<'a, T: MayHaveSideEffects<'a>> MayHaveSideEffects<'a> for Option { + fn may_have_side_effects(&self, ctx: &impl MayHaveSideEffectsContext<'a>) -> bool { + self.as_ref().is_some_and(|t| t.may_have_side_effects(ctx)) + } +} diff --git a/crates/oxc_ecmascript/src/side_effects/statements.rs b/crates/oxc_ecmascript/src/side_effects/statements.rs new file mode 100644 index 0000000000000..120498f457f20 --- /dev/null +++ b/crates/oxc_ecmascript/src/side_effects/statements.rs @@ -0,0 +1,141 @@ +use oxc_ast::ast::*; + +use crate::constant_evaluation::{DetermineValueType, ValueType}; + +use super::{MayHaveSideEffects, PropertyReadSideEffects, context::MayHaveSideEffectsContext}; + +impl<'a> MayHaveSideEffects<'a> for Statement<'a> { + fn may_have_side_effects(&self, ctx: &impl MayHaveSideEffectsContext<'a>) -> bool { + match self { + Statement::BlockStatement(block) => block.may_have_side_effects(ctx), + Statement::DoWhileStatement(do_while) => do_while.may_have_side_effects(ctx), + Statement::ExpressionStatement(expr) => expr.expression.may_have_side_effects(ctx), + Statement::IfStatement(if_stmt) => if_stmt.may_have_side_effects(ctx), + Statement::LabeledStatement(labeled) => labeled.body.may_have_side_effects(ctx), + Statement::ReturnStatement(return_stmt) => { + return_stmt.argument.may_have_side_effects(ctx) + } + Statement::SwitchStatement(switch) => switch.may_have_side_effects(ctx), + Statement::TryStatement(try_stmt) => try_stmt.may_have_side_effects(ctx), + Statement::WhileStatement(while_stmt) => while_stmt.may_have_side_effects(ctx), + Statement::BreakStatement(_) + | Statement::ContinueStatement(_) + | Statement::EmptyStatement(_) => false, + match_declaration!(Statement) => self.to_declaration().may_have_side_effects(ctx), + Statement::ForInStatement(_) + | Statement::ForOfStatement(_) + | Statement::ForStatement(_) + | Statement::ThrowStatement(_) + | Statement::WithStatement(_) + | Statement::DebuggerStatement(_) => true, + #[expect(clippy::match_same_arms)] + match_module_declaration!(Statement) => true, + } + } +} + +impl<'a> MayHaveSideEffects<'a> for BlockStatement<'a> { + fn may_have_side_effects(&self, ctx: &impl MayHaveSideEffectsContext<'a>) -> bool { + self.body.iter().any(|stmt| stmt.may_have_side_effects(ctx)) + } +} + +impl<'a> MayHaveSideEffects<'a> for DoWhileStatement<'a> { + fn may_have_side_effects(&self, ctx: &impl MayHaveSideEffectsContext<'a>) -> bool { + self.test.may_have_side_effects(ctx) || self.body.may_have_side_effects(ctx) + } +} + +impl<'a> MayHaveSideEffects<'a> for IfStatement<'a> { + fn may_have_side_effects(&self, ctx: &impl MayHaveSideEffectsContext<'a>) -> bool { + self.test.may_have_side_effects(ctx) + || self.consequent.may_have_side_effects(ctx) + || self.alternate.may_have_side_effects(ctx) + } +} + +impl<'a> MayHaveSideEffects<'a> for SwitchStatement<'a> { + fn may_have_side_effects(&self, ctx: &impl MayHaveSideEffectsContext<'a>) -> bool { + self.discriminant.may_have_side_effects(ctx) + || self.cases.iter().any(|case| { + case.test.may_have_side_effects(ctx) + || case.consequent.iter().any(|stmt| stmt.may_have_side_effects(ctx)) + }) + } +} + +impl<'a> MayHaveSideEffects<'a> for TryStatement<'a> { + fn may_have_side_effects(&self, ctx: &impl MayHaveSideEffectsContext<'a>) -> bool { + self.block.may_have_side_effects(ctx) + || self.handler.as_ref().is_some_and(|catch_clause| { + catch_clause + .param + .as_ref() + .is_some_and(|param| param.pattern.may_have_side_effects(ctx)) + || catch_clause.body.may_have_side_effects(ctx) + }) + || self.finalizer.as_ref().is_some_and(|finalizer| finalizer.may_have_side_effects(ctx)) + } +} + +impl<'a> MayHaveSideEffects<'a> for WhileStatement<'a> { + fn may_have_side_effects(&self, ctx: &impl MayHaveSideEffectsContext<'a>) -> bool { + self.test.may_have_side_effects(ctx) || self.body.may_have_side_effects(ctx) + } +} + +impl<'a> MayHaveSideEffects<'a> for Declaration<'a> { + fn may_have_side_effects(&self, ctx: &impl MayHaveSideEffectsContext<'a>) -> bool { + match self { + Declaration::VariableDeclaration(var_decl) => var_decl.may_have_side_effects(ctx), + Declaration::FunctionDeclaration(_) => false, + Declaration::ClassDeclaration(class_decl) => class_decl.may_have_side_effects(ctx), + Declaration::TSEnumDeclaration(_) + | Declaration::TSImportEqualsDeclaration(_) + | Declaration::TSModuleDeclaration(_) + | Declaration::TSInterfaceDeclaration(_) + | Declaration::TSTypeAliasDeclaration(_) => unreachable!(), + } + } +} + +impl<'a> MayHaveSideEffects<'a> for VariableDeclaration<'a> { + fn may_have_side_effects(&self, ctx: &impl MayHaveSideEffectsContext<'a>) -> bool { + if self.kind == VariableDeclarationKind::AwaitUsing { + return true; + } + if self.kind == VariableDeclarationKind::Using { + return self.declarations.iter().any(|decl| { + decl.init.as_ref().is_none_or(|init| { + !matches!(init.value_type(ctx), ValueType::Undefined | ValueType::Null) + || init.may_have_side_effects(ctx) + }) + }); + } + self.declarations + .iter() + .any(|decl| decl.id.may_have_side_effects(ctx) || decl.init.may_have_side_effects(ctx)) + } +} + +impl<'a> MayHaveSideEffects<'a> for BindingPattern<'a> { + fn may_have_side_effects(&self, ctx: &impl MayHaveSideEffectsContext<'a>) -> bool { + match &self.kind { + BindingPatternKind::ArrayPattern(array_pattern) => { + ctx.property_read_side_effects() != PropertyReadSideEffects::None + || array_pattern.elements.iter().any(|el| el.may_have_side_effects(ctx)) + } + BindingPatternKind::ObjectPattern(object_pattern) => { + ctx.property_read_side_effects() != PropertyReadSideEffects::None + || object_pattern.properties.iter().any(|prop| { + prop.key.may_have_side_effects(ctx) || prop.value.may_have_side_effects(ctx) + }) + } + BindingPatternKind::AssignmentPattern(assignment_pattern) => { + assignment_pattern.left.may_have_side_effects(ctx) + || assignment_pattern.right.may_have_side_effects(ctx) + } + BindingPatternKind::BindingIdentifier(_) => false, + } + } +} diff --git a/crates/oxc_minifier/tests/ecmascript/may_have_side_effects_statements.rs b/crates/oxc_minifier/tests/ecmascript/may_have_side_effects_statements.rs new file mode 100644 index 0000000000000..3ca3b13216a43 --- /dev/null +++ b/crates/oxc_minifier/tests/ecmascript/may_have_side_effects_statements.rs @@ -0,0 +1,178 @@ +use javascript_globals::GLOBALS; +use oxc_allocator::Allocator; +use oxc_ast::ast::{Expression, IdentifierReference, Statement}; +use oxc_ecmascript::{ + GlobalContext, + side_effects::{MayHaveSideEffects, MayHaveSideEffectsContext}, +}; +use oxc_minifier::PropertyReadSideEffects; +use oxc_parser::Parser; +use oxc_span::SourceType; +use rustc_hash::FxHashSet; + +struct Ctx { + global_variable_names: FxHashSet<&'static str>, +} + +impl Default for Ctx { + fn default() -> Self { + Self { + global_variable_names: GLOBALS["builtin"] + .keys() + .copied() + .chain(["arguments", "URL"]) + .collect::>(), + } + } +} + +impl<'a> GlobalContext<'a> for Ctx { + fn is_global_reference(&self, ident: &IdentifierReference<'a>) -> bool { + self.global_variable_names.contains(ident.name.as_str()) + } +} + +impl MayHaveSideEffectsContext<'_> for Ctx { + fn annotations(&self) -> bool { + true + } + + fn manual_pure_functions(&self, _callee: &Expression) -> bool { + false + } + + fn property_read_side_effects(&self) -> PropertyReadSideEffects { + PropertyReadSideEffects::All + } + + fn unknown_global_side_effects(&self) -> bool { + true + } +} + +#[track_caller] +fn test(source_text: &str, expected: bool) { + let allocator = Allocator::default(); + let ret = Parser::new(&allocator, source_text, SourceType::mjs()).parse(); + assert!(!ret.panicked, "{source_text}"); + assert!(ret.errors.is_empty(), "{source_text}"); + + let stmt = ret.program.body.first().unwrap(); + assert_eq!(stmt.may_have_side_effects(&Ctx::default()), expected, "{source_text}"); +} + +#[track_caller] +fn test_in_function(source_text: &str, expected: bool) { + let allocator = Allocator::default(); + let ret = Parser::new(&allocator, source_text, SourceType::mjs()).parse(); + assert!(!ret.panicked, "{source_text}"); + assert!(ret.errors.is_empty(), "{source_text}"); + + let Some(Statement::FunctionDeclaration(stmt)) = &ret.program.body.first() else { + panic!("should have a function declaration: {source_text}"); + }; + let stmt = stmt.body.as_ref().expect("should have a body").statements.first().unwrap(); + assert_eq!(stmt.may_have_side_effects(&Ctx::default()), expected, "{source_text}"); +} + +#[test] +fn test_block() { + test("{}", false); + test("{ ; ; }", false); + test("{ foo() }", true); + test("{ ; ; foo() }", true); +} + +#[test] +fn test_do_while() { + test("do { foo() } while (true)", true); + test("do {} while (foo())", true); + test("do {} while (true)", false); +} + +#[test] +fn test_expr() { + test("1", false); + test("foo()", true); +} + +#[test] +fn test_if() { + test("if (foo()) {}", true); + test("if (true) { foo() }", true); + test("if (true) {}", false); +} + +#[test] +fn test_labeled() { + test("label: foo()", true); + test("label: 1", false); +} + +#[test] +fn test_return() { + test_in_function("function _() { return foo() }", true); + test_in_function("function _() { return 1 }", false); +} + +#[test] +fn test_switch() { + test("switch (foo()) {}", true); + test("switch (true) { case foo(): }", true); + test("switch (true) { case true: foo() }", true); + test("switch (true) { case true: true }", false); +} + +#[test] +fn test_try() { + test("try { foo() } catch {}", true); + test("try { true } catch ({}) {}", true); + test("try { true } catch { foo() }", true); + test("try { true } finally { foo() }", true); + test("try { true } catch (e) { true } finally { true }", false); +} + +#[test] +fn test_while() { + test("while (true) { foo() }", true); + test("while (foo()) {}", true); + test("while (true) {}", false); +} + +#[test] +fn test_declarations() { + test("await using a = null", true); + test("await using a = true", true); + test("using a = null", false); + test("using a = void 0", false); + test("using a = null, b = 1", true); + test("using a = void foo()", true); + test("var a = foo()", true); + test("var a = true", false); + test("let a = foo()", true); + test("let a = true", false); + test("const a = foo()", true); + test("const a = true", false); + + test("var [a] = []", true); + test("var [a = foo()] = []", true); + test("var [[a] = [foo()]] = []", true); + test("var [a] = foo", true); + test("var {a} = {}", true); + test("var {a = foo()} = {}", true); + test("var {a} = foo", true); +} + +#[test] +fn test_others() { + test("for (var a in b) {}", true); + test("for (var a of b) {}", true); + test("for (;;) {}", true); + test("throw 1", true); + test("with (a) {}", true); + test("debugger", true); + + test("import 'a'", true); + test("export * from 'a'", true); + test("export { a }", true); +} diff --git a/crates/oxc_minifier/tests/ecmascript/mod.rs b/crates/oxc_minifier/tests/ecmascript/mod.rs index 411db7fb6e4f9..6f75192cd8458 100644 --- a/crates/oxc_minifier/tests/ecmascript/mod.rs +++ b/crates/oxc_minifier/tests/ecmascript/mod.rs @@ -2,6 +2,7 @@ mod array_join; mod is_int32_or_uint32; mod is_literal_value; mod may_have_side_effects; +mod may_have_side_effects_statements; mod prop_name; mod to_boolean; mod to_number;