fix(minifier): avoid merging side effectful expressions to next assignment statement if the side effect may change the left hand side reference

Author: sapphi-red

apps/oxlint/src-js/generated/deserialize.js

@@ -4991,21 +4991,22 @@ function deserializeTSModuleDeclaration(pos) {
     } else {
       let innerId = body.id;
       if (innerId.type === "Identifier") {
-        let start, end;
-        id.parent =
-          innerId.parent =
-          node.id =
-          parent =
-            {
-              __proto__: NodeProto,
-              type: "TSQualifiedName",
-              left: id,
-              right: innerId,
-              start: (start = id.start),
-              end: (end = innerId.end),
-              range: [start, end],
-              parent: node,
-            };
+        let start,
+          end,
+          outerId =
+            (node.id =
+            parent =
+              {
+                __proto__: NodeProto,
+                type: "TSQualifiedName",
+                left: id,
+                right: innerId,
+                start: (start = id.start),
+                end: (end = innerId.end),
+                range: [start, end],
+                parent: node,
+              });
+        id.parent = innerId.parent = outerId;
       } else {
         // Replace `left` of innermost `TSQualifiedName` with a nested `TSQualifiedName` with `id` of
         // this module on left, and previous `left` of innermost `TSQualifiedName` on right

crates/oxc_minifier/src/peephole/minimize_statements.rs

@@ -1384,6 +1384,38 @@ impl<'a> PeepholeOptimizations {
                     // ```
                     return Some(false);
                 }
+                if replacement_has_side_effect {
+                    // If the assignment target may depend on side effects of the replacement,
+                    // don't reorder it past the assignment target. The non-last part of the
+                    // assignment target is evaluated before the assignment evaluation so that
+                    // part may be changed by the side effect. For example, "fn()" may change
+                    // "foo" here:
+                    // ```js
+                    // let a = fn();
+                    // foo.bar = a;
+                    // ```
+                    let may_depend_on_side_effect = match &assign_expr.left {
+                        AssignmentTarget::AssignmentTargetIdentifier(_) => false,
+                        AssignmentTarget::ComputedMemberExpression(member_expr) => {
+                            Self::is_expression_that_reference_may_change(&member_expr.object, ctx)
+                        }
+                        AssignmentTarget::PrivateFieldExpression(member_expr) => {
+                            Self::is_expression_that_reference_may_change(&member_expr.object, ctx)
+                        }
+                        AssignmentTarget::StaticMemberExpression(member_expr) => {
+                            Self::is_expression_that_reference_may_change(&member_expr.object, ctx)
+                        }
+                        AssignmentTarget::ArrayAssignmentTarget(_)
+                        | AssignmentTarget::ObjectAssignmentTarget(_)
+                        | AssignmentTarget::TSAsExpression(_)
+                        | AssignmentTarget::TSNonNullExpression(_)
+                        | AssignmentTarget::TSSatisfiesExpression(_)
+                        | AssignmentTarget::TSTypeAssertion(_) => true,
+                    };
+                    if may_depend_on_side_effect {
+                        return Some(false);
+                    }
+                }
                 // If we get here then it should be safe to attempt to substitute the
                 // replacement past the left operand into the right operand.
                 if let Some(changed) = Self::substitute_single_use_symbol_in_expression(
@@ -1799,6 +1831,21 @@ impl<'a> PeepholeOptimizations {
         // Otherwise we should stop trying to substitute past this point
         Some(false)
     }
+
+    fn is_expression_that_reference_may_change(expr: &Expression<'a>, ctx: &Ctx<'a, '_>) -> bool {
+        match expr {
+            Expression::Identifier(id) => {
+                if let Some(symbol_id) = ctx.scoping().get_reference(id.reference_id()).symbol_id()
+                {
+                    ctx.scoping().symbol_is_mutated(symbol_id)
+                } else {
+                    true
+                }
+            }
+            Expression::ThisExpression(_) => false,
+            _ => true,
+        }
+    }
 }
 
 #[cfg(test)]

crates/oxc_minifier/tests/peephole/inline_single_use_variable.rs

@@ -222,6 +222,25 @@ fn test_inline_single_use_variable() {
         "function wrapper() { var __proto__ = []; return { __proto__ } }",
         "function wrapper() { return { ['__proto__']: [] } }",
     );
+
+    // cannot be compressed to `function wrapper() { var foo; globalThis[console.log(foo)] = (() => { foo = 1 })() }`
+    test(
+        "function wrapper() { var foo; var bar = (() => { foo = 1 })(); globalThis[console.log(foo)] = bar }",
+        "function wrapper() { var foo, bar = (() => { foo = 1 })(); globalThis[console.log(foo)] = bar }",
+    );
+    // cannot be compressed to `function wrapper() { let foo; return foo.bar = (() => { foo = {} })(), foo }`
+    test(
+        "function wrapper() { let foo; const bar = (() => { foo = {} })(); foo.bar = bar; return foo }",
+        "function wrapper() { let foo, bar = (() => { foo = {} })(); return foo.bar = bar, foo }",
+    );
+    test(
+        "function wrapper() { let foo = {}; const bar = (() => { console.log() })(); foo.bar = bar; return foo }",
+        "function wrapper() { let foo = {}; return foo.bar = (() => { console.log() })(), foo }",
+    );
+    test(
+        "function wrapper() { const bar = (() => { console.log() })(); this.bar = bar; return this }",
+        "function wrapper() { return this.bar = (() => { console.log() })(), this }",
+    );
 }
 
 #[test]

napi/parser/generated/deserialize/js_parent.js

@@ -4346,10 +4346,9 @@ function deserializeTSModuleDeclaration(pos) {
       body.parent = node;
     } else {
       let innerId = body.id;
-      if (innerId.type === "Identifier")
-        id.parent =
-          innerId.parent =
-          node.id =
+      if (innerId.type === "Identifier") {
+        let outerId =
+          (node.id =
           parent =
             {
               type: "TSQualifiedName",
@@ -4358,8 +4357,9 @@ function deserializeTSModuleDeclaration(pos) {
               start: id.start,
               end: innerId.end,
               parent: node,
-            };
-      else {
+            });
+        id.parent = innerId.parent = outerId;
+      } else {
         // Replace `left` of innermost `TSQualifiedName` with a nested `TSQualifiedName` with `id` of
         // this module on left, and previous `left` of innermost `TSQualifiedName` on right
         node.id = innerId;

napi/parser/generated/deserialize/js_range_parent.js

@@ -4560,20 +4560,21 @@ function deserializeTSModuleDeclaration(pos) {
     } else {
       let innerId = body.id;
       if (innerId.type === "Identifier") {
-        let start, end;
-        id.parent =
-          innerId.parent =
-          node.id =
-          parent =
-            {
-              type: "TSQualifiedName",
-              left: id,
-              right: innerId,
-              start: (start = id.start),
-              end: (end = innerId.end),
-              range: [start, end],
-              parent: node,
-            };
+        let start,
+          end,
+          outerId =
+            (node.id =
+            parent =
+              {
+                type: "TSQualifiedName",
+                left: id,
+                right: innerId,
+                start: (start = id.start),
+                end: (end = innerId.end),
+                range: [start, end],
+                parent: node,
+              });
+        id.parent = innerId.parent = outerId;
       } else {
         // Replace `left` of innermost `TSQualifiedName` with a nested `TSQualifiedName` with `id` of
         // this module on left, and previous `left` of innermost `TSQualifiedName` on right

napi/parser/generated/deserialize/ts_parent.js

@@ -4604,10 +4604,9 @@ function deserializeTSModuleDeclaration(pos) {
       body.parent = node;
     } else {
       let innerId = body.id;
-      if (innerId.type === "Identifier")
-        id.parent =
-          innerId.parent =
-          node.id =
+      if (innerId.type === "Identifier") {
+        let outerId =
+          (node.id =
           parent =
             {
               type: "TSQualifiedName",
@@ -4616,8 +4615,9 @@ function deserializeTSModuleDeclaration(pos) {
               start: id.start,
               end: innerId.end,
               parent: node,
-            };
-      else {
+            });
+        id.parent = innerId.parent = outerId;
+      } else {
         // Replace `left` of innermost `TSQualifiedName` with a nested `TSQualifiedName` with `id` of
         // this module on left, and previous `left` of innermost `TSQualifiedName` on right
         node.id = innerId;

napi/parser/generated/deserialize/ts_range_parent.js

@@ -4825,20 +4825,21 @@ function deserializeTSModuleDeclaration(pos) {
     } else {
       let innerId = body.id;
       if (innerId.type === "Identifier") {
-        let start, end;
-        id.parent =
-          innerId.parent =
-          node.id =
-          parent =
-            {
-              type: "TSQualifiedName",
-              left: id,
-              right: innerId,
-              start: (start = id.start),
-              end: (end = innerId.end),
-              range: [start, end],
-              parent: node,
-            };
+        let start,
+          end,
+          outerId =
+            (node.id =
+            parent =
+              {
+                type: "TSQualifiedName",
+                left: id,
+                right: innerId,
+                start: (start = id.start),
+                end: (end = innerId.end),
+                range: [start, end],
+                parent: node,
+              });
+        id.parent = innerId.parent = outerId;
       } else {
         // Replace `left` of innermost `TSQualifiedName` with a nested `TSQualifiedName` with `id` of
         // this module on left, and previous `left` of innermost `TSQualifiedName` on right

tasks/minsize/minsize.snap

@@ -17,11 +17,11 @@ Original   | minified   | minified   | gzip       | gzip       | Iterations | Fi
 
 1.25 MB    | 642.64 kB  | 646.76 kB  | 159.40 kB  | 163.73 kB  |          2 | three.js   
 
-2.14 MB    | 711.13 kB  | 724.14 kB  | 160.42 kB  | 181.07 kB  |          2 | victory.js 
+2.14 MB    | 711.14 kB  | 724.14 kB  | 160.43 kB  | 181.07 kB  |          2 | victory.js 
 
 3.20 MB    | 1.00 MB    | 1.01 MB    | 322.59 kB  | 331.56 kB  |          3 | echarts.js 
 
-6.69 MB    | 2.22 MB    | 2.31 MB    | 458.41 kB  | 488.28 kB  |          4 | antd.js    
+6.69 MB    | 2.22 MB    | 2.31 MB    | 458.43 kB  | 488.28 kB  |          4 | antd.js    
 
-10.95 MB   | 3.33 MB    | 3.49 MB    | 853.34 kB  | 915.50 kB  |          4 | typescript.js 
+10.95 MB   | 3.33 MB    | 3.49 MB    | 853.36 kB  | 915.50 kB  |          4 | typescript.js 
 

tasks/track_memory_allocations/allocs_minifier.snap

@@ -6,9 +6,9 @@ cal.com.tsx                | 1.06 MB        ||          40453 |           3033 |
 
 RadixUIAdoptionSection.jsx | 2.52 kB        ||             85 |              9 ||             30 |              6
 
-pdf.mjs                    | 567.30 kB      ||          20494 |           2899 ||          47442 |           7725
+pdf.mjs                    | 567.30 kB      ||          20495 |           2899 ||          47447 |           7728
 
-antd.js                    | 6.69 MB        ||          99524 |          13523 ||         331583 |          69338
+antd.js                    | 6.69 MB        ||          99534 |          13524 ||         331657 |          69358
 
 binder.ts                  | 193.08 kB      ||           4760 |            974 ||           7075 |            824