Support chained assignment expressions

Fixes codegen for chained assignment expressions. Previously each intermediate 
assignment would be generated independently _in addition_ to the final chained 
expression being emitted. We now emit a single chained expression, almost 
exactly matching the input except for expanding from `x += 1` into `x = x + 1`. 

There are two key changes: 

* Ensuring that assignment expressions always generate an lvalue, which is 
necessary for alias analysis to kick in, since it relies on the effect of the 
lvalue to know where to look for aliasing. 

* The above makes codegen think the entire assignment expression value is a 
temporary that can be emitted later, but that isn't true. The new 
PruneTemporaryLValue pass nulls out lvalues that are never read later, ensuring 
that codegen can eagerly emit the value instead of saving it as a temporary.

compiler/forget/src/CompilerPipeline.ts

@@ -23,6 +23,7 @@ import {
   inferReactiveScopeVariables,
   propagateScopeDependencies,
   pruneUnusedLabels,
+  pruneUnusedLValues,
   pruneUnusedScopes,
   renameVariables,
 } from "./ReactiveScopes";
@@ -82,6 +83,9 @@ export default function (
   pruneUnusedScopes(reactiveFunction);
   logReactiveFunction("pruneUnusedScopes", reactiveFunction);
 
+  pruneUnusedLValues(reactiveFunction);
+  logReactiveFunction("pruneUnusedLValues", reactiveFunction);
+
   renameVariables(reactiveFunction);
   logReactiveFunction("renameVariables", reactiveFunction);
 

compiler/forget/src/HIR/BuildHIR.ts

@@ -649,13 +649,17 @@ function lowerStatement(
       const stmt = stmtPath as NodePath<t.ExpressionStatement>;
       const expression = stmt.get("expression");
       const value = lowerExpression(builder, expression);
-      if (expression.isAssignmentExpression()) {
-        // instruction already emitted via lowerExpression()
+      if (expression.isAssignmentExpression() && value.kind === "Identifier") {
+        // already lowered to a place
         return;
       }
+      const place = buildTemporaryPlace(
+        builder,
+        stmt.node.loc ?? GeneratedSource
+      );
       builder.push({
         id: makeInstructionId(0),
-        lvalue: null,
+        lvalue: { kind: InstructionKind.Const, place },
         value,
         loc: stmt.node.loc ?? GeneratedSource,
       });
@@ -885,13 +889,7 @@ function lowerExpression(
           //   tmp != null ? tmp : <right>
           const left = lowerExpressionToPlace(builder, leftPath);
 
-          const nullPlace: Place = {
-            kind: "Identifier",
-            identifier: builder.makeTemporary(),
-            memberPath: null,
-            effect: Effect.Unknown,
-            loc: left.loc,
-          };
+          const nullPlace: Place = buildTemporaryPlace(builder, left.loc);
           builder.push({
             id: makeInstructionId(0),
             value: {
@@ -903,13 +901,7 @@ function lowerExpression(
             lvalue: { place: { ...nullPlace }, kind: InstructionKind.Const },
           });
 
-          const condPlace: Place = {
-            kind: "Identifier",
-            identifier: builder.makeTemporary(),
-            memberPath: null,
-            effect: Effect.Unknown,
-            loc: left.loc,
-          };
+          const condPlace: Place = buildTemporaryPlace(builder, left.loc);
           builder.push({
             id: makeInstructionId(0),
             lvalue: {
@@ -960,36 +952,23 @@ function lowerExpression(
           }
           case "MemberExpression": {
             const leftExpr = left as NodePath<t.MemberExpression>;
-            const object = lowerExpressionToPlace(
-              builder,
-              leftExpr.get("object")
-            );
             const property = leftExpr.get("property");
             invariant(
               property.isIdentifier(),
               "Assignment expression to dynamic properties is not yet supported"
             );
             const right = lowerExpressionToPlace(builder, expr.get("right"));
-            const place: Place = {
-              kind: "Identifier",
-              identifier: builder.makeTemporary(),
-              memberPath: null,
-              effect: Effect.Read,
-              loc: exprLoc,
+            const object = lowerExpressionToPlace(
+              builder,
+              leftExpr.get("object")
+            );
+            return {
+              kind: "PropertyStore",
+              object,
+              property: property.node.name,
+              value: right,
+              loc: leftNode.loc ?? GeneratedSource,
             };
-            builder.push({
-              id: makeInstructionId(0),
-              lvalue: { place: { ...place }, kind: InstructionKind.Const },
-              value: {
-                kind: "PropertyStore",
-                object,
-                property: property.node.name,
-                value: right,
-                loc: leftNode.loc ?? GeneratedSource,
-              },
-              loc: exprLoc,
-            });
-            return place;
           }
           default: {
             todoInvariant(
@@ -1056,13 +1035,10 @@ function lowerExpression(
             "Assignment expression to dynamic properties is not yet supported"
           );
           // Store the previous value to a temporary
-          const previousValuePlace: Place = {
-            kind: "Identifier",
-            identifier: builder.makeTemporary(),
-            memberPath: null,
-            effect: Effect.Read,
-            loc: exprLoc,
-          };
+          const previousValuePlace: Place = buildTemporaryPlace(
+            builder,
+            exprLoc
+          );
           builder.push({
             id: makeInstructionId(0),
             lvalue: {
@@ -1078,13 +1054,7 @@ function lowerExpression(
             loc: leftExpr.node.loc ?? GeneratedSource,
           });
           // Store the new value to a temporary
-          const newValuePlace: Place = {
-            kind: "Identifier",
-            identifier: builder.makeTemporary(),
-            memberPath: null,
-            effect: Effect.Read,
-            loc: exprLoc,
-          };
+          const newValuePlace: Place = buildTemporaryPlace(builder, exprLoc);
           builder.push({
             id: makeInstructionId(0),
             lvalue: {
@@ -1102,29 +1072,13 @@ function lowerExpression(
           });
 
           // Save the result back to the property
-          const place: Place = {
-            kind: "Identifier",
-            identifier: builder.makeTemporary(),
-            memberPath: null,
-            effect: Effect.Read,
-            loc: exprLoc,
-          };
-          builder.push({
-            id: makeInstructionId(0),
-            lvalue: {
-              place: { ...place },
-              kind: InstructionKind.Const,
-            },
-            value: {
-              kind: "PropertyStore",
-              object: { ...object },
-              property: property.node.name,
-              value: { ...newValuePlace },
-              loc: leftExpr.node.loc ?? GeneratedSource,
-            },
+          return {
+            kind: "PropertyStore",
+            object: { ...object },
+            property: property.node.name,
+            value: { ...newValuePlace },
             loc: leftExpr.node.loc ?? GeneratedSource,
-          });
-          return place;
+          };
         }
         default: {
           invariant(
@@ -1149,13 +1103,7 @@ function lowerExpression(
         property: property.node.name,
         loc: exprLoc,
       };
-      const place: Place = {
-        kind: "Identifier",
-        identifier: builder.makeTemporary(),
-        memberPath: null,
-        effect: Effect.Read,
-        loc: exprLoc,
-      };
+      const place: Place = buildTemporaryPlace(builder, exprLoc);
       builder.push({
         id: makeInstructionId(0),
         lvalue: { place: { ...place }, kind: InstructionKind.Const },
@@ -1231,13 +1179,7 @@ function lowerConditional(
   consequent: () => InstructionValue,
   alternate: () => InstructionValue
 ): Place {
-  const place: Place = {
-    kind: "Identifier",
-    identifier: builder.makeTemporary(),
-    memberPath: null,
-    effect: Effect.Read,
-    loc,
-  };
+  const place: Place = buildTemporaryPlace(builder, loc);
   //  Block for code following the if
   const continuationBlock = builder.reserve();
   //  Block for the consequent (if the test is truthy)
@@ -1311,13 +1253,7 @@ function lowerJsxElementName(
     };
     return place;
   } else {
-    const place: Place = {
-      kind: "Identifier",
-      identifier: builder.makeTemporary(),
-      memberPath: null,
-      effect: Effect.Unknown,
-      loc: exprLoc,
-    };
+    const place: Place = buildTemporaryPlace(builder, exprLoc);
     builder.push({
       id: makeInstructionId(0),
       value: {
@@ -1351,13 +1287,7 @@ function lowerJsxElement(
     todoInvariant(expression.isExpression(), "handle empty expressions");
     return lowerExpressionToPlace(builder, expression);
   } else if (exprPath.isJSXText()) {
-    const place: Place = {
-      kind: "Identifier",
-      identifier: builder.makeTemporary(),
-      memberPath: null,
-      effect: Effect.Unknown,
-      loc: exprLoc,
-    };
+    const place: Place = buildTemporaryPlace(builder, exprLoc);
     builder.push({
       id: makeInstructionId(0),
       value: {
@@ -1374,13 +1304,7 @@ function lowerJsxElement(
       t.isJSXFragment(exprNode) || t.isJSXSpreadChild(exprNode),
       "Expected refinement to work"
     );
-    const place: Place = {
-      kind: "Identifier",
-      identifier: builder.makeTemporary(),
-      memberPath: null,
-      effect: Effect.Unknown,
-      loc: exprLoc,
-    };
+    const place: Place = buildTemporaryPlace(builder, exprLoc);
     builder.push({
       id: makeInstructionId(0),
       value: {
@@ -1404,13 +1328,7 @@ function lowerExpressionToPlace(
     return instr;
   }
   const exprLoc = exprPath.node.loc ?? GeneratedSource;
-  const place: Place = {
-    kind: "Identifier",
-    identifier: builder.makeTemporary(),
-    memberPath: null,
-    effect: Effect.Unknown,
-    loc: exprLoc,
-  };
+  const place: Place = buildTemporaryPlace(builder, exprLoc);
   builder.push({
     id: makeInstructionId(0),
     value: instr,
@@ -1496,6 +1414,20 @@ function lowerLVal(builder: HIRBuilder, exprPath: NodePath<t.LVal>): Place {
   }
 }
 
+/**
+ * Creates a temporary Identifier and Place referencing that identifier.
+ */
+function buildTemporaryPlace(builder: HIRBuilder, loc: SourceLocation): Place {
+  const place: Place = {
+    kind: "Identifier",
+    identifier: builder.makeTemporary(),
+    memberPath: null,
+    effect: Effect.Unknown,
+    loc,
+  };
+  return place;
+}
+
 function lowerAssignment(
   builder: HIRBuilder,
   loc: SourceLocation,

compiler/forget/src/HIR/Codegen.ts

@@ -302,14 +302,7 @@ export function codegenInstruction(
   if (instr.lvalue === null) {
     return t.expressionStatement(value);
   }
-  if (instr.value.kind === "PropertyStore") {
-    invariant(
-      instr.lvalue.place.identifier.name === null,
-      "Expected property stores to be lowered to a temporary"
-    );
-    temp.set(instr.lvalue.place.identifier.id, value);
-    return t.expressionStatement(value);
-  } else if (
+  if (
     instr.lvalue.place.memberPath === null &&
     instr.lvalue.place.identifier.name === null
   ) {

compiler/forget/src/ReactiveScopes/PruneTemporaryLValues.ts

@@ -0,0 +1,41 @@
+/**
+ * Copyright (c) Facebook, Inc. and its affiliates.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ */
+
+import {
+  Identifier,
+  Instruction,
+  InstructionKind,
+  ReactiveFunction,
+} from "../HIR/HIR";
+import { visitFunction } from "./visitors";
+
+/**
+ * Nulls out lvalues for temporary variables that are never accessed later. This only
+ * nulls out the lvalue itself, it does not remove the corresponding instructions.
+ */
+export function pruneTemporaryLValues(fn: ReactiveFunction): void {
+  const lvalues = new Map<Identifier, Instruction>();
+  visitFunction(fn, {
+    visitInstruction: (instr) => {
+      if (
+        instr.lvalue !== null &&
+        instr.lvalue.kind === InstructionKind.Const &&
+        instr.lvalue.place.identifier.name === null
+      ) {
+        lvalues.set(instr.lvalue.place.identifier, instr);
+      }
+    },
+    visitValue: (value) => {
+      if (value.kind === "Identifier") {
+        lvalues.delete(value.identifier);
+      }
+    },
+  });
+  for (const [, instr] of lvalues) {
+    instr.lvalue = null;
+  }
+}

compiler/forget/src/ReactiveScopes/index.ts

@@ -12,6 +12,7 @@ export { inferReactiveScopes } from "./InferReactiveScopes";
 export { inferReactiveScopeVariables } from "./InferReactiveScopeVariables";
 export { printReactiveFunction } from "./PrintReactiveFunction";
 export { propagateScopeDependencies } from "./PropagateScopeDependencies";
+export { pruneTemporaryLValues as pruneUnusedLValues } from "./PruneTemporaryLValues";
 export { pruneUnusedLabels } from "./PruneUnusedLabels";
 export { pruneUnusedScopes } from "./PruneUnusedScopes";
 export { renameVariables } from "./RenameVariables";

compiler/forget/src/ReactiveScopes/visitors.ts

@@ -5,9 +5,82 @@
  * LICENSE file in the root directory of this source tree.
  */
 
-import { ReactiveBlock, ReactiveTerminal } from "../HIR/HIR";
+import {
+  Instruction,
+  InstructionValue,
+  Place,
+  ReactiveBlock,
+  ReactiveFunction,
+  ReactiveScope,
+  ReactiveTerminal,
+  ReactiveValueBlock,
+} from "../HIR/HIR";
+import { eachInstructionValueOperand } from "../HIR/visitors";
 import { assertExhaustive } from "../Utils/utils";
 
+export function visitFunction(
+  fn: ReactiveFunction,
+  visitors: {
+    visitValue?: (value: InstructionValue) => void;
+    visitInstruction?: (instr: Instruction) => void;
+    visitTerminal?: (terminal: ReactiveTerminal) => void;
+    visitScope?: (scope: ReactiveScope) => void;
+  }
+): void {
+  const { visitValue, visitInstruction, visitTerminal, visitScope } = visitors;
+  function visitBlock(block: ReactiveBlock): void {
+    for (const item of block) {
+      switch (item.kind) {
+        case "instruction": {
+          if (visitValue) {
+            for (const operand of eachInstructionValueOperand(
+              item.instruction.value
+            )) {
+              visitValue(operand);
+            }
+          }
+          if (visitInstruction) {
+            visitInstruction(item.instruction);
+          }
+          break;
+        }
+        case "terminal": {
+          if (visitValue) {
+            eachTerminalOperand(item.terminal, (operand) => {
+              visitValue(operand);
+            });
+          }
+          if (visitTerminal) {
+            visitTerminal(item.terminal);
+          }
+          eachTerminalBlock(item.terminal, visitBlock, visitValueBlock);
+          break;
+        }
+        case "scope": {
+          if (visitScope) {
+            visitScope(item.scope);
+          }
+          visitBlock(item.instructions);
+          break;
+        }
+        default: {
+          assertExhaustive(
+            item,
+            `Unexpected item kind '${(item as any).kind}'`
+          );
+        }
+      }
+    }
+  }
+  function visitValueBlock(block: ReactiveValueBlock): void {
+    visitBlock(block.instructions);
+    if (block.value !== null && visitValue) {
+      visitValue(block.value);
+    }
+  }
+  visitBlock(fn.body);
+}
+
 export function mapTerminalBlocks(
   terminal: ReactiveTerminal,
   fn: (block: ReactiveBlock) => ReactiveBlock
@@ -50,3 +123,98 @@ export function mapTerminalBlocks(
     }
   }
 }
+
+export function eachTerminalBlock(
+  terminal: ReactiveTerminal,
+  visitBlock: (block: ReactiveBlock) => void,
+  visitValueBlock: (block: ReactiveValueBlock) => void
+): void {
+  switch (terminal.kind) {
+    case "break":
+    case "continue":
+    case "return":
+    case "throw": {
+      break;
+    }
+    case "for": {
+      visitValueBlock(terminal.init);
+      visitValueBlock(terminal.test);
+      visitValueBlock(terminal.update);
+      visitBlock(terminal.loop);
+      break;
+    }
+    case "while": {
+      visitValueBlock(terminal.test);
+      visitBlock(terminal.loop);
+      break;
+    }
+    case "if": {
+      visitBlock(terminal.consequent);
+      if (terminal.alternate !== null) {
+        visitBlock(terminal.alternate);
+      }
+      break;
+    }
+    case "switch": {
+      for (const case_ of terminal.cases) {
+        if (case_.block !== undefined) {
+          visitBlock(case_.block);
+        }
+      }
+      break;
+    }
+    default: {
+      assertExhaustive(
+        terminal,
+        `Unexpected terminal kind '${(terminal as any).kind}'`
+      );
+    }
+  }
+}
+
+export function eachTerminalOperand(
+  terminal: ReactiveTerminal,
+  fn: (place: Place) => void
+): void {
+  switch (terminal.kind) {
+    case "break":
+    case "continue": {
+      break;
+    }
+    case "return": {
+      if (terminal.value !== null) {
+        fn(terminal.value);
+      }
+      break;
+    }
+    case "throw": {
+      fn(terminal.value);
+      break;
+    }
+    case "for": {
+      break;
+    }
+    case "while": {
+      break;
+    }
+    case "if": {
+      fn(terminal.test);
+      break;
+    }
+    case "switch": {
+      fn(terminal.test);
+      for (const case_ of terminal.cases) {
+        if (case_.test !== null) {
+          fn(case_.test);
+        }
+      }
+      break;
+    }
+    default: {
+      assertExhaustive(
+        terminal,
+        `Unexpected terminal kind '${(terminal as any).kind}'`
+      );
+    }
+  }
+}

compiler/forget/src/__tests__/fixtures/hir/_bug_chained-assignment-expressions.expect.md

@@ -1,28 +0,0 @@
-
-## Input
-
-```javascript
-function foo() {
-  const x = { y: 0 };
-  const y = { z: 0 };
-  x.y += y.z *= 1;
-}
-
-```
-
-## Code
-
-```javascript
-function foo() {
-  const x = {
-    y: 0,
-  };
-  const y = {
-    z: 0,
-  };
-  y.z = y.z * 1;
-  x.y = x.y + (y.z = y.z * 1);
-}
-
-```
-      

compiler/forget/src/__tests__/fixtures/hir/_bug_chained-assignment-expressions.js

@@ -1,5 +0,0 @@
-function foo() {
-  const x = { y: 0 };
-  const y = { z: 0 };
-  x.y += y.z *= 1;
-}

compiler/forget/src/__tests__/fixtures/hir/chained-assignment-expressions.expect.md

@@ -0,0 +1,35 @@
+
+## Input
+
+```javascript
+function foo() {
+  const x = { x: 0 };
+  const y = { z: 0 };
+  const z = { z: 0 };
+  x.x += y.y *= 1;
+  z.z += y.y *= x.x &= 3;
+  return z;
+}
+
+```
+
+## Code
+
+```javascript
+function foo() {
+  const x = {
+    x: 0,
+  };
+  const y = {
+    z: 0,
+  };
+  const z = {
+    z: 0,
+  };
+  x.x = x.x + (y.y = y.y * 1);
+  z.z = z.z + (y.y = y.y * (x.x = x.x & 3));
+  return z;
+}
+
+```
+      

compiler/forget/src/__tests__/fixtures/hir/chained-assignment-expressions.js

@@ -0,0 +1,8 @@
+function foo() {
+  const x = { x: 0 };
+  const y = { z: 0 };
+  const z = { z: 0 };
+  x.x += y.y *= 1;
+  z.z += y.y *= x.x &= 3;
+  return z;
+}