From 8219b34b601028bc2092bde61dcaa804f5d5919e Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Tue, 9 Sep 2025 13:15:18 -0700 Subject: [PATCH] [compiler] Fix false positive hook return mutation error This was fun. We previously added the MaybeAlias effect in #33984 in order to describe the semantic that an unknown function call _may_ alias its return value in its result, but that we don't know this for sure. We record mutations through MaybeAlias edges when walking backward in the data flow graph, but downgrade them to conditional mutations. See the original PR for full context. That change was sufficient for the original case like ```js const frozen = useContext(); useEffect(() => { frozen.method().property = true; }, [...]); ``` But it wasn't sufficient for cases where the aliasing occured between operands: ```js const dispatch = useDispatch();
{ dispatch(...e.target.value) e.target.value = ...; }} /> ``` Here we would record a `Capture dispatch <- e.target` effect. Then during processing of the `event.target.value = ...` assignment we'd eventually _forward_ from `event` to `dispatch` (along a MaybeAlias edge). But in #33984 I missed that this forward walk also has to downgrade to conditional. In addition to that change, we also have to be a bit more precise about which set of effects we create for alias/capture/maybe-alias. The new logic is a bit clearer, I think: * If the value is frozen, it's an ImmutableCapture edge * If the values are mutable, it's a Capture * If it's a context->context, context->mutable, or mutable->context, count it as MaybeAlias. --- .../Inference/InferMutationAliasingEffects.ts | 74 +++++++++++------ .../Inference/InferMutationAliasingRanges.ts | 15 +++- ...-spread-event-marks-event-frozen.expect.md | 82 +++++++++++++++++++ ...ispatch-spread-event-marks-event-frozen.js | 30 +++++++ 4 files changed, 173 insertions(+), 28 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-dispatch-spread-event-marks-event-frozen.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-dispatch-spread-event-marks-event-frozen.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 8b82e685b0bd..911f71329b44 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts @@ -748,10 +748,14 @@ function applyEffect( case 'Alias': case 'Capture': { CompilerError.invariant( - effect.kind === 'Capture' || initialized.has(effect.into.identifier.id), + effect.kind === 'Capture' || + effect.kind === 'MaybeAlias' || + initialized.has(effect.into.identifier.id), { - reason: `Expected destination value to already be initialized within this instruction for Alias effect`, - description: `Destination ${printPlace(effect.into)} is not initialized in this instruction`, + reason: `Expected destination to already be initialized within this instruction`, + description: + `Destination ${printPlace(effect.into)} is not initialized in this ` + + `instruction for effect ${printAliasingEffect(effect)}`, details: [ { kind: 'error', @@ -767,49 +771,67 @@ function applyEffect( * copy-on-write semantics, then we can prune the effect */ const intoKind = state.kind(effect.into).kind; - let isMutableDesination: boolean; + let destinationType: 'context' | 'mutable' | null = null; switch (intoKind) { - case ValueKind.Context: - case ValueKind.Mutable: - case ValueKind.MaybeFrozen: { - isMutableDesination = true; + case ValueKind.Context: { + destinationType = 'context'; break; } - default: { - isMutableDesination = false; + case ValueKind.Mutable: + case ValueKind.MaybeFrozen: { + destinationType = 'mutable'; break; } } const fromKind = state.kind(effect.from).kind; - let isMutableReferenceType: boolean; + let sourceType: 'context' | 'mutable' | 'frozen' | null = null; switch (fromKind) { + case ValueKind.Context: { + sourceType = 'context'; + break; + } case ValueKind.Global: case ValueKind.Primitive: { - isMutableReferenceType = false; break; } case ValueKind.Frozen: { - isMutableReferenceType = false; - applyEffect( - context, - state, - { - kind: 'ImmutableCapture', - from: effect.from, - into: effect.into, - }, - initialized, - effects, - ); + sourceType = 'frozen'; break; } default: { - isMutableReferenceType = true; + sourceType = 'mutable'; break; } } - if (isMutableDesination && isMutableReferenceType) { + + if (sourceType === 'frozen') { + applyEffect( + context, + state, + { + kind: 'ImmutableCapture', + from: effect.from, + into: effect.into, + }, + initialized, + effects, + ); + } else if ( + (sourceType === 'mutable' && destinationType === 'mutable') || + effect.kind === 'MaybeAlias' + ) { effects.push(effect); + } else if ( + (sourceType === 'context' && destinationType != null) || + (sourceType === 'mutable' && destinationType === 'context') + ) { + applyEffect( + context, + state, + {kind: 'MaybeAlias', from: effect.from, into: effect.into}, + initialized, + effects, + ); } break; } 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 32f84e1e28b2..43148dc4c67f 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingRanges.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingRanges.ts @@ -779,7 +779,13 @@ class AliasingState { if (edge.index >= index) { break; } - queue.push({place: edge.node, transitive, direction: 'forwards', kind}); + queue.push({ + place: edge.node, + transitive, + direction: 'forwards', + // Traversing a maybeAlias edge always downgrades to conditional mutation + kind: edge.kind === 'maybeAlias' ? MutationKind.Conditional : kind, + }); } for (const [alias, when] of node.createdFrom) { if (when >= index) { @@ -807,7 +813,12 @@ class AliasingState { if (when >= index) { continue; } - queue.push({place: alias, transitive, direction: 'backwards', kind}); + queue.push({ + place: alias, + transitive, + direction: 'backwards', + kind, + }); } /** * MaybeAlias indicates potential data flow from unknown function calls, diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-dispatch-spread-event-marks-event-frozen.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-dispatch-spread-event-marks-event-frozen.expect.md new file mode 100644 index 000000000000..699140137ac2 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-dispatch-spread-event-marks-event-frozen.expect.md @@ -0,0 +1,82 @@ + +## Input + +```javascript +// @compilationMode:"infer" +function Component() { + const dispatch = useDispatch(); + // const [state, setState] = useState(0); + + return ( +
+ { + dispatch(...event.target); + event.target.value = ''; + }} + /> +
+ ); +} + +function useDispatch() { + 'use no memo'; + // skip compilation to make it easier to debug the above function + return (...values) => { + console.log(...values); + }; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @compilationMode:"infer" +function Component() { + const $ = _c(2); + const dispatch = useDispatch(); + let t0; + if ($[0] !== dispatch) { + t0 = ( +
+ { + dispatch(...event.target); + event.target.value = ""; + }} + /> +
+ ); + $[0] = dispatch; + $[1] = t0; + } else { + t0 = $[1]; + } + return t0; +} + +function useDispatch() { + "use no memo"; + // skip compilation to make it easier to debug the above function + return (...values) => { + console.log(...values); + }; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{}], +}; + +``` + +### Eval output +(kind: ok)
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-dispatch-spread-event-marks-event-frozen.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-dispatch-spread-event-marks-event-frozen.js new file mode 100644 index 000000000000..386729e40789 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-dispatch-spread-event-marks-event-frozen.js @@ -0,0 +1,30 @@ +// @compilationMode:"infer" +function Component() { + const dispatch = useDispatch(); + // const [state, setState] = useState(0); + + return ( +
+ { + dispatch(...event.target); + event.target.value = ''; + }} + /> +
+ ); +} + +function useDispatch() { + 'use no memo'; + // skip compilation to make it easier to debug the above function + return (...values) => { + console.log(...values); + }; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{}], +};