Skip to content

Commit

Permalink
[compiler] Validate against locals being reassigned after render
Browse files Browse the repository at this point in the history
ghstack-source-id: d689a96e28742d9ae447e65256c918b8d430c7ad
Pull Request resolved: #30107
  • Loading branch information
josephsavona committed Jun 26, 2024
1 parent 363b8e4 commit 3723b07
Show file tree
Hide file tree
Showing 8 changed files with 215 additions and 129 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ import {
validatePreservedManualMemoization,
validateUseMemo,
} from "../Validation";
import { validateLocalsNotReassignedAfterRender } from "../Validation/ValidateLocalsNotReassignedAfterRender";

export type CompilerPipelineValue =
| { kind: "ast"; name: string; value: CodegenFunction }
Expand Down Expand Up @@ -196,6 +197,8 @@ function* runWithEnvironment(
validateNoCapitalizedCalls(hir);
}

validateLocalsNotReassignedAfterRender(hir);

analyseFunctions(hir);
yield log({ kind: "hir", name: "AnalyseFunctions", value: hir });

Expand Down
Original file line number Diff line number Diff line change
@@ -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<IdentifierId>();
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<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": {
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;
}
Original file line number Diff line number Diff line change
Expand Up @@ -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) => {
```
Original file line number Diff line number Diff line change
Expand Up @@ -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) => {
```
Original file line number Diff line number Diff line change
Expand Up @@ -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 = <button onClick={onClick}>Submit</button>;
$[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) => {
```

0 comments on commit 3723b07

Please sign in to comment.