fix(transformer/logic-assignment-operator): always create `Identifier…

…Reference`s with `ReferenceId` (#7745)

Use `TransformCtx::duplicate_expression` (introduced in #7754) to decide when to create temp vars for member expression object and computed property.

This fixes a bug where `IdentifierReference`s created when transforming `key` in `object[key] &&= value` were created without a `ReferenceId` (due to `clone_in`).

We didn't catch this before because Babel's test fixtures only cover `object[key++] &&= value` not the simpler `object[key] &&= value`. Add tests for this.

crates/oxc_transformer/src/es2021/logical_assignment_operators.rs

@@ -54,11 +54,10 @@
 //! * Babel plugin implementation: <https://github.com/babel/babel/tree/v7.26.2/packages/babel-plugin-transform-logical-assignment-operators>
 //! * Logical Assignment TC39 proposal: <https://github.com/tc39/proposal-logical-assignment>
 
-use oxc_allocator::CloneIn;
 use oxc_ast::ast::*;
 use oxc_semantic::ReferenceFlags;
 use oxc_span::SPAN;
-use oxc_traverse::{BoundIdentifier, MaybeBoundIdentifier, Traverse, TraverseCtx};
+use oxc_traverse::{Traverse, TraverseCtx};
 
 use crate::TransformCtx;
 
@@ -154,171 +153,52 @@ impl<'a, 'ctx> LogicalAssignmentOperators<'a, 'ctx> {
         static_expr: &mut StaticMemberExpression<'a>,
         ctx: &mut TraverseCtx<'a>,
     ) -> (Expression<'a>, AssignmentTarget<'a>) {
-        if let Some(ident) = self.maybe_generate_memoised(&static_expr.object, ctx) {
-            // (_o = o).a
-            let right = ctx.ast.move_expression(&mut static_expr.object);
-            let target = ident.create_write_target(ctx);
-            let object =
-                ctx.ast.expression_assignment(SPAN, AssignmentOperator::Assign, target, right);
-            let left_expr = Expression::from(ctx.ast.member_expression_static(
-                SPAN,
-                object,
-                static_expr.property.clone_in(ctx.ast.allocator),
-                false,
-            ));
+        let object = ctx.ast.move_expression(&mut static_expr.object);
+        let (object, object_ref) = self.ctx.duplicate_expression(object, true, ctx);
+
+        let left_expr = Expression::from(ctx.ast.member_expression_static(
+            static_expr.span,
+            object,
+            static_expr.property.clone(),
+            false,
+        ));
+
+        let assign_expr = ctx.ast.member_expression_static(
+            static_expr.span,
+            object_ref,
+            static_expr.property.clone(),
+            false,
+        );
+        let assign_target = AssignmentTarget::from(assign_expr);
 
-            // (_o.a = 1)
-            let assign_expr = ctx.ast.member_expression_static(
-                SPAN,
-                ident.create_read_expression(ctx),
-                static_expr.property.clone_in(ctx.ast.allocator),
-                false,
-            );
-            let assign_target = AssignmentTarget::from(assign_expr);
-
-            (left_expr, assign_target)
-        } else {
-            // transform `obj.x ||= 1` to `obj.x || (obj.x = 1)`
-            let object = ctx.ast.move_expression(&mut static_expr.object);
-
-            // TODO: We should use static_expr.clone_in instead of cloning the properties,
-            // but currently clone_in will get rid of IdentifierReference's reference_id
-            let static_expr_cloned = ctx.ast.alloc_static_member_expression(
-                static_expr.span,
-                Self::clone_expression(&object, ctx),
-                static_expr.property.clone_in(ctx.ast.allocator),
-                static_expr.optional,
-            );
-            let left_expr = Expression::StaticMemberExpression(static_expr_cloned);
-
-            let member_expr_moved = ctx.ast.member_expression_static(
-                static_expr.span,
-                object,
-                static_expr.property.clone_in(ctx.ast.allocator),
-                static_expr.optional,
-            );
-
-            let assign_target = AssignmentTarget::from(member_expr_moved);
-
-            (left_expr, assign_target)
-        }
+        (left_expr, assign_target)
     }
 
     fn convert_computed_member_expression(
         &mut self,
         computed_expr: &mut ComputedMemberExpression<'a>,
         ctx: &mut TraverseCtx<'a>,
     ) -> (Expression<'a>, AssignmentTarget<'a>) {
-        if let Some(ident) = self.maybe_generate_memoised(&computed_expr.object, ctx) {
-            // (_o = object)
-            let right = ctx.ast.move_expression(&mut computed_expr.object);
-            let target = ident.create_write_target(ctx);
-            let object =
-                ctx.ast.expression_assignment(SPAN, AssignmentOperator::Assign, target, right);
-
-            let mut expression = ctx.ast.move_expression(&mut computed_expr.expression);
-
-            // _b = expression
-            let property = self.maybe_generate_memoised(&expression, ctx);
-
-            if let Some(property) = &property {
-                expression = ctx.ast.expression_assignment(
-                    SPAN,
-                    AssignmentOperator::Assign,
-                    property.create_write_target(ctx),
-                    expression,
-                );
-            }
-
-            // _o[_b]
-            let assign_target = AssignmentTarget::from(ctx.ast.member_expression_computed(
-                SPAN,
-                ident.create_read_expression(ctx),
-                property.map_or_else(
-                    || expression.clone_in(ctx.ast.allocator),
-                    |ident| ident.create_read_expression(ctx),
-                ),
-                false,
-            ));
-
-            let left_expr = Expression::from(
-                ctx.ast.member_expression_computed(SPAN, object, expression, false),
-            );
-
-            (left_expr, assign_target)
-        } else {
-            // transform `obj[++key] ||= 1` to `obj[_key = ++key] || (obj[_key] = 1)`
-            let property_ident = self.maybe_generate_memoised(&computed_expr.expression, ctx);
+        let object = ctx.ast.move_expression(&mut computed_expr.object);
+        let (object, object_ref) = self.ctx.duplicate_expression(object, true, ctx);
+
+        let expression = ctx.ast.move_expression(&mut computed_expr.expression);
+        let (expression, expression_ref) = self.ctx.duplicate_expression(expression, true, ctx);
+
+        let left_expr = Expression::from(ctx.ast.member_expression_computed(
+            computed_expr.span,
+            object,
+            expression,
+            false,
+        ));
+
+        let assign_target = AssignmentTarget::from(ctx.ast.member_expression_computed(
+            computed_expr.span,
+            object_ref,
+            expression_ref,
+            false,
+        ));
 
-            let object = ctx.ast.move_expression(&mut computed_expr.object);
-            let mut expression = ctx.ast.move_expression(&mut computed_expr.expression);
-
-            // TODO: ideally we should use computed_expr.clone_in instead of cloning the properties,
-            // but currently clone_in will get rid of IdentifierReference's reference_id
-            let new_compute_expr = ctx.ast.alloc_computed_member_expression(
-                computed_expr.span,
-                Self::clone_expression(&object, ctx),
-                {
-                    // _key = ++key
-                    if let Some(property_ident) = &property_ident {
-                        ctx.ast.expression_assignment(
-                            SPAN,
-                            AssignmentOperator::Assign,
-                            property_ident.create_write_target(ctx),
-                            ctx.ast.move_expression(&mut expression),
-                        )
-                    } else {
-                        Self::clone_expression(&expression, ctx)
-                    }
-                },
-                computed_expr.optional,
-            );
-
-            let left_expr = Expression::ComputedMemberExpression(new_compute_expr);
-
-            // obj[_key] = 1
-            let new_compute_expr = ctx.ast.alloc_computed_member_expression(
-                computed_expr.span,
-                object,
-                {
-                    if let Some(property_ident) = property_ident {
-                        property_ident.create_read_expression(ctx)
-                    } else {
-                        expression
-                    }
-                },
-                computed_expr.optional,
-            );
-
-            let assign_target = AssignmentTarget::ComputedMemberExpression(new_compute_expr);
-
-            (left_expr, assign_target)
-        }
-    }
-
-    /// Clone an expression
-    ///
-    /// If it is an identifier, clone the identifier via [MaybeBoundIdentifier], otherwise, use [CloneIn].
-    ///
-    /// TODO: remove this once <https://github.com/oxc-project/oxc/issues/4804> is resolved.
-    fn clone_expression(expr: &Expression<'a>, ctx: &mut TraverseCtx<'a>) -> Expression<'a> {
-        match expr {
-            Expression::Identifier(ident) => {
-                let binding = MaybeBoundIdentifier::from_identifier_reference(ident, ctx);
-                binding.create_spanned_read_expression(ident.span, ctx)
-            }
-            _ => expr.clone_in(ctx.ast.allocator),
-        }
-    }
-
-    fn maybe_generate_memoised(
-        &mut self,
-        expr: &Expression<'a>,
-        ctx: &mut TraverseCtx<'a>,
-    ) -> Option<BoundIdentifier<'a>> {
-        if ctx.is_static(expr) {
-            return None;
-        }
-        Some(self.ctx.var_declarations.create_uid_var_based_on_node(expr, ctx))
+        (left_expr, assign_target)
     }
 }

tasks/transform_conformance/snapshots/oxc.snap.md

@@ -1,9 +1,10 @@
 commit: 54a8389f
 
-Passed: 96/108
+Passed: 99/111
 
 # All Passed:
 * babel-plugin-transform-class-static-block
+* babel-plugin-transform-logical-assignment-operators
 * babel-plugin-transform-nullish-coalescing-operator
 * babel-plugin-transform-optional-catch-binding
 * babel-plugin-transform-async-generator-functions

tasks/transform_conformance/tests/babel-plugin-transform-logical-assignment-operators/test/fixtures/computed-prop-identifier/input.js

@@ -0,0 +1,23 @@
+let boundObj, boundProp, mutatedObj, mutatedProp;
+mutatedObj = 'x';
+mutatedProp = 'x';
+
+boundObj[boundProp] &&= 1;
+boundObj[unboundProp] &&= 2;
+boundObj[mutatedProp] &&= 3;
+
+unboundObj[boundProp] &&= 4;
+unboundObj[unboundProp] &&= 5;
+unboundObj[mutatedProp] &&= 6;
+
+mutatedObj[boundProp] &&= 7;
+mutatedObj[unboundProp] &&= 8;
+mutatedObj[mutatedProp] &&= 9;
+
+boundObj.prop[boundProp] &&= 10;
+boundObj.prop[unboundProp] &&= 11;
+boundObj.prop[mutatedProp] &&= 12;
+
+this[boundProp] &&= 13;
+this[unboundProp] &&= 14;
+this[mutatedProp] &&= 15;

tasks/transform_conformance/tests/babel-plugin-transform-logical-assignment-operators/test/fixtures/computed-prop-identifier/output.js

@@ -0,0 +1,43 @@
+var _unboundProp,
+  _mutatedProp,
+  _unboundObj,
+  _unboundObj2,
+  _unboundProp2,
+  _unboundObj3,
+  _mutatedProp2,
+  _mutatedObj,
+  _mutatedObj2,
+  _unboundProp3,
+  _mutatedObj3,
+  _mutatedProp3,
+  _boundObj$prop,
+  _boundObj$prop2,
+  _unboundProp4,
+  _boundObj$prop3,
+  _mutatedProp4,
+  _unboundProp5,
+  _mutatedProp5;
+
+let boundObj, boundProp, mutatedObj, mutatedProp;
+mutatedObj = "x";
+mutatedProp = "x";
+
+boundObj[boundProp] && (boundObj[boundProp] = 1);
+boundObj[(_unboundProp = unboundProp)] && (boundObj[_unboundProp] = 2);
+boundObj[(_mutatedProp = mutatedProp)] && (boundObj[_mutatedProp] = 3);
+
+(_unboundObj = unboundObj)[boundProp] && (_unboundObj[boundProp] = 4);
+(_unboundObj2 = unboundObj)[(_unboundProp2 = unboundProp)] && (_unboundObj2[_unboundProp2] = 5);
+(_unboundObj3 = unboundObj)[(_mutatedProp2 = mutatedProp)] && (_unboundObj3[_mutatedProp2] = 6);
+
+(_mutatedObj = mutatedObj)[boundProp] && (_mutatedObj[boundProp] = 7);
+(_mutatedObj2 = mutatedObj)[(_unboundProp3 = unboundProp)] && (_mutatedObj2[_unboundProp3] = 8);
+(_mutatedObj3 = mutatedObj)[(_mutatedProp3 = mutatedProp)] && (_mutatedObj3[_mutatedProp3] = 9);
+
+(_boundObj$prop = boundObj.prop)[boundProp] && (_boundObj$prop[boundProp] = 10);
+(_boundObj$prop2 = boundObj.prop)[(_unboundProp4 = unboundProp)] && (_boundObj$prop2[_unboundProp4] = 11);
+(_boundObj$prop3 = boundObj.prop)[(_mutatedProp4 = mutatedProp)] && (_boundObj$prop3[_mutatedProp4] = 12);
+
+this[boundProp] && (this[boundProp] = 13);
+this[(_unboundProp5 = unboundProp)] && (this[_unboundProp5] = 14);
+this[(_mutatedProp5 = mutatedProp)] && (this[_mutatedProp5] = 15);

tasks/transform_conformance/tests/babel-plugin-transform-logical-assignment-operators/test/fixtures/options.json

@@ -0,0 +1,5 @@
+{
+  "plugins": [
+    "transform-logical-assignment-operators"
+  ]
+}

tasks/transform_conformance/tests/babel-plugin-transform-logical-assignment-operators/test/fixtures/super-prop-computed/input.js

@@ -0,0 +1,10 @@
+let boundProp, mutatedProp;
+mutatedProp = 'x';
+
+class C extends S {
+  method() {
+    super[boundProp] &&= 1;
+    super[unboundProp] &&= 2;
+    super[mutatedProp] &&= 3;
+  }
+}

tasks/transform_conformance/tests/babel-plugin-transform-logical-assignment-operators/test/fixtures/super-prop-computed/output.js

@@ -0,0 +1,12 @@
+let boundProp, mutatedProp;
+mutatedProp = "x";
+
+class C extends S {
+  method() {
+    var _unboundProp, _mutatedProp;
+
+    super[boundProp] && (super[boundProp] = 1);
+    super[(_unboundProp = unboundProp)] && (super[_unboundProp] = 2);
+    super[(_mutatedProp = mutatedProp)] && (super[_mutatedProp] = 3);
+  }
+}

tasks/transform_conformance/tests/babel-plugin-transform-logical-assignment-operators/test/fixtures/super-prop-static/input.js

@@ -0,0 +1,7 @@
+class C extends S {
+  method() {
+    super.prop &&= 1;
+    super.prop ||= 2;
+    super.prop ??= 3;
+  }
+}

tasks/transform_conformance/tests/babel-plugin-transform-logical-assignment-operators/test/fixtures/super-prop-static/output.js

@@ -0,0 +1,7 @@
+class C extends S {
+  method() {
+    super.prop && (super.prop = 1);
+    super.prop || (super.prop = 2);
+    super.prop ?? (super.prop = 3);
+  }
+}