@samitouri / QOS-React-1 / commits / 49ed6f0740

compiler: Allow global mutation in jsx props

Fixes https://x.com/raibima/status/1794395807216738792 The issue is that if you pass a global-modifying function as prop to JSX, we currently report that it's invalid to modify a global during rendering. The problem is that we don't really know when/if the child component will actually call that function prop. It would be against the rules to call the function during render, but it's totally fine to call it during an event handler or from a useEffect. Since we don't know at the call-site how the child will use the function, we should allow such calls. In the future we could improve this in a few ways: * For all functions that modify globals, codegen an assertion or warning into the function that fires if it's called "during render". We'd have to precisely define what "during render" is, but this would at least help developers catch this dynamically. * Use the type system to distinguish "event/effect" and "render" functions to help developers avoid accidentally mutating globals during render. ghstack-source-id: 4aba4e6d214fd6c062e4029294efe9b8fe25cd83 Pull Request resolved: https://github.com/facebook/react/pull/29591

Joe Savona committed May 25, 2024 at 22:03 UTC 49ed6f0740f9d47777faafd92046f7f044cf3e5e
13 files changed +331 -47
compiler/packages/babel-plugin-react-compiler/src/Inference/InferReferenceEffects.ts
+47 -4
@@ -1103,13 +1103,56 @@ function inferBlock(
1103 break;
1104 }
1105 case "JsxExpression": {
1106 - valueKind = {
1106 + if (instrValue.tag.kind === "Identifier") {
1107 + state.referenceAndRecordEffects(
1108 + instrValue.tag,
1109 + Effect.Freeze,
1110 + ValueReason.JsxCaptured,
1111 + functionEffects
1112 + );
1113 + }
1114 + if (instrValue.children !== null) {
1115 + for (const child of instrValue.children) {
1116 + state.referenceAndRecordEffects(
1117 + child,
1118 + Effect.Freeze,
1119 + ValueReason.JsxCaptured,
1120 + functionEffects
1121 + );
1122 + }
1123 + }
1124 + for (const attr of instrValue.props) {
1125 + if (attr.kind === "JsxSpreadAttribute") {
1126 + state.referenceAndRecordEffects(
1127 + attr.argument,
1128 + Effect.Freeze,
1129 + ValueReason.JsxCaptured,
1130 + functionEffects
1131 + );
1132 + } else {
1133 + const propEffects: Array<FunctionEffect> = [];
1134 + state.referenceAndRecordEffects(
1135 + attr.place,
1136 + Effect.Freeze,
1137 + ValueReason.JsxCaptured,
1138 + propEffects
1139 + );
1140 + functionEffects.push(
1141 + ...propEffects.filter(
1142 + (propEffect) => propEffect.kind !== "GlobalMutation"
1143 + )
1144 + );
1145 + }
1146 + }
1147 +
1148 + state.initialize(instrValue, {
1149 kind: ValueKind.Frozen,
1150 reason: new Set([ValueReason.Other]),
1151 context: new Set(),
1110 - };
1111 - effect = { kind: Effect.Freeze, reason: ValueReason.JsxCaptured };
1112 - break;
1152 + });
1153 + state.define(instr.lvalue, instrValue);
1154 + instr.lvalue.effect = Effect.ConditionallyMutate;
1155 + continue;
1156 }
1157 case "JsxFragment": {
1158 valueKind = {
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-modify-global-in-callback-jsx.expect.md new
+86
@@ -0,0 +1,86 @@
1 +
2 +## Input
3 +
4 +```javascript
5 +import { useMemo } from "react";
6 +
7 +const someGlobal = { value: 0 };
8 +
9 +function Component({ value }) {
10 + const onClick = () => {
11 + someGlobal.value = value;
12 + };
13 + return useMemo(() => {
14 + return <div onClick={onClick}>{someGlobal.value}</div>;
15 + }, []);
16 +}
17 +
18 +export const FIXTURE_ENTRYPOINT = {
19 + fn: Component,
20 + params: [{ value: 0 }],
21 + sequentialRenders: [
22 + { value: 1 },
23 + { value: 1 },
24 + { value: 42 },
25 + { value: 42 },
26 + { value: 0 },
27 + ],
28 +};
29 +
30 +```
31 +
32 +## Code
33 +
34 +```javascript
35 +import { c as _c } from "react/compiler-runtime";
36 +import { useMemo } from "react";
37 +
38 +const someGlobal = { value: 0 };
39 +
40 +function Component(t0) {
41 + const $ = _c(4);
42 + const { value } = t0;
43 + let t1;
44 + if ($[0] !== value) {
45 + t1 = () => {
46 + someGlobal.value = value;
47 + };
48 + $[0] = value;
49 + $[1] = t1;
50 + } else {
51 + t1 = $[1];
52 + }
53 + const onClick = t1;
54 + let t2;
55 + let t3;
56 + if ($[2] !== onClick) {
57 + t3 = <div onClick={onClick}>{someGlobal.value}</div>;
58 + $[2] = onClick;
59 + $[3] = t3;
60 + } else {
61 + t3 = $[3];
62 + }
63 + t2 = t3;
64 + return t2;
65 +}
66 +
67 +export const FIXTURE_ENTRYPOINT = {
68 + fn: Component,
69 + params: [{ value: 0 }],
70 + sequentialRenders: [
71 + { value: 1 },
72 + { value: 1 },
73 + { value: 42 },
74 + { value: 42 },
75 + { value: 0 },
76 + ],
77 +};
78 +
79 +```
80 +
81 +### Eval output
82 +(kind: ok) <div>0</div>
83 +<div>0</div>
84 +<div>0</div>
85 +<div>0</div>
86 +<div>0</div>
\ No newline at end of file
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-modify-global-in-callback-jsx.js new
+24
@@ -0,0 +1,24 @@
1 +import { useMemo } from "react";
2 +
3 +const someGlobal = { value: 0 };
4 +
5 +function Component({ value }) {
6 + const onClick = () => {
7 + someGlobal.value = value;
8 + };
9 + return useMemo(() => {
10 + return <div onClick={onClick}>{someGlobal.value}</div>;
11 + }, []);
12 +}
13 +
14 +export const FIXTURE_ENTRYPOINT = {
15 + fn: Component,
16 + params: [{ value: 0 }],
17 + sequentialRenders: [
18 + { value: 1 },
19 + { value: 1 },
20 + { value: 42 },
21 + { value: 42 },
22 + { value: 0 },
23 + ],
24 +};
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-reassignment-to-global-function-jsx-prop.expect.md new
+53
@@ -0,0 +1,53 @@
1 +
2 +## Input
3 +
4 +```javascript
5 +function Component() {
6 + const onClick = () => {
7 + // Cannot assign to globals
8 + someUnknownGlobal = true;
9 + moduleLocal = true;
10 + };
11 + // It's possible that this could be an event handler / effect function,
12 + // but we don't know that and optimistically assume it will only be
13 + // called by an event handler or effect, where it is allowed to modify globals
14 + return <div onClick={onClick} />;
15 +}
16 +
17 +export const FIXTURE_ENTRYPOINT = {
18 + fn: Component,
19 + params: [{}],
20 +};
21 +
22 +```
23 +
24 +## Code
25 +
26 +```javascript
27 +import { c as _c } from "react/compiler-runtime";
28 +function Component() {
29 + const $ = _c(1);
30 + let t0;
31 + if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
32 + const onClick = () => {
33 + someUnknownGlobal = true;
34 + moduleLocal = true;
35 + };
36 +
37 + t0 = <div onClick={onClick} />;
38 + $[0] = t0;
39 + } else {
40 + t0 = $[0];
41 + }
42 + return t0;
43 +}
44 +
45 +export const FIXTURE_ENTRYPOINT = {
46 + fn: Component,
47 + params: [{}],
48 +};
49 +
50 +```
51 +
52 +### Eval output
53 +(kind: ok) <div></div>
\ No newline at end of file
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-reassignment-to-global-function-jsx-prop.js new
+16
@@ -0,0 +1,16 @@
1 +function Component() {
2 + const onClick = () => {
3 + // Cannot assign to globals
4 + someUnknownGlobal = true;
5 + moduleLocal = true;
6 + };
7 + // It's possible that this could be an event handler / effect function,
8 + // but we don't know that and optimistically assume it will only be
9 + // called by an event handler or effect, where it is allowed to modify globals
10 + return <div onClick={onClick} />;
11 +}
12 +
13 +export const FIXTURE_ENTRYPOINT = {
14 + fn: Component,
15 + params: [{}],
16 +};
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.assign-global-in-component-tag-function.expect.md new
+27
@@ -0,0 +1,27 @@
1 +
2 +## Input
3 +
4 +```javascript
5 +function Component() {
6 + const Foo = () => {
7 + someGlobal = true;
8 + };
9 + return <Foo />;
10 +}
11 +
12 +```
13 +
14 +
15 +## Error
16 +
17 +```
18 + 1 | function Component() {
19 + 2 | const Foo = () => {
20 +> 3 | someGlobal = true;
21 + | ^^^^^^^^^^ 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)
22 + 4 | };
23 + 5 | return <Foo />;
24 + 6 | }
25 +```
26 +
27 +
\ No newline at end of file
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.assign-global-in-component-tag-function.js new
+6
@@ -0,0 +1,6 @@
1 +function Component() {
2 + const Foo = () => {
3 + someGlobal = true;
4 + };
5 + return <Foo />;
6 +}
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.assign-global-in-jsx-children.expect.md new
+30
@@ -0,0 +1,30 @@
1 +
2 +## Input
3 +
4 +```javascript
5 +function Component() {
6 + const foo = () => {
7 + someGlobal = true;
8 + };
9 + // Children are generally access/called during render, so
10 + // modifying a global in a children function is almost
11 + // certainly a mistake.
12 + return <Foo>{foo}</Foo>;
13 +}
14 +
15 +```
16 +
17 +
18 +## Error
19 +
20 +```
21 + 1 | function Component() {
22 + 2 | const foo = () => {
23 +> 3 | someGlobal = true;
24 + | ^^^^^^^^^^ 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)
25 + 4 | };
26 + 5 | // Children are generally access/called during render, so
27 + 6 | // modifying a global in a children function is almost
28 +```
29 +
30 +
\ No newline at end of file
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.assign-global-in-jsx-children.js new
+9
@@ -0,0 +1,9 @@
1 +function Component() {
2 + const foo = () => {
3 + someGlobal = true;
4 + };
5 + // Children are generally access/called during render, so
6 + // modifying a global in a children function is almost
7 + // certainly a mistake.
8 + return <Foo>{foo}</Foo>;
9 +}
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.assign-global-in-jsx-spread-attribute.expect.md new
+27
@@ -0,0 +1,27 @@
1 +
2 +## Input
3 +
4 +```javascript
5 +function Component() {
6 + const foo = () => {
7 + someGlobal = true;
8 + };
9 + return <div {...foo} />;
10 +}
11 +
12 +```
13 +
14 +
15 +## Error
16 +
17 +```
18 + 1 | function Component() {
19 + 2 | const foo = () => {
20 +> 3 | someGlobal = true;
21 + | ^^^^^^^^^^ 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)
22 + 4 | };
23 + 5 | return <div {...foo} />;
24 + 6 | }
25 +```
26 +
27 +
\ No newline at end of file
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.assign-global-in-jsx-spread-attribute.js new
+6
@@ -0,0 +1,6 @@
1 +function Component() {
2 + const foo = () => {
3 + someGlobal = true;
4 + };
5 + return <div {...foo} />;
6 +}
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.reassignment-to-global-function-prop.expect.md deleted
-32
@@ -1,32 +0,0 @@
1 -
2 -## Input
3 -
4 -```javascript
5 -function Component() {
6 - const foo = () => {
7 - // Cannot assign to globals
8 - someUnknownGlobal = true;
9 - moduleLocal = true;
10 - };
11 - // It's possible that this could be an event handler / effect function,
12 - // but we don't know that and conservatively assume it's a render helper
13 - // where it's disallowed to modify globals
14 - return <Foo foo={foo} />;
15 -}
16 -
17 -```
18 -
19 -
20 -## Error
21 -
22 -```
23 - 2 | const foo = () => {
24 - 3 | // Cannot assign to globals
25 -> 4 | someUnknownGlobal = true;
26 - | ^^^^^^^^^^^^^^^^^ 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)
27 - 5 | moduleLocal = true;
28 - 6 | };
29 - 7 | // It's possible that this could be an event handler / effect function,
30 -```
31 -
32 -
\ No newline at end of file
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.reassignment-to-global-function-prop.js deleted
-11
@@ -1,11 +0,0 @@
1 -function Component() {
2 - const foo = () => {
3 - // Cannot assign to globals
4 - someUnknownGlobal = true;
5 - moduleLocal = true;
6 - };
7 - // It's possible that this could be an event handler / effect function,
8 - // but we don't know that and conservatively assume it's a render helper
9 - // where it's disallowed to modify globals
10 - return <Foo foo={foo} />;
11 -}