From b24b6eaab400cfe186bd9e356ef075fbdf868e63 Mon Sep 17 00:00:00 2001 From: Mike Vitousek Date: Tue, 30 Jul 2024 16:36:48 -0700 Subject: [PATCH 1/2] [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 | 14 ++- ...ext-variable-only-chained-assign.expect.md | 65 ------------- ...variable-in-function-declaration.expect.md | 40 -------- ...ext-variable-only-chained-assign.expect.md | 40 ++++++++ ...r.context-variable-only-chained-assign.js} | 0 ...variable-in-function-declaration.expect.md | 29 ++++++ ...ssign-variable-in-function-declaration.js} | 0 ...ay-with-capturing-map-after-hook.expect.md | 47 ++++++++++ ...ze-array-with-capturing-map-after-hook.js} | 0 ...assign-local-variable-in-effect.expect.md} | 63 ++----------- ...tion-reassign-local-variable-in-effect.js} | 0 ...ange-shared-inner-outer-function.expect.md | 44 +++++++++ ...able-range-shared-inner-outer-function.js} | 0 ...ay-with-capturing-map-after-hook.expect.md | 91 ------------------- ...ange-shared-inner-outer-function.expect.md | 71 --------------- .../packages/snap/src/SproutTodoFilter.ts | 3 - 16 files changed, 179 insertions(+), 328 deletions(-) delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/context-variable-only-chained-assign.expect.md delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/declare-reassign-variable-in-function-declaration.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.context-variable-only-chained-assign.expect.md rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{context-variable-only-chained-assign.js => error.context-variable-only-chained-assign.js} (100%) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.declare-reassign-variable-in-function-declaration.expect.md rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{declare-reassign-variable-in-function-declaration.js => error.declare-reassign-variable-in-function-declaration.js} (100%) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.dont-memoize-array-with-capturing-map-after-hook.expect.md rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{repro-dont-memoize-array-with-capturing-map-after-hook.js => error.dont-memoize-array-with-capturing-map-after-hook.js} (100%) rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{todo.invalid-nested-function-reassign-local-variable-in-effect.expect.md => error.invalid-nested-function-reassign-local-variable-in-effect.expect.md} (55%) rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{todo.invalid-nested-function-reassign-local-variable-in-effect.js => error.invalid-nested-function-reassign-local-variable-in-effect.js} (100%) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.mutable-range-shared-inner-outer-function.expect.md rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{repro-mutable-range-shared-inner-outer-function.js => error.mutable-range-shared-inner-outer-function.js} (100%) delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-dont-memoize-array-with-capturing-map-after-hook.expect.md delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-mutable-range-shared-inner-outer-function.expect.md 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 cde56f74840..8a9bc6920cc 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateLocalsNotReassignedAfterRender.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateLocalsNotReassignedAfterRender.ts @@ -8,6 +8,7 @@ import {CompilerError, Effect} from '..'; import {HIRFunction, IdentifierId, Place} from '../HIR'; import { + eachInstructionLValue, eachInstructionValueOperand, eachTerminalOperand, } from '../HIR/visitors'; @@ -139,15 +140,18 @@ function getContextReassignment( const reassignment = reassigningFunctions.get( operand.identifier.id, ); - if ( - reassignment !== undefined && - operand.effect === Effect.Freeze - ) { + if (reassignment !== undefined) { /* * Functions that reassign local variables are inherently mutable and are unsafe to pass * to a place that expects a frozen value. Propagate the reassignment upward. */ - return reassignment; + if (operand.effect === Effect.Freeze) { + return reassignment; + } else { + for (const lval of eachInstructionLValue(instr)) { + reassigningFunctions.set(lval.identifier.id, reassignment); + } + } } } break; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/context-variable-only-chained-assign.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/context-variable-only-chained-assign.expect.md deleted file mode 100644 index 4b5f4690af3..00000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/context-variable-only-chained-assign.expect.md +++ /dev/null @@ -1,65 +0,0 @@ - -## Input - -```javascript -import {identity, invoke} from 'shared-runtime'; - -function foo() { - let x = 2; - const fn1 = () => { - const copy1 = (x = 3); - return identity(copy1); - }; - const fn2 = () => { - const copy2 = (x = 4); - return [invoke(fn1), copy2, identity(copy2)]; - }; - return invoke(fn2); -} - -export const FIXTURE_ENTRYPOINT = { - fn: foo, - params: [], -}; - -``` - -## Code - -```javascript -import { c as _c } from "react/compiler-runtime"; -import { identity, invoke } from "shared-runtime"; - -function foo() { - const $ = _c(1); - let t0; - if ($[0] === Symbol.for("react.memo_cache_sentinel")) { - let x; - x = 2; - const fn1 = () => { - const copy1 = (x = 3); - return identity(copy1); - }; - - const fn2 = () => { - const copy2 = (x = 4); - return [invoke(fn1), copy2, identity(copy2)]; - }; - - t0 = invoke(fn2); - $[0] = t0; - } else { - t0 = $[0]; - } - return t0; -} - -export const FIXTURE_ENTRYPOINT = { - fn: foo, - params: [], -}; - -``` - -### Eval output -(kind: ok) [3,4,4] \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/declare-reassign-variable-in-function-declaration.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/declare-reassign-variable-in-function-declaration.expect.md deleted file mode 100644 index b9fc15ea0a7..00000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/declare-reassign-variable-in-function-declaration.expect.md +++ /dev/null @@ -1,40 +0,0 @@ - -## Input - -```javascript -function Component() { - let x = null; - function foo() { - x = 9; - } - const y = bar(foo); - return ; -} - -``` - -## Code - -```javascript -import { c as _c } from "react/compiler-runtime"; -function Component() { - const $ = _c(1); - let t0; - if ($[0] === Symbol.for("react.memo_cache_sentinel")) { - let x; - x = null; - const foo = function foo() { - x = 9; - }; - - const y = bar(foo); - t0 = ; - $[0] = t0; - } else { - t0 = $[0]; - } - return t0; -} - -``` - \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.context-variable-only-chained-assign.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.context-variable-only-chained-assign.expect.md new file mode 100644 index 00000000000..0318fa9525f --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.context-variable-only-chained-assign.expect.md @@ -0,0 +1,40 @@ + +## Input + +```javascript +import {identity, invoke} from 'shared-runtime'; + +function foo() { + let x = 2; + const fn1 = () => { + const copy1 = (x = 3); + return identity(copy1); + }; + const fn2 = () => { + const copy2 = (x = 4); + return [invoke(fn1), copy2, identity(copy2)]; + }; + return invoke(fn2); +} + +export const FIXTURE_ENTRYPOINT = { + fn: foo, + params: [], +}; + +``` + + +## Error + +``` + 8 | }; + 9 | const fn2 = () => { +> 10 | const copy2 = (x = 4); + | ^ InvalidReact: Reassigning a variable after render has completed can cause inconsistent behavior on subsequent renders. Consider using state instead. Variable `x` cannot be reassigned after render (10:10) + 11 | return [invoke(fn1), copy2, identity(copy2)]; + 12 | }; + 13 | return invoke(fn2); +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/context-variable-only-chained-assign.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.context-variable-only-chained-assign.js similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/context-variable-only-chained-assign.js rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.context-variable-only-chained-assign.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.declare-reassign-variable-in-function-declaration.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.declare-reassign-variable-in-function-declaration.expect.md new file mode 100644 index 00000000000..2a6dce11f24 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.declare-reassign-variable-in-function-declaration.expect.md @@ -0,0 +1,29 @@ + +## Input + +```javascript +function Component() { + let x = null; + function foo() { + x = 9; + } + const y = bar(foo); + return ; +} + +``` + + +## Error + +``` + 2 | let x = null; + 3 | function foo() { +> 4 | x = 9; + | ^ InvalidReact: Reassigning a variable after render has completed can cause inconsistent behavior on subsequent renders. Consider using state instead. Variable `x` cannot be reassigned after render (4:4) + 5 | } + 6 | const y = bar(foo); + 7 | return ; +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/declare-reassign-variable-in-function-declaration.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.declare-reassign-variable-in-function-declaration.js similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/declare-reassign-variable-in-function-declaration.js rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.declare-reassign-variable-in-function-declaration.js 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 new file mode 100644 index 00000000000..a60c765260e --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.dont-memoize-array-with-capturing-map-after-hook.expect.md @@ -0,0 +1,47 @@ + +## 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.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.dont-memoize-array-with-capturing-map-after-hook.js similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-dont-memoize-array-with-capturing-map-after-hook.js rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.dont-memoize-array-with-capturing-map-after-hook.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo.invalid-nested-function-reassign-local-variable-in-effect.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-nested-function-reassign-local-variable-in-effect.expect.md similarity index 55% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo.invalid-nested-function-reassign-local-variable-in-effect.expect.md rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-nested-function-reassign-local-variable-in-effect.expect.md index 8e415a2fdd4..c1aec8a4325 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo.invalid-nested-function-reassign-local-variable-in-effect.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-nested-function-reassign-local-variable-in-effect.expect.md @@ -42,60 +42,17 @@ function Component() { ``` -## Code -```javascript -import { c as _c } from "react/compiler-runtime"; -import { useEffect } from "react"; -function Component() { - const $ = _c(4); - let local; - let t0; - if ($[0] === Symbol.for("react.memo_cache_sentinel")) { - const mk_reassignlocal = () => { - const reassignLocal = (newValue) => { - local = newValue; - }; - return reassignLocal; - }; - - t0 = mk_reassignlocal(); - $[0] = t0; - } else { - t0 = $[0]; - } - const reassignLocal_0 = t0; - let t1; - if ($[1] === Symbol.for("react.memo_cache_sentinel")) { - t1 = (newValue_0) => { - reassignLocal_0("hello"); - if (local === newValue_0) { - console.log("`local` was updated!"); - } else { - throw new Error("`local` not updated!"); - } - }; - $[1] = t1; - } else { - t1 = $[1]; - } - const onMount = t1; - let t2; - let t3; - if ($[2] === Symbol.for("react.memo_cache_sentinel")) { - t2 = () => { - onMount(); - }; - t3 = [onMount]; - $[2] = t2; - $[3] = t3; - } else { - t2 = $[2]; - t3 = $[3]; - } - useEffect(t2, t3); - return "ok"; -} +## Error ``` + 5 | // Create the reassignment function inside another function, then return it + 6 | const reassignLocal = newValue => { +> 7 | local = newValue; + | ^^^^^ InvalidReact: Reassigning a variable after render has completed can cause inconsistent behavior on subsequent renders. Consider using state instead. Variable `local` cannot be reassigned after render (7:7) + 8 | }; + 9 | return reassignLocal; + 10 | }; +``` + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo.invalid-nested-function-reassign-local-variable-in-effect.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-nested-function-reassign-local-variable-in-effect.js similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo.invalid-nested-function-reassign-local-variable-in-effect.js rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-nested-function-reassign-local-variable-in-effect.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.mutable-range-shared-inner-outer-function.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.mutable-range-shared-inner-outer-function.expect.md new file mode 100644 index 00000000000..cdcd6b3ffad --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.mutable-range-shared-inner-outer-function.expect.md @@ -0,0 +1,44 @@ + +## Input + +```javascript +// @enableAssumeHooksFollowRulesOfReact @enableTransitivelyFreezeFunctionExpressions +let cond = true; +function Component(props) { + let a; + let b; + const f = () => { + if (cond) { + a = {}; + b = []; + } else { + a = {}; + b = []; + } + a.property = true; + b.push(false); + }; + return
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{}], +}; + +``` + + +## Error + +``` + 6 | const f = () => { + 7 | if (cond) { +> 8 | a = {}; + | ^ InvalidReact: Reassigning a variable after render has completed can cause inconsistent behavior on subsequent renders. Consider using state instead. Variable `a` cannot be reassigned after render (8:8) + 9 | b = []; + 10 | } else { + 11 | a = {}; +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-mutable-range-shared-inner-outer-function.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.mutable-range-shared-inner-outer-function.js similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-mutable-range-shared-inner-outer-function.js rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.mutable-range-shared-inner-outer-function.js 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 deleted file mode 100644 index ace89f6ff0c..00000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-dont-memoize-array-with-capturing-map-after-hook.expect.md +++ /dev/null @@ -1,91 +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, -}; - -``` - -## 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/repro-mutable-range-shared-inner-outer-function.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-mutable-range-shared-inner-outer-function.expect.md deleted file mode 100644 index 945f68d4be1..00000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-mutable-range-shared-inner-outer-function.expect.md +++ /dev/null @@ -1,71 +0,0 @@ - -## Input - -```javascript -// @enableAssumeHooksFollowRulesOfReact @enableTransitivelyFreezeFunctionExpressions -let cond = true; -function Component(props) { - let a; - let b; - const f = () => { - if (cond) { - a = {}; - b = []; - } else { - a = {}; - b = []; - } - a.property = true; - b.push(false); - }; - return
; -} - -export const FIXTURE_ENTRYPOINT = { - fn: Component, - params: [{}], -}; - -``` - -## Code - -```javascript -import { c as _c } from "react/compiler-runtime"; // @enableAssumeHooksFollowRulesOfReact @enableTransitivelyFreezeFunctionExpressions -let cond = true; -function Component(props) { - const $ = _c(1); - let t0; - if ($[0] === Symbol.for("react.memo_cache_sentinel")) { - let a; - let b; - const f = () => { - if (cond) { - a = {}; - b = []; - } else { - a = {}; - b = []; - } - - a.property = true; - b.push(false); - }; - - t0 =
; - $[0] = t0; - } else { - t0 = $[0]; - } - return t0; -} - -export const FIXTURE_ENTRYPOINT = { - fn: Component, - params: [{}], -}; - -``` - -### Eval output -(kind: ok)
\ No newline at end of file diff --git a/compiler/packages/snap/src/SproutTodoFilter.ts b/compiler/packages/snap/src/SproutTodoFilter.ts index 943163a420c..8e7a50a6299 100644 --- a/compiler/packages/snap/src/SproutTodoFilter.ts +++ b/compiler/packages/snap/src/SproutTodoFilter.ts @@ -187,7 +187,6 @@ const skipFilter = new Set([ 'alias-nested-member-path-mutate', 'concise-arrow-expr', 'const-propagation-into-function-expression-global', - 'declare-reassign-variable-in-function-declaration', 'lambda-mutate-shadowed-object', 'fbt/lambda-with-fbt', 'recursive-function-expr', @@ -503,8 +502,6 @@ const skipFilter = new Set([ // needs to be executed as a module 'meta-property', - - 'todo.invalid-nested-function-reassign-local-variable-in-effect', ]); export default skipFilter; From f011de893865716500cf4478ff5b198d9f2694dc Mon Sep 17 00:00:00 2001 From: Mike Vitousek Date: Wed, 31 Jul 2024 11:06:06 -0700 Subject: [PATCH 2/2] 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 8a9bc6920cc..2dab01f86e8 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 a60c765260e..00000000000 --- 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 00000000000..ace89f6ff0c --- /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