From 8219b34b601028bc2092bde61dcaa804f5d5919e Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Tue, 9 Sep 2025 13:15:18 -0700 Subject: [PATCH 1/2] [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: [{}], +}; From 5eb09a72c48334f5425d528b50c91848b6195daf Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Tue, 9 Sep 2025 17:48:23 -0700 Subject: [PATCH 2/2] [compiler] More flexible/helpful lazy ref initialization Two small QoL improvements inspired by feedback: * `if (ref.current == undefined) { ref.current = ... }` is now allowed. * `if (!ref.current) { ref.current = ... }` is still disallowed, but we emit an extra hint suggesting the `if (!ref.current == null)` pattern. I was on the fence about the latter. We got feedback asking to allow `if (!ref.current)` but if your ref stores a boolean value then this would allow reading the ref in render. The unary form is also less precise in general due to sketchy truthiness conversions. I figured a hint is a good compromise. --- .../Validation/ValidateNoRefAccessInRender.ts | 43 ++++++++++ ...low-ref-initialization-undefined.expect.md | 42 ++++++++++ .../allow-ref-initialization-undefined.js | 14 ++++ ....invalid-ref-access-render-unary.expect.md | 78 +++++++++++++++++++ .../error.invalid-ref-access-render-unary.js | 13 ++++ ...lid-ref-initialization-unary-not.expect.md | 43 ++++++++++ ...or.invalid-ref-initialization-unary-not.js | 14 ++++ 7 files changed, 247 insertions(+) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-ref-initialization-undefined.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-ref-initialization-undefined.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-ref-access-render-unary.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-ref-access-render-unary.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-ref-initialization-unary-not.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-ref-initialization-unary-not.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoRefAccessInRender.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoRefAccessInRender.ts index eb053ac4196a..abbb7d847698 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoRefAccessInRender.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoRefAccessInRender.ts @@ -639,12 +639,55 @@ function validateNoRefAccessInRenderImpl( case 'StartMemoize': case 'FinishMemoize': break; + case 'LoadGlobal': { + if (instr.value.binding.name === 'undefined') { + env.set(instr.lvalue.identifier.id, {kind: 'Nullable'}); + } + break; + } case 'Primitive': { if (instr.value.value == null) { env.set(instr.lvalue.identifier.id, {kind: 'Nullable'}); } break; } + case 'UnaryExpression': { + if (instr.value.operator === '!') { + const value = env.get(instr.value.value.identifier.id); + const refId = + value?.kind === 'RefValue' && value.refId != null + ? value.refId + : null; + if (refId !== null) { + /* + * Record an error suggesting the `if (ref.current == null)` pattern, + * but also record the lvalue as a guard so that we don't emit a second + * error for the write to the ref + */ + env.set(instr.lvalue.identifier.id, {kind: 'Guard', refId}); + errors.pushDiagnostic( + CompilerDiagnostic.create({ + category: ErrorCategory.Refs, + reason: 'Cannot access refs during render', + description: ERROR_DESCRIPTION, + }) + .withDetails({ + kind: 'error', + loc: instr.value.value.loc, + message: `Cannot access ref value during render`, + }) + .withDetails({ + kind: 'hint', + message: + 'To initialize a ref only once, check that the ref is null with the pattern `if (ref.current == null) { ref.current = ... }`', + }), + ); + break; + } + } + validateNoRefValueAccess(errors, env, instr.value.value); + break; + } case 'BinaryExpression': { const left = env.get(instr.value.left.identifier.id); const right = env.get(instr.value.right.identifier.id); diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-ref-initialization-undefined.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-ref-initialization-undefined.expect.md new file mode 100644 index 000000000000..c355819320af --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-ref-initialization-undefined.expect.md @@ -0,0 +1,42 @@ + +## Input + +```javascript +//@flow +import {useRef} from 'react'; + +component C() { + const r = useRef(null); + if (r.current == undefined) { + r.current = 1; + } +} + +export const FIXTURE_ENTRYPOINT = { + fn: C, + params: [{}], +}; + +``` + +## Code + +```javascript +import { useRef } from "react"; + +function C() { + const r = useRef(null); + if (r.current == undefined) { + r.current = 1; + } +} + +export const FIXTURE_ENTRYPOINT = { + fn: C, + params: [{}], +}; + +``` + +### Eval output +(kind: ok) \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-ref-initialization-undefined.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-ref-initialization-undefined.js new file mode 100644 index 000000000000..886f025287bb --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-ref-initialization-undefined.js @@ -0,0 +1,14 @@ +//@flow +import {useRef} from 'react'; + +component C() { + const r = useRef(null); + if (r.current == undefined) { + r.current = 1; + } +} + +export const FIXTURE_ENTRYPOINT = { + fn: C, + params: [{}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-ref-access-render-unary.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-ref-access-render-unary.expect.md new file mode 100644 index 000000000000..ce1be800a13b --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-ref-access-render-unary.expect.md @@ -0,0 +1,78 @@ + +## Input + +```javascript +//@flow +import {useRef} from 'react'; + +component C() { + const r = useRef(null); + const current = !r.current; + return
{current}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: C, + params: [{}], +}; + +``` + + +## Error + +``` +Found 4 errors: + +Error: Cannot access refs during render + +React refs are values that are not needed for rendering. Refs should only be accessed outside of render, such as in event handlers or effects. Accessing a ref value (the `current` property) during render can cause your component not to update as expected (https://react.dev/reference/react/useRef). + + 4 | component C() { + 5 | const r = useRef(null); +> 6 | const current = !r.current; + | ^^^^^^^^^ Cannot access ref value during render + 7 | return
{current}
; + 8 | } + 9 | + +To initialize a ref only once, check that the ref is null with the pattern `if (ref.current == null) { ref.current = ... }` + +Error: Cannot access refs during render + +React refs are values that are not needed for rendering. Refs should only be accessed outside of render, such as in event handlers or effects. Accessing a ref value (the `current` property) during render can cause your component not to update as expected (https://react.dev/reference/react/useRef). + + 4 | component C() { + 5 | const r = useRef(null); +> 6 | const current = !r.current; + | ^^^^^^^^^^ Cannot access ref value during render + 7 | return
{current}
; + 8 | } + 9 | + +Error: Cannot access refs during render + +React refs are values that are not needed for rendering. Refs should only be accessed outside of render, such as in event handlers or effects. Accessing a ref value (the `current` property) during render can cause your component not to update as expected (https://react.dev/reference/react/useRef). + + 5 | const r = useRef(null); + 6 | const current = !r.current; +> 7 | return
{current}
; + | ^^^^^^^ Cannot access ref value during render + 8 | } + 9 | + 10 | export const FIXTURE_ENTRYPOINT = { + +Error: Cannot access refs during render + +React refs are values that are not needed for rendering. Refs should only be accessed outside of render, such as in event handlers or effects. Accessing a ref value (the `current` property) during render can cause your component not to update as expected (https://react.dev/reference/react/useRef). + + 5 | const r = useRef(null); + 6 | const current = !r.current; +> 7 | return
{current}
; + | ^^^^^^^ Cannot access ref value during render + 8 | } + 9 | + 10 | export const FIXTURE_ENTRYPOINT = { +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-ref-access-render-unary.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-ref-access-render-unary.js new file mode 100644 index 000000000000..8d99008f805b --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-ref-access-render-unary.js @@ -0,0 +1,13 @@ +//@flow +import {useRef} from 'react'; + +component C() { + const r = useRef(null); + const current = !r.current; + return
{current}
; +} + +export const FIXTURE_ENTRYPOINT = { + fn: C, + params: [{}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-ref-initialization-unary-not.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-ref-initialization-unary-not.expect.md new file mode 100644 index 000000000000..516d006c205a --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-ref-initialization-unary-not.expect.md @@ -0,0 +1,43 @@ + +## Input + +```javascript +//@flow +import {useRef} from 'react'; + +component C() { + const r = useRef(null); + if (!r.current) { + r.current = 1; + } +} + +export const FIXTURE_ENTRYPOINT = { + fn: C, + params: [{}], +}; + +``` + + +## Error + +``` +Found 1 error: + +Error: Cannot access refs during render + +React refs are values that are not needed for rendering. Refs should only be accessed outside of render, such as in event handlers or effects. Accessing a ref value (the `current` property) during render can cause your component not to update as expected (https://react.dev/reference/react/useRef). + + 4 | component C() { + 5 | const r = useRef(null); +> 6 | if (!r.current) { + | ^^^^^^^^^ Cannot access ref value during render + 7 | r.current = 1; + 8 | } + 9 | } + +To initialize a ref only once, check that the ref is null with the pattern `if (ref.current == null) { ref.current = ... }` +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-ref-initialization-unary-not.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-ref-initialization-unary-not.js new file mode 100644 index 000000000000..b9b5d41295cd --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-ref-initialization-unary-not.js @@ -0,0 +1,14 @@ +//@flow +import {useRef} from 'react'; + +component C() { + const r = useRef(null); + if (!r.current) { + r.current = 1; + } +} + +export const FIXTURE_ENTRYPOINT = { + fn: C, + params: [{}], +};