Skip to content

Commit 36386e4

Browse files
committed
feat(ecmascript): treat [...arguments] as side effect free (#13116)
Treat `[...arguments]` as side effect free.
1 parent 8459a12 commit 36386e4

File tree

2 files changed

+33
-1
lines changed

2 files changed

+33
-1
lines changed

crates/oxc_ecmascript/src/side_effects/may_have_side_effects.rs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -329,6 +329,10 @@ impl<'a> MayHaveSideEffects<'a> for ArrayExpressionElement<'a> {
329329
Expression::ArrayExpression(arr) => arr.may_have_side_effects(ctx),
330330
Expression::StringLiteral(_) => false,
331331
Expression::TemplateLiteral(t) => t.may_have_side_effects(ctx),
332+
Expression::Identifier(ident) => {
333+
// FIXME: we should treat `arguments` outside a function scope to have sideeffects
334+
!(ident.name == "arguments" && ctx.is_global_reference(ident))
335+
}
332336
_ => true,
333337
},
334338
match_expression!(ArrayExpressionElement) => {

crates/oxc_minifier/tests/ecmascript/may_have_side_effects.rs

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
use std::iter;
2+
13
use javascript_globals::GLOBALS;
24

35
use rustc_hash::FxHashSet;
@@ -22,7 +24,11 @@ struct Ctx {
2224
impl Default for Ctx {
2325
fn default() -> Self {
2426
Self {
25-
global_variable_names: GLOBALS["builtin"].keys().copied().collect::<FxHashSet<_>>(),
27+
global_variable_names: GLOBALS["builtin"]
28+
.keys()
29+
.copied()
30+
.chain(iter::once("arguments"))
31+
.collect::<FxHashSet<_>>(),
2632
annotation: true,
2733
pure_function_names: vec![],
2834
property_read_side_effects: PropertyReadSideEffects::All,
@@ -91,6 +97,26 @@ fn test_with_ctx(source_text: &str, ctx: &Ctx, expected: bool) {
9197
assert_eq!(stmt.expression.may_have_side_effects(ctx), expected, "{source_text}");
9298
}
9399

100+
#[track_caller]
101+
fn test_in_function(source_text: &str, expected: bool) {
102+
let ctx = Ctx::default();
103+
let allocator = Allocator::default();
104+
let ret = Parser::new(&allocator, source_text, SourceType::mjs()).parse();
105+
assert!(!ret.panicked, "{source_text}");
106+
assert!(ret.errors.is_empty(), "{source_text}");
107+
108+
let Some(Statement::FunctionDeclaration(stmt)) = &ret.program.body.first() else {
109+
panic!("should have a function declaration: {source_text}");
110+
};
111+
let Some(Statement::ExpressionStatement(stmt)) =
112+
&stmt.body.as_ref().expect("should have a body").statements.first()
113+
else {
114+
panic!("should have a expression statement body: {source_text}");
115+
};
116+
117+
assert_eq!(stmt.expression.may_have_side_effects(&ctx), expected, "{source_text}");
118+
}
119+
94120
/// <https://github.com/google/closure-compiler/blob/v20240609/test/com/google/javascript/jscomp/AstAnalyzerTest.java#L362>
95121
#[test]
96122
fn closure_compiler_tests() {
@@ -647,6 +673,8 @@ fn test_array_expression() {
647673
test("[...`foo${foo}`]", true);
648674
test("[...`foo${foo()}`]", true);
649675
test("[...foo()]", true);
676+
// test_in_function("[...arguments]", true);
677+
test_in_function("function foo() { [...arguments] }", false);
650678
}
651679

652680
#[test]

0 commit comments

Comments
 (0)