[compiler] Fix invalid Array.map type (#32095)
See test fixture --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/32095). * #32099 * #32104 * #32098 * #32097 * #32096 * __->__ #32095 * #32094 * #32093
mofeiZ committed
Jan 22, 2025 at 15:02 UTC
b83090fca2d96283a5c6153abb65eaa5cc81c9ba
3 files changed
+224
-1
compiler/packages/babel-plugin-react-compiler/src/HIR/ObjectShape.ts
+9
-1
@@ -549,8 +549,16 @@ addObject(BUILTIN_SHAPES, BuiltInMixedReadonlyId, [
549
[
550
'map',
551
addFunction(BUILTIN_SHAPES, [], {
552
+ /**
553
+ * Note `map`'s arguments are annotated as Effect.ConditionallyMutate as
554
+ * calling `<array>.map(fn)` might invoke `fn`, which means replaying its
555
+ * effects.
556
+ *
557
+ * (Note that Effect.Read / Effect.Capture on a function type means
558
+ * potential data dependency or aliasing respectively.)
559
+ */
560
positionalParams: [],
553
- restParam: Effect.Read,
561
+ restParam: Effect.ConditionallyMutate,
562
returnType: {kind: 'Object', shapeId: BuiltInArrayId},
563
calleeEffect: Effect.ConditionallyMutate,
564
returnValueKind: ValueKind.Mutable,
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/mixedreadonly-mutating-map.expect.md
new
+156
@@ -0,0 +1,156 @@
1
+
2
+## Input
3
+
4
+```javascript
5
+import {
6
+ arrayPush,
7
+ identity,
8
+ makeArray,
9
+ Stringify,
10
+ useFragment,
11
+} from 'shared-runtime';
12
+
13
+/**
14
+ * Bug repro showing why it's invalid for function references to be annotated
15
+ * with a `Read` effect when that reference might lead to the function being
16
+ * invoked.
17
+ *
18
+ * Note that currently, `Array.map` is annotated to have `Read` effects on its
19
+ * operands. This is incorrect as function effects must be replayed when `map`
20
+ * is called
21
+ * - Read: non-aliasing data dependency
22
+ * - Capture: maybe-aliasing data dependency
23
+ * - ConditionallyMutate: maybe-aliasing data dependency; maybe-write / invoke
24
+ * but only if the value is mutable
25
+ *
26
+ * Invalid evaluator result: Found differences in evaluator results Non-forget
27
+ * (expected): (kind: ok)
28
+ * <div>{"x":[2,2,2],"count":3}</div><div>{"item":1}</div>
29
+ * <div>{"x":[2,2,2],"count":4}</div><div>{"item":1}</div>
30
+ * Forget:
31
+ * (kind: ok)
32
+ * <div>{"x":[2,2,2],"count":3}</div><div>{"item":1}</div>
33
+ * <div>{"x":[2,2,2,2,2,2],"count":4}</div><div>{"item":1}</div>
34
+ */
35
+
36
+function Component({extraJsx}) {
37
+ const x = makeArray();
38
+ const items = useFragment();
39
+ // This closure has the following effects that must be replayed:
40
+ // - MaybeFreeze / Capture of `items`
41
+ // - ConditionalMutate of x
42
+ const jsx = items.a.map((item, i) => {
43
+ arrayPush(x, 2);
44
+ return <Stringify item={item} key={i} />;
45
+ });
46
+ const offset = jsx.length;
47
+ for (let i = 0; i < extraJsx; i++) {
48
+ jsx.push(<Stringify item={0} key={i + offset} />);
49
+ }
50
+ const count = jsx.length;
51
+ identity(count);
52
+ return (
53
+ <>
54
+ <Stringify x={x} count={count} />
55
+ {jsx[0]}
56
+ </>
57
+ );
58
+}
59
+export const FIXTURE_ENTRYPOINT = {
60
+ fn: Component,
61
+ params: [{extraJsx: 0}],
62
+ sequentialRenders: [{extraJsx: 0}, {extraJsx: 1}],
63
+};
64
+
65
+```
66
+
67
+## Code
68
+
69
+```javascript
70
+import { c as _c } from "react/compiler-runtime";
71
+import {
72
+ arrayPush,
73
+ identity,
74
+ makeArray,
75
+ Stringify,
76
+ useFragment,
77
+} from "shared-runtime";
78
+
79
+/**
80
+ * Bug repro showing why it's invalid for function references to be annotated
81
+ * with a `Read` effect when that reference might lead to the function being
82
+ * invoked.
83
+ *
84
+ * Note that currently, `Array.map` is annotated to have `Read` effects on its
85
+ * operands. This is incorrect as function effects must be replayed when `map`
86
+ * is called
87
+ * - Read: non-aliasing data dependency
88
+ * - Capture: maybe-aliasing data dependency
89
+ * - ConditionallyMutate: maybe-aliasing data dependency; maybe-write / invoke
90
+ * but only if the value is mutable
91
+ *
92
+ * Invalid evaluator result: Found differences in evaluator results Non-forget
93
+ * (expected): (kind: ok)
94
+ * <div>{"x":[2,2,2],"count":3}</div><div>{"item":1}</div>
95
+ * <div>{"x":[2,2,2],"count":4}</div><div>{"item":1}</div>
96
+ * Forget:
97
+ * (kind: ok)
98
+ * <div>{"x":[2,2,2],"count":3}</div><div>{"item":1}</div>
99
+ * <div>{"x":[2,2,2,2,2,2],"count":4}</div><div>{"item":1}</div>
100
+ */
101
+
102
+function Component(t0) {
103
+ const $ = _c(6);
104
+ const { extraJsx } = t0;
105
+ const x = makeArray();
106
+ const items = useFragment();
107
+
108
+ const jsx = items.a.map((item, i) => {
109
+ arrayPush(x, 2);
110
+ return <Stringify item={item} key={i} />;
111
+ });
112
+ const offset = jsx.length;
113
+ for (let i_0 = 0; i_0 < extraJsx; i_0++) {
114
+ jsx.push(<Stringify item={0} key={i_0 + offset} />);
115
+ }
116
+
117
+ const count = jsx.length;
118
+ identity(count);
119
+ let t1;
120
+ if ($[0] !== count || $[1] !== x) {
121
+ t1 = <Stringify x={x} count={count} />;
122
+ $[0] = count;
123
+ $[1] = x;
124
+ $[2] = t1;
125
+ } else {
126
+ t1 = $[2];
127
+ }
128
+ const t2 = jsx[0];
129
+ let t3;
130
+ if ($[3] !== t1 || $[4] !== t2) {
131
+ t3 = (
132
+ <>
133
+ {t1}
134
+ {t2}
135
+ </>
136
+ );
137
+ $[3] = t1;
138
+ $[4] = t2;
139
+ $[5] = t3;
140
+ } else {
141
+ t3 = $[5];
142
+ }
143
+ return t3;
144
+}
145
+
146
+export const FIXTURE_ENTRYPOINT = {
147
+ fn: Component,
148
+ params: [{ extraJsx: 0 }],
149
+ sequentialRenders: [{ extraJsx: 0 }, { extraJsx: 1 }],
150
+};
151
+
152
+```
153
+
154
+### Eval output
155
+(kind: ok) <div>{"x":[2,2,2],"count":3}</div><div>{"item":1}</div>
156
+<div>{"x":[2,2,2],"count":4}</div><div>{"item":1}</div>
\ No newline at end of file
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/mixedreadonly-mutating-map.js
new
+59
@@ -0,0 +1,59 @@
1
+import {
2
+ arrayPush,
3
+ identity,
4
+ makeArray,
5
+ Stringify,
6
+ useFragment,
7
+} from 'shared-runtime';
8
+
9
+/**
10
+ * Bug repro showing why it's invalid for function references to be annotated
11
+ * with a `Read` effect when that reference might lead to the function being
12
+ * invoked.
13
+ *
14
+ * Note that currently, `Array.map` is annotated to have `Read` effects on its
15
+ * operands. This is incorrect as function effects must be replayed when `map`
16
+ * is called
17
+ * - Read: non-aliasing data dependency
18
+ * - Capture: maybe-aliasing data dependency
19
+ * - ConditionallyMutate: maybe-aliasing data dependency; maybe-write / invoke
20
+ * but only if the value is mutable
21
+ *
22
+ * Invalid evaluator result: Found differences in evaluator results Non-forget
23
+ * (expected): (kind: ok)
24
+ * <div>{"x":[2,2,2],"count":3}</div><div>{"item":1}</div>
25
+ * <div>{"x":[2,2,2],"count":4}</div><div>{"item":1}</div>
26
+ * Forget:
27
+ * (kind: ok)
28
+ * <div>{"x":[2,2,2],"count":3}</div><div>{"item":1}</div>
29
+ * <div>{"x":[2,2,2,2,2,2],"count":4}</div><div>{"item":1}</div>
30
+ */
31
+
32
+function Component({extraJsx}) {
33
+ const x = makeArray();
34
+ const items = useFragment();
35
+ // This closure has the following effects that must be replayed:
36
+ // - MaybeFreeze / Capture of `items`
37
+ // - ConditionalMutate of x
38
+ const jsx = items.a.map((item, i) => {
39
+ arrayPush(x, 2);
40
+ return <Stringify item={item} key={i} />;
41
+ });
42
+ const offset = jsx.length;
43
+ for (let i = 0; i < extraJsx; i++) {
44
+ jsx.push(<Stringify item={0} key={i + offset} />);
45
+ }
46
+ const count = jsx.length;
47
+ identity(count);
48
+ return (
49
+ <>
50
+ <Stringify x={x} count={count} />
51
+ {jsx[0]}
52
+ </>
53
+ );
54
+}
55
+export const FIXTURE_ENTRYPOINT = {
56
+ fn: Component,
57
+ params: [{extraJsx: 0}],
58
+ sequentialRenders: [{extraJsx: 0}, {extraJsx: 1}],
59
+};