From e48769a45dfbf65c72c847d287a148421e38f06a Mon Sep 17 00:00:00 2001 From: overlookmotel <557937+overlookmotel@users.noreply.github.com> Date: Tue, 10 Dec 2024 02:28:26 +0000 Subject: [PATCH] fix(transformer/logic-assignment-operator): always create `IdentifierReference`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. --- .../es2021/logical_assignment_operators.rs | 198 ++++-------------- .../snapshots/oxc.snap.md | 3 +- .../computed-prop-identifier/input.js | 23 ++ .../computed-prop-identifier/output.js | 43 ++++ .../test/fixtures/options.json | 5 + .../fixtures/super-prop-computed/input.js | 10 + .../fixtures/super-prop-computed/output.js | 12 ++ .../test/fixtures/super-prop-static/input.js | 7 + .../test/fixtures/super-prop-static/output.js | 7 + 9 files changed, 148 insertions(+), 160 deletions(-) create mode 100644 tasks/transform_conformance/tests/babel-plugin-transform-logical-assignment-operators/test/fixtures/computed-prop-identifier/input.js create mode 100644 tasks/transform_conformance/tests/babel-plugin-transform-logical-assignment-operators/test/fixtures/computed-prop-identifier/output.js create mode 100644 tasks/transform_conformance/tests/babel-plugin-transform-logical-assignment-operators/test/fixtures/options.json create mode 100644 tasks/transform_conformance/tests/babel-plugin-transform-logical-assignment-operators/test/fixtures/super-prop-computed/input.js create mode 100644 tasks/transform_conformance/tests/babel-plugin-transform-logical-assignment-operators/test/fixtures/super-prop-computed/output.js create mode 100644 tasks/transform_conformance/tests/babel-plugin-transform-logical-assignment-operators/test/fixtures/super-prop-static/input.js create mode 100644 tasks/transform_conformance/tests/babel-plugin-transform-logical-assignment-operators/test/fixtures/super-prop-static/output.js diff --git a/crates/oxc_transformer/src/es2021/logical_assignment_operators.rs b/crates/oxc_transformer/src/es2021/logical_assignment_operators.rs index 9af520ae75fe1..30a5d0cc7c909 100644 --- a/crates/oxc_transformer/src/es2021/logical_assignment_operators.rs +++ b/crates/oxc_transformer/src/es2021/logical_assignment_operators.rs @@ -54,11 +54,10 @@ //! * Babel plugin implementation: //! * Logical Assignment TC39 proposal: -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,54 +153,25 @@ 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( @@ -209,116 +179,26 @@ impl<'a, 'ctx> LogicalAssignmentOperators<'a, 'ctx> { 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 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> { - if ctx.is_static(expr) { - return None; - } - Some(self.ctx.var_declarations.create_uid_var_based_on_node(expr, ctx)) + (left_expr, assign_target) } } diff --git a/tasks/transform_conformance/snapshots/oxc.snap.md b/tasks/transform_conformance/snapshots/oxc.snap.md index 50fe5267aa798..9b3374feec861 100644 --- a/tasks/transform_conformance/snapshots/oxc.snap.md +++ b/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 diff --git a/tasks/transform_conformance/tests/babel-plugin-transform-logical-assignment-operators/test/fixtures/computed-prop-identifier/input.js b/tasks/transform_conformance/tests/babel-plugin-transform-logical-assignment-operators/test/fixtures/computed-prop-identifier/input.js new file mode 100644 index 0000000000000..04d0e7463da2f --- /dev/null +++ b/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; diff --git a/tasks/transform_conformance/tests/babel-plugin-transform-logical-assignment-operators/test/fixtures/computed-prop-identifier/output.js b/tasks/transform_conformance/tests/babel-plugin-transform-logical-assignment-operators/test/fixtures/computed-prop-identifier/output.js new file mode 100644 index 0000000000000..93ed6d8ce4826 --- /dev/null +++ b/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); diff --git a/tasks/transform_conformance/tests/babel-plugin-transform-logical-assignment-operators/test/fixtures/options.json b/tasks/transform_conformance/tests/babel-plugin-transform-logical-assignment-operators/test/fixtures/options.json new file mode 100644 index 0000000000000..754c7c95050eb --- /dev/null +++ b/tasks/transform_conformance/tests/babel-plugin-transform-logical-assignment-operators/test/fixtures/options.json @@ -0,0 +1,5 @@ +{ + "plugins": [ + "transform-logical-assignment-operators" + ] +} diff --git a/tasks/transform_conformance/tests/babel-plugin-transform-logical-assignment-operators/test/fixtures/super-prop-computed/input.js b/tasks/transform_conformance/tests/babel-plugin-transform-logical-assignment-operators/test/fixtures/super-prop-computed/input.js new file mode 100644 index 0000000000000..8da15626f3896 --- /dev/null +++ b/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; + } +} diff --git a/tasks/transform_conformance/tests/babel-plugin-transform-logical-assignment-operators/test/fixtures/super-prop-computed/output.js b/tasks/transform_conformance/tests/babel-plugin-transform-logical-assignment-operators/test/fixtures/super-prop-computed/output.js new file mode 100644 index 0000000000000..293474e6211fe --- /dev/null +++ b/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); + } +} diff --git a/tasks/transform_conformance/tests/babel-plugin-transform-logical-assignment-operators/test/fixtures/super-prop-static/input.js b/tasks/transform_conformance/tests/babel-plugin-transform-logical-assignment-operators/test/fixtures/super-prop-static/input.js new file mode 100644 index 0000000000000..a35f62ad75c70 --- /dev/null +++ b/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; + } +} diff --git a/tasks/transform_conformance/tests/babel-plugin-transform-logical-assignment-operators/test/fixtures/super-prop-static/output.js b/tasks/transform_conformance/tests/babel-plugin-transform-logical-assignment-operators/test/fixtures/super-prop-static/output.js new file mode 100644 index 0000000000000..f0d11a53c87f6 --- /dev/null +++ b/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); + } +}