From f011de893865716500cf4478ff5b198d9f2694dc Mon Sep 17 00:00:00 2001 From: Mike Vitousek Date: Wed, 31 Jul 2024 11:06:06 -0700 Subject: [PATCH] Update on "[compiler] More complete validation against locals being reassigned after render" Summary: This diff extends the existing work on validating against locals being reassigned after render, by propagating the reassignment "effect" into the lvalues of instructions when the rvalue operands include values known to cause reassignments. In particular, this "closes the loop" for function definitions and function calls: a function that returns a function that reassigns will be considered to also perform reassignments, but previous to this we didn't consider the result of a `Call` of a function that reassigns to itself be a value that reassigns. This causes a number of new bailouts in test cases, all of which appear to me to be legit. [ghstack-poisoned] --- .../ValidateLocalsNotReassignedAfterRender.ts | 26 +++++- ...ay-with-capturing-map-after-hook.expect.md | 47 ---------- ...ay-with-capturing-map-after-hook.expect.md | 91 +++++++++++++++++++ ...ze-array-with-capturing-map-after-hook.js} | 0 4 files changed, 116 insertions(+), 48 deletions(-) delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.dont-memoize-array-with-capturing-map-after-hook.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-dont-memoize-array-with-capturing-map-after-hook.expect.md rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{error.dont-memoize-array-with-capturing-map-after-hook.js => repro-dont-memoize-array-with-capturing-map-after-hook.js} (100%) diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateLocalsNotReassignedAfterRender.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateLocalsNotReassignedAfterRender.ts index 8a9bc6920cc1a..2dab01f86e838 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateLocalsNotReassignedAfterRender.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateLocalsNotReassignedAfterRender.ts @@ -12,6 +12,7 @@ import { eachInstructionValueOperand, eachTerminalOperand, } from '../HIR/visitors'; +import {getFunctionCallSignature} from '../Inference/InferReferenceEffects'; /** * Validates that local variables cannot be reassigned after render. @@ -132,7 +133,26 @@ function getContextReassignment( break; } default: { - for (const operand of eachInstructionValueOperand(value)) { + let operands = eachInstructionValueOperand(value); + // If we're calling a function that doesn't let its arguments escape, only test the callee + if (value.kind === 'CallExpression') { + const signature = getFunctionCallSignature( + fn.env, + value.callee.identifier.type, + ); + if (signature?.noAlias) { + operands = [value.callee]; + } + } else if (value.kind === 'MethodCall') { + const signature = getFunctionCallSignature( + fn.env, + value.property.identifier.type, + ); + if (signature?.noAlias) { + operands = [value.receiver, value.property]; + } + } + for (const operand of operands) { CompilerError.invariant(operand.effect !== Effect.Unknown, { reason: `Expected effects to be inferred prior to ValidateLocalsNotReassignedAfterRender`, loc: operand.loc, @@ -148,6 +168,10 @@ function getContextReassignment( if (operand.effect === Effect.Freeze) { return reassignment; } else { + /* + * If the operand is not frozen but it does reassign, then the lvalues + * of the instruction could also be reassigning + */ for (const lval of eachInstructionLValue(instr)) { reassigningFunctions.set(lval.identifier.id, reassignment); } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.dont-memoize-array-with-capturing-map-after-hook.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.dont-memoize-array-with-capturing-map-after-hook.expect.md deleted file mode 100644 index a60c765260ebd..0000000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.dont-memoize-array-with-capturing-map-after-hook.expect.md +++ /dev/null @@ -1,47 +0,0 @@ - -## Input - -```javascript -import {useEffect, useState} from 'react'; -import {mutate} from 'shared-runtime'; - -function Component(props) { - const x = [{...props.value}]; - useEffect(() => {}, []); - const onClick = () => { - console.log(x.length); - }; - let y; - return ( -
- {x.map(item => { - y = item; - return {item.text}; - })} - {mutate(y)} -
- ); -} - -export const FIXTURE_ENTRYPOINT = { - fn: Component, - params: [{value: {id: 0, text: 'Hello!'}}], - isComponent: true, -}; - -``` - - -## Error - -``` - 12 |
- 13 | {x.map(item => { -> 14 | y = item; - | ^ InvalidReact: Reassigning a variable after render has completed can cause inconsistent behavior on subsequent renders. Consider using state instead. Variable `y` cannot be reassigned after render (14:14) - 15 | return {item.text}; - 16 | })} - 17 | {mutate(y)} -``` - - \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-dont-memoize-array-with-capturing-map-after-hook.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-dont-memoize-array-with-capturing-map-after-hook.expect.md new file mode 100644 index 0000000000000..ace89f6ff0c06 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-dont-memoize-array-with-capturing-map-after-hook.expect.md @@ -0,0 +1,91 @@ + +## Input + +```javascript +import {useEffect, useState} from 'react'; +import {mutate} from 'shared-runtime'; + +function Component(props) { + const x = [{...props.value}]; + useEffect(() => {}, []); + const onClick = () => { + console.log(x.length); + }; + let y; + return ( +
+ {x.map(item => { + y = item; + return {item.text}; + })} + {mutate(y)} +
+ ); +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{value: {id: 0, text: 'Hello!'}}], + isComponent: true, +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import { useEffect, useState } from "react"; +import { mutate } from "shared-runtime"; + +function Component(props) { + const $ = _c(5); + const x = [{ ...props.value }]; + let t0; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + t0 = []; + $[0] = t0; + } else { + t0 = $[0]; + } + useEffect(_temp, t0); + const onClick = () => { + console.log(x.length); + }; + + let y; + + const t1 = x.map((item) => { + y = item; + return {item.text}; + }); + const t2 = mutate(y); + let t3; + if ($[1] !== onClick || $[2] !== t1 || $[3] !== t2) { + t3 = ( +
+ {t1} + {t2} +
+ ); + $[1] = onClick; + $[2] = t1; + $[3] = t2; + $[4] = t3; + } else { + t3 = $[4]; + } + return t3; +} +function _temp() {} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ value: { id: 0, text: "Hello!" } }], + isComponent: true, +}; + +``` + +### Eval output +(kind: ok)
Hello!
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.dont-memoize-array-with-capturing-map-after-hook.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-dont-memoize-array-with-capturing-map-after-hook.js similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.dont-memoize-array-with-capturing-map-after-hook.js rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-dont-memoize-array-with-capturing-map-after-hook.js