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 cde56f748404a..2dab01f86e838 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateLocalsNotReassignedAfterRender.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateLocalsNotReassignedAfterRender.ts @@ -8,9 +8,11 @@ import {CompilerError, Effect} from '..'; import {HIRFunction, IdentifierId, Place} from '../HIR'; import { + eachInstructionLValue, eachInstructionValueOperand, eachTerminalOperand, } from '../HIR/visitors'; +import {getFunctionCallSignature} from '../Inference/InferReferenceEffects'; /** * Validates that local variables cannot be reassigned after render. @@ -131,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, @@ -139,15 +160,22 @@ 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 { + /* + * 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); + } + } } } 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 4b5f4690af392..0000000000000 --- 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 b9fc15ea0a7be..0000000000000 --- 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 0000000000000..0318fa9525fda --- /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 0000000000000..2a6dce11f242e --- /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/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 8e415a2fdd4cc..c1aec8a432544 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 0000000000000..cdcd6b3ffad99 --- /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-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 945f68d4be107..0000000000000 --- 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 943163a420c4b..8e7a50a629977 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;