[compiler] bugfix for hoistable deps for nested functions (#31345)
`PropertyPathRegistry` is responsible for uniqueing identifier and property paths. This is necessary for the hoistability CFG merging logic which takes unions and intersections of these nodes to determine a basic block's hoistable reads, as a function of its neighbors. We also depend on this to merge optional chained and non-optional chained property paths This fixes a small bug in #31066 in which we create a new registry for nested functions. Now, we use the same registry for a component / hook and all its inner functions ' --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/31345). * #31204 * #31202 * #31203 * #31201 * #31200 * #31346 * #31199 * #31431 * __->__ #31345 * #31197
mofeiZ committed
Nov 5, 2024 at 15:25 UTC
e7e269b7265ec94929a53f4d402037261c87cf44
4 files changed
+138
-39
compiler/packages/babel-plugin-react-compiler/src/HIR/CollectHoistablePropertyLoads.ts
+62
-38
@@ -1,5 +1,6 @@
1
import {CompilerError} from '../CompilerError';
2
import {inRange} from '../ReactiveScopes/InferReactiveScopeVariables';
3
+import {printDependency} from '../ReactiveScopes/PrintReactiveFunction';
4
import {
5
Set_equal,
6
Set_filter,
@@ -23,6 +24,8 @@ import {
24
} from './HIR';
25
import {collectTemporariesSidemap} from './PropagateScopeDependenciesHIR';
26
27
+const DEBUG_PRINT = false;
28
+
29
/**
30
* Helper function for `PropagateScopeDependencies`. Uses control flow graph
31
* analysis to determine which `Identifier`s can be assumed to be non-null
@@ -86,15 +89,8 @@ export function collectHoistablePropertyLoads(
89
fn: HIRFunction,
90
temporaries: ReadonlyMap<IdentifierId, ReactiveScopeDependency>,
91
hoistableFromOptionals: ReadonlyMap<BlockId, ReactiveScopeDependency>,
89
- nestedFnImmutableContext: ReadonlySet<IdentifierId> | null,
92
): ReadonlyMap<BlockId, BlockInfo> {
93
const registry = new PropertyPathRegistry();
92
-
93
- const functionExpressionLoads = collectFunctionExpressionFakeLoads(fn);
94
- const actuallyEvaluatedTemporaries = new Map(
95
- [...temporaries].filter(([id]) => !functionExpressionLoads.has(id)),
96
- );
97
-
94
/**
95
* Due to current limitations of mutable range inference, there are edge cases in
96
* which we infer known-immutable values (e.g. props or hook params) to have a
@@ -111,14 +107,51 @@ export function collectHoistablePropertyLoads(
107
}
108
}
109
}
114
- const nodes = collectNonNullsInBlocks(fn, {
115
- temporaries: actuallyEvaluatedTemporaries,
110
+ return collectHoistablePropertyLoadsImpl(fn, {
111
+ temporaries,
112
knownImmutableIdentifiers,
113
hoistableFromOptionals,
114
registry,
119
- nestedFnImmutableContext,
115
+ nestedFnImmutableContext: null,
116
});
121
- propagateNonNull(fn, nodes, registry);
117
+}
118
+
119
+type CollectHoistablePropertyLoadsContext = {
120
+ temporaries: ReadonlyMap<IdentifierId, ReactiveScopeDependency>;
121
+ knownImmutableIdentifiers: ReadonlySet<IdentifierId>;
122
+ hoistableFromOptionals: ReadonlyMap<BlockId, ReactiveScopeDependency>;
123
+ registry: PropertyPathRegistry;
124
+ /**
125
+ * (For nested / inner function declarations)
126
+ * Context variables (i.e. captured from an outer scope) that are immutable.
127
+ * Note that this technically could be merged into `knownImmutableIdentifiers`,
128
+ * but are currently kept separate for readability.
129
+ */
130
+ nestedFnImmutableContext: ReadonlySet<IdentifierId> | null;
131
+};
132
+function collectHoistablePropertyLoadsImpl(
133
+ fn: HIRFunction,
134
+ context: CollectHoistablePropertyLoadsContext,
135
+): ReadonlyMap<BlockId, BlockInfo> {
136
+ const functionExpressionLoads = collectFunctionExpressionFakeLoads(fn);
137
+ const actuallyEvaluatedTemporaries = new Map(
138
+ [...context.temporaries].filter(([id]) => !functionExpressionLoads.has(id)),
139
+ );
140
+
141
+ const nodes = collectNonNullsInBlocks(fn, {
142
+ ...context,
143
+ temporaries: actuallyEvaluatedTemporaries,
144
+ });
145
+ propagateNonNull(fn, nodes, context.registry);
146
+
147
+ if (DEBUG_PRINT) {
148
+ console.log('(printing hoistable nodes in blocks)');
149
+ for (const [blockId, node] of nodes) {
150
+ console.log(
151
+ `bb${blockId}: ${[...node.assumedNonNullObjects].map(n => printDependency(n.fullPath)).join(' ')}`,
152
+ );
153
+ }
154
+ }
155
156
return nodes;
157
}
@@ -243,7 +276,7 @@ class PropertyPathRegistry {
276
277
function getMaybeNonNullInInstruction(
278
instr: InstructionValue,
246
- context: CollectNonNullsInBlocksContext,
279
+ context: CollectHoistablePropertyLoadsContext,
280
): PropertyPathNode | null {
281
let path = null;
282
if (instr.kind === 'PropertyLoad') {
@@ -262,7 +295,7 @@ function getMaybeNonNullInInstruction(
295
function isImmutableAtInstr(
296
identifier: Identifier,
297
instr: InstructionId,
265
- context: CollectNonNullsInBlocksContext,
298
+ context: CollectHoistablePropertyLoadsContext,
299
): boolean {
300
if (context.nestedFnImmutableContext != null) {
301
/**
@@ -295,22 +328,9 @@ function isImmutableAtInstr(
328
}
329
}
330
298
-type CollectNonNullsInBlocksContext = {
299
- temporaries: ReadonlyMap<IdentifierId, ReactiveScopeDependency>;
300
- knownImmutableIdentifiers: ReadonlySet<IdentifierId>;
301
- hoistableFromOptionals: ReadonlyMap<BlockId, ReactiveScopeDependency>;
302
- registry: PropertyPathRegistry;
303
- /**
304
- * (For nested / inner function declarations)
305
- * Context variables (i.e. captured from an outer scope) that are immutable.
306
- * Note that this technically could be merged into `knownImmutableIdentifiers`,
307
- * but are currently kept separate for readability.
308
- */
309
- nestedFnImmutableContext: ReadonlySet<IdentifierId> | null;
310
-};
331
function collectNonNullsInBlocks(
332
fn: HIRFunction,
313
- context: CollectNonNullsInBlocksContext,
333
+ context: CollectHoistablePropertyLoadsContext,
334
): ReadonlyMap<BlockId, BlockInfo> {
335
/**
336
* Known non-null objects such as functional component props can be safely
@@ -358,18 +378,22 @@ function collectNonNullsInBlocks(
378
new Set(),
379
);
380
const innerOptionals = collectOptionalChainSidemap(innerFn.func);
361
- const innerHoistableMap = collectHoistablePropertyLoads(
381
+ const innerHoistableMap = collectHoistablePropertyLoadsImpl(
382
innerFn.func,
363
- innerTemporaries,
364
- innerOptionals.hoistableObjects,
365
- context.nestedFnImmutableContext ??
366
- new Set(
367
- innerFn.func.context
368
- .filter(place =>
369
- isImmutableAtInstr(place.identifier, instr.id, context),
370
- )
371
- .map(place => place.identifier.id),
372
- ),
383
+ {
384
+ ...context,
385
+ temporaries: innerTemporaries, // TODO: remove in later PR
386
+ hoistableFromOptionals: innerOptionals.hoistableObjects, // TODO: remove in later PR
387
+ nestedFnImmutableContext:
388
+ context.nestedFnImmutableContext ??
389
+ new Set(
390
+ innerFn.func.context
391
+ .filter(place =>
392
+ isImmutableAtInstr(place.identifier, instr.id, context),
393
+ )
394
+ .map(place => place.identifier.id),
395
+ ),
396
+ },
397
);
398
const innerHoistables = assertNonNull(
399
innerHoistableMap.get(innerFn.func.body.entry),
compiler/packages/babel-plugin-react-compiler/src/HIR/PropagateScopeDependenciesHIR.ts
+1
-1
@@ -46,7 +46,7 @@ export function propagateScopeDependenciesHIR(fn: HIRFunction): void {
46
47
const hoistablePropertyLoads = keyByScopeId(
48
fn,
49
- collectHoistablePropertyLoads(fn, temporaries, hoistableObjects, null),
49
+ collectHoistablePropertyLoads(fn, temporaries, hoistableObjects),
50
);
51
52
const scopeDeps = collectDependencies(
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/repro-invariant.expect.md
new
+61
@@ -0,0 +1,61 @@
1
+
2
+## Input
3
+
4
+```javascript
5
+// @enablePropagateDepsInHIR
6
+import {Stringify} from 'shared-runtime';
7
+
8
+function Foo({data}) {
9
+ return (
10
+ <Stringify foo={() => data.a.d} bar={data.a?.b.c} shouldInvokeFns={true} />
11
+ );
12
+}
13
+
14
+export const FIXTURE_ENTRYPOINT = {
15
+ fn: Foo,
16
+ params: [{data: {a: null}}],
17
+ sequentialRenders: [{data: {a: {b: {c: 4}}}}],
18
+};
19
+
20
+```
21
+
22
+## Code
23
+
24
+```javascript
25
+import { c as _c } from "react/compiler-runtime"; // @enablePropagateDepsInHIR
26
+import { Stringify } from "shared-runtime";
27
+
28
+function Foo(t0) {
29
+ const $ = _c(5);
30
+ const { data } = t0;
31
+ let t1;
32
+ if ($[0] !== data.a.d) {
33
+ t1 = () => data.a.d;
34
+ $[0] = data.a.d;
35
+ $[1] = t1;
36
+ } else {
37
+ t1 = $[1];
38
+ }
39
+ const t2 = data.a?.b.c;
40
+ let t3;
41
+ if ($[2] !== t1 || $[3] !== t2) {
42
+ t3 = <Stringify foo={t1} bar={t2} shouldInvokeFns={true} />;
43
+ $[2] = t1;
44
+ $[3] = t2;
45
+ $[4] = t3;
46
+ } else {
47
+ t3 = $[4];
48
+ }
49
+ return t3;
50
+}
51
+
52
+export const FIXTURE_ENTRYPOINT = {
53
+ fn: Foo,
54
+ params: [{ data: { a: null } }],
55
+ sequentialRenders: [{ data: { a: { b: { c: 4 } } } }],
56
+};
57
+
58
+```
59
+
60
+### Eval output
61
+(kind: ok) <div>{"foo":{"kind":"Function"},"bar":4,"shouldInvokeFns":true}</div>
\ No newline at end of file
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/repro-invariant.tsx
new
+14
@@ -0,0 +1,14 @@
1
+// @enablePropagateDepsInHIR
2
+import {Stringify} from 'shared-runtime';
3
+
4
+function Foo({data}) {
5
+ return (
6
+ <Stringify foo={() => data.a.d} bar={data.a?.b.c} shouldInvokeFns={true} />
7
+ );
8
+}
9
+
10
+export const FIXTURE_ENTRYPOINT = {
11
+ fn: Foo,
12
+ params: [{data: {a: null}}],
13
+ sequentialRenders: [{data: {a: {b: {c: 4}}}}],
14
+};