Add functionality of moving computed fields with side effects into computed member functions in RewriteClassMembers.

Closure Team · copybara-github · commit fbc7922d62ec · 2023-08-09T09:46:06.000-07:00
This ensures that the original behavior of the side effects is preserved if the computed member functions also contain side effects.

PiperOrigin-RevId: 555190394
diff --git a/src/com/google/javascript/jscomp/RewriteClassMembers.java b/src/com/google/javascript/jscomp/RewriteClassMembers.java
@@ -79,7 +79,7 @@ public boolean shouldTraverse(NodeTraversal t, Node n, Node parent) {
       case COMPUTED_FIELD_DEF:
         checkState(!classStack.isEmpty());
         if (NodeUtil.canBeSideEffected(n.getFirstChild())) {
-          classStack.peek().hasCompFieldWithSideEffect = true;
+          classStack.peek().computedFieldsWithSideEffectsToMove.push(n);
         }
         classStack.peek().enterField(n);
         break;
@@ -114,13 +114,10 @@ public boolean shouldTraverse(NodeTraversal t, Node n, Node parent) {
         break;
       case COMPUTED_PROP:
         checkState(!classStack.isEmpty());
-        if (classStack.peek().hasCompFieldWithSideEffect) {
-          t.report(
-              n,
-              TranspilationUtil.CANNOT_CONVERT_YET,
-              "Class contains computed field with side effect and computed function");
-          classStack.peek().cannotConvert = true;
-          break;
+        ClassRecord record = classStack.peek();
+        if (!record.computedFieldsWithSideEffectsToMove.isEmpty()) {
+          moveComputedFieldsWithSideEffectsInsideComputedFunction(t, record, n);
+          record.computedFieldsWithSideEffectsToMove.clear();
         }
         break;
       default:
@@ -182,9 +179,72 @@ private void visitClass(NodeTraversal t) {
     rewriteStaticMembers(t, currClassRecord);
   }
 
+  /**
+   * Move computed fields with side effects into a computed function (Token is COMPUTED_PROP) to
+   * preserve the original behavior of possible side effects such as printing something in the call
+   * to a function in the computed field and function.
+   *
+   * <p>The computed fields that get moved into a computed function must be located above the
+   * computed function and below any other computed functions in the class. Any computed field not
+   * above a computed function gets extracted by {@link #extractExpressionFromCompField}.
+   *
+   * <p>E.g.
+   *
+   * <pre>
+   *   function bar(num) {}
+   *   class Foo {
+   *     [bar(1)] = 9;
+   *     [bar(2)]() {}
+   *   }
+   * </pre>
+   *
+   * becomes
+   *
+   * <pre>
+   *   function bar(num) {}
+   *   var $jscomp$compfield$0;
+   *   class Foo {
+   *     [$jscomp$compfield$0] = 9;
+   *     [($jscomp$compfield$0 = bar(1), bar(2))]() {}
+   *   }
+   * </pre>
+   */
+  private void moveComputedFieldsWithSideEffectsInsideComputedFunction(
+      NodeTraversal t, ClassRecord record, Node computedFunction) {
+    checkArgument(computedFunction.isComputedProp(), computedFunction);
+    Deque<Node> computedFields = record.computedFieldsWithSideEffectsToMove;
+    while (!computedFields.isEmpty()) {
+      Node computedField = computedFields.pop();
+      Node computedFieldVar =
+          astFactory
+              .createSingleVarNameDeclaration(generateUniqueCompFieldVarName(t))
+              .srcrefTreeIfMissing(record.classNode);
+      // `var $jscomp$compfield$0` is inserted before the class
+      computedFieldVar.insertBefore(record.insertionPointBeforeClass);
+      Node newNameNode = computedFieldVar.getFirstChild();
+      Node fieldLhsExpression = computedField.getFirstChild();
+      // Computed field changes from `[fieldLhs] = rhs;` to `[$jscomp$compfield$0] = rhs;`
+      fieldLhsExpression.replaceWith(newNameNode.cloneNode());
+      Node assignComputedLhsExpressionToNewVarName =
+          astFactory
+              .createAssign(newNameNode.cloneNode(), fieldLhsExpression)
+              .srcrefTreeIfMissing(computedField);
+      Node existingComputedFunctionExpression = computedFunction.getFirstChild();
+      Node newComma =
+          astFactory
+              .createComma(
+                  assignComputedLhsExpressionToNewVarName,
+                  existingComputedFunctionExpression.detach())
+              .srcrefTreeIfMissing(computedFunction);
+      // `[funcExpr]() {}` becomes `[($jscomp$compfield$0 = fieldLhs, funcExpr)]() {}`
+      computedFunction.addChildToFront(newComma);
+    }
+  }
+
   /**
    * Extracts the expression in the LHS of a computed field to not disturb possible side effects and
-   * allow for this and super to be used in the LHS of a computed field in certain cases
+   * allow for this and super to be used in the LHS of a computed field in certain cases. Does not
+   * extract a computed field that was already moved into a computed function.
    *
    * <p>E.g.
    *
@@ -207,14 +267,15 @@ private void visitClass(NodeTraversal t) {
   private void extractExpressionFromCompField(
       NodeTraversal t, ClassRecord record, Node memberField) {
     checkArgument(memberField.isComputedFieldDef(), memberField);
-    checkState(
-        !memberField.getFirstChild().isName()
-            || !memberField.getFirstChild().getString().startsWith(COMP_FIELD_VAR),
-        memberField);
+    // Computed field was already moved into a computed function and doesn't need to be extracted
+    if (memberField.getFirstChild().isName()
+        && memberField.getFirstChild().getString().startsWith(COMP_FIELD_VAR)) {
+      return;
+    }
+
     Node compExpression = memberField.getFirstChild().detach();
     Node compFieldVar =
-        astFactory.createSingleVarNameDeclaration(
-            COMP_FIELD_VAR + compiler.getUniqueIdSupplier().getUniqueId(t.getInput()));
+        astFactory.createSingleVarNameDeclaration(generateUniqueCompFieldVarName(t));
     Node compFieldName = compFieldVar.getFirstChild();
     memberField.addChildToFront(compFieldName.cloneNode());
     compFieldVar.insertBefore(record.insertionPointBeforeClass);
@@ -226,6 +287,11 @@ private void extractExpressionFromCompField(
     exprResult.srcrefTreeIfMissing(record.classNode);
   }
 
+  /** Returns $jscomp$compfield$[FILE_ID]$[number] */
+  private String generateUniqueCompFieldVarName(NodeTraversal t) {
+    return COMP_FIELD_VAR + compiler.getUniqueIdSupplier().getUniqueId(t.getInput());
+  }
+
   /** Rewrites and moves all instance fields */
   private void rewriteInstanceMembers(NodeTraversal t, ClassRecord record) {
     Deque<Node> instanceMembers = record.instanceMembers;
@@ -404,10 +470,6 @@ private Node findInitialInstanceInsertionPoint(Node ctorBlock) {
    */
   private static final class ClassRecord {
 
-    // Keeps track of if there are any computed fields with side effects in the class to throw
-    // an error if any computed functions are encountered because it requires a unique way to
-    // transpile to preserve the behavior of the side effects
-    boolean hasCompFieldWithSideEffect;
     // During traversal, contains the current member being traversed. After traversal, always null
     @Nullable Node currentMember;
     boolean cannotConvert;
@@ -416,6 +478,8 @@ private static final class ClassRecord {
     final Deque<Node> instanceMembers = new ArrayDeque<>();
     // Static fields + static blocks
     final Deque<Node> staticMembers = new ArrayDeque<>();
+    // Computed fields with side effects after the most recent computed member function
+    final Deque<Node> computedFieldsWithSideEffectsToMove = new ArrayDeque<>();
 
     // Mapping from MEMBER_FIELD_DEF (& COMPUTED_FIELD_DEF) nodes to all name nodes in that RHS
     final SetMultimap<Node, Node> referencedNamesByMember =
diff --git a/test/com/google/javascript/jscomp/RewriteClassMembersTest.java b/test/com/google/javascript/jscomp/RewriteClassMembersTest.java
@@ -134,40 +134,6 @@ public void testCannotConvertYet() {
             "  }",
             "}"),
         CANNOT_CONVERT_YET); // `var` in static block
-
-    test(
-        srcs(
-            lines(
-                "function foo(num) {}",
-                "/** @unrestricted */",
-                "class Baz {",
-                "  ['f' + foo(1)] = 5;",
-                "  static x = foo(6);",
-                "  ['m' + foo(2)]() {};",
-                "  static [foo(3)] = foo(7);",
-                "  [foo(4)] = 2;",
-                "  get [foo(5)]() {}",
-                "}")),
-        error(CANNOT_CONVERT_YET), // Comp field with side effect and computed function
-        error(CANNOT_CONVERT_YET)); // Comp field with side effect and computed function
-    /* TODO(user): Move computed fields with side effects into computed functions if
-       present
-    expected(
-    lines(
-        "function foo(num) {}",
-        "var COMPFIELD$0;",
-        "var COMPFIELD$1;",
-        "var COMPFIELD$2;",
-        "class Baz {",
-        "  constructor() {",
-        "    this[COMPFIELD$2] = 5;",
-        "    this[COMPFIELD$0] = 2;",
-        "  }",
-        "  [(COMPFIELD$2 = 'f' + foo(1), 'm' + foo(2))]() {}",
-        "  get [(COMPFIELD$1 = foo(3), COMPFIELD$0 = foo(4), foo(5))]() {}",
-        "}",
-        "Baz.x = foo(6);",
-        "Baz[COMPFIELD$1] = foo(7);"))); */
   }
 
   @Test
@@ -582,6 +548,36 @@ public void testSideEffectsInComputedField() {
                 "COMPFIELD$0 = x;",
                 "Foo.n = x = 5;",
                 "Foo[COMPFIELD$0] = 'world';")));
+
+    computedFieldTest(
+        srcs(
+            lines(
+                "function foo(num) {}",
+                "/** @unrestricted */",
+                "class Baz {",
+                "  ['f' + foo(1)];",
+                "  static x = foo(6);",
+                "  ['m' + foo(2)]() {};",
+                "  static [foo(3)] = foo(7);",
+                "  [foo(4)] = 2;",
+                "  get [foo(5)]() {}",
+                "}")),
+        expected(
+            lines(
+                "function foo(num) {}",
+                "var COMPFIELD$0;",
+                "var COMPFIELD$1;",
+                "var COMPFIELD$2;",
+                "class Baz {",
+                "  constructor() {",
+                "    this[COMPFIELD$0];",
+                "    this[COMPFIELD$1] = 2;",
+                "  }",
+                "  [(COMPFIELD$0 = 'f' + foo(1), 'm' + foo(2))]() {}",
+                "  get [(COMPFIELD$2 = foo(3), (COMPFIELD$1 = foo(4), foo(5)))]() {}",
+                "}",
+                "Baz.x = foo(6);",
+                "Baz[COMPFIELD$2] = foo(7);")));
   }
 
   @Test
