-
Notifications
You must be signed in to change notification settings - Fork 47.6k
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: 88eb900061bb19edb66d9ef6bc471a0ba4ad76e8 Pull Request resolved: #30107
- Loading branch information
1 parent
85a6fbf
commit eda87ce
Showing
12 changed files
with
221 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
150 changes: 150 additions & 0 deletions
150
...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,150 @@ | ||
| /** | ||
| * 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, Effect } 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<IdentifierId>(); | ||
| const reassignment = getContextReassignment(fn, contextVariables, false); | ||
| if (reassignment !== null) { | ||
| CompilerError.throwInvalidJS({ | ||
| 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; | ||
| | ^ InvalidJS: 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.