Skip to content

Commit

Permalink
[compiler] Always error on async reassignments
Browse files Browse the repository at this point in the history
Summary: Addresses the issue in #30109: any mutation of a local in an async function may occur after rendering has finished.

ghstack-source-id: 9f15cf0f144c0badd6009ceb51df43a50399d82b
Pull Request resolved: #30111
  • Loading branch information
mvitousek committed Jun 27, 2024
1 parent 1f59d07 commit fcfbfc1
Show file tree
Hide file tree
Showing 5 changed files with 59 additions and 67 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,12 @@ import {
*/
export function validateLocalsNotReassignedAfterRender(fn: HIRFunction): void {
const contextVariables = new Set<IdentifierId>();
const reassignment = getContextReassignment(fn, contextVariables, false);
const reassignment = getContextReassignment(
fn,
contextVariables,
false,
false
);
if (reassignment !== null) {
CompilerError.throwInvalidReact({
reason:
Expand All @@ -37,7 +42,8 @@ export function validateLocalsNotReassignedAfterRender(fn: HIRFunction): void {
function getContextReassignment(
fn: HIRFunction,
contextVariables: Set<IdentifierId>,
isFunctionExpression: boolean
isFunctionExpression: boolean,
isAsync: boolean
): Place | null {
const reassigningFunctions = new Map<IdentifierId, Place>();
for (const [, block] of fn.body.blocks) {
Expand All @@ -49,7 +55,8 @@ function getContextReassignment(
let reassignment = getContextReassignment(
value.loweredFunc.func,
contextVariables,
true
true,
isAsync || value.loweredFunc.func.async
);
if (reassignment === null) {
// If the function itself doesn't reassign, does one of its dependencies?
Expand All @@ -65,6 +72,18 @@ function getContextReassignment(
}
// if the function or its depends reassign, propagate that fact on the lvalue
if (reassignment !== null) {
if (isAsync || value.loweredFunc.func.async) {
CompilerError.throwInvalidReact({
reason:
"Reassigning a variable in an async function 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,
});
}
reassigningFunctions.set(lvalue.identifier.id, reassignment);
}
break;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@

## Input

```javascript
function Component() {
let value = null;
const reassign = async () => {
await foo().then((result) => {
// Reassigning a local variable in an async function is *always* mutating
// after render, so this should error regardless of where this ends up
// getting called
value = result;
});
};

const onClick = async () => {
await reassign();
};
return <div onClick={onClick}>Click</div>;
}

```


## Error

```
6 | // after render, so this should error regardless of where this ends up
7 | // getting called
> 8 | value = result;
| ^^^^^ InvalidReact: Reassigning a variable in an async function can cause inconsistent behavior on subsequent renders. Consider using state instead. Variable `value` cannot be reassigned after render (8:8)
9 | });
10 | };
11 |
```

This file was deleted.

6 changes: 0 additions & 6 deletions compiler/packages/snap/src/SproutTodoFilter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -483,12 +483,6 @@ const skipFilter = new Set([
"rules-of-hooks/rules-of-hooks-93dc5d5e538a",
"rules-of-hooks/rules-of-hooks-69521d94fa03",

// should error
"todo.invalid-reassign-local-variable-in-jsx-callback",
"todo.invalid-reassign-local-variable-in-hook-argument",
"todo.invalid-reassign-local-variable-in-effect",
"todo.invalid-reassign-local-variable-in-async-callback",

// bugs
"bug-invalid-hoisting-functionexpr",
"original-reactive-scopes-fork/bug-nonmutating-capture-in-unsplittable-memo-block",
Expand Down

0 comments on commit fcfbfc1

Please sign in to comment.