Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 6 additions & 0 deletions .changeset/cold-feet-dance.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
swc_core: patch
swc_ecma_minifier: patch
---

fix(es/minifier): Prevent compress.comparisons from transforming expressions with side effects
73 changes: 72 additions & 1 deletion crates/swc_ecma_minifier/src/compress/optimize/ops.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,10 @@ impl Optimizer<'_> {
}

if e.op == op!("===") || e.op == op!("!==") {
if (e.left.is_ident() || e.left.is_member()) && e.left.eq_ignore_span(&e.right) {
if (e.left.is_ident() || e.left.is_member())
&& e.left.eq_ignore_span(&e.right)
&& !contains_update_or_assign(&e.left)
{
self.changed = true;
report_change!("Reducing comparison of same variable ({})", e.op);

Expand Down Expand Up @@ -288,3 +291,71 @@ impl Optimizer<'_> {
}
}
}

/// Check if an expression contains update expressions (++, --) or assignments
/// that would make duplicate evaluations produce different results.
fn contains_update_or_assign(expr: &Expr) -> bool {
match expr {
Expr::Update(..) | Expr::Assign(..) => true,

Expr::Bin(BinExpr { left, right, .. }) => {
contains_update_or_assign(left) || contains_update_or_assign(right)
}

Expr::Unary(UnaryExpr { arg, .. }) => contains_update_or_assign(arg),

Expr::Cond(CondExpr {
test, cons, alt, ..
}) => {
contains_update_or_assign(test)
|| contains_update_or_assign(cons)
|| contains_update_or_assign(alt)
}

Expr::Member(MemberExpr { obj, prop, .. }) => {
contains_update_or_assign(obj)
|| match prop {
MemberProp::Computed(ComputedPropName { expr, .. }) => {
contains_update_or_assign(expr)
}
_ => false,
}
}

Expr::Call(CallExpr {
callee: Callee::Expr(callee),
args,
..
}) => {
contains_update_or_assign(callee)
|| args.iter().any(|arg| contains_update_or_assign(&arg.expr))
}

Expr::Seq(SeqExpr { exprs, .. }) => {
exprs.iter().any(|expr| contains_update_or_assign(expr))
}

Expr::Paren(ParenExpr { expr, .. }) => contains_update_or_assign(expr),

Expr::OptChain(OptChainExpr { base, .. }) => match &**base {
OptChainBase::Member(member) => {
contains_update_or_assign(&member.obj)
|| match &member.prop {
MemberProp::Computed(ComputedPropName { expr, .. }) => {
contains_update_or_assign(expr)
}
_ => false,
}
}
OptChainBase::Call(call) => {
contains_update_or_assign(&call.callee)
|| call
.args
.iter()
.any(|arg| contains_update_or_assign(&arg.expr))
}
},

Copy link

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The contains_update_or_assign function is missing handling for Expr::Await expressions. Await expressions can contain update or assignment operations in their argument (e.g., await Promise.resolve(++x)), which should be detected to prevent incorrect optimizations.

Add a case for Expr::Await:

Expr::Await(AwaitExpr { arg, .. }) => contains_update_or_assign(arg),
Suggested change
Expr::Await(AwaitExpr { arg, .. }) => contains_update_or_assign(arg),

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The contains_update_or_assign function is missing handling for Expr::TaggedTpl (tagged template literals). Tagged templates can contain update or assignment operations in their tag expression or template expressions (e.g., (++x)`${++y}`), which should be detected to prevent incorrect optimizations.

Add a case for Expr::TaggedTpl:

Expr::TaggedTpl(TaggedTpl { tag, tpl, .. }) => {
    contains_update_or_assign(tag)
        || tpl.exprs.iter().any(|expr| contains_update_or_assign(expr))
}
Suggested change
Expr::TaggedTpl(TaggedTpl { tag, tpl, .. }) => {
contains_update_or_assign(tag)
|| tpl.exprs.iter().any(|expr| contains_update_or_assign(expr))
}

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The contains_update_or_assign function is missing handling for Expr::SuperProp (super property access). Super property expressions can have computed properties containing update or assignment operations (e.g., super[++x]), which should be detected to prevent incorrect optimizations.

Add a case for Expr::SuperProp:

Expr::SuperProp(SuperPropExpr { prop, .. }) => match prop {
    SuperProp::Computed(ComputedPropName { expr, .. }) => {
        contains_update_or_assign(expr)
    }
    _ => false,
},
Suggested change
Expr::SuperProp(SuperPropExpr { prop, .. }) => match prop {
SuperProp::Computed(ComputedPropName { expr, .. }) => {
contains_update_or_assign(expr)
}
_ => false,
},

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The contains_update_or_assign function is missing handling for Expr::Yield expressions. Yield expressions can contain update or assignment operations in their argument (e.g., yield ++x), which should be detected to prevent incorrect optimizations.

Add a case for Expr::Yield:

Expr::Yield(YieldExpr { arg, .. }) => {
    arg.as_ref()
        .map(|arg| contains_update_or_assign(arg))
        .unwrap_or(false)
}
Suggested change
Expr::Yield(YieldExpr { arg, .. }) => {
arg.as_ref()
.map(|arg| contains_update_or_assign(arg))
.unwrap_or(false)
}

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The contains_update_or_assign function is missing handling for TypeScript-specific expression wrappers (TsTypeAssertion, TsConstAssertion, TsNonNull, TsAs, TsInstantiation, TsSatisfies). These expressions wrap other expressions that may contain update or assignment operations, which should be detected to prevent incorrect optimizations.

Add cases for TypeScript expressions:

Expr::TsTypeAssertion(TsTypeAssertion { expr, .. })
| Expr::TsConstAssertion(TsConstAssertion { expr, .. })
| Expr::TsNonNull(TsNonNullExpr { expr, .. })
| Expr::TsAs(TsAsExpr { expr, .. })
| Expr::TsInstantiation(TsInstantiation { expr, .. })
| Expr::TsSatisfies(TsSatisfiesExpr { expr, .. }) => contains_update_or_assign(expr),
Suggested change
// Handle TypeScript-specific expression wrappers
Expr::TsTypeAssertion(TsTypeAssertion { expr, .. })
| Expr::TsConstAssertion(TsConstAssertion { expr, .. })
| Expr::TsNonNull(TsNonNullExpr { expr, .. })
| Expr::TsAs(TsAsExpr { expr, .. })
| Expr::TsInstantiation(TsInstantiation { expr, .. })
| Expr::TsSatisfies(TsSatisfiesExpr { expr, .. }) => contains_update_or_assign(expr),

Copilot uses AI. Check for mistakes.
_ => false,
}
}
Comment on lines +297 to +361
Copy link

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The contains_update_or_assign function is missing handling for Expr::New expressions. New expressions can contain update or assignment operations in their arguments (e.g., new Foo(++x)), which should be detected to prevent incorrect optimizations.

Add a case for Expr::New:

Expr::New(NewExpr { args, .. }) => {
    args.as_ref()
        .map(|args| args.iter().any(|arg| contains_update_or_assign(&arg.expr)))
        .unwrap_or(false)
}

Copilot uses AI. Check for mistakes.
Comment on lines +297 to +361
Copy link

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The contains_update_or_assign function is missing handling for Expr::Tpl (template literals). Template literals can contain update or assignment operations in their template expressions (e.g., `${++x}`), which should be detected to prevent incorrect optimizations.

Add a case for Expr::Tpl:

Expr::Tpl(Tpl { exprs, .. }) => {
    exprs.iter().any(|expr| contains_update_or_assign(expr))
}

Copilot uses AI. Check for mistakes.
Comment on lines +297 to +361
Copy link

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The contains_update_or_assign function is missing handling for Expr::Array (array literals). Array literals can contain update or assignment operations in their elements (e.g., [++x, y]), which should be detected to prevent incorrect optimizations.

Add a case for Expr::Array:

Expr::Array(ArrayLit { elems, .. }) => {
    elems.iter().any(|elem| {
        elem.as_ref()
            .map(|e| contains_update_or_assign(&e.expr))
            .unwrap_or(false)
    })
}

Copilot uses AI. Check for mistakes.
Comment on lines +358 to +361
Copy link

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The contains_update_or_assign function is missing handling for Expr::Object (object literals). Object literals can contain update or assignment operations in their property values, spread elements, or computed property keys (e.g., {[++x]: y, z: ++w, ...obj}), which should be detected to prevent incorrect optimizations.

Add a case for Expr::Object:

Expr::Object(ObjectLit { props, .. }) => {
    props.iter().any(|prop| match prop {
        PropOrSpread::Spread(SpreadElement { expr, .. }) => {
            contains_update_or_assign(expr)
        }
        PropOrSpread::Prop(prop) => match &**prop {
            Prop::KeyValue(KeyValueProp { key, value }) => {
                check_prop_name_for_side_effects(key)
                    || contains_update_or_assign(value)
            }
            Prop::Assign(AssignProp { value, .. }) => {
                contains_update_or_assign(value)
            }
            Prop::Getter(GetterProp { key, .. })
            | Prop::Setter(SetterProp { key, .. })
            | Prop::Method(MethodProp { key, .. }) => {
                check_prop_name_for_side_effects(key)
            }
            Prop::Shorthand(_) => false,
        },
    })
}

where check_prop_name_for_side_effects checks if a PropName::Computed contains side effects.

Suggested change
_ => false,
}
}
Expr::Object(ObjectLit { props, .. }) => {
props.iter().any(|prop| match prop {
PropOrSpread::Spread(SpreadElement { expr, .. }) => {
contains_update_or_assign(expr)
}
PropOrSpread::Prop(prop) => match &**prop {
Prop::KeyValue(KeyValueProp { key, value }) => {
check_prop_name_for_side_effects(key)
|| contains_update_or_assign(value)
}
Prop::Assign(AssignProp { value, .. }) => {
contains_update_or_assign(value)
}
Prop::Getter(GetterProp { key, .. })
| Prop::Setter(SetterProp { key, .. })
| Prop::Method(MethodProp { key, .. }) => {
check_prop_name_for_side_effects(key)
}
Prop::Shorthand(_) => false,
},
})
}
_ => false,
}
}
/// Checks if a property name contains side effects, i.e., if it's a computed property whose expression contains update or assignment.
fn check_prop_name_for_side_effects(key: &PropName) -> bool {
match key {
PropName::Computed(ComputedPropName { expr, .. }) => contains_update_or_assign(expr),
_ => false,
}
}

Copilot uses AI. Check for mistakes.
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"comparisons": true
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
// Issue #11255: compress.comparisons should not optimize comparisons with side effects
let PC = 0;
const Stack = [0, ''];
const Code = [0, 0, 1];
console.log(Stack[Code[++PC]] === Stack[Code[++PC]]);
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
// Issue #11255: compress.comparisons should not optimize comparisons with side effects
let PC = 0;
const Stack = [0, ''];
const Code = [0, 0, 1];
console.log(Stack[Code[++PC]] === Stack[Code[++PC]]);
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// Issue #11255: compress.comparisons should not optimize comparisons with side effects
let o = 0;
const c = [
0,
''
];
const l = [
0,
0,
1
];
console.log(c[l[++o]] === c[l[++o]]);
Loading