compiler: Promote pruned scope declarations to temporaries if used in a later scope
There's a category of bug currently where pruned reactive scopes whose outputs are non-reactive can have their code end up inlining into another scope, moving the location of the instruction. Any value that had a scope assigned has to have its order of evaluation preserved, despite the fact that it got pruned, so naively we could just force every pruned scope to have its declarations promoted to named variables. However, that ends up assigning names to _tons_ of scope declarations that don't really need to be promoted. For example, a scope with just a hook call ends up with: ``` const x = useFoo(); => scope { $t0 = Call read useFoo$ (...); } $t1 = StoreLocal 'x' = read $t0; ``` Where t0 doesn't need to be promoted since it's used immediately to assign to another value which is a non-temporary. So the idea of this PR is that we can track outputs of pruned scopes which are directly referenced from inside a later scope. This fixes one of the two cases of the above pattern. We'll also likely have to consider values from pruned scopes as always reactive, i'll do that in the next PR. ghstack-source-id: b37fb9a7cb1430b7c35ec5946269ce5a886a486a Pull Request resolved: https://github.com/facebook/react/pull/29789
Joe Savona committed
Jun 7, 2024 at 12:10 UTC
d193455fc37a91045112fbf0240a289a45173de0
4 files changed
+96
-27
compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts
+3
-3
@@ -36,7 +36,7 @@ import {
36
getHookKind,
37
makeIdentifierName,
38
} from "../HIR/HIR";
39
-import { printPlace } from "../HIR/PrintHIR";
39
+import { printIdentifier, printPlace } from "../HIR/PrintHIR";
40
import { eachPatternOperand } from "../HIR/visitors";
41
import { Err, Ok, Result } from "../Utils/Result";
42
import { GuardKind } from "../Utils/RuntimeDiagnosticConstants";
@@ -524,8 +524,8 @@ function codegenReactiveScope(
524
}
525
526
CompilerError.invariant(identifier.name != null, {
527
- reason: `Expected identifier '@${identifier.id}' to be named`,
528
- description: null,
527
+ reason: `Expected scope declaration identifier to be named`,
528
+ description: `Declaration \`${printIdentifier(identifier)}\` is unnamed in scope @${scope.id}`,
529
loc: null,
530
suggestions: null,
531
});
compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PromoteUsedTemporaries.ts
+73
-19
@@ -12,21 +12,20 @@ import {
12
IdentifierId,
13
InstructionId,
14
Place,
15
+ PrunedReactiveScopeBlock,
16
ReactiveFunction,
17
ReactiveScopeBlock,
18
ReactiveValue,
19
+ ScopeId,
20
promoteTemporary,
21
promoteTemporaryJsxTag,
22
} from "../HIR/HIR";
23
import { ReactiveFunctionVisitor, visitReactiveFunction } from "./visitors";
24
23
-type VisitorState = {
24
- tags: JsxExpressionTags;
25
-};
26
-class Visitor extends ReactiveFunctionVisitor<VisitorState> {
27
- override visitScope(block: ReactiveScopeBlock, state: VisitorState): void {
28
- this.traverseScope(block, state);
29
- for (const dep of block.scope.dependencies) {
25
+class Visitor extends ReactiveFunctionVisitor<State> {
26
+ override visitScope(scopeBlock: ReactiveScopeBlock, state: State): void {
27
+ this.traverseScope(scopeBlock, state);
28
+ for (const dep of scopeBlock.scope.dependencies) {
29
const { identifier } = dep;
30
if (identifier.name == null) {
31
promoteIdentifier(identifier, state);
@@ -39,14 +38,29 @@ class Visitor extends ReactiveFunctionVisitor<VisitorState> {
38
* Many of our current test fixtures do not return a value, so
39
* it is better for now to promote (and memoize) every output.
40
*/
42
- for (const [, declaration] of block.scope.declarations) {
41
+ for (const [, declaration] of scopeBlock.scope.declarations) {
42
if (declaration.identifier.name == null) {
43
promoteIdentifier(declaration.identifier, state);
44
}
45
}
46
}
47
49
- override visitParam(place: Place, state: VisitorState): void {
48
+ override visitPrunedScope(
49
+ scopeBlock: PrunedReactiveScopeBlock,
50
+ state: State
51
+ ): void {
52
+ this.traversePrunedScope(scopeBlock, state);
53
+ for (const [, declaration] of scopeBlock.scope.declarations) {
54
+ if (
55
+ declaration.identifier.name == null &&
56
+ state.pruned.get(declaration.identifier.id)?.usedOutsideScope === true
57
+ ) {
58
+ promoteIdentifier(declaration.identifier, state);
59
+ }
60
+ }
61
+ }
62
+
63
+ override visitParam(place: Place, state: State): void {
64
if (place.identifier.name === null) {
65
promoteIdentifier(place.identifier, state);
66
}
@@ -55,7 +69,7 @@ class Visitor extends ReactiveFunctionVisitor<VisitorState> {
69
override visitValue(
70
id: InstructionId,
71
value: ReactiveValue,
58
- state: VisitorState
72
+ state: State
73
): void {
74
this.traverseValue(id, value, state);
75
if (value.kind === "FunctionExpression" || value.kind === "ObjectMethod") {
@@ -67,7 +81,7 @@ class Visitor extends ReactiveFunctionVisitor<VisitorState> {
81
_id: InstructionId,
82
_dependencies: Array<Place>,
83
fn: ReactiveFunction,
70
- state: VisitorState
84
+ state: State
85
): void {
86
for (const operand of fn.params) {
87
const place = operand.kind === "Identifier" ? operand : operand.place;
@@ -80,25 +94,65 @@ class Visitor extends ReactiveFunctionVisitor<VisitorState> {
94
}
95
96
type JsxExpressionTags = Set<IdentifierId>;
83
-class CollectJsxTagsVisitor extends ReactiveFunctionVisitor<JsxExpressionTags> {
97
+type State = {
98
+ tags: JsxExpressionTags;
99
+ pruned: Map<
100
+ IdentifierId,
101
+ { activeScopes: Array<ScopeId>; usedOutsideScope: boolean }
102
+ >; // true if referenced within another scope, false if only accessed outside of scopes
103
+};
104
+
105
+class CollectPromotableTemporaries extends ReactiveFunctionVisitor<State> {
106
+ activeScopes: Array<ScopeId> = [];
107
+
108
+ override visitPlace(_id: InstructionId, place: Place, state: State): void {
109
+ if (
110
+ this.activeScopes.length !== 0 &&
111
+ state.pruned.has(place.identifier.id)
112
+ ) {
113
+ const prunedPlace = state.pruned.get(place.identifier.id)!;
114
+ if (prunedPlace.activeScopes.indexOf(this.activeScopes.at(-1)!) === -1) {
115
+ prunedPlace.usedOutsideScope = true;
116
+ }
117
+ }
118
+ }
119
+
120
override visitValue(
121
id: InstructionId,
122
value: ReactiveValue,
87
- state: JsxExpressionTags
123
+ state: State
124
): void {
125
this.traverseValue(id, value, state);
126
if (value.kind === "JsxExpression" && value.tag.kind === "Identifier") {
91
- state.add(value.tag.identifier.id);
127
+ state.tags.add(value.tag.identifier.id);
128
}
129
}
130
+
131
+ override visitPrunedScope(
132
+ scopeBlock: PrunedReactiveScopeBlock,
133
+ state: State
134
+ ): void {
135
+ for (const [id] of scopeBlock.scope.declarations) {
136
+ state.pruned.set(id, {
137
+ activeScopes: [...this.activeScopes],
138
+ usedOutsideScope: false,
139
+ });
140
+ }
141
+ }
142
+
143
+ override visitScope(scopeBlock: ReactiveScopeBlock, state: State): void {
144
+ this.activeScopes.push(scopeBlock.scope.id);
145
+ this.traverseScope(scopeBlock, state);
146
+ this.activeScopes.pop();
147
+ }
148
}
149
150
export function promoteUsedTemporaries(fn: ReactiveFunction): void {
97
- const tags: JsxExpressionTags = new Set();
98
- visitReactiveFunction(fn, new CollectJsxTagsVisitor(), tags);
99
- const state: VisitorState = {
100
- tags,
151
+ const state: State = {
152
+ tags: new Set(),
153
+ pruned: new Map(),
154
};
155
+ visitReactiveFunction(fn, new CollectPromotableTemporaries(), state);
156
for (const operand of fn.params) {
157
const place = operand.kind === "Identifier" ? operand : operand.place;
158
if (place.identifier.name === null) {
@@ -108,7 +162,7 @@ export function promoteUsedTemporaries(fn: ReactiveFunction): void {
162
visitReactiveFunction(fn, new Visitor(), state);
163
}
164
111
-function promoteIdentifier(identifier: Identifier, state: VisitorState): void {
165
+function promoteIdentifier(identifier: Identifier, state: State): void {
166
CompilerError.invariant(identifier.name === null, {
167
reason:
168
"promoteTemporary: Expected to be called only for temporary variables",
compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PropagateScopeDependencies.ts
+14
@@ -18,6 +18,7 @@ import {
18
isUseRefType,
19
makeInstructionId,
20
Place,
21
+ PrunedReactiveScopeBlock,
22
ReactiveFunction,
23
ReactiveInstruction,
24
ReactiveScope,
@@ -687,6 +688,19 @@ class PropagationVisitor extends ReactiveFunctionVisitor<Context> {
688
scope.scope.dependencies = scopeDependencies;
689
}
690
691
+ override visitPrunedScope(
692
+ scopeBlock: PrunedReactiveScopeBlock,
693
+ context: Context
694
+ ): void {
695
+ /*
696
+ * NOTE: we explicitly throw away the deps, we only enter() the scope to record its
697
+ * declarations
698
+ */
699
+ const _scopeDepdencies = context.enter(scopeBlock.scope, () => {
700
+ this.visitBlock(scopeBlock.instructions, context);
701
+ });
702
+ }
703
+
704
override visitInstruction(
705
instruction: ReactiveInstruction,
706
context: Context
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-invalid-reactivity-value-block.expect.md
+6
-5
@@ -71,14 +71,15 @@ function Foo() {
71
useNoAlias();
72
73
const shouldCaptureObj = obj != null && CONST_TRUE;
74
- let t0;
74
+ const t0 = shouldCaptureObj ? identity(obj) : null;
75
+ let t1;
76
if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
76
- t0 = [shouldCaptureObj ? identity(obj) : null, obj];
77
- $[0] = t0;
77
+ t1 = [t0, obj];
78
+ $[0] = t1;
79
} else {
79
- t0 = $[0];
80
+ t1 = $[0];
81
}
81
- const result = t0;
82
+ const result = t1;
83
84
useNoAlias(result, obj);
85
if (shouldCaptureObj && result[0] !== obj) {