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

Stop reporting errors on mutations of primitives (and associated false positives)

Browse Source 
ghstack-source-id: 4dc9b3c51953f8ed2184ebc5e438bc877959b5d0
Pull Request resolved: https://github.com/facebook/react-forget/pull/2903
This commit is contained in:
Joe Savona
2024-04-25 09:39:20 -07:00
parent 0a7f4427f1
commit 1c7e34cdb6
8 changed files with 130 additions and 73 deletions
Download Patch File Download Diff File Expand all files Collapse all files
compiler/packages/babel-plugin-react-forget/src/HIR/Globals.ts
+11 -11
View File
@@ -89,7 +89,7 @@ const TYPED_GLOBALS: Array<[string, BuiltInType]> = [
restParam: null,
returnType: { kind: "Primitive" },
calleeEffect: Effect.Read,
returnValueKind: ValueKind.Immutable,
returnValueKind: ValueKind.Primitive,
}),
],
/*
@@ -130,7 +130,7 @@ const TYPED_GLOBALS: Array<[string, BuiltInType]> = [
restParam: Effect.Read,
returnType: { kind: "Primitive" },
calleeEffect: Effect.Read,
returnValueKind: ValueKind.Immutable,
returnValueKind: ValueKind.Primitive,
}),
],
]),
@@ -147,7 +147,7 @@ const TYPED_GLOBALS: Array<[string, BuiltInType]> = [
restParam: Effect.Read,
returnType: { kind: "Primitive" },
calleeEffect: Effect.Read,
returnValueKind: ValueKind.Immutable,
returnValueKind: ValueKind.Primitive,
}),
],
[
@@ -157,7 +157,7 @@ const TYPED_GLOBALS: Array<[string, BuiltInType]> = [
restParam: Effect.Read,
returnType: { kind: "Primitive" },
calleeEffect: Effect.Read,
returnValueKind: ValueKind.Immutable,
returnValueKind: ValueKind.Primitive,
}),
],
[
@@ -167,7 +167,7 @@ const TYPED_GLOBALS: Array<[string, BuiltInType]> = [
restParam: Effect.Read,
returnType: { kind: "Primitive" },
calleeEffect: Effect.Read,
returnValueKind: ValueKind.Immutable,
returnValueKind: ValueKind.Primitive,
}),
],
[
@@ -177,7 +177,7 @@ const TYPED_GLOBALS: Array<[string, BuiltInType]> = [
restParam: Effect.Read,
returnType: { kind: "Primitive" },
calleeEffect: Effect.Read,
returnValueKind: ValueKind.Immutable,
returnValueKind: ValueKind.Primitive,
}),
],
[
@@ -187,7 +187,7 @@ const TYPED_GLOBALS: Array<[string, BuiltInType]> = [
restParam: Effect.Read,
returnType: { kind: "Primitive" },
calleeEffect: Effect.Read,
returnValueKind: ValueKind.Immutable,
returnValueKind: ValueKind.Primitive,
}),
],
[
@@ -197,7 +197,7 @@ const TYPED_GLOBALS: Array<[string, BuiltInType]> = [
restParam: Effect.Read,
returnType: { kind: "Primitive" },
calleeEffect: Effect.Read,
returnValueKind: ValueKind.Immutable,
returnValueKind: ValueKind.Primitive,
}),
],
]),
@@ -209,7 +209,7 @@ const TYPED_GLOBALS: Array<[string, BuiltInType]> = [
restParam: Effect.Read,
returnType: { kind: "Primitive" },
calleeEffect: Effect.Read,
returnValueKind: ValueKind.Immutable,
returnValueKind: ValueKind.Primitive,
}),
],
[
@@ -219,7 +219,7 @@ const TYPED_GLOBALS: Array<[string, BuiltInType]> = [
restParam: Effect.Read,
returnType: { kind: "Primitive" },
calleeEffect: Effect.Read,
returnValueKind: ValueKind.Immutable,
returnValueKind: ValueKind.Primitive,
}),
],
[
@@ -229,7 +229,7 @@ const TYPED_GLOBALS: Array<[string, BuiltInType]> = [
restParam: Effect.Read,
returnType: { kind: "Primitive" },
calleeEffect: Effect.Read,
returnValueKind: ValueKind.Immutable,
returnValueKind: ValueKind.Primitive,
}),
],
// TODO: rest of Global objects
compiler/packages/babel-plugin-react-forget/src/HIR/HIR.ts
+2 -1
View File
@@ -1244,7 +1244,8 @@ export enum ValueReason {
export enum ValueKind {
MaybeFrozen = "maybefrozen",
Frozen = "frozen",
Immutable = "immutable",
Primitive = "primitive",
Global = "global",
Mutable = "mutable",
Context = "context",
}
compiler/packages/babel-plugin-react-forget/src/HIR/ObjectShape.ts
+8 -8
View File
@@ -235,7 +235,7 @@ addObject(BUILTIN_SHAPES, BuiltInArrayId, [
restParam: Effect.Capture,
returnType: PRIMITIVE_TYPE,
calleeEffect: Effect.Store,
returnValueKind: ValueKind.Immutable,
returnValueKind: ValueKind.Primitive,
}),
],
[
@@ -284,7 +284,7 @@ addObject(BUILTIN_SHAPES, BuiltInArrayId, [
* the array object itself is not modified
*/
calleeEffect: Effect.ConditionallyMutate,
returnValueKind: ValueKind.Immutable,
returnValueKind: ValueKind.Primitive,
noAlias: true,
mutableOnlyIfOperandsAreMutable: true,
}),
@@ -301,7 +301,7 @@ addObject(BUILTIN_SHAPES, BuiltInArrayId, [
* the array object itself is not modified
*/
calleeEffect: Effect.ConditionallyMutate,
returnValueKind: ValueKind.Immutable,
returnValueKind: ValueKind.Primitive,
noAlias: true,
mutableOnlyIfOperandsAreMutable: true,
}),
@@ -330,7 +330,7 @@ addObject(BUILTIN_SHAPES, BuiltInArrayId, [
* the array object itself is not modified
*/
calleeEffect: Effect.ConditionallyMutate,
returnValueKind: ValueKind.Immutable,
returnValueKind: ValueKind.Primitive,
noAlias: true,
mutableOnlyIfOperandsAreMutable: true,
}),
@@ -342,7 +342,7 @@ addObject(BUILTIN_SHAPES, BuiltInArrayId, [
restParam: Effect.ConditionallyMutate,
returnType: PRIMITIVE_TYPE,
calleeEffect: Effect.Read,
returnValueKind: ValueKind.Immutable,
returnValueKind: ValueKind.Primitive,
}),
],
// TODO: rest of Array properties
@@ -357,7 +357,7 @@ addObject(BUILTIN_SHAPES, BuiltInObjectId, [
restParam: null,
returnType: PRIMITIVE_TYPE,
calleeEffect: Effect.Read,
returnValueKind: ValueKind.Immutable,
returnValueKind: ValueKind.Primitive,
}),
],
/*
@@ -378,7 +378,7 @@ addObject(BUILTIN_SHAPES, BuiltInUseStateId, [
restParam: Effect.Freeze,
returnType: PRIMITIVE_TYPE,
calleeEffect: Effect.Read,
returnValueKind: ValueKind.Immutable,
returnValueKind: ValueKind.Primitive,
},
BuiltInSetStateId
),
@@ -401,7 +401,7 @@ addObject(BUILTIN_SHAPES, BuiltInMixedReadonlyId, [
restParam: Effect.Read,
returnType: PRIMITIVE_TYPE,
calleeEffect: Effect.Read,
returnValueKind: ValueKind.Immutable,
returnValueKind: ValueKind.Primitive,
}),
],
[
compiler/packages/babel-plugin-react-forget/src/Inference/InferReferenceEffects.ts
+37 -16
View File
@@ -531,7 +531,11 @@ class InferenceState {
? new Set([place])
: valueKind.context,
};
} else if (valueKind.kind !== ValueKind.Mutable) {
} else if (
valueKind.kind !== ValueKind.Mutable &&
// We ignore mutations of primitives since this is not a React-specific problem
valueKind.kind !== ValueKind.Primitive
) {
let reason = getWriteErrorReason(valueKind);
functionEffect = {
kind:
@@ -566,7 +570,11 @@ class InferenceState {
? new Set([place])
: valueKind.context,
};
} else if (valueKind.kind !== ValueKind.Mutable) {
} else if (
valueKind.kind !== ValueKind.Mutable &&
// We ignore mutations of primitives since this is not a React-specific problem
valueKind.kind !== ValueKind.Primitive
) {
let reason = getWriteErrorReason(valueKind);
functionEffect = {
kind:
@@ -601,7 +609,8 @@ class InferenceState {
}
case Effect.Capture: {
if (
valueKind.kind === ValueKind.Immutable ||
valueKind.kind === ValueKind.Primitive ||
valueKind.kind === ValueKind.Global ||
valueKind.kind === ValueKind.Frozen ||
valueKind.kind === ValueKind.MaybeFrozen
) {
@@ -876,9 +885,20 @@ function mergeValues(a: ValueKind, b: ValueKind): ValueKind {
// context | immutable
return ValueKind.Context;
}
} else {
// frozen | immutable
} else if (a === ValueKind.Frozen || b === ValueKind.Frozen) {
return ValueKind.Frozen;
} else if (a === ValueKind.Global || b === ValueKind.Global) {
return ValueKind.Global;
} else {
CompilerError.invariant(
a === ValueKind.Primitive && b == ValueKind.Primitive,
{
reason: `Unexpected value kind in mergeValues()`,
description: `Found kinds ${a} and ${b}`,
loc: GeneratedSource,
}
);
return ValueKind.Primitive;
}
}
@@ -940,7 +960,7 @@ function inferBlock(
switch (instrValue.kind) {
case "BinaryExpression": {
valueKind = {
kind: ValueKind.Immutable,
kind: ValueKind.Primitive,
reason: new Set([ValueReason.Other]),
context: new Set(),
};
@@ -1066,7 +1086,7 @@ function inferBlock(
}
case "UnaryExpression": {
valueKind = {
kind: ValueKind.Immutable,
kind: ValueKind.Primitive,
reason: new Set([ValueReason.Other]),
context: new Set(),
};
@@ -1121,7 +1141,7 @@ function inferBlock(
* an immutable string
*/
valueKind = {
kind: ValueKind.Immutable,
kind: ValueKind.Primitive,
reason: new Set([ValueReason.Other]),
context: new Set(),
};
@@ -1143,7 +1163,7 @@ function inferBlock(
}
case "LoadGlobal":
valueKind = {
kind: ValueKind.Immutable,
kind: ValueKind.Global,
reason: new Set([ValueReason.Global]),
context: new Set(),
};
@@ -1152,7 +1172,7 @@ function inferBlock(
case "JSXText":
case "Primitive": {
valueKind = {
kind: ValueKind.Immutable,
kind: ValueKind.Primitive,
reason: new Set([ValueReason.Other]),
context: new Set(),
};
@@ -1454,7 +1474,7 @@ function inferBlock(
case "PropertyDelete": {
// `delete` returns a boolean (immutable) and modifies the object
valueKind = {
kind: ValueKind.Immutable,
kind: ValueKind.Primitive,
reason: new Set([ValueReason.Other]),
context: new Set(),
};
@@ -1517,7 +1537,7 @@ function inferBlock(
functionEffects
);
state.initialize(instrValue, {
kind: ValueKind.Immutable,
kind: ValueKind.Primitive,
reason: new Set([ValueReason.Other]),
context: new Set(),
});
@@ -1605,7 +1625,7 @@ function inferBlock(
const lvalue = instr.lvalue;
lvalue.effect = Effect.ConditionallyMutate;
state.initialize(instrValue, {
kind: ValueKind.Immutable,
kind: ValueKind.Frozen,
reason: new Set([ValueReason.Other]),
context: new Set(),
});
@@ -1656,7 +1676,7 @@ function inferBlock(
context: new Set(),
}
: {
kind: ValueKind.Immutable,
kind: ValueKind.Primitive,
reason: new Set([ValueReason.Other]),
context: new Set(),
}
@@ -1878,7 +1898,7 @@ function inferBlock(
effect = { kind: Effect.Read, reason: ValueReason.Other };
lvalueEffect = Effect.Store;
valueKind = {
kind: ValueKind.Immutable,
kind: ValueKind.Primitive,
reason: new Set([ValueReason.Other]),
context: new Set(),
};
@@ -2009,7 +2029,8 @@ function areArgumentsImmutableAndNonMutating(
const place = arg.kind === "Identifier" ? arg : arg.place;
const kind = state.kind(place).kind;
switch (kind) {
case ValueKind.Immutable:
case ValueKind.Global:
case ValueKind.Primitive:
case ValueKind.Frozen: {
/*
* Only immutable values, or frozen lambdas are allowed.
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-mutate-props-in-effect-fixpoint.expect.md
+1 -1
View File
@@ -28,7 +28,7 @@ function Component(props) {
8 | let y = x;
9 | let mutateProps = () => {
> 10 | y.foo = true;
| ^ InvalidReact: This mutates a variable that React considers immutable. Found mutation of `y` (10:10)
| ^ InvalidReact: Mutating component props or hook arguments is not allowed. Consider using a local variable instead. Found mutation of `y` (10:10)
11 | };
12 | let mutatePropsIndirect = () => {
13 | mutateProps();
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.todo-for-of-capture-item-of-local-collection-mutate-later-value-initially-null.expect.md
-34
View File
@@ -1,34 +0,0 @@
## Input
```javascript
import { makeObject_Primitives } from "shared-runtime";
function Component(props) {
let lastItem = null; // we reject this code bc `lastItem` could be null and you can't mutate null
const items = [makeObject_Primitives(), makeObject_Primitives()];
for (const x of items) {
lastItem = x;
}
if (lastItem != null) {
lastItem.mutated = true;
}
return items;
}
```
## Error
```
8 | }
9 | if (lastItem != null) {
> 10 | lastItem.mutated = true;
| ^^^^^^^^ InvalidReact: This mutates a variable that React considers immutable (10:10)
11 | }
12 | return items;
13 | }
```
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/for-of-capture-item-of-local-collection-mutate-later-value-initially-null.expect.md
+63
View File
@@ -0,0 +1,63 @@
## Input
```javascript
import { makeObject_Primitives } from "shared-runtime";
function Component(props) {
let lastItem = null;
const items = [makeObject_Primitives(), makeObject_Primitives()];
for (const x of items) {
lastItem = x;
}
if (lastItem != null) {
lastItem.a += 1;
}
return items;
}
export const FIXTURE_ENTRYPOINT = {
fn: Component,
params: [{}],
sequentialRenders: [{}, {}, {}],
};
```
## Code
```javascript
import { unstable_useMemoCache as useMemoCache } from "react";
import { makeObject_Primitives } from "shared-runtime";
function Component(props) {
const $ = useMemoCache(1);
let items;
if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
let lastItem = null;
items = [makeObject_Primitives(), makeObject_Primitives()];
for (const x of items) {
lastItem = x;
}
if (lastItem != null) {
lastItem.a = lastItem.a + 1;
}
$[0] = items;
} else {
items = $[0];
}
return items;
}
export const FIXTURE_ENTRYPOINT = {
fn: Component,
params: [{}],
sequentialRenders: [{}, {}, {}],
};
```
### Eval output
(kind: ok) [{"a":0,"b":"value1","c":true},{"a":1,"b":"value1","c":true}]
[{"a":0,"b":"value1","c":true},{"a":1,"b":"value1","c":true}]
[{"a":0,"b":"value1","c":true},{"a":1,"b":"value1","c":true}]
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.todo-for-of-capture-item-of-local-collection-mutate-later-value-initially-null.js → compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/for-of-capture-item-of-local-collection-mutate-later-value-initially-null.js
+8 -2
View File
@@ -1,13 +1,19 @@
import { makeObject_Primitives } from "shared-runtime";
function Component(props) {
let lastItem = null; // we reject this code bc `lastItem` could be null and you can't mutate null
let lastItem = null;
const items = [makeObject_Primitives(), makeObject_Primitives()];
for (const x of items) {
lastItem = x;
}
if (lastItem != null) {
lastItem.mutated = true;
lastItem.a += 1;
}
return items;
}
export const FIXTURE_ENTRYPOINT = {
fn: Component,
params: [{}],
sequentialRenders: [{}, {}, {}],
};