Add instructions for UpdateExpression variants

Adds new instructions to accurately model UpdateExpression semantics, since `x++` is un-intuitively not the same as `x = x + 1`. There are a few different ways to model the combination of prefix/postfix and increment/decrement: * One instruction for all combinations of prefix/postfix and increment/decrement, eg 'UpdateExpression' * Instructions for Increment/Decrement, each with a property to distinguish prefix/postfix * Instructions for Prefix/Postfix, each with aproperty to distinguish increment/decrement. I chose the latter, `PrefixUpdate` and `PostfixUpdate`, because it keeps the number of new instructions minimal while keeping separate instructions for the most important distinction: whether the result of the instruction is the value before applying the operation or after. I'm open to suggestions about this though. A few quick notes: * Constant propagation is supported but only for numbers (we don't support bigint yet anyway) * LeaveSSA needs to know about these instructions since their presence requires making the original variable declaration Let, not Const. * EnterSSA mapped lvalues before rvalues, which is out of order but didn't previously matter. I just had to flip the order and everything worked.
2025-11-01 09:12:30 +00:00 · 2023-08-07 13:50:32 -07:00
parent e33c9c43cc
commit 43677da7e4
22 changed files with 348 additions and 54 deletions
@@ -1753,56 +1753,43 @@ function lowerExpression(
        });
        return { kind: "UnsupportedNode", node: exprNode, loc: exprLoc };
      }
-      if (expr.node.prefix) {
-        builder.errors.push({
-          reason: `(BuildHIR::lowerExpression) Handle prefix UpdateExpression`,
-          severity: ErrorSeverity.Todo,
-          loc: exprPath.node.loc ?? null,
-          suggestions: null,
-        });
-        return { kind: "UnsupportedNode", node: exprNode, loc: exprLoc };
-      }
-      const primitiveTemp = lowerValueToTemporary(builder, {
-        kind: "Primitive",
-        value: 1,
-        loc: expr.node.loc ?? GeneratedSource,
-      });
-      const temp = buildTemporaryPlace(
-        builder,
-        expr.node.loc ?? GeneratedSource
-      );
-      const identifier = lowerIdentifierForAssignment(
+      const lvalue = lowerIdentifierForAssignment(
        builder,
        argument.node.loc ?? GeneratedSource,
        InstructionKind.Reassign,
-        argument as NodePath<t.Identifier>
+        argument
      );
-      if (identifier === null) {
+      if (lvalue === null) {
+        // lowerIdentifierForAssignment should have already reported an error if it returned null,
+        // we check here just in case
+        if (!builder.errors.hasErrors()) {
+          builder.errors.push({
+            reason: `(BuildHIR::lowerExpression) Found an invalid UpdateExpression without a previously reported error`,
+            severity: ErrorSeverity.Invariant,
+            loc: exprLoc,
+            suggestions: null,
+          });
+        }
        return { kind: "UnsupportedNode", node: exprNode, loc: exprLoc };
      }
-      builder.push({
-        id: makeInstructionId(0),
-        lvalue: { ...temp },
-        value: {
-          kind: "BinaryExpression",
-          operator: expr.node.operator === "++" ? "+" : "-",
-          left: { ...identifier },
-          right: { ...primitiveTemp },
+      const value = lowerIdentifier(builder, argument);
+      if (expr.node.prefix) {
+        return {
+          kind: "PrefixUpdate",
+          lvalue,
+          operation: expr.node.operator,
+          value,
          loc: exprLoc,
-        },
-        loc: exprLoc,
-      });
-      lowerValueToTemporary(builder, {
-        kind: getStoreKind(builder, argument),
-        lvalue: { place: { ...identifier }, kind: InstructionKind.Reassign },
-        value: { ...temp },
-        loc: exprLoc,
-      });
-      return {
-        kind: "LoadLocal",
-        place: { ...identifier },
-        loc: exprLoc,
-      };
+        };
+      } else {
+        return {
+          kind: "PostfixUpdate",
+          lvalue,
+          operation: expr.node.operator,
+          value,
+          loc: exprLoc,
+        };
+      }
    }
    case "RegExpLiteral": {
      let expr = exprPath as NodePath<t.RegExpLiteral>;
@@ -736,6 +736,26 @@ export type InstructionValue =
      value: Place; // the collection
      loc: SourceLocation;
    }
+  // Models a prefix update expression such as --x or ++y
+  // This instructions increments or decrements the <lvalue>
+  // but evaluates to the value of <value> prior to the update.
+  | {
+      kind: "PrefixUpdate";
+      lvalue: Place;
+      operation: t.UpdateExpression["operator"];
+      value: Place;
+      loc: SourceLocation;
+    }
+  // Models a postfix update expression such as x-- or y++
+  // This instructions increments or decrements the <lvalue>
+  // and evaluates to the value after the update
+  | {
+      kind: "PostfixUpdate";
+      lvalue: Place;
+      operation: t.UpdateExpression["operator"];
+      value: Place;
+      loc: SourceLocation;
+    }
  // `debugger` statement
  | { kind: "Debugger"; loc: SourceLocation }
  /**
@@ -513,6 +513,18 @@ export function printInstructionValue(instrValue: ReactiveValue): string {
      value = `Debugger`;
      break;
    }
+    case "PostfixUpdate": {
+      value = `PostfixUpdate ${printPlace(instrValue.lvalue)} = ${printPlace(
+        instrValue.value
+      )} ${instrValue.operation}`;
+      break;
+    }
+    case "PrefixUpdate": {
+      value = `PrefixUpdate ${printPlace(instrValue.lvalue)} = ${
+        instrValue.operation
+      } ${printPlace(instrValue.value)}`;
+      break;
+    }
    default: {
      assertExhaustive(
        instrValue,
@@ -35,6 +35,11 @@ export function* eachInstructionLValue(
      yield* eachPatternOperand(instr.value.lvalue.pattern);
      break;
    }
+    case "PostfixUpdate":
+    case "PrefixUpdate": {
+      yield instr.value.lvalue;
+      break;
+    }
  }
 }

@@ -188,6 +193,11 @@ export function* eachInstructionValueOperand(
      yield instrValue.value;
      break;
    }
+    case "PostfixUpdate":
+    case "PrefixUpdate": {
+      yield instrValue.value;
+      break;
+    }
    case "Debugger":
    case "RegExpLiteral":
    case "LoadGlobal":
@@ -305,6 +315,11 @@ export function mapInstructionLValues(
      mapPatternOperands(instr.value.lvalue.pattern, fn);
      break;
    }
+    case "PostfixUpdate":
+    case "PrefixUpdate": {
+      instr.value.lvalue = fn(instr.value.lvalue);
+      break;
+    }
  }
  if (instr.lvalue !== null) {
    instr.lvalue = fn(instr.lvalue);
@@ -463,6 +478,11 @@ export function mapInstructionOperands(
      instrValue.value = fn(instrValue.value);
      break;
    }
+    case "PostfixUpdate":
+    case "PrefixUpdate": {
+      instrValue.value = fn(instrValue.value);
+      break;
+    }
    case "Debugger":
    case "RegExpLiteral":
    case "LoadGlobal":
@@ -933,6 +933,26 @@ function inferBlock(
        state.define(instrValue.lvalue.place, instrValue);
        continue;
      }
+      case "PostfixUpdate":
+      case "PrefixUpdate": {
+        const effect =
+          state.isDefined(instrValue.lvalue) &&
+          state.kind(instrValue.lvalue) === ValueKind.Context
+            ? Effect.ConditionallyMutate
+            : Effect.Capture;
+        state.reference(instrValue.value, effect);
+
+        const lvalue = instr.lvalue;
+        state.alias(lvalue, instrValue.value);
+        lvalue.effect = Effect.Store;
+        state.alias(instrValue.lvalue, instrValue.value);
+        // NOTE: *not* using state.reference since this is an assignment.
+        // reference() checks if the effect is valid given the value kind,
+        // but here the previous value kind doesn't matter since we are
+        // replacing it
+        instrValue.lvalue.effect = Effect.Store;
+        continue;
+      }
      case "StoreLocal": {
        const effect =
          state.isDefined(instrValue.lvalue.place) &&
@@ -237,6 +237,45 @@ function evaluateInstruction(
      }
      return null;
    }
+    case "PostfixUpdate": {
+      const previous = read(constants, value.value);
+      if (
+        previous !== null &&
+        previous.kind === "Primitive" &&
+        typeof previous.value === "number"
+      ) {
+        const next =
+          value.operation === "++" ? previous.value + 1 : previous.value - 1;
+        // Store the updated value
+        constants.set(value.lvalue.identifier.id, {
+          kind: "Primitive",
+          value: next,
+          loc: value.loc,
+        });
+        // But return the value prior to the update
+        return previous;
+      }
+      return null;
+    }
+    case "PrefixUpdate": {
+      const previous = read(constants, value.value);
+      if (
+        previous !== null &&
+        previous.kind === "Primitive" &&
+        typeof previous.value === "number"
+      ) {
+        const next: Primitive = {
+          kind: "Primitive",
+          value:
+            value.operation === "++" ? previous.value + 1 : previous.value - 1,
+          loc: value.loc,
+        };
+        // Store and return the updated value
+        constants.set(value.lvalue.identifier.id, next);
+        return next;
+      }
+      return null;
+    }
    case "BinaryExpression": {
      const lhsValue = read(constants, value.left);
      const rhsValue = read(constants, value.right);
@@ -190,6 +190,11 @@ function pruneableValue(value: InstructionValue, state: State): boolean {
      }
      return true;
    }
+    case "PostfixUpdate":
+    case "PrefixUpdate": {
+      // Updates are pruneable only if the identifier being stored to is never read later
+      return !state.used(value.lvalue.identifier);
+    }
    case "Debugger": {
      // explicitly retain debugger statements to not break debugging workflows
      return false;
@@ -1125,6 +1125,22 @@ function codegenInstructionValue(
      value = codegenPlace(cx, instrValue.value);
      break;
    }
+    case "PostfixUpdate": {
+      value = t.updateExpression(
+        instrValue.operation,
+        codegenPlace(cx, instrValue.lvalue),
+        false
+      );
+      break;
+    }
+    case "PrefixUpdate": {
+      value = t.updateExpression(
+        instrValue.operation,
+        codegenPlace(cx, instrValue.lvalue),
+        true
+      );
+      break;
+    }
    case "Debugger":
    case "DeclareLocal":
    case "DeclareContext":
@@ -223,6 +223,8 @@ function mayAllocate(env: Environment, instruction: Instruction): boolean {
    case "Destructure": {
      return doesPatternContainSpreadElement(value.lvalue.pattern);
    }
+    case "PostfixUpdate":
+    case "PrefixUpdate":
    case "Await":
    case "DeclareLocal":
    case "DeclareContext":
@@ -430,6 +430,8 @@ function computeMemoizationInputs(
        rvalues: value.children,
      };
    }
+    case "PrefixUpdate":
+    case "PostfixUpdate":
    case "Debugger":
    case "ComputedDelete":
    case "PropertyDelete":
@@ -264,8 +264,8 @@ function enterSSAImpl(
    }

    for (const instr of block.instructions) {
-      mapInstructionLValues(instr, (lvalue) => builder.definePlace(lvalue));
      mapInstructionOperands(instr, (place) => builder.getPlace(place));
+      mapInstructionLValues(instr, (lvalue) => builder.definePlace(lvalue));

      if (
        instr.value.kind === "FunctionExpression" &&
@@ -142,6 +142,24 @@ export function leaveSSA(fn: HIRFunction): void {
            place: value.lvalue.place,
          });
        }
+      } else if (
+        value.kind === "PrefixUpdate" ||
+        value.kind === "PostfixUpdate"
+      ) {
+        CompilerError.invariant(value.lvalue.identifier.name !== null, {
+          reason: `Expected update expression to be applied to a named variable`,
+          description: null,
+          loc: value.lvalue.loc,
+          suggestions: null,
+        });
+        const originalLVal = declarations.get(value.lvalue.identifier.name);
+        CompilerError.invariant(originalLVal !== undefined, {
+          reason: `Expected update expression to be applied to a previously defined variable`,
+          description: null,
+          loc: value.lvalue.loc,
+          suggestions: null,
+        });
+        originalLVal.lvalue.kind = InstructionKind.Let;
      } else if (value.kind === "StoreLocal") {
        if (value.lvalue.place.identifier.name != null) {
          const originalLVal = declarations.get(
@@ -157,6 +157,14 @@ function* generateInstructionTypes(
      break;
    }

+    case "PostfixUpdate":
+    case "PrefixUpdate": {
+      yield equation(value.value.identifier.type, { kind: "Primitive" });
+      yield equation(value.lvalue.identifier.type, { kind: "Primitive" });
+      yield equation(left, { kind: "Primitive" });
+      break;
+    }
+
    case "LoadGlobal": {
      const globalType = env.getGlobalDeclaration(value.name);
      if (globalType) {
@@ -0,0 +1,48 @@
+/**
+ * Copyright (c) Meta Platforms, Inc. and affiliates.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ */
+
+import { render, screen, fireEvent } from "@testing-library/react";
+import * as React from "react";
+import { expectLogsAndClear, log } from "./expectLogs";
+
+function Counter(props) {
+  let value = props.value;
+  let a = value++;
+  expect(a).toBe(props.value); // postfix
+  let b = ++value;
+  expect(b).toBe(props.value + 2); // previous postfix operation + prefix operation
+  let c = ++value;
+  expect(c).toBe(props.value + 3);
+  let d = value--;
+  expect(d).toBe(props.value + 3);
+  let e = --value;
+  expect(e).toBe(props.value + 1);
+  let f = --value;
+  expect(f).toBe(props.value);
+  expect(value).toBe(props.value);
+  return <span>{value}</span>;
+}
+
+test("use-state", async () => {
+  const { asFragment, rerender } = render(<Counter value={0} />);
+  expect(asFragment()).toMatchInlineSnapshot(`
+    <DocumentFragment>
+      <span>
+        0
+      </span>
+    </DocumentFragment>
+  `);
+
+  rerender(<Counter value={1} />);
+  expect(asFragment()).toMatchInlineSnapshot(`
+    <DocumentFragment>
+      <span>
+        1
+      </span>
+    </DocumentFragment>
+  `);
+});
@@ -20,7 +20,7 @@ function foo(props) {
 function foo(props) {
  let y = 0;
  while (y < props.max) {
-    y = y + 1;
+    y++;
  }
  return y;
 }
@@ -109,10 +109,6 @@ let moduleLocal = false;

 [ReactForget] Todo: (BuildHIR::lowerStatement) Handle ForInStatement statements (43:44)

-[ReactForget] Todo: (BuildHIR::lowerExpression) Handle prefix UpdateExpression (47:47)
-
-[ReactForget] Todo: (BuildHIR::lowerExpression) Handle prefix UpdateExpression (48:48)
-
 [ReactForget] Todo: (BuildHIR::lowerExpression) Handle UpdateExpression with MemberExpression argument (49:49)

 [ReactForget] Todo: (BuildHIR::lowerExpression) Handle UpdateExpression with MemberExpression argument (50:50)
@@ -17,7 +17,7 @@ function foo() {
 ```javascript
 function foo() {
  let x = 1;
-  for (let i = 0; i < 10; i = i + 1, i) {
+  for (let i = 0; i < 10; i++) {
    x = x + 1;
  }
  return x;
@@ -0,0 +1,33 @@
+
+## Input
+
+```javascript
+function Component() {
+  let a = 0;
+  const b = a++;
+  const c = ++a;
+  const d = a--;
+  const e = --a;
+  return { a, b, c, d, e };
+}
+
+```
+
+## Code
+
+```javascript
+import { unstable_useMemoCache as useMemoCache } from "react";
+function Component() {
+  const $ = useMemoCache(1);
+  let t0;
+  if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
+    t0 = { a: 0, b: 0, c: 2, d: 2, e: 0 };
+    $[0] = t0;
+  } else {
+    t0 = $[0];
+  }
+  return t0;
+}
+
+```
+      
@@ -0,0 +1,8 @@
+function Component() {
+  let a = 0;
+  const b = a++;
+  const c = ++a;
+  const d = a--;
+  const e = --a;
+  return { a, b, c, d, e };
+}
@@ -0,0 +1,51 @@
+
+## Input
+
+```javascript
+// @debug
+function Component(props) {
+  let a = props.x;
+  let b;
+  let c;
+  let d;
+  if (props.cond) {
+    d = ((b = a), a++, (c = a), ++a);
+  }
+  return [a, b, c, d];
+}
+
+```
+
+## Code
+
+```javascript
+import { unstable_useMemoCache as useMemoCache } from "react"; // @debug
+function Component(props) {
+  const $ = useMemoCache(5);
+  let a = props.x;
+  let b;
+  let c;
+  let d;
+  if (props.cond) {
+    d = ((b = a), a++, (c = a), ++a);
+  }
+  const c_0 = $[0] !== a;
+  const c_1 = $[1] !== b;
+  const c_2 = $[2] !== c;
+  const c_3 = $[3] !== d;
+  let t0;
+  if (c_0 || c_1 || c_2 || c_3) {
+    t0 = [a, b, c, d];
+    $[0] = a;
+    $[1] = b;
+    $[2] = c;
+    $[3] = d;
+    $[4] = t0;
+  } else {
+    t0 = $[4];
+  }
+  return t0;
+}
+
+```
+      
@@ -0,0 +1,11 @@
+// @debug
+function Component(props) {
+  let a = props.x;
+  let b;
+  let c;
+  let d;
+  if (props.cond) {
+    d = ((b = a), a++, (c = a), ++a);
+  }
+  return [a, b, c, d];
+}
@@ -18,10 +18,8 @@ import { unstable_useMemoCache as useMemoCache } from "react";
 function foo(props) {
  const $ = useMemoCache(4);
  let x = props.x;
-  x = x + 1;
-  const y = x;
-  x = x - 1;
-  const z = x;
+  const y = x++;
+  const z = x--;
  const c_0 = $[0] !== x;
  const c_1 = $[1] !== y;
  const c_2 = $[2] !== z;