Skip to content

Commit 8b8087f

Browse files
josephsavonajosephsavona
committed
[compiler][newinference] Improve hoisted functions, validation of mutate-after-render
Updates ValidateNoFreezingMutableFunctions to account for indirections, where the frozen function calls another function that actually does the mutation. We just need to propagate on the mutation. Also some fixes for hoisted functions, DeclareContext can actually appear twice, so we need to check that subsequent (re)declarations are treated as a mutation, not a creation. ghstack-source-id: 8b05494 Pull Request resolved: #33470
1 parent 2d3c85b commit 8b8087f

File tree

7 files changed

+225
-26
lines changed

7 files changed

+225
-26
lines changed

‎compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -247,6 +247,7 @@ function runWithEnvironment(
247247
}
248248

249249
if (!env.config.enableNewMutationAliasingModel) {
250+
// NOTE: in the new model this is part of validateNoFreezingKnownMutableFunctions
250251
validateLocalsNotReassignedAfterRender(hir);
251252
}
252253

‎compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts‎

Lines changed: 56 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -527,7 +527,11 @@ function applyEffect(
527527
}
528528
case 'CreateFunction': {
529529
effects.push(effect);
530-
const isMutable = effect.captures.some(capture => {
530+
/**
531+
* We consider the function mutable if it has any mutable context variables or
532+
* any side-effects that need to be tracked if the function is called.
533+
*/
534+
const hasCaptures = effect.captures.some(capture => {
531535
switch (state.kind(capture).kind) {
532536
case ValueKind.Context:
533537
case ValueKind.Mutable: {
@@ -538,6 +542,12 @@ function applyEffect(
538542
}
539543
}
540544
});
545+
const hasTrackedSideEffects =
546+
effect.function.loweredFunc.func.aliasingEffects?.some(
547+
effect =>
548+
effect.kind === 'MutateFrozen' || effect.kind === 'MutateGlobal',
549+
);
550+
const isMutable = hasCaptures || hasTrackedSideEffects;
541551
for (const operand of effect.function.loweredFunc.func.context) {
542552
if (operand.effect !== Effect.Capture) {
543553
continue;
@@ -1593,23 +1603,6 @@ function computeSignatureForInstruction(
15931603
}
15941604
break;
15951605
}
1596-
case 'DeclareContext': {
1597-
// Context variables are conceptually like mutable boxes
1598-
effects.push({
1599-
kind: 'Create',
1600-
into: value.lvalue.place,
1601-
value: ValueKind.Mutable,
1602-
reason: ValueReason.Other,
1603-
});
1604-
effects.push({
1605-
kind: 'Create',
1606-
into: lvalue,
1607-
// The result can't be referenced so this value doesn't matter
1608-
value: ValueKind.Primitive,
1609-
reason: ValueReason.Other,
1610-
});
1611-
break;
1612-
}
16131606
case 'DeclareLocal': {
16141607
// TODO check this
16151608
effects.push({
@@ -1648,6 +1641,43 @@ function computeSignatureForInstruction(
16481641
effects.push({kind: 'CreateFrom', from: value.place, into: lvalue});
16491642
break;
16501643
}
1644+
case 'DeclareContext': {
1645+
// Context variables are conceptually like mutable boxes
1646+
const kind = value.lvalue.kind;
1647+
if (
1648+
!context.hoistedContextDeclarations.has(
1649+
value.lvalue.place.identifier.declarationId,
1650+
) ||
1651+
kind === InstructionKind.HoistedConst ||
1652+
kind === InstructionKind.HoistedFunction ||
1653+
kind === InstructionKind.HoistedLet
1654+
) {
1655+
/**
1656+
* If this context variable is not hoisted, or this is the declaration doing the hoisting,
1657+
* then we create the box.
1658+
*/
1659+
effects.push({
1660+
kind: 'Create',
1661+
into: value.lvalue.place,
1662+
value: ValueKind.Mutable,
1663+
reason: ValueReason.Other,
1664+
});
1665+
} else {
1666+
/**
1667+
* Otherwise this may be a "declare", but there was a previous DeclareContext that
1668+
* hoisted this variable, and we're mutating it here.
1669+
*/
1670+
effects.push({kind: 'Mutate', value: value.lvalue.place});
1671+
}
1672+
effects.push({
1673+
kind: 'Create',
1674+
into: lvalue,
1675+
// The result can't be referenced so this value doesn't matter
1676+
value: ValueKind.Primitive,
1677+
reason: ValueReason.Other,
1678+
});
1679+
break;
1680+
}
16511681
case 'StoreContext': {
16521682
/*
16531683
* Context variables are like mutable boxes, so semantically
@@ -1921,6 +1951,14 @@ function computeEffectsForLegacySignature(
19211951
arg.kind === 'Identifier' && i < signature.positionalParams.length
19221952
? signature.positionalParams[i]!
19231953
: (signature.restParam ?? Effect.ConditionallyMutate);
1954+
1955+
if (arg.kind === 'Spread' && effect === Effect.Freeze) {
1956+
CompilerError.throwTodo({
1957+
reason: 'Support spread syntax for hook arguments',
1958+
loc: arg.place.loc,
1959+
});
1960+
}
1961+
19241962
visit(place, effect);
19251963
}
19261964
if (captures.length !== 0) {

‎compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingRanges.ts‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -321,10 +321,6 @@ export function inferMutationAliasingRanges(
321321
}
322322
break;
323323
}
324-
case 'ImmutableCapture': {
325-
operandEffects.set(effect.from.identifier.id, Effect.Read);
326-
break;
327-
}
328324
case 'CreateFunction':
329325
case 'Create': {
330326
break;
@@ -352,6 +348,10 @@ export function inferMutationAliasingRanges(
352348
operandEffects.set(effect.value.identifier.id, Effect.Freeze);
353349
break;
354350
}
351+
case 'ImmutableCapture': {
352+
// no-op, Read is the default
353+
break;
354+
}
355355
case 'MutateFrozen':
356356
case 'MutateGlobal': {
357357
// no-op

‎compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoFreezingKnownMutableFunctions.ts‎

Lines changed: 23 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -58,8 +58,7 @@ export function validateNoFreezingKnownMutableFunctions(
5858
const effect = contextMutationEffects.get(operand.identifier.id);
5959
if (effect != null) {
6060
errors.push({
61-
reason: `This argument is a function which modifies local variables when called, which can bypass memoization and cause the UI not to update`,
62-
description: `Functions that are returned from hooks, passed as arguments to hooks, or passed as props to components may not mutate local variables`,
61+
reason: `This argument is a function which may reassign or mutate local variables after render, which can cause inconsistent behavior on subsequent renders. Consider using state instead`,
6362
loc: operand.loc,
6463
severity: ErrorSeverity.InvalidReact,
6564
});
@@ -124,7 +123,15 @@ export function validateNoFreezingKnownMutableFunctions(
124123
switch (effect.kind) {
125124
case 'Mutate':
126125
case 'MutateTransitive': {
127-
if (context.has(effect.value.identifier.id)) {
126+
const knownMutation = contextMutationEffects.get(
127+
effect.value.identifier.id,
128+
);
129+
if (knownMutation != null) {
130+
contextMutationEffects.set(
131+
lvalue.identifier.id,
132+
knownMutation,
133+
);
134+
} else if (context.has(effect.value.identifier.id)) {
128135
contextMutationEffects.set(lvalue.identifier.id, {
129136
kind: 'ContextMutation',
130137
effect: Effect.Mutate,
@@ -135,6 +142,19 @@ export function validateNoFreezingKnownMutableFunctions(
135142
}
136143
break;
137144
}
145+
case 'MutateConditionally':
146+
case 'MutateTransitiveConditionally': {
147+
const knownMutation = contextMutationEffects.get(
148+
effect.value.identifier.id,
149+
);
150+
if (knownMutation != null) {
151+
contextMutationEffects.set(
152+
lvalue.identifier.id,
153+
knownMutation,
154+
);
155+
}
156+
break;
157+
}
138158
}
139159
}
140160
}

‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.mutable-range-shared-inner-outer-function.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ function Component(props) {
1414
a.property = true;
1515
b.push(false);
1616
};
17-
return <div onClick={f()} />;
17+
return <div onClick={f} />;
1818
}
1919

2020
export const FIXTURE_ENTRYPOINT = {

‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/error.invalid-reassign-local-variable-in-jsx-callback.expect.md‎

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,108 @@
1+
2+
## Input
3+
4+
```javascript
5+
function Component() {
6+
let local;
7+
8+
const reassignLocal = newValue => {
9+
local = newValue;
10+
};
11+
12+
const onClick = newValue => {
13+
reassignLocal('hello');
14+
15+
if (local === newValue) {
16+
// Without React Compiler, `reassignLocal` is freshly created
17+
// on each render, capturing a binding to the latest `local`,
18+
// such that invoking reassignLocal will reassign the same
19+
// binding that we are observing in the if condition, and
20+
// we reach this branch
21+
console.log('`local` was updated!');
22+
} else {
23+
// With React Compiler enabled, `reassignLocal` is only created
24+
// once, capturing a binding to `local` in that render pass.
25+
// Therefore, calling `reassignLocal` will reassign the wrong
26+
// version of `local`, and not update the binding we are checking
27+
// in the if condition.
28+
//
29+
// To protect against this, we disallow reassigning locals from
30+
// functions that escape
31+
throw new Error('`local` not updated!');
32+
}
33+
};
34+
35+
return <button onClick={onClick}>Submit</button>;
36+
}
37+
38+
```
39+
40+
41+
## Error
42+
43+
```
44+
29 | };
45+
30 |
46+
> 31 | return <button onClick={onClick}>Submit</button>;
47+
| ^^^^^^^ InvalidReact: This argument is a function which may reassign or mutate local variables after render, which can cause inconsistent behavior on subsequent renders. Consider using state instead (31:31)
48+
49+
InvalidReact: The function modifies a local variable here (5:5)
50+
32 | }
51+
33 |
52+
53+
ReactCompilerError: InvalidReact: This argument is a function which may reassign or mutate local variables after render, which can cause inconsistent behavior on subsequent renders. Consider using state instead (31:31)
54+
55+
InvalidReact: The function modifies a local variable here (5:5)
56+
at validateNoFreezingKnownMutableFunctions (/Users/joesavona/github/react/compiler/packages/babel-plugin-react-compiler/dist/index.js:113829:18)
57+
at runWithEnvironment (/Users/joesavona/github/react/compiler/packages/babel-plugin-react-compiler/dist/index.js:114076:7)
58+
at run (/Users/joesavona/github/react/compiler/packages/babel-plugin-react-compiler/dist/index.js:113959:10)
59+
at compileFn (/Users/joesavona/github/react/compiler/packages/babel-plugin-react-compiler/dist/index.js:114301:10)
60+
at tryCompileFunction (/Users/joesavona/github/react/compiler/packages/babel-plugin-react-compiler/dist/index.js:114793:19)
61+
at processFn (/Users/joesavona/github/react/compiler/packages/babel-plugin-react-compiler/dist/index.js:114730:25)
62+
at compileProgram (/Users/joesavona/github/react/compiler/packages/babel-plugin-react-compiler/dist/index.js:114638:22)
63+
at PluginPass.enter (/Users/joesavona/github/react/compiler/packages/babel-plugin-react-compiler/dist/index.js:115773:26)
64+
at newFn (/Users/joesavona/github/react/compiler/node_modules/@babel/traverse/lib/visitors.js:172:14)
65+
at NodePath._call (/Users/joesavona/github/react/compiler/node_modules/@babel/traverse/lib/path/context.js:49:20)
66+
at NodePath.call (/Users/joesavona/github/react/compiler/node_modules/@babel/traverse/lib/path/context.js:39:18)
67+
at NodePath.visit (/Users/joesavona/github/react/compiler/node_modules/@babel/traverse/lib/path/context.js:88:31)
68+
at TraversalContext.visitQueue (/Users/joesavona/github/react/compiler/node_modules/@babel/traverse/lib/context.js:90:16)
69+
at TraversalContext.visitSingle (/Users/joesavona/github/react/compiler/node_modules/@babel/traverse/lib/context.js:66:19)
70+
at TraversalContext.visit (/Users/joesavona/github/react/compiler/node_modules/@babel/traverse/lib/context.js:113:19)
71+
at traverseNode (/Users/joesavona/github/react/compiler/node_modules/@babel/traverse/lib/traverse-node.js:22:17)
72+
at traverse (/Users/joesavona/github/react/compiler/node_modules/@babel/traverse/lib/index.js:53:34)
73+
at transformFile (/Users/joesavona/github/react/compiler/node_modules/@babel/core/lib/transformation/index.js:80:31)
74+
at transformFile.next (<anonymous>)
75+
at run (/Users/joesavona/github/react/compiler/node_modules/@babel/core/lib/transformation/index.js:25:12)
76+
at run.next (<anonymous>)
77+
at /Users/joesavona/github/react/compiler/node_modules/@babel/core/lib/transform-ast.js:23:33
78+
at Generator.next (<anonymous>)
79+
at evaluateSync (/Users/joesavona/github/react/compiler/node_modules/gensync/index.js:251:28)
80+
at sync (/Users/joesavona/github/react/compiler/node_modules/gensync/index.js:89:14)
81+
at stopHiding - secret - don't use this - v1 (/Users/joesavona/github/react/compiler/node_modules/@babel/core/lib/errors/rewrite-stack-trace.js:47:12)
82+
at transformFromAstSync (/Users/joesavona/github/react/compiler/node_modules/@babel/core/lib/transform-ast.js:43:83)
83+
at /Users/joesavona/github/react/compiler/packages/snap/dist/compiler.js:204:62
84+
at Generator.next (<anonymous>)
85+
at /Users/joesavona/github/react/compiler/packages/snap/dist/compiler.js:37:71
86+
at new Promise (<anonymous>)
87+
at __awaiter (/Users/joesavona/github/react/compiler/packages/snap/dist/compiler.js:33:12)
88+
at transformFixtureInput (/Users/joesavona/github/react/compiler/packages/snap/dist/compiler.js:186:12)
89+
at /Users/joesavona/github/react/compiler/packages/snap/dist/runner-worker.js:97:71
90+
at Generator.next (<anonymous>)
91+
at /Users/joesavona/github/react/compiler/packages/snap/dist/runner-worker.js:14:71
92+
at new Promise (<anonymous>)
93+
at __awaiter (/Users/joesavona/github/react/compiler/packages/snap/dist/runner-worker.js:10:12)
94+
at compile (/Users/joesavona/github/react/compiler/packages/snap/dist/runner-worker.js:44:12)
95+
at Object.<anonymous> (/Users/joesavona/github/react/compiler/packages/snap/dist/runner-worker.js:163:48)
96+
at Generator.next (<anonymous>)
97+
at /Users/joesavona/github/react/compiler/packages/snap/dist/runner-worker.js:14:71
98+
at new Promise (<anonymous>)
99+
at __awaiter (/Users/joesavona/github/react/compiler/packages/snap/dist/runner-worker.js:10:12)
100+
at Object.transformFixture (/Users/joesavona/github/react/compiler/packages/snap/dist/runner-worker.js:148:12)
101+
at execFunction (/Users/joesavona/github/react/compiler/node_modules/jest-worker/build/workers/threadChild.js:148:17)
102+
at execHelper (/Users/joesavona/github/react/compiler/node_modules/jest-worker/build/workers/threadChild.js:127:5)
103+
at execMethod (/Users/joesavona/github/react/compiler/node_modules/jest-worker/build/workers/threadChild.js:131:5)
104+
at MessagePort.messageListener (/Users/joesavona/github/react/compiler/node_modules/jest-worker/build/workers/threadChild.js:49:7)
105+
at [nodejs.internal.kHybridDispatch] (node:internal/event_target:827:20)
106+
```
107+
108+

‎compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/error.invalid-reassign-local-variable-in-jsx-callback.js‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
function Component() {
2+
let local;
3+
4+
const reassignLocal = newValue => {
5+
local = newValue;
6+
};
7+
8+
const onClick = newValue => {
9+
reassignLocal('hello');
10+
11+
if (local === newValue) {
12+
// Without React Compiler, `reassignLocal` is freshly created
13+
// on each render, capturing a binding to the latest `local`,
14+
// such that invoking reassignLocal will reassign the same
15+
// binding that we are observing in the if condition, and
16+
// we reach this branch
17+
console.log('`local` was updated!');
18+
} else {
19+
// With React Compiler enabled, `reassignLocal` is only created
20+
// once, capturing a binding to `local` in that render pass.
21+
// Therefore, calling `reassignLocal` will reassign the wrong
22+
// version of `local`, and not update the binding we are checking
23+
// in the if condition.
24+
//
25+
// To protect against this, we disallow reassigning locals from
26+
// functions that escape
27+
throw new Error('`local` not updated!');
28+
}
29+
};
30+
31+
return <button onClick={onClick}>Submit</button>;
32+
}

0 commit comments

Comments
 (0)