[compiler] Stop relying on identifier mutable ranges after constructing scopes
Addresses discussion at https://github.com/facebook/react/pull/30399#discussion_r1684693021. Once we've constructed scopes it's invalid to use identifier mutable ranges. The only places we can do this which i can find are ValidateMemoizedEffectDeps (which is already flawed and disabled by default) and ValidatePreservedManualMemoization. I added a todo to the former, and fixed up the latter. The idea of the fix is that for StartMemo dependencies, if they needed to be memoized (identifier.scope != null) then that scope should exist and should have already completed. If they didn't need a scope or can't have one created (eg their range spans a hook), then their scope would be pruned. So if the scope is set, not pruned, and not completed, then it's an error. For declarations (FinishMemo) the existing logic applies unchanged. ghstack-source-id: af5bfd88553de3e30621695f9d139c4dc5efb997 Pull Request resolved: https://github.com/facebook/react/pull/30428
Joe Savona committed
Jul 24, 2024 at 09:14 UTC
b943feba35476196fa674de276f2b87624086896
7 files changed
+76
-61
compiler/packages/babel-plugin-react-compiler/src/HIR/BuildReactiveScopeTerminalsHIR.ts
-20
@@ -9,7 +9,6 @@ import {
9
GotoVariant,
10
HIRFunction,
11
InstructionId,
12
- makeInstructionId,
12
ReactiveScope,
13
ReactiveScopeTerminal,
14
ScopeId,
@@ -19,7 +18,6 @@ import {
18
markPredecessors,
19
reversePostorderBlocks,
20
} from './HIRBuilder';
22
-import {eachInstructionLValue} from './visitors';
21
22
/**
23
* This pass assumes that all program blocks are properly nested with respect to fallthroughs
@@ -179,24 +177,6 @@ export function buildReactiveScopeTerminalsHIR(fn: HIRFunction): void {
177
* Fix scope and identifier ranges to account for renumbered instructions
178
*/
179
for (const [, block] of fn.body.blocks) {
182
- for (const instruction of block.instructions) {
183
- for (const lvalue of eachInstructionLValue(instruction)) {
184
- /*
185
- * Any lvalues whose mutable range was a single instruction must have
186
- * started at the current instruction, so update the range to match
187
- * the instruction's new id
188
- */
189
- if (
190
- lvalue.identifier.mutableRange.end ===
191
- lvalue.identifier.mutableRange.start + 1
192
- ) {
193
- lvalue.identifier.mutableRange.start = instruction.id;
194
- lvalue.identifier.mutableRange.end = makeInstructionId(
195
- instruction.id + 1,
196
- );
197
- }
198
- }
199
- }
180
const terminal = block.terminal;
181
if (terminal.kind === 'scope' || terminal.kind === 'pruned-scope') {
182
/*
compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateMemoizedEffectDependencies.ts
+4
@@ -99,6 +99,10 @@ class Visitor extends ReactiveFunctionVisitor<CompilerError> {
99
const deps = instruction.value.args[1]!;
100
if (
101
deps.kind === 'Identifier' &&
102
+ /*
103
+ * TODO: isMutable is not safe to call here as it relies on identifier mutableRange which is no longer valid at this point
104
+ * in the pipeline
105
+ */
106
(isMutable(instruction as Instruction, deps) ||
107
isUnmemoized(deps.identifier, this.scopes))
108
) {
compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts
+14
-3
@@ -10,10 +10,10 @@ import {
10
GeneratedSource,
11
Identifier,
12
IdentifierId,
13
- Instruction,
13
InstructionValue,
14
ManualMemoDependency,
15
Place,
16
+ PrunedReactiveScopeBlock,
17
ReactiveFunction,
18
ReactiveInstruction,
19
ReactiveScopeBlock,
@@ -25,7 +25,6 @@ import {
25
import {printManualMemoDependency} from '../HIR/PrintHIR';
26
import {eachInstructionValueOperand} from '../HIR/visitors';
27
import {collectMaybeMemoDependencies} from '../Inference/DropManualMemoization';
28
-import {isMutable} from '../ReactiveScopes/InferReactiveScopeVariables';
28
import {
29
ReactiveFunctionVisitor,
30
visitReactiveFunction,
@@ -277,6 +276,7 @@ function validateInferredDep(
276
277
class Visitor extends ReactiveFunctionVisitor<VisitorState> {
278
scopes: Set<ScopeId> = new Set();
279
+ prunedScopes: Set<ScopeId> = new Set();
280
scopeMapping = new Map();
281
temporaries: Map<IdentifierId, ManualMemoDependency> = new Map();
282
@@ -414,6 +414,14 @@ class Visitor extends ReactiveFunctionVisitor<VisitorState> {
414
}
415
}
416
417
+ override visitPrunedScope(
418
+ scopeBlock: PrunedReactiveScopeBlock,
419
+ state: VisitorState,
420
+ ): void {
421
+ this.traversePrunedScope(scopeBlock, state);
422
+ this.prunedScopes.add(scopeBlock.scope.id);
423
+ }
424
+
425
override visitInstruction(
426
instruction: ReactiveInstruction,
427
state: VisitorState,
@@ -464,7 +472,10 @@ class Visitor extends ReactiveFunctionVisitor<VisitorState> {
472
instruction.value as InstructionValue,
473
)) {
474
if (
467
- isMutable(instruction as Instruction, value) ||
475
+ (isDep &&
476
+ value.identifier.scope != null &&
477
+ !this.scopes.has(value.identifier.scope.id) &&
478
+ !this.prunedScopes.has(value.identifier.scope.id)) ||
479
(isDecl && isUnmemoized(value.identifier, this.scopes))
480
) {
481
state.errors.push({
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.false-positive-useMemo-with-refs.flow.expect.md
deleted
-38
@@ -1,38 +0,0 @@
1
-
2
-## Input
3
-
4
-```javascript
5
-// @flow @validatePreserveExistingMemoizationGuarantees
6
-import { identity } from "shared-runtime";
7
-
8
-component Component(
9
- disableLocalRef,
10
- ref,
11
-) {
12
- const localRef = useFooRef();
13
- const mergedRef = useMemo(() => {
14
- return disableLocalRef ? ref : identity(ref, localRef);
15
- }, [disableLocalRef, ref, localRef]);
16
- return <div ref={mergedRef} />;
17
-}
18
-
19
-```
20
-
21
-
22
-## Error
23
-
24
-```
25
- 7 | ) {
26
- 8 | const localRef = useFooRef();
27
-> 9 | const mergedRef = useMemo(() => {
28
- | ^^^^^^^
29
-> 10 | return disableLocalRef ? ref : identity(ref, localRef);
30
- | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
31
-> 11 | }, [disableLocalRef, ref, localRef]);
32
- | ^^^^ CannotPreserveMemoization: React Compiler has skipped optimizing this component because the existing manual memoization could not be preserved. This value may be mutated later, which could cause the value to change unexpectedly (9:11)
33
- 12 | return <div ref={mergedRef} />;
34
- 13 | }
35
- 14 |
36
-```
37
-
38
-
\ No newline at end of file
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useMemo-with-refs.flow.expect.md
new
+57
@@ -0,0 +1,57 @@
1
+
2
+## Input
3
+
4
+```javascript
5
+// @flow @validatePreserveExistingMemoizationGuarantees
6
+import { identity } from "shared-runtime";
7
+
8
+component Component(
9
+ disableLocalRef,
10
+ ref,
11
+) {
12
+ const localRef = useFooRef();
13
+ const mergedRef = useMemo(() => {
14
+ return disableLocalRef ? ref : identity(ref, localRef);
15
+ }, [disableLocalRef, ref, localRef]);
16
+ return <div ref={mergedRef} />;
17
+}
18
+
19
+```
20
+
21
+## Code
22
+
23
+```javascript
24
+import { c as _c } from "react/compiler-runtime";
25
+import { identity } from "shared-runtime";
26
+
27
+const Component = React.forwardRef(Component_withRef);
28
+function Component_withRef(t0, ref) {
29
+ const $ = _c(6);
30
+ const { disableLocalRef } = t0;
31
+ const localRef = useFooRef();
32
+ let t1;
33
+ let t2;
34
+ if ($[0] !== disableLocalRef || $[1] !== ref || $[2] !== localRef) {
35
+ t2 = disableLocalRef ? ref : identity(ref, localRef);
36
+ $[0] = disableLocalRef;
37
+ $[1] = ref;
38
+ $[2] = localRef;
39
+ $[3] = t2;
40
+ } else {
41
+ t2 = $[3];
42
+ }
43
+ t1 = t2;
44
+ const mergedRef = t1;
45
+ let t3;
46
+ if ($[4] !== mergedRef) {
47
+ t3 = <div ref={mergedRef} />;
48
+ $[4] = mergedRef;
49
+ $[5] = t3;
50
+ } else {
51
+ t3 = $[5];
52
+ }
53
+ return t3;
54
+}
55
+
56
+```
57
+
\ No newline at end of file
compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useMemo-with-refs.flow.js
renamed
compiler/packages/snap/src/SproutTodoFilter.ts
+1
@@ -398,6 +398,7 @@ const skipFilter = new Set([
398
'deeply-nested-function-expressions-with-params',
399
'readonly-object-method-calls',
400
'readonly-object-method-calls-mutable-lambda',
401
+ 'preserve-memo-validation/useMemo-with-refs.flow',
402
403
// TODO: we probably want to always skip these
404
'rules-of-hooks/rules-of-hooks-0592bd574811',