From 58a19b2e7a7c2fdd166edac05d9bda9e109ac945 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Fri, 30 May 2025 16:30:03 -0700 Subject: [PATCH] [compiler] Fix mutable ranges for StoreContext [ghstack-poisoned] --- .../Inference/InferMutationAliasingEffects.ts | 35 +++++++++++++-- .../Inference/InferMutationAliasingRanges.ts | 5 ++- ...back-captures-reassigned-context.expect.md | 43 +++++++++++++++++++ ...useCallback-captures-reassigned-context.js | 20 +++++++++ .../new-mutability/reactive-ref.expect.md | 22 ++++++---- 5 files changed, 112 insertions(+), 13 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/error.invalid-useCallback-captures-reassigned-context.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/error.invalid-useCallback-captures-reassigned-context.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts index 2e99e60f639c2..0fd3a31c5ebf4 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts @@ -15,6 +15,7 @@ import { Hole, IdentifierId, Instruction, + InstructionKind, InstructionValue, isArrayType, isMapType, @@ -768,7 +769,7 @@ function applyEffect( } } -const DEBUG = true; +const DEBUG = false; class InferenceState { env: Environment; @@ -1452,7 +1453,21 @@ function computeSignatureForInstruction( } break; } - case 'DeclareContext': + case 'DeclareContext': { + // Context variables are conceptually like mutable boxes + effects.push({ + kind: 'Create', + into: value.lvalue.place, + value: ValueKind.Mutable, + }); + effects.push({ + kind: 'Create', + into: lvalue, + // The result can't be referenced so this value doesn't matter + value: ValueKind.Primitive, + }); + break; + } case 'DeclareLocal': { // TODO check this effects.push({ @@ -1481,13 +1496,25 @@ function computeSignatureForInstruction( break; } case 'LoadContext': { + // We load from the context effects.push({kind: 'Assign', from: value.place, into: lvalue}); break; } case 'StoreContext': { - effects.push({kind: 'Mutate', value: value.lvalue.place}); + // We're storing into the conceptual box + if (value.lvalue.kind === InstructionKind.Reassign) { + effects.push({kind: 'Mutate', value: value.lvalue.place}); + } else { + // Context variables are conceptually like mutable boxes + effects.push({ + kind: 'Create', + into: value.lvalue.place, + value: ValueKind.Mutable, + }); + } + // Which aliases the value effects.push({ - kind: 'Assign', + kind: 'Alias', from: value.value, into: value.lvalue.place, }); diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingRanges.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingRanges.ts index 98edefb52fc35..3e6f82999a9e4 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingRanges.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingRanges.ts @@ -20,6 +20,7 @@ import { } from '../HIR/visitors'; import DisjointSet from '../Utils/DisjointSet'; import {assertExhaustive} from '../Utils/utils'; +import {debugAliases} from './InferMutableRanges'; import {inferMutableRangesForAlias} from './InferMutableRangesForAlias'; /** @@ -44,7 +45,9 @@ export function inferMutationAliasingRanges(fn: HIRFunction): void { for (const instr of block.instructions) { for (const lvalue of eachInstructionLValue(instr)) { - lvalue.identifier.mutableRange.start = instr.id; + if (lvalue.identifier.mutableRange.start === 0) { + lvalue.identifier.mutableRange.start = instr.id; + } lvalue.identifier.mutableRange.end = makeInstructionId( Math.max(instr.id + 1, lvalue.identifier.mutableRange.end), ); diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/error.invalid-useCallback-captures-reassigned-context.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/error.invalid-useCallback-captures-reassigned-context.expect.md new file mode 100644 index 0000000000000..498f3d8a0766f --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/error.invalid-useCallback-captures-reassigned-context.expect.md @@ -0,0 +1,43 @@ + +## Input + +```javascript +// @validatePreserveExistingMemoizationGuarantees @enableNewMutationAliasingModel +import {useCallback} from 'react'; +import {makeArray} from 'shared-runtime'; + +// This case is already unsound in source, so we can safely bailout +function Foo(props) { + let x = []; + x.push(props); + + // makeArray() is captured, but depsList contains [props] + const cb = useCallback(() => [x], [x]); + + x = makeArray(); + + return cb; +} +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{}], +}; + +``` + + +## Error + +``` + 9 | + 10 | // makeArray() is captured, but depsList contains [props] +> 11 | const cb = useCallback(() => [x], [x]); + | ^ CannotPreserveMemoization: React Compiler has skipped optimizing this component because the existing manual memoization could not be preserved. This dependency may be mutated later, which could cause the value to change unexpectedly (11:11) + +CannotPreserveMemoization: React Compiler has skipped optimizing this component because the existing manual memoization could not be preserved. This value was memoized in source but not in compilation output. (11:11) + 12 | + 13 | x = makeArray(); + 14 | +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/error.invalid-useCallback-captures-reassigned-context.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/error.invalid-useCallback-captures-reassigned-context.js new file mode 100644 index 0000000000000..b9b914d30ec90 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/error.invalid-useCallback-captures-reassigned-context.js @@ -0,0 +1,20 @@ +// @validatePreserveExistingMemoizationGuarantees @enableNewMutationAliasingModel +import {useCallback} from 'react'; +import {makeArray} from 'shared-runtime'; + +// This case is already unsound in source, so we can safely bailout +function Foo(props) { + let x = []; + x.push(props); + + // makeArray() is captured, but depsList contains [props] + const cb = useCallback(() => [x], [x]); + + x = makeArray(); + + return cb; +} +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/reactive-ref.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/reactive-ref.expect.md index 7939a03da7293..26757db1a3c28 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/reactive-ref.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/new-mutability/reactive-ref.expect.md @@ -22,22 +22,28 @@ function ReactiveRefInEffect(props) { ```javascript import { c as _c } from "react/compiler-runtime"; // @enableNewMutationAliasingModel function ReactiveRefInEffect(props) { - const $ = _c(2); + const $ = _c(4); const ref1 = useRef("initial value"); const ref2 = useRef("initial value"); let ref; - if (props.foo) { - ref = ref1; + if ($[0] !== props.foo) { + if (props.foo) { + ref = ref1; + } else { + ref = ref2; + } + $[0] = props.foo; + $[1] = ref; } else { - ref = ref2; + ref = $[1]; } let t0; - if ($[0] !== ref) { + if ($[2] !== ref) { t0 = () => print(ref); - $[0] = ref; - $[1] = t0; + $[2] = ref; + $[3] = t0; } else { - t0 = $[1]; + t0 = $[3]; } useEffect(t0); }