From 629c19cffe6a74798cce9d4f743ebe0ffbb3aaa2 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Mon, 23 Jun 2025 17:38:24 -0700 Subject: [PATCH] [compiler] Alternate take on ref validation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Some of the false positives we've seen have to do with the need to align our ref validation with our understanding of which functions may be called during render. The new mutability/aliasing model makes this much more explicit, with the ability to create Impure effects which we then throw as errors if they are reachable during render. This means we can now revisit ref validation by just emitting impure effects. That's what this new pass does. It's a bit simpler: it implements the check for `ref.current == null` guarded if blocks. Otherwise it disallows access to `ref.current` specifically. Unlike before, we intentionally allow passing ref objects to functions — we just see a lot of many false positives on disallowing things like `children({ref})` or similar. Open to feedback! This is also still WIP. --- .../src/Entrypoint/Pipeline.ts | 2 +- .../Inference/InferMutationAliasingEffects.ts | 129 ++++++++++++++++++ .../compiler/error.hook-ref-value.expect.md | 2 - ...invalid-access-ref-during-render.expect.md | 2 + ...-callback-invoked-during-render-.expect.md | 17 ++- ...tating-refs-in-render-transitive.expect.md | 16 +-- ...d-ref-prop-in-render-destructure.expect.md | 4 + ...ref-prop-in-render-property-load.expect.md | 4 + ...n-callback-invoked-during-render.expect.md | 17 ++- ...d-set-and-read-ref-during-render.expect.md | 2 - .../error.ref-initialization-nonif.expect.md | 4 +- ...ia-function-preserve-memoization.expect.md | 12 +- ...-current-aliased-no-added-to-dep.expect.md | 4 +- .../ref-current-aliased-no-added-to-dep.js | 2 +- 14 files changed, 179 insertions(+), 38 deletions(-) diff --git a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts index c5ca3434b1..d06b53c1b6 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts @@ -285,7 +285,7 @@ function runWithEnvironment( } if (env.config.validateRefAccessDuringRender) { - validateNoRefAccessInRender(hir).unwrap(); + // validateNoRefAccessInRender(hir).unwrap(); } if (env.config.validateNoSetStateInRender) { 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 b91b606d50..37defd18b3 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts @@ -28,7 +28,9 @@ import { isMapType, isPrimitiveType, isRefOrRefValue, + isRefValueType, isSetType, + isUseRefType, makeIdentifierId, Phi, Place, @@ -219,6 +221,9 @@ export function inferMutationAliasingEffects( } } } + if (fn.env.config.validateRefAccessDuringRender) { + inferRefAccessEffects(fn, isFunctionExpression); + } return Ok(undefined); } @@ -2513,3 +2518,127 @@ export type AbstractValue = { kind: ValueKind; reason: ReadonlySet; }; + +function inferRefAccessEffects( + fn: HIRFunction, + _isFunctionExpression: boolean, +): void { + const nullish = new Set(); + const nullishTest = new Map(); + let guard: {ref: IdentifierId; fallthrough: BlockId} | null = null; + const temporaries: Map = new Map(); + + function visitOperand(operand: Place): AliasingEffect | null { + const nullTestRef = nullishTest.get(operand.identifier.id); + if (isRefValueType(operand.identifier) || nullTestRef != null) { + const refOperand = nullTestRef ?? operand; + return { + kind: 'Impure', + error: { + severity: ErrorSeverity.InvalidReact, + reason: + 'Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef)', + loc: refOperand.loc, + description: + refOperand.identifier.name !== null && + refOperand.identifier.name.kind === 'named' + ? `Cannot access ref value \`${refOperand.identifier.name.value}\`` + : null, + suggestions: null, + }, + place: refOperand, + }; + } + return null; + } + + for (const block of fn.body.blocks.values()) { + if (guard !== null && guard.fallthrough === block.id) { + guard = null; + } + for (const instr of block.instructions) { + const {lvalue, value} = instr; + if (value.kind === 'LoadLocal' && isUseRefType(value.place.identifier)) { + temporaries.set(lvalue.identifier.id, value.place); + } else if ( + value.kind === 'StoreLocal' && + isUseRefType(value.value.identifier) + ) { + temporaries.set(value.lvalue.place.identifier.id, value.value); + temporaries.set(lvalue.identifier.id, value.value); + } else if ( + value.kind === 'BinaryExpression' && + ((isRefValueType(value.left.identifier) && + nullish.has(value.right.identifier.id)) || + (nullish.has(value.left.identifier.id) && + isRefValueType(value.right.identifier))) + ) { + const refOperand = isRefValueType(value.left.identifier) + ? value.left + : value.right; + const operand = temporaries.get(refOperand.identifier.id) ?? refOperand; + nullishTest.set(lvalue.identifier.id, operand); + } else if (value.kind === 'Primitive' && value.value == null) { + nullish.add(lvalue.identifier.id); + } else if ( + value.kind === 'PropertyLoad' && + isUseRefType(value.object.identifier) && + value.property === 'current' + ) { + const refOperand = + temporaries.get(value.object.identifier.id) ?? value.object; + temporaries.set(lvalue.identifier.id, refOperand); + } else if ( + value.kind === 'PropertyStore' && + value.property === 'current' && + isUseRefType(value.object.identifier) + ) { + const refOperand = + temporaries.get(value.object.identifier.id) ?? value.object; + if (guard != null && refOperand.identifier.id === guard.ref) { + // Allow a single write within the guard + guard = null; + } else { + instr.effects ??= []; + instr.effects.push({ + kind: 'Impure', + error: { + severity: ErrorSeverity.InvalidReact, + reason: + 'Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef)', + loc: value.loc, + description: + value.object.identifier.name !== null && + value.object.identifier.name.kind === 'named' + ? `Cannot access ref value \`${value.object.identifier.name.value}\`` + : null, + suggestions: null, + }, + place: value.object, + }); + } + } else { + for (const operand of eachInstructionValueOperand(value)) { + const error = visitOperand(operand); + if (error) { + instr.effects ??= []; + instr.effects.push(error); + } + } + } + } + if ( + guard == null && + block.terminal.kind === 'if' && + nullishTest.has(block.terminal.test.identifier.id) + ) { + const ref = nullishTest.get(block.terminal.test.identifier.id)!; + guard = {ref: ref.identifier.id, fallthrough: block.terminal.fallthrough}; + } else { + for (const operand of eachTerminalOperand(block.terminal)) { + const _effect = visitOperand(operand); + // TODO: need a place to store terminal effects generically + } + } + } +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.hook-ref-value.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.hook-ref-value.expect.md index d92d918fe9..05a2b3cc0e 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.hook-ref-value.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.hook-ref-value.expect.md @@ -24,8 +24,6 @@ export const FIXTURE_ENTRYPOINT = { 4 | const ref = useRef(); > 5 | useEffect(() => {}, [ref.current]); | ^^^^^^^^^^^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (5:5) - -InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (5:5) 6 | } 7 | 8 | export const FIXTURE_ENTRYPOINT = { diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-access-ref-during-render.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-access-ref-during-render.expect.md index 0274836645..5e65ecbc1b 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-access-ref-during-render.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-access-ref-during-render.expect.md @@ -19,6 +19,8 @@ function Component(props) { 3 | const ref = useRef(null); > 4 | const value = ref.current; | ^^^^^^^^^^^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (4:4) + +InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef). Cannot access ref value `value` (5:5) 5 | return value; 6 | } 7 | diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-aliased-ref-in-callback-invoked-during-render-.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-aliased-ref-in-callback-invoked-during-render-.expect.md index e2ce2cceae..135b547d23 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-aliased-ref-in-callback-invoked-during-render-.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-aliased-ref-in-callback-invoked-during-render-.expect.md @@ -19,12 +19,17 @@ function Component(props) { ## Error ``` - 7 | return ; - 8 | }; -> 9 | return {props.items.map(item => renderItem(item))}; - | ^^^^^^^^^^^^^^^^^^^^^^^^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (9:9) - 10 | } - 11 | + 4 | const renderItem = item => { + 5 | const aliasedRef = ref; +> 6 | const current = aliasedRef.current; + | ^^^^^^^^^^^^^^^^^^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (6:6) + +InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef). Cannot access ref value `current` (7:7) + +InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (7:7) + 7 | return ; + 8 | }; + 9 | return {props.items.map(item => renderItem(item))}; ``` \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-disallow-mutating-refs-in-render-transitive.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-disallow-mutating-refs-in-render-transitive.expect.md index f23ff6f3c8..3d5902bdb3 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-disallow-mutating-refs-in-render-transitive.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-disallow-mutating-refs-in-render-transitive.expect.md @@ -21,15 +21,13 @@ function Component() { ## Error ``` - 7 | }; - 8 | const changeRef = setRef; -> 9 | changeRef(); - | ^^^^^^^^^ InvalidReact: This function accesses a ref value (the `current` property), which may not be accessed during render. (https://react.dev/reference/react/useRef) (9:9) - -InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (9:9) - 10 | - 11 | return