mirror of
https://github.com/facebook/react.git
synced 2025-11-01 09:12:30 +00:00
[compiler][newinference] Impure/Render effects
Adds two new effects:
- `Impure` is for builtin functions that are known to be impure and not safe to call during render. This is already implemented via an optional `impure` flag on function declarations and ValidateNoImpureFunctionsInRender. That validation still exists, but we can avoid the need for the special validation pass by relying on the function effect.
- `Render` is an effect that indicates a value is assumed to be called at render time, even if it would not normally appear to be called. Examples are JSX tags and children — if these values are functions, they are definitely called at render (tag) or almost certainly called in render (children). Any function that transitively flows into a Render effect must not have MutateGlobal or Impure effects. We can also extend this to include other APIs that should be pure, for example we could consider adding it to `useReducer()` callbacks or `useState()` initializers.
ghstack-source-id: 8367f4621c
Pull Request resolved: https://github.com/facebook/react/pull/33488
This commit is contained in:
Joe Savona
@@ -416,6 +416,7 @@ addObject(BUILTIN_SHAPES, BuiltInArrayId, [
|
||||
into: signatureArgument(4),
|
||||
signature: null,
|
||||
mutatesFunction: false,
|
||||
loc: GeneratedSource,
|
||||
},
|
||||
// captures the result of the callback into the return array
|
||||
{
|
||||
|
||||
@@ -1015,6 +1015,12 @@ export function printAliasingEffect(effect: AliasingEffect): string {
|
||||
case 'MutateGlobal': {
|
||||
return `MutateGlobal ${printPlaceForAliasEffect(effect.place)} reason=${JSON.stringify(effect.error.reason)}`;
|
||||
}
|
||||
case 'Impure': {
|
||||
return `Impure ${printPlaceForAliasEffect(effect.place)} reason=${JSON.stringify(effect.error.reason)}`;
|
||||
}
|
||||
case 'Render': {
|
||||
return `Render ${printPlaceForAliasEffect(effect.place)}`;
|
||||
}
|
||||
default: {
|
||||
assertExhaustive(effect, `Unexpected kind '${(effect as any).kind}'`);
|
||||
}
|
||||
|
||||
@@ -100,6 +100,8 @@ function lowerWithMutationAliasing(fn: HIRFunction): void {
|
||||
capturedOrMutated.add(effect.value.identifier.id);
|
||||
break;
|
||||
}
|
||||
case 'Impure':
|
||||
case 'Render':
|
||||
case 'MutateFrozen':
|
||||
case 'MutateGlobal':
|
||||
case 'CreateFunction':
|
||||
|
||||
+76
-5
@@ -10,6 +10,7 @@ import {
|
||||
CompilerErrorDetailOptions,
|
||||
Effect,
|
||||
ErrorSeverity,
|
||||
SourceLocation,
|
||||
ValueKind,
|
||||
} from '..';
|
||||
import {
|
||||
@@ -380,10 +381,7 @@ function applySignature(
|
||||
context: new Set(),
|
||||
});
|
||||
effects.push({
|
||||
kind:
|
||||
value.kind === ValueKind.Frozen
|
||||
? 'MutateFrozen'
|
||||
: 'MutateGlobal',
|
||||
kind: 'MutateFrozen',
|
||||
place: effect.value,
|
||||
error: {
|
||||
severity: ErrorSeverity.InvalidReact,
|
||||
@@ -546,7 +544,10 @@ function applyEffect(
|
||||
const hasTrackedSideEffects =
|
||||
effect.function.loweredFunc.func.aliasingEffects?.some(
|
||||
effect =>
|
||||
effect.kind === 'MutateFrozen' || effect.kind === 'MutateGlobal',
|
||||
// TODO; include "render" here?
|
||||
effect.kind === 'MutateFrozen' ||
|
||||
effect.kind === 'MutateGlobal' ||
|
||||
effect.kind === 'Impure',
|
||||
);
|
||||
// For legacy compatibility
|
||||
const capturesRef = effect.function.loweredFunc.func.context.some(
|
||||
@@ -721,6 +722,7 @@ function applyEffect(
|
||||
effect.receiver,
|
||||
effect.args,
|
||||
functionValues[0].loweredFunc.func.context,
|
||||
effect.loc,
|
||||
);
|
||||
if (signatureEffects != null) {
|
||||
if (DEBUG) {
|
||||
@@ -747,6 +749,8 @@ function applyEffect(
|
||||
effect.into,
|
||||
effect.receiver,
|
||||
effect.args,
|
||||
[],
|
||||
effect.loc,
|
||||
)
|
||||
: null;
|
||||
if (signatureEffects != null) {
|
||||
@@ -766,6 +770,7 @@ function applyEffect(
|
||||
effect.into,
|
||||
effect.receiver,
|
||||
effect.args,
|
||||
effect.loc,
|
||||
);
|
||||
for (const legacyEffect of legacyEffects) {
|
||||
applyEffect(context, state, legacyEffect, aliased, effects);
|
||||
@@ -899,6 +904,8 @@ function applyEffect(
|
||||
}
|
||||
break;
|
||||
}
|
||||
case 'Impure':
|
||||
case 'Render':
|
||||
case 'MutateFrozen':
|
||||
case 'MutateGlobal': {
|
||||
effects.push(effect);
|
||||
@@ -1452,6 +1459,7 @@ function computeSignatureForInstruction(
|
||||
args: value.args,
|
||||
into: lvalue,
|
||||
signature,
|
||||
loc: value.loc,
|
||||
});
|
||||
break;
|
||||
}
|
||||
@@ -1631,6 +1639,24 @@ function computeSignatureForInstruction(
|
||||
into: lvalue,
|
||||
});
|
||||
}
|
||||
if (value.kind === 'JsxExpression') {
|
||||
if (value.tag.kind === 'Identifier') {
|
||||
// Tags are render function, by definition they're called during render
|
||||
effects.push({
|
||||
kind: 'Render',
|
||||
place: value.tag,
|
||||
});
|
||||
}
|
||||
if (value.children != null) {
|
||||
// Children are typically called during render, not used as an event/effect callback
|
||||
for (const child of value.children) {
|
||||
effects.push({
|
||||
kind: 'Render',
|
||||
place: child,
|
||||
});
|
||||
}
|
||||
}
|
||||
}
|
||||
break;
|
||||
}
|
||||
case 'DeclareLocal': {
|
||||
@@ -1861,6 +1887,7 @@ function computeEffectsForLegacySignature(
|
||||
lvalue: Place,
|
||||
receiver: Place,
|
||||
args: Array<Place | SpreadPattern | Hole>,
|
||||
loc: SourceLocation,
|
||||
): Array<AliasingEffect> {
|
||||
const returnValueReason = signature.returnValueReason ?? ValueReason.Other;
|
||||
const effects: Array<AliasingEffect> = [];
|
||||
@@ -1870,6 +1897,23 @@ function computeEffectsForLegacySignature(
|
||||
value: signature.returnValueKind,
|
||||
reason: returnValueReason,
|
||||
});
|
||||
if (signature.impure && state.env.config.validateNoImpureFunctionsInRender) {
|
||||
effects.push({
|
||||
kind: 'Impure',
|
||||
place: receiver,
|
||||
error: {
|
||||
reason:
|
||||
'Calling an impure function can produce unstable results. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#components-and-hooks-must-be-idempotent)',
|
||||
description:
|
||||
signature.canonicalName != null
|
||||
? `\`${signature.canonicalName}\` is an impure function whose results may change on every call`
|
||||
: null,
|
||||
severity: ErrorSeverity.InvalidReact,
|
||||
loc,
|
||||
suggestions: null,
|
||||
},
|
||||
});
|
||||
}
|
||||
const stores: Array<Place> = [];
|
||||
const captures: Array<Place> = [];
|
||||
function visit(place: Place, effect: Effect): void {
|
||||
@@ -2083,6 +2127,7 @@ function computeEffectsForSignature(
|
||||
args: Array<Place | SpreadPattern | Hole>,
|
||||
// Used for signatures constructed dynamically which reference context variables
|
||||
context: Array<Place> = [],
|
||||
loc: SourceLocation,
|
||||
): Array<AliasingEffect> | null {
|
||||
if (
|
||||
// Not enough args
|
||||
@@ -2163,6 +2208,7 @@ function computeEffectsForSignature(
|
||||
}
|
||||
break;
|
||||
}
|
||||
case 'Impure':
|
||||
case 'MutateFrozen':
|
||||
case 'MutateGlobal': {
|
||||
const values = substitutions.get(effect.place.identifier.id) ?? [];
|
||||
@@ -2171,6 +2217,13 @@ function computeEffectsForSignature(
|
||||
}
|
||||
break;
|
||||
}
|
||||
case 'Render': {
|
||||
const values = substitutions.get(effect.place.identifier.id) ?? [];
|
||||
for (const value of values) {
|
||||
effects.push({kind: effect.kind, place: value});
|
||||
}
|
||||
break;
|
||||
}
|
||||
case 'Mutate':
|
||||
case 'MutateTransitive':
|
||||
case 'MutateTransitiveConditionally':
|
||||
@@ -2254,6 +2307,7 @@ function computeEffectsForSignature(
|
||||
function: applyFunction[0],
|
||||
into: applyInto[0],
|
||||
signature: effect.signature,
|
||||
loc,
|
||||
});
|
||||
break;
|
||||
}
|
||||
@@ -2468,6 +2522,7 @@ export type AliasingEffect =
|
||||
args: Array<Place | SpreadPattern | Hole>;
|
||||
into: Place;
|
||||
signature: FunctionSignature | null;
|
||||
loc: SourceLocation;
|
||||
}
|
||||
/**
|
||||
* Constructs a function value with the given captures. The mutability of the function
|
||||
@@ -2490,6 +2545,20 @@ export type AliasingEffect =
|
||||
kind: 'MutateGlobal';
|
||||
place: Place;
|
||||
error: CompilerErrorDetailOptions;
|
||||
}
|
||||
/**
|
||||
* Indicates a side-effect that is not safe during render
|
||||
*/
|
||||
| {kind: 'Impure'; place: Place; error: CompilerErrorDetailOptions}
|
||||
/**
|
||||
* Indicates that a given place is accessed during render. Used to distingush
|
||||
* hook arguments that are known to be called immediately vs those used for
|
||||
* event handlers/effects, and for JSX values known to be called during render
|
||||
* (tags, children) vs those that may be events/effect (other props).
|
||||
*/
|
||||
| {
|
||||
kind: 'Render';
|
||||
place: Place;
|
||||
};
|
||||
|
||||
function hashEffect(effect: AliasingEffect): string {
|
||||
@@ -2536,6 +2605,8 @@ function hashEffect(effect: AliasingEffect): string {
|
||||
case 'Freeze': {
|
||||
return [effect.kind, effect.value.identifier.id, effect.reason].join(':');
|
||||
}
|
||||
case 'Impure':
|
||||
case 'Render':
|
||||
case 'MutateFrozen':
|
||||
case 'MutateGlobal': {
|
||||
return [effect.kind, effect.place.identifier.id].join(':');
|
||||
|
||||
+3
-1
@@ -108,7 +108,9 @@ export function inferMutationAliasingFunctionEffects(
|
||||
]);
|
||||
} else if (
|
||||
effect.kind === 'MutateFrozen' ||
|
||||
effect.kind === 'MutateGlobal'
|
||||
effect.kind === 'MutateGlobal' ||
|
||||
effect.kind === 'Impure' ||
|
||||
effect.kind === 'Render'
|
||||
) {
|
||||
effects.push(effect);
|
||||
}
|
||||
|
||||
+48
-1
@@ -66,6 +66,7 @@ export function inferMutationAliasingRanges(
|
||||
kind: MutationKind;
|
||||
place: Place;
|
||||
}> = [];
|
||||
const renders: Array<{index: number; place: Place}> = [];
|
||||
|
||||
let index = 0;
|
||||
|
||||
@@ -160,9 +161,12 @@ export function inferMutationAliasingRanges(
|
||||
});
|
||||
} else if (
|
||||
effect.kind === 'MutateFrozen' ||
|
||||
effect.kind === 'MutateGlobal'
|
||||
effect.kind === 'MutateGlobal' ||
|
||||
effect.kind === 'Impure'
|
||||
) {
|
||||
errors.push(effect.error);
|
||||
} else if (effect.kind === 'Render') {
|
||||
renders.push({index: index++, place: effect.place});
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -209,6 +213,9 @@ export function inferMutationAliasingRanges(
|
||||
errors,
|
||||
);
|
||||
}
|
||||
for (const render of renders) {
|
||||
state.render(render.index, render.place.identifier, errors);
|
||||
}
|
||||
if (DEBUG) {
|
||||
console.log(pretty([...state.nodes.keys()]));
|
||||
}
|
||||
@@ -357,6 +364,8 @@ export function inferMutationAliasingRanges(
|
||||
// no-op, Read is the default
|
||||
break;
|
||||
}
|
||||
case 'Impure':
|
||||
case 'Render':
|
||||
case 'MutateFrozen':
|
||||
case 'MutateGlobal': {
|
||||
// no-op
|
||||
@@ -440,6 +449,7 @@ export function inferMutationAliasingRanges(
|
||||
function appendFunctionErrors(errors: CompilerError, fn: HIRFunction): void {
|
||||
for (const effect of fn.aliasingEffects ?? []) {
|
||||
switch (effect.kind) {
|
||||
case 'Impure':
|
||||
case 'MutateFrozen':
|
||||
case 'MutateGlobal': {
|
||||
errors.push(effect.error);
|
||||
@@ -530,6 +540,43 @@ class AliasingState {
|
||||
}
|
||||
}
|
||||
|
||||
render(index: number, start: Identifier, errors: CompilerError): void {
|
||||
const seen = new Set<Identifier>();
|
||||
const queue: Array<Identifier> = [start];
|
||||
while (queue.length !== 0) {
|
||||
const current = queue.pop()!;
|
||||
if (seen.has(current)) {
|
||||
continue;
|
||||
}
|
||||
seen.add(current);
|
||||
const node = this.nodes.get(current);
|
||||
if (node == null || node.transitive != null || node.local != null) {
|
||||
continue;
|
||||
}
|
||||
if (node.value.kind === 'Function') {
|
||||
appendFunctionErrors(errors, node.value.function);
|
||||
}
|
||||
for (const [alias, when] of node.createdFrom) {
|
||||
if (when >= index) {
|
||||
continue;
|
||||
}
|
||||
queue.push(alias);
|
||||
}
|
||||
for (const [alias, when] of node.aliases) {
|
||||
if (when >= index) {
|
||||
continue;
|
||||
}
|
||||
queue.push(alias);
|
||||
}
|
||||
for (const [capture, when] of node.captures) {
|
||||
if (when >= index) {
|
||||
continue;
|
||||
}
|
||||
queue.push(capture);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
mutate(
|
||||
index: number,
|
||||
start: Identifier,
|
||||
|
||||
+8
-7
@@ -2,6 +2,7 @@
|
||||
## Input
|
||||
|
||||
```javascript
|
||||
// @enableNewMutationAliasingModel:false
|
||||
function Component() {
|
||||
const foo = () => {
|
||||
someGlobal = true;
|
||||
@@ -15,13 +16,13 @@ function Component() {
|
||||
## Error
|
||||
|
||||
```
|
||||
1 | function Component() {
|
||||
2 | const foo = () => {
|
||||
> 3 | someGlobal = true;
|
||||
| ^^^^^^^^^^ InvalidReact: Unexpected reassignment of a variable which was defined outside of the component. Components and hooks should be pure and side-effect free, but variable reassignment is a form of side-effect. If this variable is used in rendering, use useState instead. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#side-effects-must-run-outside-of-render) (3:3)
|
||||
4 | };
|
||||
5 | return <div {...foo} />;
|
||||
6 | }
|
||||
2 | function Component() {
|
||||
3 | const foo = () => {
|
||||
> 4 | someGlobal = true;
|
||||
| ^^^^^^^^^^ InvalidReact: Unexpected reassignment of a variable which was defined outside of the component. Components and hooks should be pure and side-effect free, but variable reassignment is a form of side-effect. If this variable is used in rendering, use useState instead. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#side-effects-must-run-outside-of-render) (4:4)
|
||||
5 | };
|
||||
6 | return <div {...foo} />;
|
||||
7 | }
|
||||
```
|
||||
|
||||
|
||||
+1
@@ -1,3 +1,4 @@
|
||||
// @enableNewMutationAliasingModel:false
|
||||
function Component() {
|
||||
const foo = () => {
|
||||
someGlobal = true;
|
||||
|
||||
+1
-1
@@ -20,7 +20,7 @@ function Component() {
|
||||
2 |
|
||||
3 | function Component() {
|
||||
> 4 | const date = Date.now();
|
||||
| ^^^^^^^^ InvalidReact: Calling an impure function can produce unstable results. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#components-and-hooks-must-be-idempotent). `Date.now` is an impure function whose results may change on every call (4:4)
|
||||
| ^^^^^^^^^^ InvalidReact: Calling an impure function can produce unstable results. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#components-and-hooks-must-be-idempotent). `Date.now` is an impure function whose results may change on every call (4:4)
|
||||
|
||||
InvalidReact: Calling an impure function can produce unstable results. (https://react.dev/reference/rules/components-and-hooks-must-be-pure#components-and-hooks-must-be-idempotent). `performance.now` is an impure function whose results may change on every call (5:5)
|
||||
|
||||
|
||||
+8
-7
@@ -2,6 +2,7 @@
|
||||
## Input
|
||||
|
||||
```javascript
|
||||
// @enableNewMutationAliasingModel:false
|
||||
function Foo() {
|
||||
const x = () => {
|
||||
window.href = 'foo';
|
||||
@@ -21,13 +22,13 @@ export const FIXTURE_ENTRYPOINT = {
|
||||
## Error
|
||||
|
||||
```
|
||||
1 | function Foo() {
|
||||
2 | const x = () => {
|
||||
> 3 | window.href = 'foo';
|
||||
| ^^^^^^ InvalidReact: Writing to a variable defined outside a component or hook is not allowed. Consider using an effect (3:3)
|
||||
4 | };
|
||||
5 | const y = {x};
|
||||
6 | return <Bar y={y} />;
|
||||
2 | function Foo() {
|
||||
3 | const x = () => {
|
||||
> 4 | window.href = 'foo';
|
||||
| ^^^^^^ InvalidReact: Writing to a variable defined outside a component or hook is not allowed. Consider using an effect (4:4)
|
||||
5 | };
|
||||
6 | const y = {x};
|
||||
7 | return <Bar y={y} />;
|
||||
```
|
||||
|
||||
|
||||
+1
@@ -1,3 +1,4 @@
|
||||
// @enableNewMutationAliasingModel:false
|
||||
function Foo() {
|
||||
const x = () => {
|
||||
window.href = 'foo';
|
||||
|
||||
+19
-22
@@ -8,7 +8,6 @@ import {arrayPush, Stringify} from 'shared-runtime';
|
||||
function Component({prop1, prop2}) {
|
||||
'use memo';
|
||||
|
||||
// we'll ultimately extract the item from this array as z, and mutate later
|
||||
let x = [{value: prop1}];
|
||||
let z;
|
||||
while (x.length < 2) {
|
||||
@@ -17,27 +16,25 @@ function Component({prop1, prop2}) {
|
||||
// this mutation occurs before the reassigned value
|
||||
arrayPush(x, {value: prop2});
|
||||
|
||||
// this condition will never be true, so x doesn't get reassigned
|
||||
if (x[0].value === null) {
|
||||
if (x[0].value === prop1) {
|
||||
x = [{value: prop2}];
|
||||
const y = x;
|
||||
z = y[0];
|
||||
}
|
||||
}
|
||||
// the code is set up so that z will always be the value from the original x
|
||||
z.other = true;
|
||||
return <Stringify z={z} />;
|
||||
}
|
||||
|
||||
export const FIXTURE_ENTRYPOINT = {
|
||||
fn: Component,
|
||||
params: [{prop1: 0, prop2: 0}],
|
||||
params: [{prop1: 0, prop2: 'a'}],
|
||||
sequentialRenders: [
|
||||
{prop1: 0, prop2: 0},
|
||||
{prop1: 1, prop2: 0},
|
||||
{prop1: 1, prop2: 1},
|
||||
{prop1: 0, prop2: 1},
|
||||
{prop1: 0, prop2: 0},
|
||||
{prop1: 0, prop2: 'a'},
|
||||
{prop1: 1, prop2: 'a'},
|
||||
{prop1: 1, prop2: 'b'},
|
||||
{prop1: 0, prop2: 'b'},
|
||||
{prop1: 0, prop2: 'a'},
|
||||
],
|
||||
};
|
||||
|
||||
@@ -58,7 +55,7 @@ function Component(t0) {
|
||||
let x = [{ value: prop1 }];
|
||||
while (x.length < 2) {
|
||||
arrayPush(x, { value: prop2 });
|
||||
if (x[0].value === null) {
|
||||
if (x[0].value === prop1) {
|
||||
x = [{ value: prop2 }];
|
||||
const y = x;
|
||||
z = y[0];
|
||||
@@ -85,21 +82,21 @@ function Component(t0) {
|
||||
|
||||
export const FIXTURE_ENTRYPOINT = {
|
||||
fn: Component,
|
||||
params: [{ prop1: 0, prop2: 0 }],
|
||||
params: [{ prop1: 0, prop2: "a" }],
|
||||
sequentialRenders: [
|
||||
{ prop1: 0, prop2: 0 },
|
||||
{ prop1: 1, prop2: 0 },
|
||||
{ prop1: 1, prop2: 1 },
|
||||
{ prop1: 0, prop2: 1 },
|
||||
{ prop1: 0, prop2: 0 },
|
||||
{ prop1: 0, prop2: "a" },
|
||||
{ prop1: 1, prop2: "a" },
|
||||
{ prop1: 1, prop2: "b" },
|
||||
{ prop1: 0, prop2: "b" },
|
||||
{ prop1: 0, prop2: "a" },
|
||||
],
|
||||
};
|
||||
|
||||
```
|
||||
|
||||
### Eval output
|
||||
(kind: ok) [[ (exception in render) TypeError: Cannot set properties of undefined (setting 'other') ]]
|
||||
[[ (exception in render) TypeError: Cannot set properties of undefined (setting 'other') ]]
|
||||
[[ (exception in render) TypeError: Cannot set properties of undefined (setting 'other') ]]
|
||||
[[ (exception in render) TypeError: Cannot set properties of undefined (setting 'other') ]]
|
||||
[[ (exception in render) TypeError: Cannot set properties of undefined (setting 'other') ]]
|
||||
(kind: ok) <div>{"z":{"value":"a","other":true}}</div>
|
||||
<div>{"z":{"value":"a","other":true}}</div>
|
||||
<div>{"z":{"value":"b","other":true}}</div>
|
||||
<div>{"z":{"value":"b","other":true}}</div>
|
||||
<div>{"z":{"value":"a","other":true}}</div>
|
||||
+11
-11
@@ -22,14 +22,14 @@ function Component({prop1, prop2}) {
|
||||
return <Stringify z={z} />;
|
||||
}
|
||||
|
||||
// export const FIXTURE_ENTRYPOINT = {
|
||||
// fn: Component,
|
||||
// params: [{prop1: 0, prop2: 0}],
|
||||
// sequentialRenders: [
|
||||
// {prop1: 0, prop2: 0},
|
||||
// {prop1: 1, prop2: 0},
|
||||
// {prop1: 1, prop2: 1},
|
||||
// {prop1: 0, prop2: 1},
|
||||
// {prop1: 0, prop2: 0},
|
||||
// ],
|
||||
// };
|
||||
export const FIXTURE_ENTRYPOINT = {
|
||||
fn: Component,
|
||||
params: [{prop1: 0, prop2: 'a'}],
|
||||
sequentialRenders: [
|
||||
{prop1: 0, prop2: 'a'},
|
||||
{prop1: 1, prop2: 'a'},
|
||||
{prop1: 1, prop2: 'b'},
|
||||
{prop1: 0, prop2: 'b'},
|
||||
{prop1: 0, prop2: 'a'},
|
||||
],
|
||||
};
|
||||
|
||||
Reference in New Issue
Block a user