From 3723b071df1cdf35f1bda323a06f7c628d0f46f5 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Wed, 26 Jun 2024 13:06:27 -0700 Subject: [PATCH] [compiler] Validate against locals being reassigned after render ghstack-source-id: d689a96e28742d9ae447e65256c918b8d430c7ad Pull Request resolved: https://github.com/facebook/react/pull/30107 --- .../src/Entrypoint/Pipeline.ts | 3 + .../ValidateLocalsNotReassignedAfterRender.ts | 182 ++++++++++++++++++ ...assign-local-variable-in-effect.expect.md} | 59 +----- ...alid-reassign-local-variable-in-effect.js} | 0 ...local-variable-in-hook-argument.expect.md} | 56 +----- ...assign-local-variable-in-hook-argument.js} | 0 ...-local-variable-in-jsx-callback.expect.md} | 44 +---- ...eassign-local-variable-in-jsx-callback.js} | 0 8 files changed, 215 insertions(+), 129 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateLocalsNotReassignedAfterRender.ts rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{todo.invalid-reassign-local-variable-in-effect.expect.md => error.invalid-reassign-local-variable-in-effect.expect.md} (54%) rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{todo.invalid-reassign-local-variable-in-effect.js => error.invalid-reassign-local-variable-in-effect.js} (100%) rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{todo.invalid-reassign-local-variable-in-hook-argument.expect.md => error.invalid-reassign-local-variable-in-hook-argument.expect.md} (56%) rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{todo.invalid-reassign-local-variable-in-hook-argument.js => error.invalid-reassign-local-variable-in-hook-argument.js} (100%) rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{todo.invalid-reassign-local-variable-in-jsx-callback.expect.md => error.invalid-reassign-local-variable-in-jsx-callback.expect.md} (60%) rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{todo.invalid-reassign-local-variable-in-jsx-callback.js => error.invalid-reassign-local-variable-in-jsx-callback.js} (100%) diff --git a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts index 2e7613f0a2adf..5e33ed2b93c84 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts @@ -96,6 +96,7 @@ import { validatePreservedManualMemoization, validateUseMemo, } from "../Validation"; +import { validateLocalsNotReassignedAfterRender } from "../Validation/ValidateLocalsNotReassignedAfterRender"; export type CompilerPipelineValue = | { kind: "ast"; name: string; value: CodegenFunction } @@ -196,6 +197,8 @@ function* runWithEnvironment( validateNoCapitalizedCalls(hir); } + validateLocalsNotReassignedAfterRender(hir); + analyseFunctions(hir); yield log({ kind: "hir", name: "AnalyseFunctions", value: hir }); diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateLocalsNotReassignedAfterRender.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateLocalsNotReassignedAfterRender.ts new file mode 100644 index 0000000000000..c6e1213db6870 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateLocalsNotReassignedAfterRender.ts @@ -0,0 +1,182 @@ +/** + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +import prettyFormat from "pretty-format"; +import { CompilerError } from ".."; +import { + HIRFunction, + IdentifierId, + Place, + getHookKind, + isUseOperator, +} from "../HIR"; +import { + eachInstructionValueOperand, + eachTerminalOperand, +} from "../HIR/visitors"; +import { isEffectHook } from "./ValidateMemoizedEffectDependencies"; + +/** + * Validates that local variables cannot be reassigned after render. + * This prevents a category of bugs in which a closure captures a + * binding from one render but does not update + */ +export function validateLocalsNotReassignedAfterRender(fn: HIRFunction): void { + const contextVariables = new Set(); + const reassignment = getContextReassignment(fn, contextVariables, false); + if (reassignment !== null) { + CompilerError.throwInvalidJS({ + reason: + "This potentially reassigns a local variable after render has completed. Local variables may not be changed after render. ", + description: + reassignment.identifier.name !== null && + reassignment.identifier.name.kind === "named" + ? `Variable \`${reassignment.identifier.name.value}\` cannot be reassigned after render` + : "", + loc: reassignment.loc, + }); + } +} + +function getContextReassignment( + fn: HIRFunction, + contextVariables: Set, + isFunctionExpression: boolean +): Place | null { + const reassigningFunctions = new Map(); + for (const [, block] of fn.body.blocks) { + for (const instr of block.instructions) { + const { lvalue, value } = instr; + switch (value.kind) { + case "FunctionExpression": + case "ObjectMethod": { + const reassignment = getContextReassignment( + value.loweredFunc.func, + contextVariables, + true + ); + if (reassignment !== null) { + reassigningFunctions.set(lvalue.identifier.id, reassignment); + } + for (const operand of eachInstructionValueOperand(value)) { + const reassignment = reassigningFunctions.get( + operand.identifier.id + ); + if (reassignment !== undefined) { + reassigningFunctions.set(lvalue.identifier.id, reassignment); + break; + } + } + break; + } + case "StoreLocal": { + const reassignment = reassigningFunctions.get( + value.value.identifier.id + ); + if (reassignment !== undefined) { + reassigningFunctions.set( + value.lvalue.place.identifier.id, + reassignment + ); + reassigningFunctions.set(lvalue.identifier.id, reassignment); + } + break; + } + case "LoadLocal": { + const reassignment = reassigningFunctions.get( + value.place.identifier.id + ); + if (reassignment !== undefined) { + reassigningFunctions.set(lvalue.identifier.id, reassignment); + } + break; + } + case "ArrayExpression": + case "ObjectExpression": { + for (const operand of eachInstructionValueOperand(value)) { + const reassignment = reassigningFunctions.get( + operand.identifier.id + ); + if (reassignment !== undefined) { + reassigningFunctions.set(lvalue.identifier.id, reassignment); + break; + } + } + break; + } + case "DeclareContext": { + if (!isFunctionExpression) { + contextVariables.add(value.lvalue.place.identifier.id); + } + break; + } + case "StoreContext": { + if (isFunctionExpression) { + if (contextVariables.has(value.lvalue.place.identifier.id)) { + return value.lvalue.place; + } + } else { + contextVariables.add(value.lvalue.place.identifier.id); + } + break; + } + case "MethodCall": + case "CallExpression": { + const callee = + value.kind === "MethodCall" ? value.property : value.callee; + const isHook = + getHookKind(fn.env, callee.identifier) != null || + isUseOperator(callee.identifier); + for (const operand of eachInstructionValueOperand(value)) { + const reassignment = reassigningFunctions.get( + operand.identifier.id + ); + if (reassignment !== undefined) { + if (isHook) { + return reassignment; + } else { + // reassigningFunctions.set(lvalue.identifier.id, reassignment); + } + } + } + break; + } + case "JsxFragment": + case "JsxExpression": { + for (const operand of eachInstructionValueOperand(value)) { + const reassignment = reassigningFunctions.get( + operand.identifier.id + ); + if (reassignment !== undefined) { + reassigningFunctions.set(lvalue.identifier.id, reassignment); + } + } + break; + } + default: { + for (const operand of eachInstructionValueOperand(value)) { + const reassignment = reassigningFunctions.get( + operand.identifier.id + ); + if (reassignment !== undefined) { + reassigningFunctions.set(lvalue.identifier.id, reassignment); + break; + } + } + break; + } + } + } + for (const operand of eachTerminalOperand(block.terminal)) { + const reassignment = reassigningFunctions.get(operand.identifier.id); + if (reassignment !== undefined) { + return reassignment; + } + } + } + return null; +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo.invalid-reassign-local-variable-in-effect.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-reassign-local-variable-in-effect.expect.md similarity index 54% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo.invalid-reassign-local-variable-in-effect.expect.md rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-reassign-local-variable-in-effect.expect.md index ede6507782402..07b9d08f1d579 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo.invalid-reassign-local-variable-in-effect.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-reassign-local-variable-in-effect.expect.md @@ -43,56 +43,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")) { - t0 = (newValue) => { - local = newValue; - }; - $[0] = t0; - } else { - t0 = $[0]; - } - const reassignLocal = t0; - let t1; - if ($[1] === Symbol.for("react.memo_cache_sentinel")) { - t1 = (newValue_0) => { - reassignLocal("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 | + 6 | const reassignLocal = (newValue) => { +> 7 | local = newValue; + | ^^^^^ InvalidJS: This potentially reassigns a local variable after render has completed. Local variables may not be changed after render. . Variable `local` cannot be reassigned after render (7:7) + 8 | }; + 9 | + 10 | const onMount = (newValue) => { +``` + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo.invalid-reassign-local-variable-in-effect.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-reassign-local-variable-in-effect.js similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo.invalid-reassign-local-variable-in-effect.js rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-reassign-local-variable-in-effect.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo.invalid-reassign-local-variable-in-hook-argument.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-reassign-local-variable-in-hook-argument.expect.md similarity index 56% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo.invalid-reassign-local-variable-in-hook-argument.expect.md rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-reassign-local-variable-in-hook-argument.expect.md index e0da1fd2a0fd0..24431748e489d 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo.invalid-reassign-local-variable-in-hook-argument.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-reassign-local-variable-in-hook-argument.expect.md @@ -44,53 +44,17 @@ function Component() { ``` -## Code -```javascript -import { c as _c } from "react/compiler-runtime"; -import { useEffect } from "react"; -import { useIdentity } from "shared-runtime"; - -function Component() { - const $ = _c(3); - let local; - let t0; - if ($[0] === Symbol.for("react.memo_cache_sentinel")) { - t0 = (newValue) => { - local = newValue; - }; - $[0] = t0; - } else { - t0 = $[0]; - } - const reassignLocal = t0; - let t1; - if ($[1] === Symbol.for("react.memo_cache_sentinel")) { - t1 = (newValue_0) => { - reassignLocal("hello"); - if (local === newValue_0) { - console.log("`local` was updated!"); - } else { - throw new Error("`local` not updated!"); - } - }; - $[1] = t1; - } else { - t1 = $[1]; - } - const callback = t1; - let t2; - if ($[2] === Symbol.for("react.memo_cache_sentinel")) { - t2 = () => { - callback(); - }; - $[2] = t2; - } else { - t2 = $[2]; - } - useIdentity(t2); - return "ok"; -} +## Error ``` + 6 | + 7 | const reassignLocal = (newValue) => { +> 8 | local = newValue; + | ^^^^^ InvalidJS: This potentially reassigns a local variable after render has completed. Local variables may not be changed after render. . Variable `local` cannot be reassigned after render (8:8) + 9 | }; + 10 | + 11 | const callback = (newValue) => { +``` + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo.invalid-reassign-local-variable-in-hook-argument.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-reassign-local-variable-in-hook-argument.js similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo.invalid-reassign-local-variable-in-hook-argument.js rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-reassign-local-variable-in-hook-argument.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo.invalid-reassign-local-variable-in-jsx-callback.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-reassign-local-variable-in-jsx-callback.expect.md similarity index 60% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo.invalid-reassign-local-variable-in-jsx-callback.expect.md rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-reassign-local-variable-in-jsx-callback.expect.md index af2b89700abce..288773b8a5737 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo.invalid-reassign-local-variable-in-jsx-callback.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-reassign-local-variable-in-jsx-callback.expect.md @@ -37,41 +37,17 @@ function Component() { ``` -## Code -```javascript -import { c as _c } from "react/compiler-runtime"; -function Component() { - const $ = _c(2); - let local; - let t0; - if ($[0] === Symbol.for("react.memo_cache_sentinel")) { - t0 = (newValue) => { - local = newValue; - }; - $[0] = t0; - } else { - t0 = $[0]; - } - const reassignLocal = t0; - let t1; - if ($[1] === Symbol.for("react.memo_cache_sentinel")) { - const onClick = (newValue_0) => { - reassignLocal("hello"); - if (local === newValue_0) { - console.log("`local` was updated!"); - } else { - throw new Error("`local` not updated!"); - } - }; - - t1 = ; - $[1] = t1; - } else { - t1 = $[1]; - } - return t1; -} +## Error ``` + 3 | + 4 | const reassignLocal = (newValue) => { +> 5 | local = newValue; + | ^^^^^ InvalidJS: This potentially reassigns a local variable after render has completed. Local variables may not be changed after render. . Variable `local` cannot be reassigned after render (5:5) + 6 | }; + 7 | + 8 | const onClick = (newValue) => { +``` + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo.invalid-reassign-local-variable-in-jsx-callback.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-reassign-local-variable-in-jsx-callback.js similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo.invalid-reassign-local-variable-in-jsx-callback.js rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-reassign-local-variable-in-jsx-callback.js