[compiler][fixtures] test repros: codegen, alignScope, phis
ghstack-source-id: 04b1526c8567f8b7b59d198f022d10cf837e4c5b Pull Request resolved: https://github.com/facebook/react/pull/29878 The AlignReactiveScope bug should be simplest to fix, but it's also caught by an invariant assertion. I think a fix could be either keeping track of "active" block-fallthrough pairs (`retainWhere(pair => pair.range.end > current.instr[0].id)`) or following the approach in `assertValidBlockNesting`. I'm tempted to pull the value-block aligning logic out into its own pass (using the current `node` tree traversal), then align to non-value blocks with the `assertValidBlockNesting` approach. Happy to hear feedback on this though! The other two are likely bigger issues, as they're not caught by static invariants. Update: - removed bug-phi-reference-effect as it's been patched by @josephsavona - added bug-array-concat-should-capture
Mofei Zhang committed
Jun 25, 2024 at 16:03 UTC
4d11e1e88d6be1c244cb8ae76c377eaf195167ec
10 files changed
+412
-19
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-array-concat-should-capture.expect.md
new
+72
@@ -0,0 +1,72 @@
1
+
2
+## Input
3
+
4
+```javascript
5
+import { mutate } from "shared-runtime";
6
+
7
+/**
8
+ * Fixture showing why `concat` needs to capture both the callee and rest args.
9
+ * Here, observe that arr1's values are captured into arr2.
10
+ * - Later mutations of arr2 may write to values within arr1.
11
+ * - Observe that it's technically valid to separately memoize the array arr1
12
+ * itself.
13
+ */
14
+function Foo({ inputNum }) {
15
+ const arr1: Array<number | object> = [{ a: 1 }, {}];
16
+ const arr2 = arr1.concat([1, inputNum]);
17
+ mutate(arr2[0]);
18
+ return arr2;
19
+}
20
+
21
+export const FIXTURE_ENTRYPOINT = {
22
+ fn: Foo,
23
+ params: [{ inputNum: 2 }],
24
+ sequentialRenders: [{ inputNum: 2 }, { inputNum: 3 }],
25
+};
26
+
27
+```
28
+
29
+## Code
30
+
31
+```javascript
32
+import { c as _c } from "react/compiler-runtime";
33
+import { mutate } from "shared-runtime";
34
+
35
+/**
36
+ * Fixture showing why `concat` needs to capture both the callee and rest args.
37
+ * Here, observe that arr1's values are captured into arr2.
38
+ * - Later mutations of arr2 may write to values within arr1.
39
+ * - Observe that it's technically valid to separately memoize the array arr1
40
+ * itself.
41
+ */
42
+function Foo(t0) {
43
+ const $ = _c(3);
44
+ const { inputNum } = t0;
45
+ let t1;
46
+ if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
47
+ t1 = [{ a: 1 }, {}];
48
+ $[0] = t1;
49
+ } else {
50
+ t1 = $[0];
51
+ }
52
+ const arr1 = t1;
53
+ let arr2;
54
+ if ($[1] !== inputNum) {
55
+ arr2 = arr1.concat([1, inputNum]);
56
+ mutate(arr2[0]);
57
+ $[1] = inputNum;
58
+ $[2] = arr2;
59
+ } else {
60
+ arr2 = $[2];
61
+ }
62
+ return arr2;
63
+}
64
+
65
+export const FIXTURE_ENTRYPOINT = {
66
+ fn: Foo,
67
+ params: [{ inputNum: 2 }],
68
+ sequentialRenders: [{ inputNum: 2 }, { inputNum: 3 }],
69
+};
70
+
71
+```
72
+
\ No newline at end of file
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-array-concat-should-capture.ts
new
+21
@@ -0,0 +1,21 @@
1
+import { mutate } from "shared-runtime";
2
+
3
+/**
4
+ * Fixture showing why `concat` needs to capture both the callee and rest args.
5
+ * Here, observe that arr1's values are captured into arr2.
6
+ * - Later mutations of arr2 may write to values within arr1.
7
+ * - Observe that it's technically valid to separately memoize the array arr1
8
+ * itself.
9
+ */
10
+function Foo({ inputNum }) {
11
+ const arr1: Array<number | object> = [{ a: 1 }, {}];
12
+ const arr2 = arr1.concat([1, inputNum]);
13
+ mutate(arr2[0]);
14
+ return arr2;
15
+}
16
+
17
+export const FIXTURE_ENTRYPOINT = {
18
+ fn: Foo,
19
+ params: [{ inputNum: 2 }],
20
+ sequentialRenders: [{ inputNum: 2 }, { inputNum: 3 }],
21
+};
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-codegen-inline-iife.expect.md
new
+92
@@ -0,0 +1,92 @@
1
+
2
+## Input
3
+
4
+```javascript
5
+import { makeArray, print } from "shared-runtime";
6
+
7
+/**
8
+ * Exposes bug involving iife inlining + codegen.
9
+ * We currently inline iifes to labeled blocks (not value-blocks).
10
+ *
11
+ * Here, print(1) and the evaluation of makeArray(...) get the same scope
12
+ * as the compiler infers that the makeArray call may mutate its arguments.
13
+ * Since print(1) does not get its own scope (and is thus not a declaration
14
+ * or dependency), it does not get promoted.
15
+ * As a result, print(1) gets reordered across the labeled-block instructions
16
+ * to be inlined at the makeArray callsite.
17
+ *
18
+ * Current evaluator results:
19
+ * Found differences in evaluator results
20
+ * Non-forget (expected):
21
+ * (kind: ok) [null,2]
22
+ * logs: [1,2]
23
+ * Forget:
24
+ * (kind: ok) [null,2]
25
+ * logs: [2,1]
26
+ */
27
+function useTest() {
28
+ return makeArray<number | void>(
29
+ print(1),
30
+ (function foo() {
31
+ print(2);
32
+ return 2;
33
+ })()
34
+ );
35
+}
36
+
37
+export const FIXTURE_ENTRYPOINT = {
38
+ fn: useTest,
39
+ params: [],
40
+};
41
+
42
+```
43
+
44
+## Code
45
+
46
+```javascript
47
+import { c as _c } from "react/compiler-runtime";
48
+import { makeArray, print } from "shared-runtime";
49
+
50
+/**
51
+ * Exposes bug involving iife inlining + codegen.
52
+ * We currently inline iifes to labeled blocks (not value-blocks).
53
+ *
54
+ * Here, print(1) and the evaluation of makeArray(...) get the same scope
55
+ * as the compiler infers that the makeArray call may mutate its arguments.
56
+ * Since print(1) does not get its own scope (and is thus not a declaration
57
+ * or dependency), it does not get promoted.
58
+ * As a result, print(1) gets reordered across the labeled-block instructions
59
+ * to be inlined at the makeArray callsite.
60
+ *
61
+ * Current evaluator results:
62
+ * Found differences in evaluator results
63
+ * Non-forget (expected):
64
+ * (kind: ok) [null,2]
65
+ * logs: [1,2]
66
+ * Forget:
67
+ * (kind: ok) [null,2]
68
+ * logs: [2,1]
69
+ */
70
+function useTest() {
71
+ const $ = _c(1);
72
+ let t0;
73
+ if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
74
+ let t1;
75
+
76
+ print(2);
77
+ t1 = 2;
78
+ t0 = makeArray(print(1), t1);
79
+ $[0] = t0;
80
+ } else {
81
+ t0 = $[0];
82
+ }
83
+ return t0;
84
+}
85
+
86
+export const FIXTURE_ENTRYPOINT = {
87
+ fn: useTest,
88
+ params: [],
89
+};
90
+
91
+```
92
+
\ No newline at end of file
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-codegen-inline-iife.ts
new
+36
@@ -0,0 +1,36 @@
1
+import { makeArray, print } from "shared-runtime";
2
+
3
+/**
4
+ * Exposes bug involving iife inlining + codegen.
5
+ * We currently inline iifes to labeled blocks (not value-blocks).
6
+ *
7
+ * Here, print(1) and the evaluation of makeArray(...) get the same scope
8
+ * as the compiler infers that the makeArray call may mutate its arguments.
9
+ * Since print(1) does not get its own scope (and is thus not a declaration
10
+ * or dependency), it does not get promoted.
11
+ * As a result, print(1) gets reordered across the labeled-block instructions
12
+ * to be inlined at the makeArray callsite.
13
+ *
14
+ * Current evaluator results:
15
+ * Found differences in evaluator results
16
+ * Non-forget (expected):
17
+ * (kind: ok) [null,2]
18
+ * logs: [1,2]
19
+ * Forget:
20
+ * (kind: ok) [null,2]
21
+ * logs: [2,1]
22
+ */
23
+function useTest() {
24
+ return makeArray<number | void>(
25
+ print(1),
26
+ (function foo() {
27
+ print(2);
28
+ return 2;
29
+ })()
30
+ );
31
+}
32
+
33
+export const FIXTURE_ENTRYPOINT = {
34
+ fn: useTest,
35
+ params: [],
36
+};
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-align-scope-starts-within-cond.expect.md
new
+29
@@ -0,0 +1,29 @@
1
+
2
+## Input
3
+
4
+```javascript
5
+/**
6
+ * Similar fixture to `error.todo-align-scopes-nested-block-structure`, but
7
+ * a simpler case.
8
+ */
9
+function useFoo(cond) {
10
+ let s = null;
11
+ if (cond) {
12
+ s = {};
13
+ } else {
14
+ return null;
15
+ }
16
+ mutate(s);
17
+ return s;
18
+}
19
+
20
+```
21
+
22
+
23
+## Error
24
+
25
+```
26
+Invariant: Invalid nesting in program blocks or scopes. Items overlap but are not nested: 4:10(5:13)
27
+```
28
+
29
+
\ No newline at end of file
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-align-scope-starts-within-cond.ts
new
+14
@@ -0,0 +1,14 @@
1
+/**
2
+ * Similar fixture to `error.todo-align-scopes-nested-block-structure`, but
3
+ * a simpler case.
4
+ */
5
+function useFoo(cond) {
6
+ let s = null;
7
+ if (cond) {
8
+ s = {};
9
+ } else {
10
+ return null;
11
+ }
12
+ mutate(s);
13
+ return s;
14
+}
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-align-scopes-nested-block-structure.expect.md
new
+68
@@ -0,0 +1,68 @@
1
+
2
+## Input
3
+
4
+```javascript
5
+/**
6
+ * Fixture showing that it's not sufficient to only align direct scoped
7
+ * accesses of a block-fallthrough pair.
8
+ * Below is a simplified view of HIR blocks in this fixture.
9
+ * Note that here, s is mutated in both bb1 and bb4. However, neither
10
+ * bb1 nor bb4 have terminal fallthroughs or are fallthroughs themselves.
11
+ *
12
+ * This means that we need to recursively visit all scopes accessed between
13
+ * a block and its fallthrough and extend the range of those scopes which overlap
14
+ * with an active block/fallthrough pair,
15
+ *
16
+ * bb0
17
+ * ┌──────────────┐
18
+ * │let s = null │
19
+ * │test cond1 │
20
+ * │ <fallthr=bb3>│
21
+ * └┬─────────────┘
22
+ * │ bb1
23
+ * ├─►┌───────┐
24
+ * │ │s = {} ├────┐
25
+ * │ └───────┘ │
26
+ * │ bb2 │
27
+ * └─►┌───────┐ │
28
+ * │return;│ │
29
+ * └───────┘ │
30
+ * bb3 │
31
+ * ┌──────────────┐◄┘
32
+ * │test cond2 │
33
+ * │ <fallthr=bb5>│
34
+ * └┬─────────────┘
35
+ * │ bb4
36
+ * ├─►┌─────────┐
37
+ * │ │mutate(s)├─┐
38
+ * ▼ └─────────┘ │
39
+ * bb5 │
40
+ * ┌───────────┐ │
41
+ * │return s; │◄──┘
42
+ * └───────────┘
43
+ */
44
+function useFoo(cond1, cond2) {
45
+ let s = null;
46
+ if (cond1) {
47
+ s = {};
48
+ } else {
49
+ return null;
50
+ }
51
+
52
+ if (cond2) {
53
+ mutate(s);
54
+ }
55
+
56
+ return s;
57
+}
58
+
59
+```
60
+
61
+
62
+## Error
63
+
64
+```
65
+Invariant: Invalid nesting in program blocks or scopes. Items overlap but are not nested: 4:10(5:15)
66
+```
67
+
68
+
\ No newline at end of file
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-align-scopes-nested-block-structure.ts
new
+53
@@ -0,0 +1,53 @@
1
+/**
2
+ * Fixture showing that it's not sufficient to only align direct scoped
3
+ * accesses of a block-fallthrough pair.
4
+ * Below is a simplified view of HIR blocks in this fixture.
5
+ * Note that here, s is mutated in both bb1 and bb4. However, neither
6
+ * bb1 nor bb4 have terminal fallthroughs or are fallthroughs themselves.
7
+ *
8
+ * This means that we need to recursively visit all scopes accessed between
9
+ * a block and its fallthrough and extend the range of those scopes which overlap
10
+ * with an active block/fallthrough pair,
11
+ *
12
+ * bb0
13
+ * ┌──────────────┐
14
+ * │let s = null │
15
+ * │test cond1 │
16
+ * │ <fallthr=bb3>│
17
+ * └┬─────────────┘
18
+ * │ bb1
19
+ * ├─►┌───────┐
20
+ * │ │s = {} ├────┐
21
+ * │ └───────┘ │
22
+ * │ bb2 │
23
+ * └─►┌───────┐ │
24
+ * │return;│ │
25
+ * └───────┘ │
26
+ * bb3 │
27
+ * ┌──────────────┐◄┘
28
+ * │test cond2 │
29
+ * │ <fallthr=bb5>│
30
+ * └┬─────────────┘
31
+ * │ bb4
32
+ * ├─►┌─────────┐
33
+ * │ │mutate(s)├─┐
34
+ * ▼ └─────────┘ │
35
+ * bb5 │
36
+ * ┌───────────┐ │
37
+ * │return s; │◄──┘
38
+ * └───────────┘
39
+ */
40
+function useFoo(cond1, cond2) {
41
+ let s = null;
42
+ if (cond1) {
43
+ s = {};
44
+ } else {
45
+ return null;
46
+ }
47
+
48
+ if (cond2) {
49
+ mutate(s);
50
+ }
51
+
52
+ return s;
53
+}
compiler/packages/snap/src/SproutTodoFilter.ts
+2
@@ -487,6 +487,8 @@ const skipFilter = new Set([
487
"bug-invalid-hoisting-functionexpr",
488
"original-reactive-scopes-fork/bug-nonmutating-capture-in-unsplittable-memo-block",
489
"original-reactive-scopes-fork/bug-hoisted-declaration-with-scope",
490
+ "bug-codegen-inline-iife",
491
+ "bug-array-concat-should-capture",
492
493
// 'react-compiler-runtime' not yet supported
494
"flag-enable-emit-hook-guards",
compiler/packages/snap/src/sprout/shared-runtime.ts
+25
-19
@@ -36,7 +36,7 @@ export const CONST_NUMBER2 = 2;
36
export const CONST_TRUE = true;
37
export const CONST_FALSE = false;
38
39
-export function initFbt() {
39
+export function initFbt(): void {
40
const viewerContext: IntlViewerContext = {
41
GENDER: IntlVariations.GENDER_UNKNOWN,
42
locale: "en_US",
@@ -52,7 +52,7 @@ export function initFbt() {
52
53
export function mutate(arg: any): void {
54
// don't mutate primitive
55
- if (typeof arg === null || typeof arg !== "object") {
55
+ if (arg == null || typeof arg !== "object") {
56
return;
57
}
58
@@ -80,7 +80,7 @@ export function mutateAndReturnNewValue<T>(arg: T): string {
80
81
export function setProperty(arg: any, property: any): void {
82
// don't mutate primitive
83
- if (typeof arg === null || typeof arg !== "object") {
83
+ if (arg == null || typeof arg !== "object") {
84
return arg;
85
}
86
@@ -123,7 +123,7 @@ export function calculateExpensiveNumber(x: number): number {
123
/**
124
* Functions that do not mutate their parameters
125
*/
126
-export function shallowCopy(obj: Object): object {
126
+export function shallowCopy(obj: object): object {
127
return Object.assign({}, obj);
128
}
129
@@ -139,9 +139,11 @@ export function addOne(value: number): number {
139
return value + 1;
140
}
141
142
-// Alias console.log, as it is defined as a global and may have
143
-// different compiler handling than unknown functions
144
-export function print(...args: Array<unknown>) {
142
+/*
143
+ * Alias console.log, as it is defined as a global and may have
144
+ * different compiler handling than unknown functions
145
+ */
146
+export function print(...args: Array<unknown>): void {
147
console.log(...args);
148
}
149
@@ -153,7 +155,7 @@ export function throwErrorWithMessage(message: string): never {
155
throw new Error(message);
156
}
157
156
-export function throwInput(x: Object): never {
158
+export function throwInput(x: object): never {
159
throw x;
160
}
161
@@ -167,12 +169,12 @@ export function logValue<T>(value: T): void {
169
console.log(value);
170
}
171
170
-export function useHook(): Object {
172
+export function useHook(): object {
173
return makeObject_Primitives();
174
}
175
176
const noAliasObject = Object.freeze({});
175
-export function useNoAlias(...args: Array<any>): object {
177
+export function useNoAlias(..._args: Array<any>): object {
178
return noAliasObject;
179
}
180
@@ -183,7 +185,7 @@ export function useIdentity<T>(arg: T): T {
185
export function invoke<T extends Array<any>, ReturnType>(
186
fn: (...input: T) => ReturnType,
187
...params: T
186
-) {
188
+): ReturnType {
189
return fn(...params);
190
}
191
@@ -191,7 +193,7 @@ export function conditionalInvoke<T extends Array<any>, ReturnType>(
193
shouldInvoke: boolean,
194
fn: (...input: T) => ReturnType,
195
...params: T
194
-) {
196
+): ReturnType | null {
197
if (shouldInvoke) {
198
return fn(...params);
199
} else {
@@ -205,21 +207,25 @@ export function conditionalInvoke<T extends Array<any>, ReturnType>(
207
export function Text(props: {
208
value: string;
209
children?: Array<React.ReactNode>;
208
-}) {
210
+}): React.ReactElement {
211
return React.createElement("div", null, props.value, props.children);
212
}
213
212
-export function StaticText1(props: { children?: Array<React.ReactNode> }) {
214
+export function StaticText1(props: {
215
+ children?: Array<React.ReactNode>;
216
+}): React.ReactElement {
217
return React.createElement("div", null, "StaticText1", props.children);
218
}
219
216
-export function StaticText2(props: { children?: Array<React.ReactNode> }) {
220
+export function StaticText2(props: {
221
+ children?: Array<React.ReactNode>;
222
+}): React.ReactElement {
223
return React.createElement("div", null, "StaticText2", props.children);
224
}
225
226
export function RenderPropAsChild(props: {
227
items: Array<() => React.ReactNode>;
222
-}) {
228
+}): React.ReactElement {
229
return React.createElement(
230
"div",
231
null,
@@ -242,7 +248,7 @@ export function ValidateMemoization({
248
}: {
249
inputs: Array<any>;
250
output: any;
245
-}) {
251
+}): React.ReactElement {
252
"use no forget";
253
const [previousInputs, setPreviousInputs] = React.useState(inputs);
254
const [previousOutput, setPreviousOutput] = React.useState(output);
@@ -273,7 +279,7 @@ export function createHookWrapper<TProps, TRet>(
279
}
280
281
// helper functions
276
-export function toJSON(value: any, invokeFns: boolean = false) {
282
+export function toJSON(value: any, invokeFns: boolean = false): string {
283
const seen = new Map();
284
285
return JSON.stringify(value, (_key: string, val: any) => {
@@ -319,7 +325,7 @@ export const ObjectWithHooks = {
325
},
326
};
327
322
-export function useFragment(...args: Array<any>): Object {
328
+export function useFragment(..._args: Array<any>): object {
329
return {
330
a: [1, 2, 3],
331
b: { c: { d: 4 } },