diff --git a/compiler/packages/babel-plugin-react-forget/src/Entrypoint/Pipeline.ts b/compiler/packages/babel-plugin-react-forget/src/Entrypoint/Pipeline.ts index 1de72d65fa..e29e79f4e0 100644 --- a/compiler/packages/babel-plugin-react-forget/src/Entrypoint/Pipeline.ts +++ b/compiler/packages/babel-plugin-react-forget/src/Entrypoint/Pipeline.ts @@ -180,7 +180,7 @@ function* runWithEnvironment( } if (env.config.validateRefAccessDuringRender) { - validateNoRefAccessInRender(hir); + validateNoRefAccessInRender(hir).unwrap(); } if (env.config.validateNoSetStateInRender) { diff --git a/compiler/packages/babel-plugin-react-forget/src/Validation/ValidateNoRefAccesInRender.ts b/compiler/packages/babel-plugin-react-forget/src/Validation/ValidateNoRefAccesInRender.ts index 1844e3ea90..02d0558313 100644 --- a/compiler/packages/babel-plugin-react-forget/src/Validation/ValidateNoRefAccesInRender.ts +++ b/compiler/packages/babel-plugin-react-forget/src/Validation/ValidateNoRefAccesInRender.ts @@ -5,135 +5,183 @@ * LICENSE file in the root directory of this source tree. */ +import { CompilerError, ErrorSeverity } from "../CompilerError"; import { - CompilerError, - CompilerErrorDetail, - ErrorSeverity, -} from "../CompilerError"; -import { HIRFunction, Place, isRefValueType, isUseRefType } from "../HIR/HIR"; + HIRFunction, + IdentifierId, + Place, + isRefValueType, + isUseRefType, +} from "../HIR"; import { printPlace } from "../HIR/PrintHIR"; import { eachInstructionValueOperand, eachTerminalOperand, } from "../HIR/visitors"; +import { Err, Ok, Result } from "../Utils/Result"; -/* - * Validates that ref values (the `current` property) are not accessed during render. - * This validation is conservative and only rejects accesses of known ref values: - * - * ```javascript - * // ERROR - * const ref = useRef(); - * ref.current; - * - * const ref = useRef(); - * foo(ref); // may access .current - * - * // ALLOWED - * const ref = useHookThatReturnsRef(); - * ref.current; - * ``` - * - * In the future we may reject more cases, based on either object names (`fooRef.current` is likely a ref) - * or based on property name alone (`foo.current` might be a ref). +/** + * Validates that a function does not access a ref value during render. This includes a partial check + * for ref values which are accessed indirectly via function expressions. */ -export function validateNoRefAccessInRender(fn: HIRFunction): void { - const error = new CompilerError(); +export function validateNoRefAccessInRender( + fn: HIRFunction +): Result { + const refAccessingFunctions: Set = new Set(); + return validateNoRefAccessInRenderImpl(fn, refAccessingFunctions); +} +function validateNoRefAccessInRenderImpl( + fn: HIRFunction, + refAccessingFunctions: Set +): Result { + const errors = new CompilerError(); for (const [, block] of fn.body.blocks) { for (const instr of block.instructions) { switch (instr.value.kind) { - case "PropertyLoad": - case "LoadLocal": - case "StoreLocal": - case "Destructure": { - /* - * These instructions are necessary for storing the results of a useRef into - * a variable and referencing them in functions. We can propagate type info - * for these instructions so they ensure we have a complete analysis. - */ + case "JsxExpression": + case "JsxFragment": { + for (const operand of eachInstructionValueOperand(instr.value)) { + if (isRefValueType(operand.identifier)) { + errors.push({ + 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 + )}`, + suggestions: null, + }); + } + } break; } - case "JsxExpression": { - // It's okay to pass refs to JSX, but not ref *values* - for (const operand of eachInstructionValueOperand(instr.value)) { - validateNonRefValue(error, operand); + case "PropertyLoad": { + break; + } + case "LoadLocal": { + if (refAccessingFunctions.has(instr.value.place.identifier.id)) { + refAccessingFunctions.add(instr.lvalue.identifier.id); + } + break; + } + case "StoreLocal": { + if (refAccessingFunctions.has(instr.value.value.identifier.id)) { + refAccessingFunctions.add(instr.value.lvalue.place.identifier.id); + refAccessingFunctions.add(instr.lvalue.identifier.id); } break; } case "ObjectMethod": case "FunctionExpression": { - if (fn.env.config.validateRefAccessDuringRenderFunctionExpressions) { + if ( /* - * functions are allowed to capture refs, so long as the function is not called - * during render. see AnalyzeFunctions for how we ensure that functions which - * capture refs get assigned a mutable range so we know here whether the function - * is called or not + * check if the function expression accesses a ref *or* some other + * function which accesses a ref */ - const mutableRange = instr.lvalue.identifier.mutableRange; - if (mutableRange.end > mutableRange.start + 1) { - for (const operand of eachInstructionValueOperand(instr.value)) { - validateNonRefValue(error, operand); - validateNonRefObject(error, operand); - } - } + [...eachInstructionValueOperand(instr.value)].some( + (operand) => + isRefValueType(operand.identifier) || + refAccessingFunctions.has(operand.identifier.id) + ) || + // check for cases where .current is accessed through an aliased ref + ([...eachInstructionValueOperand(instr.value)].some((operand) => + isUseRefType(operand.identifier) + ) && + validateNoRefAccessInRenderImpl( + instr.value.loweredFunc.func, + refAccessingFunctions + ).isErr()) + ) { + // This function expression unconditionally accesses a ref + refAccessingFunctions.add(instr.lvalue.identifier.id); } break; } - case "CallExpression": - case "NewExpression": { + case "CallExpression": { + const callee = instr.value.callee; + // Report a more precise error when calling a local function that accesses a ref + if (refAccessingFunctions.has(callee.identifier.id)) { + errors.push({ + severity: ErrorSeverity.InvalidReact, + reason: + "This function accesses a ref, which not be accessed during render. (https://react.dev/reference/react/useRef)", + loc: callee.loc, + description: `Function ${printPlace(callee)} accesses a ref`, + suggestions: null, + }); + } for (const operand of eachInstructionValueOperand(instr.value)) { - validateNonRefValue(error, operand); - validateNonRefObject(error, operand); + validateNoRefAccess(errors, refAccessingFunctions, operand); + } + break; + } + case "ObjectExpression": + case "ArrayExpression": + case "MethodCall": { + for (const operand of eachInstructionValueOperand(instr.value)) { + validateNoRefAccess(errors, refAccessingFunctions, operand); } break; } default: { for (const operand of eachInstructionValueOperand(instr.value)) { - validateNonRefValue(error, operand); - validateNonRefObject(error, operand); + validateNoRefValueAccess(errors, refAccessingFunctions, operand); } + break; } } } for (const operand of eachTerminalOperand(block.terminal)) { - validateNonRefValue(error, operand); + validateNoRefValueAccess(errors, refAccessingFunctions, operand); } } - if (error.hasErrors()) { - throw error; + if (errors.hasErrors()) { + return Err(errors); + } else { + return Ok(undefined); } } -// Check that the operand's type is not that of useRef().current (the ref's current value) -function validateNonRefValue(error: CompilerError, operand: Place): void { - if (isRefValueType(operand.identifier)) { - error.pushErrorDetail( - new CompilerErrorDetail({ - description: `Cannot access ref value at ${printPlace(operand)}`, - loc: typeof operand.loc !== "symbol" ? operand.loc : null, - reason: - "Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef)", - severity: ErrorSeverity.InvalidReact, - suggestions: null, - }) - ); +function validateNoRefValueAccess( + errors: CompilerError, + unconditionalSetStateFunctions: Set, + operand: Place +): void { + if ( + isRefValueType(operand.identifier) || + unconditionalSetStateFunctions.has(operand.identifier.id) + ) { + errors.push({ + 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)}`, + suggestions: null, + }); } } -// Check that the operand's type is not that of useRef() return value (the ref container) -function validateNonRefObject(error: CompilerError, operand: Place): void { - if (isUseRefType(operand.identifier)) { - error.pushErrorDetail( - new CompilerErrorDetail({ - description: `Cannot access ref object at ${printPlace(operand)}`, - loc: typeof operand.loc !== "symbol" ? operand.loc : null, - reason: - "Ref values may not be passed to functions because they could read the ref value (`current` property) during render. (https://react.dev/reference/react/useRef)", - severity: ErrorSeverity.InvalidReact, - suggestions: null, - }) - ); +function validateNoRefAccess( + errors: CompilerError, + unconditionalSetStateFunctions: Set, + operand: Place +): void { + if ( + isRefValueType(operand.identifier) || + isUseRefType(operand.identifier) || + unconditionalSetStateFunctions.has(operand.identifier.id) + ) { + errors.push({ + 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)}`, + suggestions: null, + }); } } diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-aliased-ref-in-callback-invoked-during-render-.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-aliased-ref-in-callback-invoked-during-render-.expect.md new file mode 100644 index 0000000000..9f76b4e299 --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-aliased-ref-in-callback-invoked-during-render-.expect.md @@ -0,0 +1,25 @@ + +## Input + +```javascript +// @validateRefAccessDuringRender @validateRefAccessDuringRenderFunctionExpressions +function Component(props) { + const ref = useRef(null); + const renderItem = (item) => { + const aliasedRef = ref; + const current = aliasedRef.current; + return ; + }; + return {props.items.map((item) => renderItem(item))}; +} + +``` + + +## Error + +``` +[ReactForget] InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef). Cannot access ref value at mutate? $64[13:15] (9:9) +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-aliased-ref-in-callback-invoked-during-render-.js b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-aliased-ref-in-callback-invoked-during-render-.js new file mode 100644 index 0000000000..4a73445e92 --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-aliased-ref-in-callback-invoked-during-render-.js @@ -0,0 +1,10 @@ +// @validateRefAccessDuringRender @validateRefAccessDuringRenderFunctionExpressions +function Component(props) { + const ref = useRef(null); + const renderItem = (item) => { + const aliasedRef = ref; + const current = aliasedRef.current; + return ; + }; + return {props.items.map((item) => renderItem(item))}; +} diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-pass-ref-to-function.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-pass-ref-to-function.expect.md index a2c7181cbc..73c56feb52 100644 --- a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-pass-ref-to-function.expect.md +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-pass-ref-to-function.expect.md @@ -15,7 +15,7 @@ function Component(props) { ## Error ``` -[ReactForget] InvalidReact: Ref values may not be passed to functions because they could read the ref value (`current` property) during render. (https://react.dev/reference/react/useRef). Cannot access ref object at mutate? $21[6:8]:TObject (4:4) +[ReactForget] InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef). Cannot access ref value at mutate? $21[6:8]:TObject (4:4) ``` \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-ref-in-callback-invoked-during-render.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-ref-in-callback-invoked-during-render.expect.md index 4f8e34ddea..bc2278d7ec 100644 --- a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-ref-in-callback-invoked-during-render.expect.md +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-ref-in-callback-invoked-during-render.expect.md @@ -18,7 +18,7 @@ function Component(props) { ## Error ``` -[ReactForget] InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef). Cannot access ref value at capture $42[6:16]:TObject (5:5) +[ReactForget] InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef). Cannot access ref value at mutate? $60[14:16] (8:8) ``` \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-set-and-read-ref-during-render.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-set-and-read-ref-during-render.expect.md index c728ae32de..d0f30003e6 100644 --- a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-set-and-read-ref-during-render.expect.md +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-set-and-read-ref-during-render.expect.md @@ -15,8 +15,6 @@ function Component(props) { ## Error ``` -[ReactForget] InvalidReact: Ref values may not be passed to functions because they could read the ref value (`current` property) during render. (https://react.dev/reference/react/useRef). Cannot access ref object at store $21[7:9]:TObject (4:4) - [ReactForget] 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) ``` diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-use-ref-added-to-dep-without-type-info.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-use-ref-added-to-dep-without-type-info.expect.md index 6b40cae910..4eb60d532a 100644 --- a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-use-ref-added-to-dep-without-type-info.expect.md +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.invalid-use-ref-added-to-dep-without-type-info.expect.md @@ -22,7 +22,7 @@ function Foo({ a }) { ## Error ``` -[ReactForget] InvalidReact: Ref values may not be passed to functions because they could read the ref value (`current` property) during render. (https://react.dev/reference/react/useRef). Cannot access ref object at capture $29:TObject (5:5) +[ReactForget] InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef). Cannot access ref value at capture $29:TObject (5:5) ``` \ No newline at end of file