From b943feba35476196fa674de276f2b87624086896 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Wed, 24 Jul 2024 09:14:23 +0900 Subject: [PATCH] [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 --- .../src/HIR/BuildReactiveScopeTerminalsHIR.ts | 20 ------- .../ValidateMemoizedEffectDependencies.ts | 4 ++ .../ValidatePreservedManualMemoization.ts | 17 +++++- ...-positive-useMemo-with-refs.flow.expect.md | 38 ------------- .../useMemo-with-refs.flow.expect.md | 57 +++++++++++++++++++ ...refs.flow.js => useMemo-with-refs.flow.js} | 0 .../packages/snap/src/SproutTodoFilter.ts | 1 + 7 files changed, 76 insertions(+), 61 deletions(-) delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.false-positive-useMemo-with-refs.flow.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useMemo-with-refs.flow.expect.md rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/{error.false-positive-useMemo-with-refs.flow.js => useMemo-with-refs.flow.js} (100%) diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/BuildReactiveScopeTerminalsHIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/BuildReactiveScopeTerminalsHIR.ts index 6819c2c6c5..0999a3492b 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/BuildReactiveScopeTerminalsHIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/BuildReactiveScopeTerminalsHIR.ts @@ -9,7 +9,6 @@ import { GotoVariant, HIRFunction, InstructionId, - makeInstructionId, ReactiveScope, ReactiveScopeTerminal, ScopeId, @@ -19,7 +18,6 @@ import { markPredecessors, reversePostorderBlocks, } from './HIRBuilder'; -import {eachInstructionLValue} from './visitors'; /** * This pass assumes that all program blocks are properly nested with respect to fallthroughs @@ -179,24 +177,6 @@ export function buildReactiveScopeTerminalsHIR(fn: HIRFunction): void { * Fix scope and identifier ranges to account for renumbered instructions */ for (const [, block] of fn.body.blocks) { - for (const instruction of block.instructions) { - for (const lvalue of eachInstructionLValue(instruction)) { - /* - * Any lvalues whose mutable range was a single instruction must have - * started at the current instruction, so update the range to match - * the instruction's new id - */ - if ( - lvalue.identifier.mutableRange.end === - lvalue.identifier.mutableRange.start + 1 - ) { - lvalue.identifier.mutableRange.start = instruction.id; - lvalue.identifier.mutableRange.end = makeInstructionId( - instruction.id + 1, - ); - } - } - } const terminal = block.terminal; if (terminal.kind === 'scope' || terminal.kind === 'pruned-scope') { /* diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateMemoizedEffectDependencies.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateMemoizedEffectDependencies.ts index dcd12abf34..364e78ae6b 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateMemoizedEffectDependencies.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateMemoizedEffectDependencies.ts @@ -99,6 +99,10 @@ class Visitor extends ReactiveFunctionVisitor { const deps = instruction.value.args[1]!; if ( deps.kind === 'Identifier' && + /* + * TODO: isMutable is not safe to call here as it relies on identifier mutableRange which is no longer valid at this point + * in the pipeline + */ (isMutable(instruction as Instruction, deps) || isUnmemoized(deps.identifier, this.scopes)) ) { diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts index bbf271c50e..3d2b01725c 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts @@ -10,10 +10,10 @@ import { GeneratedSource, Identifier, IdentifierId, - Instruction, InstructionValue, ManualMemoDependency, Place, + PrunedReactiveScopeBlock, ReactiveFunction, ReactiveInstruction, ReactiveScopeBlock, @@ -25,7 +25,6 @@ import { import {printManualMemoDependency} from '../HIR/PrintHIR'; import {eachInstructionValueOperand} from '../HIR/visitors'; import {collectMaybeMemoDependencies} from '../Inference/DropManualMemoization'; -import {isMutable} from '../ReactiveScopes/InferReactiveScopeVariables'; import { ReactiveFunctionVisitor, visitReactiveFunction, @@ -277,6 +276,7 @@ function validateInferredDep( class Visitor extends ReactiveFunctionVisitor { scopes: Set = new Set(); + prunedScopes: Set = new Set(); scopeMapping = new Map(); temporaries: Map = new Map(); @@ -414,6 +414,14 @@ class Visitor extends ReactiveFunctionVisitor { } } + override visitPrunedScope( + scopeBlock: PrunedReactiveScopeBlock, + state: VisitorState, + ): void { + this.traversePrunedScope(scopeBlock, state); + this.prunedScopes.add(scopeBlock.scope.id); + } + override visitInstruction( instruction: ReactiveInstruction, state: VisitorState, @@ -464,7 +472,10 @@ class Visitor extends ReactiveFunctionVisitor { instruction.value as InstructionValue, )) { if ( - isMutable(instruction as Instruction, value) || + (isDep && + value.identifier.scope != null && + !this.scopes.has(value.identifier.scope.id) && + !this.prunedScopes.has(value.identifier.scope.id)) || (isDecl && isUnmemoized(value.identifier, this.scopes)) ) { state.errors.push({ diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.false-positive-useMemo-with-refs.flow.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.false-positive-useMemo-with-refs.flow.expect.md deleted file mode 100644 index 369fd75571..0000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.false-positive-useMemo-with-refs.flow.expect.md +++ /dev/null @@ -1,38 +0,0 @@ - -## Input - -```javascript -// @flow @validatePreserveExistingMemoizationGuarantees -import { identity } from "shared-runtime"; - -component Component( - disableLocalRef, - ref, -) { - const localRef = useFooRef(); - const mergedRef = useMemo(() => { - return disableLocalRef ? ref : identity(ref, localRef); - }, [disableLocalRef, ref, localRef]); - return
; -} - -``` - - -## Error - -``` - 7 | ) { - 8 | const localRef = useFooRef(); -> 9 | const mergedRef = useMemo(() => { - | ^^^^^^^ -> 10 | return disableLocalRef ? ref : identity(ref, localRef); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -> 11 | }, [disableLocalRef, ref, localRef]); - | ^^^^ 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) - 12 | return
; - 13 | } - 14 | -``` - - \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useMemo-with-refs.flow.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useMemo-with-refs.flow.expect.md new file mode 100644 index 0000000000..e70a64a126 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useMemo-with-refs.flow.expect.md @@ -0,0 +1,57 @@ + +## Input + +```javascript +// @flow @validatePreserveExistingMemoizationGuarantees +import { identity } from "shared-runtime"; + +component Component( + disableLocalRef, + ref, +) { + const localRef = useFooRef(); + const mergedRef = useMemo(() => { + return disableLocalRef ? ref : identity(ref, localRef); + }, [disableLocalRef, ref, localRef]); + return
; +} + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import { identity } from "shared-runtime"; + +const Component = React.forwardRef(Component_withRef); +function Component_withRef(t0, ref) { + const $ = _c(6); + const { disableLocalRef } = t0; + const localRef = useFooRef(); + let t1; + let t2; + if ($[0] !== disableLocalRef || $[1] !== ref || $[2] !== localRef) { + t2 = disableLocalRef ? ref : identity(ref, localRef); + $[0] = disableLocalRef; + $[1] = ref; + $[2] = localRef; + $[3] = t2; + } else { + t2 = $[3]; + } + t1 = t2; + const mergedRef = t1; + let t3; + if ($[4] !== mergedRef) { + t3 =
; + $[4] = mergedRef; + $[5] = t3; + } else { + t3 = $[5]; + } + return t3; +} + +``` + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.false-positive-useMemo-with-refs.flow.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useMemo-with-refs.flow.js similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.false-positive-useMemo-with-refs.flow.js rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/useMemo-with-refs.flow.js diff --git a/compiler/packages/snap/src/SproutTodoFilter.ts b/compiler/packages/snap/src/SproutTodoFilter.ts index ebc49f68ab..25a3d19947 100644 --- a/compiler/packages/snap/src/SproutTodoFilter.ts +++ b/compiler/packages/snap/src/SproutTodoFilter.ts @@ -398,6 +398,7 @@ const skipFilter = new Set([ 'deeply-nested-function-expressions-with-params', 'readonly-object-method-calls', 'readonly-object-method-calls-mutable-lambda', + 'preserve-memo-validation/useMemo-with-refs.flow', // TODO: we probably want to always skip these 'rules-of-hooks/rules-of-hooks-0592bd574811',