@samitouri / QOS-React / commits / b11074772b

InferReferenceEffects defers function effects until used

ghstack-source-id: 186951086ac1dd8ebd75d6fdfca5c1a41e54a33e Pull Request resolved: https://github.com/facebook/react-forget/pull/2881

Joe Savona committed Apr 22, 2024 at 08:14 UTC b11074772b39c65189d9e923f5830f727b1ee06d
13 files changed +498 -65
compiler/packages/babel-plugin-react-forget/src/HIR/HIR.ts
+4
@@ -289,6 +289,10 @@ export type FunctionEffect =
289 kind: "GlobalMutation";
290 error: CompilerErrorDetailOptions;
291 }
292 + | {
293 + kind: "ReactMutation";
294 + error: CompilerErrorDetailOptions;
295 + }
296 | {
297 kind: "ContextMutation";
298 places: ReadonlySet<Place>;
compiler/packages/babel-plugin-react-forget/src/Inference/InferReferenceEffects.ts
+107 -17
@@ -237,6 +237,7 @@ export default function inferReferenceEffects(
237 if (!options.isFunctionExpression) {
238 functionEffects.forEach((eff) => {
239 switch (eff.kind) {
240 + case "ReactMutation":
241 case "GlobalMutation": {
242 CompilerError.throw(eff.error);
243 }
@@ -395,6 +396,7 @@ class InferenceState {
396 return;
397 }
398
399 + // Propagate effects of function expressions to the outer (ie current) effect context
400 for (const value of values) {
401 if (
402 (value.kind === "FunctionExpression" ||
@@ -402,25 +404,68 @@ class InferenceState {
404 value.loweredFunc.func.effects != null
405 ) {
406 for (const effect of value.loweredFunc.func.effects) {
405 - if (effect.kind === "GlobalMutation") {
407 + if (
408 + effect.kind === "GlobalMutation" ||
409 + effect.kind === "ReactMutation"
410 + ) {
411 + // Known effects are always propagated upwards
412 functionEffects.push(effect);
413 } else {
414 + /**
415 + * Contextual effects need to be replayed against the current inference
416 + * state, which may know more about the value to which the effect applied.
417 + * The main cases are:
418 + * 1. The mutated context value is _still_ a context value in the current scope,
419 + * so we have to continue propagating the original context mutation.
420 + * 2. The mutated context value is a mutable value in the current scope,
421 + * so the context mutation was fine and we can skip propagating the effect.
422 + * 3. The mutated context value is an immutable value in the current scope,
423 + * resulting in a non-ContextMutation FunctionEffect. We propagate that new,
424 + * more detailed effect to the current function context.
425 + */
426 for (const place of effect.places) {
427 if (this.isDefined(place)) {
410 - this.referenceAndRecordEffects(
428 + const replayedEffect = this.reference(
429 { ...place, loc: effect.loc },
430 effect.effect,
413 - reason,
414 - functionEffects
431 + reason
432 );
433 + if (replayedEffect != null) {
434 + if (replayedEffect.kind === "ContextMutation") {
435 + // Case 1, still a context value so propagate the original effect
436 + functionEffects.push(effect);
437 + } else {
438 + // Case 3, immutable value so propagate the more precise effect
439 + functionEffects.push(replayedEffect);
440 + }
441 + } // else case 2, local mutable value so this effect was fine
442 }
443 }
444 }
445 }
446 }
447 }
448 + const functionEffect = this.reference(place, effectKind, reason);
449 + if (functionEffect !== null) {
450 + functionEffects.push(functionEffect);
451 + }
452 + }
453 +
454 + reference(
455 + place: Place,
456 + effectKind: Effect,
457 + reason: ValueReason
458 + ): FunctionEffect | null {
459 + const values = this.#variables.get(place.identifier.id);
460 + CompilerError.invariant(values !== undefined, {
461 + reason: "[InferReferenceEffects] Expected value to be initialized",
462 + description: null,
463 + loc: place.loc,
464 + suggestions: null,
465 + });
466 let valueKind: AbstractValue | null = this.kind(place);
467 let effect: Effect | null = null;
468 + let functionEffect: FunctionEffect | null = null;
469 switch (effectKind) {
470 case Effect.Freeze: {
471 if (
@@ -452,7 +497,7 @@ class InferenceState {
497 operand,
498 Effect.Freeze,
499 ValueReason.Other,
455 - functionEffects
500 + []
501 );
502 }
503 }
@@ -476,7 +521,7 @@ class InferenceState {
521 }
522 case Effect.Mutate: {
523 if (valueKind.kind === ValueKind.Context) {
479 - functionEffects.push({
524 + functionEffect = {
525 kind: "ContextMutation",
526 loc: place.loc,
527 effect: effectKind,
@@ -484,11 +529,15 @@ class InferenceState {
529 valueKind.context.size === 0
530 ? new Set([place])
531 : valueKind.context,
487 - });
532 + };
533 } else if (valueKind.kind !== ValueKind.Mutable) {
534 let reason = getWriteErrorReason(valueKind);
490 - functionEffects.push({
491 - kind: "GlobalMutation",
535 + functionEffect = {
536 + kind:
537 + valueKind.reason.size === 1 &&
538 + valueKind.reason.has(ValueReason.Global)
539 + ? "GlobalMutation"
540 + : "ReactMutation",
541 error: {
542 reason,
543 description:
@@ -499,14 +548,14 @@ class InferenceState {
548 suggestions: null,
549 severity: ErrorSeverity.InvalidReact,
550 },
502 - });
551 + };
552 }
553 effect = Effect.Mutate;
554 break;
555 }
556 case Effect.Store: {
557 if (valueKind.kind === ValueKind.Context) {
509 - functionEffects.push({
558 + functionEffect = {
559 kind: "ContextMutation",
560 loc: place.loc,
561 effect: effectKind,
@@ -514,11 +563,15 @@ class InferenceState {
563 valueKind.context.size === 0
564 ? new Set([place])
565 : valueKind.context,
517 - });
566 + };
567 } else if (valueKind.kind !== ValueKind.Mutable) {
568 let reason = getWriteErrorReason(valueKind);
520 - functionEffects.push({
521 - kind: "GlobalMutation",
569 + functionEffect = {
570 + kind:
571 + valueKind.reason.size === 1 &&
572 + valueKind.reason.has(ValueReason.Global)
573 + ? "GlobalMutation"
574 + : "ReactMutation",
575 error: {
576 reason,
577 description:
@@ -529,7 +582,7 @@ class InferenceState {
582 suggestions: null,
583 severity: ErrorSeverity.InvalidReact,
584 },
532 - });
585 + };
586 }
587
588 /*
@@ -582,6 +635,7 @@ class InferenceState {
635 suggestions: null,
636 });
637 place.effect = effect;
638 + return functionEffect;
639 }
640
641 /*
@@ -1112,6 +1166,42 @@ function inferBlock(
1166 []
1167 );
1168 hasMutableOperand ||= isMutableEffect(operand.effect, operand.loc);
1169 +
1170 + /**
1171 + * If this function references other functions, propagate the referenced function's
1172 + * effects to this function.
1173 + *
1174 + * ```
1175 + * let f = () => global = true;
1176 + * let g = () => f();
1177 + * g();
1178 + * ```
1179 + *
1180 + * In this example, because `g` references `f`, we propagate the GlobalMutation from
1181 + * `f` to `g`. Thus, referencing `g` in `g()` will evaluate the GlobalMutation in the outer
1182 + * function effect context and report an error. But if instead we do:
1183 + *
1184 + * ```
1185 + * let f = () => global = true;
1186 + * let g = () => f();
1187 + * useEffect(() => g(), [g])
1188 + * ```
1189 + *
1190 + * Now `g`'s effects will be discarded since they're in a useEffect.
1191 + */
1192 + const values = state.values(operand);
1193 + for (const value of values) {
1194 + if (
1195 + (value.kind === "ObjectMethod" ||
1196 + value.kind === "FunctionExpression") &&
1197 + value.loweredFunc.func.effects !== null
1198 + ) {
1199 + instrValue.loweredFunc.func.effects ??= [];
1200 + instrValue.loweredFunc.func.effects.push(
1201 + ...value.loweredFunc.func.effects
1202 + );
1203 + }
1204 + }
1205 }
1206 /*
1207 * If a closure did not capture any mutable values, then we can consider it to be
@@ -1530,7 +1620,7 @@ function inferBlock(
1620 instrValue.place,
1621 effect,
1622 ValueReason.Other,
1533 - functionEffects
1623 + []
1624 );
1625 lvalue.effect = Effect.ConditionallyMutate;
1626 // direct aliasing: `a = b`;
@@ -1628,7 +1718,7 @@ function inferBlock(
1718 instrValue.value,
1719 effect,
1720 ValueReason.Other,
1631 - functionEffects
1721 + []
1722 );
1723
1724 const lvalue = instr.lvalue;
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/allow-global-reassignment-in-effect-indirect.expect.md new
+109
@@ -0,0 +1,109 @@
1 +
2 +## Input
3 +
4 +```javascript
5 +import { useEffect, useState } from "react";
6 +
7 +let someGlobal = false;
8 +
9 +function Component() {
10 + const [state, setState] = useState(someGlobal);
11 +
12 + const setGlobal = () => {
13 + someGlobal = true;
14 + };
15 + useEffect(() => {
16 + setGlobal();
17 + }, []);
18 +
19 + useEffect(() => {
20 + setState(someGlobal);
21 + }, [someGlobal]);
22 +
23 + return <div>{String(state)}</div>;
24 +}
25 +
26 +export const FIXTURE_ENTRYPOINT = {
27 + fn: Component,
28 + params: [{}],
29 +};
30 +
31 +```
32 +
33 +## Code
34 +
35 +```javascript
36 +import {
37 + useEffect,
38 + useState,
39 + unstable_useMemoCache as useMemoCache,
40 +} from "react";
41 +
42 +let someGlobal = false;
43 +
44 +function Component() {
45 + const $ = useMemoCache(7);
46 + const [state, setState] = useState(someGlobal);
47 + let t0;
48 + if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
49 + t0 = () => {
50 + someGlobal = true;
51 + };
52 + $[0] = t0;
53 + } else {
54 + t0 = $[0];
55 + }
56 + const setGlobal = t0;
57 + let t1;
58 + let t2;
59 + if ($[1] === Symbol.for("react.memo_cache_sentinel")) {
60 + t1 = () => {
61 + setGlobal();
62 + };
63 + t2 = [];
64 + $[1] = t1;
65 + $[2] = t2;
66 + } else {
67 + t1 = $[1];
68 + t2 = $[2];
69 + }
70 + useEffect(t1, t2);
71 + let t3;
72 + if ($[3] === Symbol.for("react.memo_cache_sentinel")) {
73 + t3 = () => {
74 + setState(someGlobal);
75 + };
76 + $[3] = t3;
77 + } else {
78 + t3 = $[3];
79 + }
80 + let t4;
81 + if ($[4] === Symbol.for("react.memo_cache_sentinel")) {
82 + t4 = [someGlobal];
83 + $[4] = t4;
84 + } else {
85 + t4 = $[4];
86 + }
87 + useEffect(t3, t4);
88 +
89 + const t5 = String(state);
90 + let t6;
91 + if ($[5] !== t5) {
92 + t6 = <div>{t5}</div>;
93 + $[5] = t5;
94 + $[6] = t6;
95 + } else {
96 + t6 = $[6];
97 + }
98 + return t6;
99 +}
100 +
101 +export const FIXTURE_ENTRYPOINT = {
102 + fn: Component,
103 + params: [{}],
104 +};
105 +
106 +```
107 +
108 +### Eval output
109 +(kind: ok) <div>true</div>
\ No newline at end of file
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/allow-global-reassignment-in-effect-indirect.js renamed
-1
@@ -6,7 +6,6 @@ function Component() {
6 const [state, setState] = useState(someGlobal);
7
8 const setGlobal = () => {
9 - // TODO: this should be allowed since setGlobal is only used in an effect
9 someGlobal = true;
10 };
11 useEffect(() => {
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/allow-mutate-global-in-effect-fixpoint.expect.md new
+113
@@ -0,0 +1,113 @@
1 +
2 +## Input
3 +
4 +```javascript
5 +import { useEffect, useState } from "react";
6 +
7 +let someGlobal = { value: null };
8 +
9 +function Component() {
10 + const [state, setState] = useState(someGlobal);
11 +
12 + // NOTE: if we initialize to eg null or a local, then it won't be a definitively global
13 + // mutation below when we modify `y`. The point of this is example is that if all control
14 + // flow paths produce a global, we allow the mutation in an effect
15 + let x = someGlobal;
16 + while (x == null) {
17 + x = someGlobal;
18 + }
19 +
20 + // capture into a separate variable that is not a context variable.
21 + const y = x;
22 + useEffect(() => {
23 + y.value = "hello";
24 + }, []);
25 +
26 + useEffect(() => {
27 + setState(someGlobal.value);
28 + }, [someGlobal]);
29 +
30 + return <div>{String(state)}</div>;
31 +}
32 +
33 +export const FIXTURE_ENTRYPOINT = {
34 + fn: Component,
35 + params: [{}],
36 +};
37 +
38 +```
39 +
40 +## Code
41 +
42 +```javascript
43 +import {
44 + useEffect,
45 + useState,
46 + unstable_useMemoCache as useMemoCache,
47 +} from "react";
48 +
49 +let someGlobal = { value: null };
50 +
51 +function Component() {
52 + const $ = useMemoCache(6);
53 + const [state, setState] = useState(someGlobal);
54 +
55 + let x = someGlobal;
56 + while (x == null) {
57 + x = someGlobal;
58 + }
59 +
60 + const y = x;
61 + let t0;
62 + let t1;
63 + if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
64 + t0 = () => {
65 + y.value = "hello";
66 + };
67 + t1 = [];
68 + $[0] = t0;
69 + $[1] = t1;
70 + } else {
71 + t0 = $[0];
72 + t1 = $[1];
73 + }
74 + useEffect(t0, t1);
75 + let t2;
76 + if ($[2] === Symbol.for("react.memo_cache_sentinel")) {
77 + t2 = () => {
78 + setState(someGlobal.value);
79 + };
80 + $[2] = t2;
81 + } else {
82 + t2 = $[2];
83 + }
84 + let t3;
85 + if ($[3] === Symbol.for("react.memo_cache_sentinel")) {
86 + t3 = [someGlobal];
87 + $[3] = t3;
88 + } else {
89 + t3 = $[3];
90 + }
91 + useEffect(t2, t3);
92 +
93 + const t4 = String(state);
94 + let t5;
95 + if ($[4] !== t4) {
96 + t5 = <div>{t4}</div>;
97 + $[4] = t4;
98 + $[5] = t5;
99 + } else {
100 + t5 = $[5];
101 + }
102 + return t5;
103 +}
104 +
105 +export const FIXTURE_ENTRYPOINT = {
106 + fn: Component,
107 + params: [{}],
108 +};
109 +
110 +```
111 +
112 +### Eval output
113 +(kind: ok) <div>hello</div>
\ No newline at end of file
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/allow-mutate-global-in-effect-fixpoint.js new
+32
@@ -0,0 +1,32 @@
1 +import { useEffect, useState } from "react";
2 +
3 +let someGlobal = { value: null };
4 +
5 +function Component() {
6 + const [state, setState] = useState(someGlobal);
7 +
8 + // NOTE: if we initialize to eg null or a local, then it won't be a definitively global
9 + // mutation below when we modify `y`. The point of this is example is that if all control
10 + // flow paths produce a global, we allow the mutation in an effect
11 + let x = someGlobal;
12 + while (x == null) {
13 + x = someGlobal;
14 + }
15 +
16 + // capture into a separate variable that is not a context variable.
17 + const y = x;
18 + useEffect(() => {
19 + y.value = "hello";
20 + }, []);
21 +
22 + useEffect(() => {
23 + setState(someGlobal.value);
24 + }, [someGlobal]);
25 +
26 + return <div>{String(state)}</div>;
27 +}
28 +
29 +export const FIXTURE_ENTRYPOINT = {
30 + fn: Component,
31 + params: [{}],
32 +};
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-mutate-props-in-effect-fixpoint.expect.md new
+37
@@ -0,0 +1,37 @@
1 +
2 +## Input
3 +
4 +```javascript
5 +import { useEffect } from "react";
6 +
7 +function Component(props) {
8 + let x = null;
9 + while (x == null) {
10 + x = props.value;
11 + }
12 + let y = x;
13 + let mutateProps = () => {
14 + y.foo = true;
15 + };
16 + let mutatePropsIndirect = () => {
17 + mutateProps();
18 + };
19 + useEffect(() => mutatePropsIndirect(), [mutatePropsIndirect]);
20 +}
21 +
22 +```
23 +
24 +
25 +## Error
26 +
27 +```
28 + 8 | let y = x;
29 + 9 | let mutateProps = () => {
30 +> 10 | y.foo = true;
31 + | ^ InvalidReact: This mutates a variable that React considers immutable.. Found mutation of [object Object] (10:10)
32 + 11 | };
33 + 12 | let mutatePropsIndirect = () => {
34 + 13 | mutateProps();
35 +```
36 +
37 +
\ No newline at end of file
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-mutate-props-in-effect-fixpoint.js new
+16
@@ -0,0 +1,16 @@
1 +import { useEffect } from "react";
2 +
3 +function Component(props) {
4 + let x = null;
5 + while (x == null) {
6 + x = props.value;
7 + }
8 + let y = x;
9 + let mutateProps = () => {
10 + y.foo = true;
11 + };
12 + let mutatePropsIndirect = () => {
13 + mutateProps();
14 + };
15 + useEffect(() => mutatePropsIndirect(), [mutatePropsIndirect]);
16 +}
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-mutation-of-possible-props-phi-indirect.expect.md new
+31
@@ -0,0 +1,31 @@
1 +
2 +## Input
3 +
4 +```javascript
5 +function Component(props) {
6 + let x = cond ? someGlobal : props.foo;
7 + const mutatePhiThatCouldBeProps = () => {
8 + x.y = true;
9 + };
10 + const indirectMutateProps = () => {
11 + mutatePhiThatCouldBeProps();
12 + };
13 + useEffect(() => indirectMutateProps(), []);
14 +}
15 +
16 +```
17 +
18 +
19 +## Error
20 +
21 +```
22 + 2 | let x = cond ? someGlobal : props.foo;
23 + 3 | const mutatePhiThatCouldBeProps = () => {
24 +> 4 | x.y = true;
25 + | ^ InvalidReact: Writing to a variable defined outside a component or hook is not allowed. Consider using an effect.. Found mutation of [object Object] (4:4)
26 + 5 | };
27 + 6 | const indirectMutateProps = () => {
28 + 7 | mutatePhiThatCouldBeProps();
29 +```
30 +
31 +
\ No newline at end of file
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-mutation-of-possible-props-phi-indirect.js new
+10
@@ -0,0 +1,10 @@
1 +function Component(props) {
2 + let x = cond ? someGlobal : props.foo;
3 + const mutatePhiThatCouldBeProps = () => {
4 + x.y = true;
5 + };
6 + const indirectMutateProps = () => {
7 + mutatePhiThatCouldBeProps();
8 + };
9 + useEffect(() => indirectMutateProps(), []);
10 +}
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-props-mutation-in-effect-indirect.expect.md new
+30
@@ -0,0 +1,30 @@
1 +
2 +## Input
3 +
4 +```javascript
5 +function Component(props) {
6 + const mutateProps = () => {
7 + props.value = true;
8 + };
9 + const indirectMutateProps = () => {
10 + mutateProps();
11 + };
12 + useEffect(() => indirectMutateProps(), []);
13 +}
14 +
15 +```
16 +
17 +
18 +## Error
19 +
20 +```
21 + 1 | function Component(props) {
22 + 2 | const mutateProps = () => {
23 +> 3 | props.value = true;
24 + | ^^^^^ InvalidReact: Mutating props or hook arguments is not allowed. Consider using a local variable instead.. Found mutation of [object Object] (3:3)
25 + 4 | };
26 + 5 | const indirectMutateProps = () => {
27 + 6 | mutateProps();
28 +```
29 +
30 +
\ No newline at end of file
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-props-mutation-in-effect-indirect.js new
+9
@@ -0,0 +1,9 @@
1 +function Component(props) {
2 + const mutateProps = () => {
3 + props.value = true;
4 + };
5 + const indirectMutateProps = () => {
6 + mutateProps();
7 + };
8 + useEffect(() => indirectMutateProps(), []);
9 +}
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.todo-allow-global-reassignment-in-effect-indirect.expect.md deleted
-47
@@ -1,47 +0,0 @@
1 -
2 -## Input
3 -
4 -```javascript
5 -import { useEffect, useState } from "react";
6 -
7 -let someGlobal = false;
8 -
9 -function Component() {
10 - const [state, setState] = useState(someGlobal);
11 -
12 - const setGlobal = () => {
13 - // TODO: this should be allowed since setGlobal is only used in an effect
14 - someGlobal = true;
15 - };
16 - useEffect(() => {
17 - setGlobal();
18 - }, []);
19 -
20 - useEffect(() => {
21 - setState(someGlobal);
22 - }, [someGlobal]);
23 -
24 - return <div>{String(state)}</div>;
25 -}
26 -
27 -export const FIXTURE_ENTRYPOINT = {
28 - fn: Component,
29 - params: [{}],
30 -};
31 -
32 -```
33 -
34 -
35 -## Error
36 -
37 -```
38 - 8 | const setGlobal = () => {
39 - 9 | // TODO: this should be allowed since setGlobal is only used in an effect
40 -> 10 | someGlobal = true;
41 - | ^^^^^^^^^^ 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) (10:10)
42 - 11 | };
43 - 12 | useEffect(() => {
44 - 13 | setGlobal();
45 -```
46 -
47 -
\ No newline at end of file