Facebook/react
1
0
Fork 0
mirror of https://github.com/facebook/react.git synced 2025-11-01 09:12:30 +00:00
Code Issues Packages Projects Releases Wiki Activity

Allow global mutation within useEffect (#2646)

Browse Source 
Summary: Currently Forget bails on mutations to globals within any callback function. However, callbacks passed to useEffect should not bail and are not subject to the rules of react in the same way.

We allow this by instead of immediately raising errors when we see illegal writes, storing the error as part of the function. When the function is called, or passed to a position that could call it during rendering, we bail as before; but if it's passed to `useEffect`, we don't raise the errors.
This commit is contained in:
Michael Vitousek
2024-03-05 11:54:29 -08:00
parent f5d99baf8e
commit 22ea72d4e9
9 changed files with 349 additions and 51 deletions
Download Patch File Download Diff File Expand all files Collapse all files
compiler/packages/babel-plugin-react-forget/src/CompilerError.ts
+6
View File
@@ -165,6 +165,12 @@ export class CompilerError extends Error {
throw errors;
}
static throw(options: CompilerErrorDetailOptions): never {
const errors = new CompilerError();
errors.pushErrorDetail(new CompilerErrorDetail(options));
throw errors;
}
constructor(...args: any[]) {
super(...args);
this.name = "ReactForgetCompilerError";
compiler/packages/babel-plugin-react-forget/src/HIR/BuildHIR.ts
+1
View File
@@ -216,6 +216,7 @@ export function lower(
async: func.node.async === true,
loc: func.node.loc ?? GeneratedSource,
env,
effects: null,
});
}
compiler/packages/babel-plugin-react-forget/src/HIR/HIR.ts
+7 -1
View File
@@ -6,7 +6,7 @@
*/
import * as t from "@babel/types";
import { CompilerError } from "../CompilerError";
import { CompilerError, CompilerErrorDetailOptions } from "../CompilerError";
import { assertExhaustive } from "../Utils/utils";
import { Environment } from "./Environment";
import { HookKind } from "./ObjectShape";
@@ -244,11 +244,17 @@ export type HIRFunction = {
params: Array<Place | SpreadPattern>;
returnType: t.FlowType | t.TSType | null;
context: Array<Place>;
effects: Array<FunctionEffect> | null;
body: HIR;
generator: boolean;
async: boolean;
};
export type FunctionEffect = {
kind: "GlobalMutation";
error: CompilerErrorDetailOptions;
};
/*
* Each reactive scope may have its own control-flow, so the instructions form
* a control-flow graph. The graph comprises a set of basic blocks which reference
compiler/packages/babel-plugin-react-forget/src/Inference/InferReferenceEffects.ts
+232 -49
View File
@@ -5,7 +5,7 @@
* LICENSE file in the root directory of this source tree.
*/
import { CompilerError } from "../CompilerError";
import { CompilerError, ErrorSeverity } from "../CompilerError";
import { Environment } from "../HIR";
import {
AbstractValue,
@@ -13,6 +13,7 @@ import {
BlockId,
CallExpression,
Effect,
FunctionEffect,
GeneratedSource,
HIRFunction,
IdentifierId,
@@ -43,6 +44,7 @@ import {
eachTerminalSuccessor,
} from "../HIR/visitors";
import { assertExhaustive } from "../Utils/utils";
import { isEffectHook } from "../Validation/ValidateMemoizedEffectDependencies";
const UndefinedValue: InstructionValue = {
kind: "Primitive",
@@ -206,6 +208,8 @@ export default function inferReferenceEffects(
}
queue(fn.body.entry, initialState);
const functionEffects: Array<FunctionEffect> = fn.effects ?? [];
while (queuedStates.size !== 0) {
for (const [blockId, block] of fn.body.blocks) {
const incomingState = queuedStates.get(blockId);
@@ -216,13 +220,29 @@ export default function inferReferenceEffects(
statesByBlock.set(blockId, incomingState);
const state = incomingState.clone();
inferBlock(fn.env, state, block);
inferBlock(fn.env, functionEffects, state, block);
for (const nextBlockId of eachTerminalSuccessor(block.terminal)) {
queue(nextBlockId, state);
}
}
}
if (!options.isFunctionExpression) {
functionEffects.forEach((eff) => {
switch (eff.kind) {
case "GlobalMutation":
CompilerError.throw(eff.error);
default:
assertExhaustive(
eff.kind,
`Unexpected function effect kind '${eff.kind}'`
);
}
});
} else {
fn.effects = functionEffects;
}
}
// Maintains a mapping of top-level variables to the kind of value they hold
@@ -340,7 +360,12 @@ class InferenceState {
* Similarly, a freeze reference is converted to readonly if the
* value is already frozen or is immutable.
*/
reference(place: Place, effectKind: Effect, reason: ValueReason): void {
reference(
place: Place,
functionEffects: Array<FunctionEffect>,
effectKind: Effect,
reason: ValueReason
): void {
const values = this.#variables.get(place.identifier.id);
if (values === undefined) {
CompilerError.invariant(effectKind !== Effect.Store, {
@@ -355,6 +380,16 @@ class InferenceState {
: Effect.Read;
return;
}
for (const value of values) {
if (
(value.kind === "FunctionExpression" ||
value.kind === "ObjectMethod") &&
value.loweredFunc.func.effects != null
) {
functionEffects.push(...value.loweredFunc.func.effects);
}
}
let valueKind: AbstractValue | null = this.kind(place);
let effect: Effect | null = null;
switch (effectKind) {
@@ -382,7 +417,12 @@ class InferenceState {
) {
if (value.kind === "FunctionExpression") {
for (const operand of eachInstructionValueOperand(value)) {
this.reference(operand, Effect.Freeze, ValueReason.Other);
this.reference(
operand,
functionEffects,
Effect.Freeze,
ValueReason.Other
);
}
}
}
@@ -405,22 +445,25 @@ class InferenceState {
}
case Effect.Mutate: {
if (
valueKind.kind === ValueKind.Mutable ||
valueKind.kind === ValueKind.Context
valueKind.kind !== ValueKind.Mutable &&
valueKind.kind !== ValueKind.Context
) {
effect = Effect.Mutate;
} else {
let reason = getWriteErrorReason(valueKind);
CompilerError.throwInvalidReact({
reason,
description:
place.identifier.name !== null
? `Found mutation of ${place.identifier.name}`
: null,
loc: place.loc,
suggestions: null,
functionEffects.push({
kind: "GlobalMutation",
error: {
reason,
description:
place.identifier.name !== null
? `Found mutation of ${place.identifier.name}`
: null,
loc: place.loc,
suggestions: null,
severity: ErrorSeverity.InvalidReact,
},
});
}
effect = Effect.Mutate;
break;
}
case Effect.Store: {
@@ -429,15 +472,18 @@ class InferenceState {
valueKind.kind !== ValueKind.Context
) {
let reason = getWriteErrorReason(valueKind);
CompilerError.throwInvalidReact({
reason,
description:
place.identifier.name !== null
? `Found mutation of ${place.identifier.name}`
: null,
loc: place.loc,
suggestions: null,
functionEffects.push({
kind: "GlobalMutation",
error: {
reason,
description:
place.identifier.name !== null
? `Found mutation of ${place.identifier.name}`
: null,
loc: place.loc,
suggestions: null,
severity: ErrorSeverity.InvalidReact,
},
});
}
@@ -767,6 +813,7 @@ function mergeAbstractValues(
*/
function inferBlock(
env: Environment,
functionEffects: Array<FunctionEffect>,
state: InferenceState,
block: BasicBlock
): void {
@@ -828,6 +875,7 @@ function inferBlock(
// Object keys must be primitives, so we know they're frozen at this point
state.reference(
property.key.name,
functionEffects,
Effect.Freeze,
ValueReason.Other
);
@@ -835,6 +883,7 @@ function inferBlock(
// Object construction captures but does not modify the key/property values
state.reference(
property.place,
functionEffects,
Effect.Capture,
ValueReason.Other
);
@@ -844,6 +893,7 @@ function inferBlock(
// Object construction captures but does not modify the key/property values
state.reference(
property.place,
functionEffects,
Effect.Capture,
ValueReason.Other
);
@@ -954,6 +1004,7 @@ function inferBlock(
for (const operand of eachInstructionOperand(instr)) {
state.reference(
operand,
functionEffects,
operand.effect === Effect.Unknown ? Effect.Read : operand.effect,
ValueReason.Other
);
@@ -990,29 +1041,49 @@ function inferBlock(
}
: { kind: ValueKind.Mutable, reason: new Set([ValueReason.Other]) };
let hasCaptureArgument = false;
let isUseEffect = isEffectHook(instrValue.callee.identifier);
for (let i = 0; i < instrValue.args.length; i++) {
const argumentEffects: Array<FunctionEffect> = [];
const arg = instrValue.args[i];
const place = arg.kind === "Identifier" ? arg : arg.place;
if (effects !== null) {
state.reference(place, effects[i], ValueReason.Other);
state.reference(
place,
argumentEffects,
effects[i],
ValueReason.Other
);
} else {
state.reference(
place,
argumentEffects,
Effect.ConditionallyMutate,
ValueReason.Other
);
}
/*
* Join the effects of the argument with the effects of the enclosing function,
* unless the we're detecting a global mutation inside a useEffect hook
*/
functionEffects.push(
...argumentEffects.filter(
(argEffect) =>
!isUseEffect || i !== 0 || argEffect.kind !== "GlobalMutation"
)
);
hasCaptureArgument ||= place.effect === Effect.Capture;
}
if (signature !== null) {
state.reference(
instrValue.callee,
functionEffects,
signature.calleeEffect,
ValueReason.Other
);
} else {
state.reference(
instrValue.callee,
functionEffects,
Effect.ConditionallyMutate,
ValueReason.Other
);
@@ -1034,7 +1105,12 @@ function inferBlock(
loc: instrValue.loc,
suggestions: null,
});
state.reference(instrValue.property, Effect.Read, ValueReason.Other);
state.reference(
instrValue.property,
functionEffects,
Effect.Read,
ValueReason.Other
);
const signature = getFunctionCallSignature(
env,
@@ -1060,10 +1136,16 @@ function inferBlock(
*/
for (const arg of instrValue.args) {
const place = arg.kind === "Identifier" ? arg : arg.place;
state.reference(place, Effect.Read, ValueReason.Other);
state.reference(
place,
functionEffects,
Effect.Read,
ValueReason.Other
);
}
state.reference(
instrValue.receiver,
functionEffects,
Effect.Capture,
ValueReason.Other
);
@@ -1087,10 +1169,16 @@ function inferBlock(
* If effects are inferred for an argument, we should fail invalid
* mutating effects
*/
state.reference(place, effects[i], ValueReason.Other);
state.reference(
place,
functionEffects,
effects[i],
ValueReason.Other
);
} else {
state.reference(
place,
functionEffects,
Effect.ConditionallyMutate,
ValueReason.Other
);
@@ -1100,12 +1188,14 @@ function inferBlock(
if (signature !== null) {
state.reference(
instrValue.receiver,
functionEffects,
signature.calleeEffect,
ValueReason.Other
);
} else {
state.reference(
instrValue.receiver,
functionEffects,
Effect.ConditionallyMutate,
ValueReason.Other
);
@@ -1124,8 +1214,18 @@ function inferBlock(
state.kind(instrValue.object).kind === ValueKind.Context
? Effect.ConditionallyMutate
: Effect.Capture;
state.reference(instrValue.value, effect, ValueReason.Other);
state.reference(instrValue.object, Effect.Store, ValueReason.Other);
state.reference(
instrValue.value,
functionEffects,
effect,
ValueReason.Other
);
state.reference(
instrValue.object,
functionEffects,
Effect.Store,
ValueReason.Other
);
const lvalue = instr.lvalue;
state.alias(lvalue, instrValue.value);
@@ -1142,7 +1242,12 @@ function inferBlock(
break;
}
case "PropertyLoad": {
state.reference(instrValue.object, Effect.Read, ValueReason.Other);
state.reference(
instrValue.object,
functionEffects,
Effect.Read,
ValueReason.Other
);
const lvalue = instr.lvalue;
lvalue.effect = Effect.ConditionallyMutate;
state.initialize(instrValue, state.kind(instrValue.object));
@@ -1154,9 +1259,24 @@ function inferBlock(
state.kind(instrValue.object).kind === ValueKind.Context
? Effect.ConditionallyMutate
: Effect.Capture;
state.reference(instrValue.value, effect, ValueReason.Other);
state.reference(instrValue.property, Effect.Capture, ValueReason.Other);
state.reference(instrValue.object, Effect.Store, ValueReason.Other);
state.reference(
instrValue.value,
functionEffects,
effect,
ValueReason.Other
);
state.reference(
instrValue.property,
functionEffects,
Effect.Capture,
ValueReason.Other
);
state.reference(
instrValue.object,
functionEffects,
Effect.Store,
ValueReason.Other
);
const lvalue = instr.lvalue;
state.alias(lvalue, instrValue.value);
@@ -1164,8 +1284,18 @@ function inferBlock(
continue;
}
case "ComputedDelete": {
state.reference(instrValue.object, Effect.Mutate, ValueReason.Other);
state.reference(instrValue.property, Effect.Read, ValueReason.Other);
state.reference(
instrValue.object,
functionEffects,
Effect.Mutate,
ValueReason.Other
);
state.reference(
instrValue.property,
functionEffects,
Effect.Read,
ValueReason.Other
);
state.initialize(instrValue, {
kind: ValueKind.Immutable,
reason: new Set([ValueReason.Other]),
@@ -1175,8 +1305,18 @@ function inferBlock(
continue;
}
case "ComputedLoad": {
state.reference(instrValue.object, Effect.Read, ValueReason.Other);
state.reference(instrValue.property, Effect.Read, ValueReason.Other);
state.reference(
instrValue.object,
functionEffects,
Effect.Read,
ValueReason.Other
);
state.reference(
instrValue.property,
functionEffects,
Effect.Read,
ValueReason.Other
);
const lvalue = instr.lvalue;
lvalue.effect = Effect.ConditionallyMutate;
state.initialize(instrValue, state.kind(instrValue.object));
@@ -1192,6 +1332,7 @@ function inferBlock(
*/
state.reference(
instrValue.value,
functionEffects,
Effect.ConditionallyMutate,
ValueReason.Other
);
@@ -1210,7 +1351,12 @@ function inferBlock(
* ```
*/
state.initialize(instrValue, state.kind(instrValue.value));
state.reference(instrValue.value, Effect.Read, ValueReason.Other);
state.reference(
instrValue.value,
functionEffects,
Effect.Read,
ValueReason.Other
);
const lvalue = instr.lvalue;
lvalue.effect = Effect.ConditionallyMutate;
state.alias(lvalue, instrValue.value);
@@ -1218,9 +1364,19 @@ function inferBlock(
}
case "Memoize": {
if (env.config.enablePreserveExistingMemoizationGuarantees) {
state.reference(instrValue.value, Effect.Freeze, ValueReason.Other);
state.reference(
instrValue.value,
functionEffects,
Effect.Freeze,
ValueReason.Other
);
} else {
state.reference(instrValue.value, Effect.Read, ValueReason.Other);
state.reference(
instrValue.value,
functionEffects,
Effect.Read,
ValueReason.Other
);
}
const lvalue = instr.lvalue;
lvalue.effect = Effect.ConditionallyMutate;
@@ -1238,14 +1394,24 @@ function inferBlock(
state.kind(lvalue).kind === ValueKind.Context
? Effect.ConditionallyMutate
: Effect.Capture;
state.reference(instrValue.place, effect, ValueReason.Other);
state.reference(
instrValue.place,
functionEffects,
effect,
ValueReason.Other
);
lvalue.effect = Effect.ConditionallyMutate;
// direct aliasing: `a = b`;
state.alias(lvalue, instrValue.place);
continue;
}
case "LoadContext": {
state.reference(instrValue.place, Effect.Capture, ValueReason.Other);
state.reference(
instrValue.place,
functionEffects,
Effect.Capture,
ValueReason.Other
);
const lvalue = instr.lvalue;
lvalue.effect = Effect.ConditionallyMutate;
const valueKind = state.kind(instrValue.place);
@@ -1297,7 +1463,12 @@ function inferBlock(
state.kind(instrValue.lvalue).kind === ValueKind.Context
? Effect.ConditionallyMutate
: Effect.Capture;
state.reference(instrValue.value, effect, ValueReason.Other);
state.reference(
instrValue.value,
functionEffects,
effect,
ValueReason.Other
);
const lvalue = instr.lvalue;
state.alias(lvalue, instrValue.value);
@@ -1318,7 +1489,12 @@ function inferBlock(
state.kind(instrValue.lvalue.place).kind === ValueKind.Context
? Effect.ConditionallyMutate
: Effect.Capture;
state.reference(instrValue.value, effect, ValueReason.Other);
state.reference(
instrValue.value,
functionEffects,
effect,
ValueReason.Other
);
const lvalue = instr.lvalue;
state.alias(lvalue, instrValue.value);
@@ -1336,11 +1512,13 @@ function inferBlock(
case "StoreContext": {
state.reference(
instrValue.value,
functionEffects,
Effect.ConditionallyMutate,
ValueReason.Other
);
state.reference(
instrValue.lvalue.place,
functionEffects,
Effect.Mutate,
ValueReason.Other
);
@@ -1361,7 +1539,12 @@ function inferBlock(
break;
}
}
state.reference(instrValue.value, effect, ValueReason.Other);
state.reference(
instrValue.value,
functionEffects,
effect,
ValueReason.Other
);
const lvalue = instr.lvalue;
state.alias(lvalue, instrValue.value);
@@ -1408,7 +1591,7 @@ function inferBlock(
loc: instrValue.loc,
suggestions: null,
});
state.reference(operand, effect.kind, effect.reason);
state.reference(operand, functionEffects, effect.kind, effect.reason);
}
state.initialize(instrValue, valueKind);
@@ -1430,7 +1613,7 @@ function inferBlock(
} else {
effect = Effect.Read;
}
state.reference(operand, effect, ValueReason.Other);
state.reference(operand, functionEffects, effect, ValueReason.Other);
}
}
compiler/packages/babel-plugin-react-forget/src/Validation/ValidateMemoizedEffectDependencies.ts
+1 -1
View File
@@ -119,7 +119,7 @@ function isUnmemoized(operand: Identifier, scopes: Set<ScopeId>): boolean {
return operand.scope != null && !scopes.has(operand.scope.id);
}
function isEffectHook(identifier: Identifier): boolean {
export function isEffectHook(identifier: Identifier): boolean {
return (
isUseEffectHookType(identifier) ||
isUseLayoutEffectHookType(identifier) ||
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.not-useEffect-external-mutate.expect.md
+29
View File
@@ -0,0 +1,29 @@
## Input
```javascript
let x = { a: 42 };
function Component(props) {
foo(() => {
x.a = 10;
x.a = 20;
});
}
```
## Error
```
3 | function Component(props) {
4 | foo(() => {
> 5 | x.a = 10;
| ^ [ReactForget] InvalidReact: Writing to a variable defined outside a component or hook is not allowed. Consider using an effect. (5:5)
6 | x.a = 20;
7 | });
8 | }
```
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.not-useEffect-external-mutate.js
+8
View File
@@ -0,0 +1,8 @@
let x = { a: 42 };
function Component(props) {
foo(() => {
x.a = 10;
x.a = 20;
});
}
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/useEffect-external-mutate.expect.md
+51
View File
@@ -0,0 +1,51 @@
## Input
```javascript
import { useEffect } from "react";
let x = { a: 42 };
function Component(props) {
useEffect(() => {
x.a = 10;
});
}
export const FIXTURE_ENTRYPOINT = {
fn: Component,
params: [],
};
```
## Code
```javascript
import { useEffect, unstable_useMemoCache as useMemoCache } from "react";
let x = { a: 42 };
function Component(props) {
const $ = useMemoCache(1);
let t0;
if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
t0 = () => {
x.a = 10;
};
$[0] = t0;
} else {
t0 = $[0];
}
useEffect(t0);
}
export const FIXTURE_ENTRYPOINT = {
fn: Component,
params: [],
};
```
### Eval output
(kind: ok)
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/useEffect-external-mutate.js
+14
View File
@@ -0,0 +1,14 @@
import { useEffect } from "react";
let x = { a: 42 };
function Component(props) {
useEffect(() => {
x.a = 10;
});
}
export const FIXTURE_ENTRYPOINT = {
fn: Component,
params: [],
};