[compiler] patch: retain UpdateExpression for globals

ghstack-source-id: aff6606f85698ccda13d1deffb6faa9d91f9821a Pull Request resolved: #30471
facebook · Jul 29, 2024 · a8f5c4e · a8f5c4e
1 parent 2a9274a
commit a8f5c4e
Show file tree

Hide file tree

Showing 7 changed files with 67 additions and 56 deletions.
diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/BuildHIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/BuildHIR.ts
@@ -1934,7 +1934,6 @@ function lowerExpression(
       switch (leftNode.type) {
         case 'Identifier': {
           const leftExpr = left as NodePath<t.Identifier>;
-          const identifier = lowerIdentifier(builder, leftExpr);
           const leftPlace = lowerExpressionToTemporary(builder, leftExpr);
           const right = lowerExpressionToTemporary(builder, expr.get('right'));
           const binaryPlace = lowerValueToTemporary(builder, {
@@ -1944,30 +1943,42 @@ function lowerExpression(
             right,
             loc: exprLoc,
           });
-          const kind = getStoreKind(builder, leftExpr);
-          if (kind === 'StoreLocal') {
-            lowerValueToTemporary(builder, {
-              kind: 'StoreLocal',
-              lvalue: {
-                place: {...identifier},
-                kind: InstructionKind.Reassign,
-              },
-              value: {...binaryPlace},
-              type: null,
-              loc: exprLoc,
-            });
-            return {kind: 'LoadLocal', place: identifier, loc: exprLoc};
+          const binding = builder.resolveIdentifier(leftExpr);
+          if (binding.kind === 'Identifier') {
+            const identifier = lowerIdentifier(builder, leftExpr);
+            const kind = getStoreKind(builder, leftExpr);
+            if (kind === 'StoreLocal') {
+              lowerValueToTemporary(builder, {
+                kind: 'StoreLocal',
+                lvalue: {
+                  place: {...identifier},
+                  kind: InstructionKind.Reassign,
+                },
+                value: {...binaryPlace},
+                type: null,
+                loc: exprLoc,
+              });
+              return {kind: 'LoadLocal', place: identifier, loc: exprLoc};
+            } else {
+              lowerValueToTemporary(builder, {
+                kind: 'StoreContext',
+                lvalue: {
+                  place: {...identifier},
+                  kind: InstructionKind.Reassign,
+                },
+                value: {...binaryPlace},
+                loc: exprLoc,
+              });
+              return {kind: 'LoadContext', place: identifier, loc: exprLoc};
+            }
           } else {
-            lowerValueToTemporary(builder, {
-              kind: 'StoreContext',
-              lvalue: {
-                place: {...identifier},
-                kind: InstructionKind.Reassign,
-              },
+            const temporary = lowerValueToTemporary(builder, {
+              kind: 'StoreGlobal',
+              name: leftExpr.node.name,
               value: {...binaryPlace},
               loc: exprLoc,
             });
-            return {kind: 'LoadContext', place: identifier, loc: exprLoc};
+            return {kind: 'LoadLocal', place: temporary, loc: temporary.loc};
           }
         }
         case 'MemberExpression': {

diff --git a/.../__tests__/fixtures/compiler/bug-invalid-update-global-should-bailout.expect.md b/.../__tests__/fixtures/compiler/bug-invalid-update-global-should-bailout.expect.md
diff --git a/...er/src/__tests__/fixtures/compiler/error.update-global-should-bailout.expect.md b/...er/src/__tests__/fixtures/compiler/error.update-global-should-bailout.expect.md
@@ -0,0 +1,31 @@
+
+## Input
+
+```javascript
+let renderCount = 0;
+function useFoo() {
+  renderCount += 1;
+  return renderCount;
+}
+
+export const FIXTURE_ENTRYPOINT = {
+  fn: useFoo,
+  params: [],
+};
+
+```
+
+
+## Error
+
+```
+  1 | let renderCount = 0;
+  2 | function useFoo() {
+> 3 |   renderCount += 1;
+    |   ^^^^^^^^^^^^^^^^ InvalidReact: Unexpected reassignment of a variable which was defined outside of the component. Components and hooks should be pure and side-effect free, but variable reassignment is a form of side-effect. If this variable is used in rendering, use useState instead. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#side-effects-must-run-outside-of-render) (3:3)
+  4 |   return renderCount;
+  5 | }
+  6 |
+```
+          
+      
diff --git a/...-invalid-update-global-should-bailout.tsx → ...er/error.update-global-should-bailout.tsx b/...-invalid-update-global-should-bailout.tsx → ...er/error.update-global-should-bailout.tsx
diff --git a/...r/bug-update-global-in-callback.expect.md → ...piler/update-global-in-callback.expect.md b/...r/bug-update-global-in-callback.expect.md → ...piler/update-global-in-callback.expect.md
@@ -40,6 +40,7 @@ function Foo() {
   return t0;
 }
 function _temp() {
+  renderCount = renderCount + 1;
   return renderCount;
 }
 
@@ -49,4 +50,6 @@ export const FIXTURE_ENTRYPOINT = {
 };
 
 ```
-      
+      
+### Eval output
+(kind: ok) <div>{"cb":{"kind":"Function","result":1},"shouldInvokeFns":true}</div>
diff --git a/...ompiler/bug-update-global-in-callback.tsx → ...es/compiler/update-global-in-callback.tsx b/...ompiler/bug-update-global-in-callback.tsx → ...es/compiler/update-global-in-callback.tsx
diff --git a/compiler/packages/snap/src/SproutTodoFilter.ts b/compiler/packages/snap/src/SproutTodoFilter.ts
@@ -484,8 +484,6 @@ const skipFilter = new Set([
   'rules-of-hooks/rules-of-hooks-69521d94fa03',
 
   // bugs
-  'bug-invalid-update-global-should-bailout',
-  'bug-update-global-in-callback',
   'bug-invalid-hoisting-functionexpr',
   'original-reactive-scopes-fork/bug-nonmutating-capture-in-unsplittable-memo-block',
   'original-reactive-scopes-fork/bug-hoisted-declaration-with-scope',