From a8f5c4e16d46666453a102665370b3119dbbb029 Mon Sep 17 00:00:00 2001 From: Mofei Zhang Date: Fri, 26 Jul 2024 17:36:04 -0400 Subject: [PATCH] [compiler] patch: retain UpdateExpression for globals ghstack-source-id: aff6606f85698ccda13d1deffb6faa9d91f9821a Pull Request resolved: https://github.com/facebook/react/pull/30471 --- .../src/HIR/BuildHIR.ts | 53 +++++++++++-------- ...lid-update-global-should-bailout.expect.md | 32 ----------- ...ror.update-global-should-bailout.expect.md | 31 +++++++++++ ...=> error.update-global-should-bailout.tsx} | 0 ...md => update-global-in-callback.expect.md} | 5 +- ...back.tsx => update-global-in-callback.tsx} | 0 .../packages/snap/src/SproutTodoFilter.ts | 2 - 7 files changed, 67 insertions(+), 56 deletions(-) delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-invalid-update-global-should-bailout.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.update-global-should-bailout.expect.md rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{bug-invalid-update-global-should-bailout.tsx => error.update-global-should-bailout.tsx} (100%) rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{bug-update-global-in-callback.expect.md => update-global-in-callback.expect.md} (85%) rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{bug-update-global-in-callback.tsx => update-global-in-callback.tsx} (100%) diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/BuildHIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/BuildHIR.ts index 5dccb5ffd278f..b2ee773c26511 100644 --- 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; - 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/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-invalid-update-global-should-bailout.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-invalid-update-global-should-bailout.expect.md deleted file mode 100644 index d8804cad09ec5..0000000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-invalid-update-global-should-bailout.expect.md +++ /dev/null @@ -1,32 +0,0 @@ - -## Input - -```javascript -let renderCount = 0; -function useFoo() { - renderCount += 1; - return renderCount; -} - -export const FIXTURE_ENTRYPOINT = { - fn: useFoo, - params: [], -}; - -``` - -## Code - -```javascript -let renderCount = 0; -function useFoo() { - return renderCount; -} - -export const FIXTURE_ENTRYPOINT = { - fn: useFoo, - params: [], -}; - -``` - \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.update-global-should-bailout.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.update-global-should-bailout.expect.md new file mode 100644 index 0000000000000..ca299dbdec6e4 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/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 | +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-invalid-update-global-should-bailout.tsx b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.update-global-should-bailout.tsx similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-invalid-update-global-should-bailout.tsx rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.update-global-should-bailout.tsx diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-update-global-in-callback.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/update-global-in-callback.expect.md similarity index 85% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-update-global-in-callback.expect.md rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/update-global-in-callback.expect.md index 682259ca3a3c4..a250ce4a10c23 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-update-global-in-callback.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/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 = { }; ``` - \ No newline at end of file + +### Eval output +(kind: ok)
{"cb":{"kind":"Function","result":1},"shouldInvokeFns":true}
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-update-global-in-callback.tsx b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/update-global-in-callback.tsx similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-update-global-in-callback.tsx rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/update-global-in-callback.tsx diff --git a/compiler/packages/snap/src/SproutTodoFilter.ts b/compiler/packages/snap/src/SproutTodoFilter.ts index 9bc39d72b1a15..ff542dfb9730c 100644 --- 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',