-
Notifications
You must be signed in to change notification settings - Fork 47.2k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
[compiler] Validate against locals being reassigned after render
Adds a pass which validates that local variables are not reassigned by functions which may be called after render. This is a straightforward forward data-flow analysis, where we: 1. Build up a mapping of context variables in the outer component/hook 2. Find ObjectMethod/FunctionExpressions which may reassign those context variables 3. Propagate aliases of those functions via StoreLocal/LoadLocal 4. Disallow passing those functions with a Freeze effect. This includes JSX arguments, hook arguments, hook return types, etc. Conceptually, a function that reassigns a local is inherently mutable. Frozen functions must be side-effect free, so these two categories are incompatible and we can use the freeze effect to find all instances of where such functions are disallowed rather than special-casing eg hook calls and JSX. ghstack-source-id: c2b22e3d62a1ab490a6a2150e28b934b9dc8676b Pull Request resolved: #30107
- Loading branch information
1 parent
b1d10c8
commit 4c9a2d2
Showing
12 changed files
with
217 additions
and
134 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
146 changes: 146 additions & 0 deletions
146
...ages/babel-plugin-react-compiler/src/Validation/ValidateLocalsNotReassignedAfterRender.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,146 @@ | ||
| /** | ||
| * 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 { CompilerError, Effect } from ".."; | ||
| import { HIRFunction, IdentifierId, Place } from "../HIR"; | ||
| import { | ||
| eachInstructionValueOperand, | ||
| eachTerminalOperand, | ||
| } from "../HIR/visitors"; | ||
|
|
||
| /** | ||
| * 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<IdentifierId>(); | ||
| const reassignment = getContextReassignment(fn, contextVariables, false); | ||
| if (reassignment !== null) { | ||
| CompilerError.throwInvalidReact({ | ||
| reason: | ||
| "Reassigning a variable after render has completed can cause inconsistent behavior on subsequent renders. Consider using state instead", | ||
| 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<IdentifierId>, | ||
| isFunctionExpression: boolean | ||
| ): Place | null { | ||
| const reassigningFunctions = new Map<IdentifierId, Place>(); | ||
| for (const [, block] of fn.body.blocks) { | ||
| for (const instr of block.instructions) { | ||
| const { lvalue, value } = instr; | ||
| switch (value.kind) { | ||
| case "FunctionExpression": | ||
| case "ObjectMethod": { | ||
| let reassignment = getContextReassignment( | ||
| value.loweredFunc.func, | ||
| contextVariables, | ||
| true | ||
| ); | ||
| if (reassignment === null) { | ||
| // If the function itself doesn't reassign, does one of its dependencies? | ||
| for (const operand of eachInstructionValueOperand(value)) { | ||
| const reassignmentFromOperand = reassigningFunctions.get( | ||
| operand.identifier.id | ||
| ); | ||
| if (reassignmentFromOperand !== undefined) { | ||
| reassignment = reassignmentFromOperand; | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| // if the function or its depends reassign, propagate that fact on the lvalue | ||
| if (reassignment !== null) { | ||
| reassigningFunctions.set(lvalue.identifier.id, reassignment); | ||
| } | ||
| 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 "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 { | ||
| /* | ||
| * We only track reassignments of variables defined in the outer | ||
| * component or hook. | ||
| */ | ||
| contextVariables.add(value.lvalue.place.identifier.id); | ||
| } | ||
| break; | ||
| } | ||
| default: { | ||
| for (const operand of eachInstructionValueOperand(value)) { | ||
| CompilerError.invariant(operand.effect !== Effect.Unknown, { | ||
| reason: `Expected effects to be inferred prior to ValidateLocalsNotReassignedAfterRender`, | ||
| loc: operand.loc, | ||
| }); | ||
| const reassignment = reassigningFunctions.get( | ||
| operand.identifier.id | ||
| ); | ||
| if ( | ||
| reassignment !== undefined && | ||
| operand.effect === Effect.Freeze | ||
| ) { | ||
| /* | ||
| * 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; | ||
| } | ||
| } | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| for (const operand of eachTerminalOperand(block.terminal)) { | ||
| const reassignment = reassigningFunctions.get(operand.identifier.id); | ||
| if (reassignment !== undefined) { | ||
| return reassignment; | ||
| } | ||
| } | ||
| } | ||
| return null; | ||
| } |
27 changes: 27 additions & 0 deletions
27
..._/fixtures/compiler/error.invalid-reassign-local-in-hook-return-value.expect.md
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
|
|
||
| ## Input | ||
|
|
||
| ```javascript | ||
| function useFoo() { | ||
| let x = 0; | ||
| return (value) => { | ||
| x = value; | ||
| }; | ||
| } | ||
|
|
||
| ``` | ||
|
|
||
|
|
||
| ## Error | ||
|
|
||
| ``` | ||
| 2 | let x = 0; | ||
| 3 | return (value) => { | ||
| > 4 | x = value; | ||
| | ^ 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 | } | ||
| 7 | | ||
| ``` | ||
6 changes: 6 additions & 0 deletions
6
...iler/src/__tests__/fixtures/compiler/error.invalid-reassign-local-in-hook-return-value.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| function useFoo() { | ||
| let x = 0; | ||
| return (value) => { | ||
| x = value; | ||
| }; | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
File renamed without changes.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
File renamed without changes.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
File renamed without changes.
Oops, something went wrong.