From 476a869295e7c5eeca9ff82fdc384c3309124964 Mon Sep 17 00:00:00 2001 From: Mike Vitousek Date: Fri, 16 Aug 2024 12:48:23 -0400 Subject: [PATCH] [compiler] Reposition ref-in-render errors to the read location of .current Summary: Since we want to make ref-in-render errors enabled by default, we should position those errors at the location of the read. Not only will this be a better experience, but it also aligns the behavior of Forget and Flow. This PR also cleans up the resulting error messages to not emit implementation details about place values. [ghstack-poisoned] --- .../Validation/ValidateNoRefAccesInRender.ts | 73 +++++++++++++++---- ...invalid-access-ref-during-render.expect.md | 7 +- ...tating-refs-in-render-transitive.expect.md | 2 +- ...d-ref-prop-in-render-destructure.expect.md | 7 +- ...ref-prop-in-render-property-load.expect.md | 7 +- ...error.invalid-ref-value-as-props.expect.md | 2 +- ...d-set-and-read-ref-during-render.expect.md | 2 +- ...ef-nested-property-during-render.expect.md | 4 +- .../error.return-ref-callback.expect.md | 2 +- ...ified-later-preserve-memoization.expect.md | 2 +- ...ia-function-preserve-memoization.expect.md | 2 +- .../useCallback-ref-in-render.expect.md | 2 +- .../compiler/useCallback-ref-in-render.js | 2 +- 13 files changed, 81 insertions(+), 33 deletions(-) diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoRefAccesInRender.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoRefAccesInRender.ts index a855d9f8b414e..4da6b23a1a9ed 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoRefAccesInRender.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoRefAccesInRender.ts @@ -15,7 +15,6 @@ import { isRefValueType, isUseRefType, } from '../HIR'; -import {printPlace} from '../HIR/PrintHIR'; import { eachInstructionValueOperand, eachTerminalOperand, @@ -53,6 +52,7 @@ function validateNoRefAccessInRenderImpl( refAccessingFunctions: Set, ): Result { const errors = new CompilerError(); + const lookupLocations: Map = new Map(); for (const [, block] of fn.body.blocks) { for (const instr of block.instructions) { switch (instr.value.kind) { @@ -64,10 +64,12 @@ function validateNoRefAccessInRenderImpl( severity: ErrorSeverity.InvalidReact, reason: 'Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef)', - loc: operand.loc, - description: `Cannot access ref value at ${printPlace( - operand, - )}`, + loc: lookupLocations.get(operand.identifier.id) ?? operand.loc, + description: + operand.identifier.name !== null && + operand.identifier.name.kind === 'named' + ? `Cannot access ref value \`${operand.identifier.name.value}\`` + : null, suggestions: null, }); } @@ -75,12 +77,24 @@ function validateNoRefAccessInRenderImpl( break; } case 'PropertyLoad': { + if ( + isRefValueType(instr.lvalue.identifier) && + instr.value.property === 'current' + ) { + lookupLocations.set(instr.lvalue.identifier.id, instr.loc); + } break; } case 'LoadLocal': { if (refAccessingFunctions.has(instr.value.place.identifier.id)) { refAccessingFunctions.add(instr.lvalue.identifier.id); } + if (isRefValueType(instr.lvalue.identifier)) { + const loc = lookupLocations.get(instr.value.place.identifier.id); + if (loc !== undefined) { + lookupLocations.set(instr.lvalue.identifier.id, loc); + } + } break; } case 'StoreLocal': { @@ -88,6 +102,13 @@ function validateNoRefAccessInRenderImpl( refAccessingFunctions.add(instr.value.lvalue.place.identifier.id); refAccessingFunctions.add(instr.lvalue.identifier.id); } + if (isRefValueType(instr.value.lvalue.place.identifier)) { + const loc = lookupLocations.get(instr.value.value.identifier.id); + if (loc !== undefined) { + lookupLocations.set(instr.value.lvalue.place.identifier.id, loc); + lookupLocations.set(instr.lvalue.identifier.id, loc); + } + } break; } case 'ObjectMethod': @@ -140,7 +161,11 @@ function validateNoRefAccessInRenderImpl( reason: 'This function accesses a ref value (the `current` property), which may not be accessed during render. (https://react.dev/reference/react/useRef)', loc: callee.loc, - description: `Function ${printPlace(callee)} accesses a ref`, + description: + callee.identifier.name !== null && + callee.identifier.name.kind === 'named' + ? `Function \`${callee.identifier.name.value}\` accesses a ref` + : null, suggestions: null, }); } @@ -149,7 +174,7 @@ function validateNoRefAccessInRenderImpl( errors, refAccessingFunctions, operand, - operand.loc, + lookupLocations.get(operand.identifier.id) ?? operand.loc, ); } } @@ -162,7 +187,7 @@ function validateNoRefAccessInRenderImpl( errors, refAccessingFunctions, operand, - operand.loc, + lookupLocations.get(operand.identifier.id) ?? operand.loc, ); } break; @@ -175,13 +200,18 @@ function validateNoRefAccessInRenderImpl( errors, refAccessingFunctions, instr.value.object, - instr.loc, + lookupLocations.get(instr.value.object.identifier.id) ?? instr.loc, ); for (const operand of eachInstructionValueOperand(instr.value)) { if (operand === instr.value.object) { continue; } - validateNoRefValueAccess(errors, refAccessingFunctions, operand); + validateNoRefValueAccess( + errors, + refAccessingFunctions, + lookupLocations, + operand, + ); } break; } @@ -190,14 +220,24 @@ function validateNoRefAccessInRenderImpl( break; default: { for (const operand of eachInstructionValueOperand(instr.value)) { - validateNoRefValueAccess(errors, refAccessingFunctions, operand); + validateNoRefValueAccess( + errors, + refAccessingFunctions, + lookupLocations, + operand, + ); } break; } } } for (const operand of eachTerminalOperand(block.terminal)) { - validateNoRefValueAccess(errors, refAccessingFunctions, operand); + validateNoRefValueAccess( + errors, + refAccessingFunctions, + lookupLocations, + operand, + ); } } @@ -211,6 +251,7 @@ function validateNoRefAccessInRenderImpl( function validateNoRefValueAccess( errors: CompilerError, refAccessingFunctions: Set, + lookupLocations: Map, operand: Place, ): void { if ( @@ -221,8 +262,12 @@ function validateNoRefValueAccess( severity: ErrorSeverity.InvalidReact, reason: 'Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef)', - loc: operand.loc, - description: `Cannot access ref value at ${printPlace(operand)}`, + loc: lookupLocations.get(operand.identifier.id) ?? operand.loc, + description: + operand.identifier.name !== null && + operand.identifier.name.kind === 'named' + ? `Cannot access ref value \`${operand.identifier.name.value}\`` + : null, suggestions: null, }); } 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 c5e8b68c04002..02748366456fb 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 @@ -15,10 +15,11 @@ function Component(props) { ## Error ``` + 2 | function Component(props) { 3 | const ref = useRef(null); - 4 | const value = ref.current; -> 5 | return value; - | ^^^^^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef). Cannot access ref value at freeze $22:TObject (5:5) +> 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) + 5 | return value; 6 | } 7 | ``` 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 0d4cb1318911b..f23ff6f3c8c57 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 @@ -24,7 +24,7 @@ function Component() { 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). Function mutate? $39[11:13]:TObject accesses a ref (9:9) + | ^^^^^^^^^ 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 | diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-read-ref-prop-in-render-destructure.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-read-ref-prop-in-render-destructure.expect.md index d8e8174d2cc62..100dafaf38a88 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-read-ref-prop-in-render-destructure.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-read-ref-prop-in-render-destructure.expect.md @@ -14,10 +14,11 @@ function Component({ref}) { ## Error ``` + 1 | // @validateRefAccessDuringRender @compilationMode(infer) 2 | function Component({ref}) { - 3 | const value = ref.current; -> 4 | return
{value}
; - | ^^^^^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef). Cannot access ref value at read $17:TObject (4:4) +> 3 | const value = ref.current; + | ^^^^^^^^^^^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (3:3) + 4 | return
{value}
; 5 | } 6 | ``` diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-read-ref-prop-in-render-property-load.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-read-ref-prop-in-render-property-load.expect.md index e374900a95ad4..4d8c4c735c70e 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-read-ref-prop-in-render-property-load.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-read-ref-prop-in-render-property-load.expect.md @@ -14,10 +14,11 @@ function Component(props) { ## Error ``` + 1 | // @validateRefAccessDuringRender @compilationMode(infer) 2 | function Component(props) { - 3 | const value = props.ref.current; -> 4 | return
{value}
; - | ^^^^^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef). Cannot access ref value at read $15:TObject (4:4) +> 3 | const value = props.ref.current; + | ^^^^^^^^^^^^^^^^^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (3:3) + 4 | return
{value}
; 5 | } 6 | ``` diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-ref-value-as-props.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-ref-value-as-props.expect.md index 96e2ca9a1ef28..f86f6b7a39145 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-ref-value-as-props.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-ref-value-as-props.expect.md @@ -17,7 +17,7 @@ function Component(props) { 2 | function Component(props) { 3 | const ref = useRef(null); > 4 | return ; - | ^^^^^^^^^^^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef). Cannot access ref value at freeze $19:TObject (4:4) + | ^^^^^^^^^^^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (4:4) 5 | } 6 | ``` diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-set-and-read-ref-during-render.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-set-and-read-ref-during-render.expect.md index 5db1568427835..d38adb5589db3 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-set-and-read-ref-during-render.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-set-and-read-ref-during-render.expect.md @@ -20,7 +20,7 @@ function Component(props) { > 4 | ref.current = props.value; | ^^^^^^^^^^^ 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 at freeze $24:TObject (5:5) +InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (5:5) 5 | return ref.current; 6 | } 7 | diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-set-and-read-ref-nested-property-during-render.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-set-and-read-ref-nested-property-during-render.expect.md index 290b8539e7b14..527adfc6e9fc5 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-set-and-read-ref-nested-property-during-render.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-set-and-read-ref-nested-property-during-render.expect.md @@ -18,9 +18,9 @@ function Component(props) { 2 | function Component(props) { 3 | const ref = useRef({inner: null}); > 4 | ref.current.inner = props.value; - | ^^^^^^^^^^^^^^^^^ 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) (4:4) -InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef). Cannot access ref value at freeze $30:TObject (5:5) +InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (5:5) 5 | return ref.current.inner; 6 | } 7 | diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.return-ref-callback.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.return-ref-callback.expect.md index ae42ba1ee5422..98f2a86714034 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.return-ref-callback.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.return-ref-callback.expect.md @@ -28,7 +28,7 @@ export const FIXTURE_ENTRYPOINT = { 8 | }; 9 | > 10 | return s; - | ^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef). Cannot access ref value at freeze $25:TObject (10:10) + | ^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (10:10) 11 | } 12 | 13 | export const FIXTURE_ENTRYPOINT = { diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-useCallback-set-ref-nested-property-ref-modified-later-preserve-memoization.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-useCallback-set-ref-nested-property-ref-modified-later-preserve-memoization.expect.md index bff8f0e01f582..7a8f271bd2879 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-useCallback-set-ref-nested-property-ref-modified-later-preserve-memoization.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-useCallback-set-ref-nested-property-ref-modified-later-preserve-memoization.expect.md @@ -34,7 +34,7 @@ export const FIXTURE_ENTRYPOINT = { 12 | 13 | // The ref is modified later, extending its range and preventing memoization of onChange > 14 | ref.current.inner = null; - | ^^^^^^^^^^^^^^^^^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (14:14) + | ^^^^^^^^^^^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (14:14) 15 | 16 | return ; 17 | } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.useCallback-accesses-ref-mutated-later-via-function-preserve-memoization.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.useCallback-accesses-ref-mutated-later-via-function-preserve-memoization.expect.md index 9014de67d4478..f3ab9816218f9 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.useCallback-accesses-ref-mutated-later-via-function-preserve-memoization.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.useCallback-accesses-ref-mutated-later-via-function-preserve-memoization.expect.md @@ -37,7 +37,7 @@ export const FIXTURE_ENTRYPOINT = { 15 | ref.current.inner = null; 16 | }; > 17 | reset(); - | ^^^^^ InvalidReact: This function accesses a ref value (the `current` property), which may not be accessed during render. (https://react.dev/reference/react/useRef). Function mutate? $77[20:22]:TObject accesses a ref (17:17) + | ^^^^^ InvalidReact: This function accesses a ref value (the `current` property), which may not be accessed during render. (https://react.dev/reference/react/useRef) (17:17) InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (17:17) 18 | diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useCallback-ref-in-render.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useCallback-ref-in-render.expect.md index 2baa222fd324c..e1427437ce86d 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useCallback-ref-in-render.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useCallback-ref-in-render.expect.md @@ -16,7 +16,7 @@ component Foo() { } component A(r: mixed) { - return
; + return
; } export const FIXTURE_ENTRYPOINT = { diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useCallback-ref-in-render.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useCallback-ref-in-render.js index 16a12d4bae6a9..9fefa503203e2 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useCallback-ref-in-render.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useCallback-ref-in-render.js @@ -12,7 +12,7 @@ component Foo() { } component A(r: mixed) { - return
; + return
; } export const FIXTURE_ENTRYPOINT = {