From 0b6a09d95323198ecec2962c941efca4376c2d93 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Mon, 3 Jun 2024 11:31:29 -0700 Subject: [PATCH] compiler: Add support for ref effects Fixes false positives where we currently disallow mutations of refs from callbacks passed to JSX, if the ref is also passed to jsx. We consider these to be mutations of "frozen" values, but refs are explicitly allowed to have interior mutability. The fix is to add a new RefMutation variant to FunctionEffect, which we emit whenever a ref is mutated. We disallow this effect type from within a function body, but allow it within function expressions that are passed to useEffect and friends as well as to jsx. ghstack-source-id: 057e505c16be50d16fc1dc12c1e5ed754ace87ed Pull Request resolved: https://github.com/facebook/react/pull/29733 --- .../src/HIR/HIR.ts | 4 + .../src/Inference/InferReferenceEffects.ts | 68 +++++++++-- ...-callback-passed-to-jsx-indirect.expect.md | 109 ++++++++++++++++++ ...ref-in-callback-passed-to-jsx-indirect.tsx | 28 +++++ ...ng-ref-in-callback-passed-to-jsx.expect.md | 94 +++++++++++++++ ...mutating-ref-in-callback-passed-to-jsx.tsx | 24 ++++ ...-callback-passed-to-jsx-indirect.expect.md | 109 ++++++++++++++++++ ...rty-in-callback-passed-to-jsx-indirect.tsx | 28 +++++ ...operty-in-callback-passed-to-jsx.expect.md | 94 +++++++++++++++ ...ref-property-in-callback-passed-to-jsx.tsx | 24 ++++ ...-disallow-mutating-ref-in-render.expect.md | 28 +++++ ...invalid-disallow-mutating-ref-in-render.js | 7 ++ ...tating-refs-in-render-transitive.expect.md | 33 ++++++ ...llow-mutating-refs-in-render-transitive.js | 12 ++ ...d-set-and-read-ref-during-render.expect.md | 4 +- ...ef-nested-property-during-render.expect.md | 4 +- ...rite-but-dont-read-ref-in-render.expect.md | 2 +- 17 files changed, 658 insertions(+), 14 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-mutating-ref-in-callback-passed-to-jsx-indirect.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-mutating-ref-in-callback-passed-to-jsx-indirect.tsx create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-mutating-ref-in-callback-passed-to-jsx.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-mutating-ref-in-callback-passed-to-jsx.tsx create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-mutating-ref-property-in-callback-passed-to-jsx-indirect.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-mutating-ref-property-in-callback-passed-to-jsx-indirect.tsx create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-mutating-ref-property-in-callback-passed-to-jsx.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-mutating-ref-property-in-callback-passed-to-jsx.tsx create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-disallow-mutating-ref-in-render.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-disallow-mutating-ref-in-render.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-disallow-mutating-refs-in-render-transitive.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-disallow-mutating-refs-in-render-transitive.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts index f9dfea52f363e..c02fe5af10301 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts @@ -291,6 +291,10 @@ export type FunctionEffect = kind: "GlobalMutation"; error: CompilerErrorDetailOptions; } + | { + kind: "RefMutation"; + error: CompilerErrorDetailOptions; + } | { kind: "ReactMutation"; error: CompilerErrorDetailOptions; diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferReferenceEffects.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferReferenceEffects.ts index 520684c026bda..7b9d81d81a9be 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferReferenceEffects.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferReferenceEffects.ts @@ -29,6 +29,8 @@ import { isArrayType, isMutableEffect, isObjectType, + isRefValueType, + isUseRefType, } from "../HIR/HIR"; import { FunctionSignature } from "../HIR/ObjectShape"; import { @@ -239,7 +241,8 @@ export default function inferReferenceEffects( functionEffects.forEach((eff) => { switch (eff.kind) { case "ReactMutation": - case "GlobalMutation": { + case "GlobalMutation": + case "RefMutation": { CompilerError.throw(eff.error); } case "ContextMutation": { @@ -407,7 +410,8 @@ class InferenceState { for (const effect of value.loweredFunc.func.effects) { if ( effect.kind === "GlobalMutation" || - effect.kind === "ReactMutation" + effect.kind === "ReactMutation" || + effect.kind === "RefMutation" ) { // Known effects are always propagated upwards functionEffects.push(effect); @@ -521,7 +525,28 @@ class InferenceState { break; } case Effect.Mutate: { - if (valueKind.kind === ValueKind.Context) { + if ( + isRefValueType(place.identifier) || + isUseRefType(place.identifier) + ) { + if (this.#env.config.validateRefAccessDuringRender) { + functionEffect = { + kind: "RefMutation", + error: { + reason: + "Ref values (the `current` property) may not be modified during render. (https://react.dev/reference/react/useRef)", + description: + place.identifier.name !== null && + place.identifier.name.kind === "named" + ? `Found mutation of \`${place.identifier.name.value}\`` + : null, + loc: place.loc, + suggestions: null, + severity: ErrorSeverity.InvalidReact, + }, + }; + } + } else if (valueKind.kind === ValueKind.Context) { functionEffect = { kind: "ContextMutation", loc: place.loc, @@ -560,7 +585,28 @@ class InferenceState { break; } case Effect.Store: { - if (valueKind.kind === ValueKind.Context) { + if ( + isRefValueType(place.identifier) || + isUseRefType(place.identifier) + ) { + if (this.#env.config.validateRefAccessDuringRender) { + functionEffect = { + kind: "RefMutation", + error: { + reason: + "Ref values (the `current` property) may not be modified during render. (https://react.dev/reference/react/useRef)", + description: + place.identifier.name !== null && + place.identifier.name.kind === "named" + ? `Found mutation of \`${place.identifier.name.value}\`` + : null, + loc: place.loc, + suggestions: null, + severity: ErrorSeverity.InvalidReact, + }, + }; + } + } else if (valueKind.kind === ValueKind.Context) { functionEffect = { kind: "ContextMutation", loc: place.loc, @@ -1139,7 +1185,9 @@ function inferBlock( ); functionEffects.push( ...propEffects.filter( - (propEffect) => propEffect.kind !== "GlobalMutation" + (propEffect) => + propEffect.kind !== "GlobalMutation" && + propEffect.kind !== "RefMutation" ) ); } @@ -1333,7 +1381,10 @@ function inferBlock( functionEffects.push( ...argumentEffects.filter( (argEffect) => - !isUseEffect || i !== 0 || argEffect.kind !== "GlobalMutation" + !isUseEffect || + i !== 0 || + (argEffect.kind !== "GlobalMutation" && + argEffect.kind !== "RefMutation") ) ); hasCaptureArgument ||= place.effect === Effect.Capture; @@ -1462,7 +1513,10 @@ function inferBlock( functionEffects.push( ...argumentEffects.filter( (argEffect) => - !isUseEffect || i !== 0 || argEffect.kind !== "GlobalMutation" + !isUseEffect || + i !== 0 || + (argEffect.kind !== "GlobalMutation" && + argEffect.kind !== "RefMutation") ) ); hasCaptureArgument ||= place.effect === Effect.Capture; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-mutating-ref-in-callback-passed-to-jsx-indirect.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-mutating-ref-in-callback-passed-to-jsx-indirect.expect.md new file mode 100644 index 0000000000000..539c9e71ec828 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-mutating-ref-in-callback-passed-to-jsx-indirect.expect.md @@ -0,0 +1,109 @@ + +## Input + +```javascript +// @validateRefAccessDuringRender +import { useRef } from "react"; + +function Component() { + const ref = useRef(null); + + const setRef = () => { + if (ref.current !== null) { + ref.current = ""; + } + }; + + const onClick = () => { + setRef(); + }; + + return ( + <> + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-mutating-ref-in-callback-passed-to-jsx-indirect.tsx b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-mutating-ref-in-callback-passed-to-jsx-indirect.tsx new file mode 100644 index 0000000000000..14a5fd9aa5bc4 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-mutating-ref-in-callback-passed-to-jsx-indirect.tsx @@ -0,0 +1,28 @@ +// @validateRefAccessDuringRender +import { useRef } from "react"; + +function Component() { + const ref = useRef(null); + + const setRef = () => { + if (ref.current !== null) { + ref.current = ""; + } + }; + + const onClick = () => { + setRef(); + }; + + return ( + <> + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-mutating-ref-in-callback-passed-to-jsx.tsx b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-mutating-ref-in-callback-passed-to-jsx.tsx new file mode 100644 index 0000000000000..74410e119a285 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-mutating-ref-in-callback-passed-to-jsx.tsx @@ -0,0 +1,24 @@ +// @validateRefAccessDuringRender +import { useRef } from "react"; + +function Component() { + const ref = useRef(null); + + const onClick = () => { + if (ref.current !== null) { + ref.current = ""; + } + }; + + return ( + <> + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-mutating-ref-property-in-callback-passed-to-jsx-indirect.tsx b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-mutating-ref-property-in-callback-passed-to-jsx-indirect.tsx new file mode 100644 index 0000000000000..87cc6d42446be --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-mutating-ref-property-in-callback-passed-to-jsx-indirect.tsx @@ -0,0 +1,28 @@ +// @validateRefAccessDuringRender +import { useRef } from "react"; + +function Component() { + const ref = useRef(null); + + const setRef = () => { + if (ref.current !== null) { + ref.current.value = ""; + } + }; + + const onClick = () => { + setRef(); + }; + + return ( + <> + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-mutating-ref-property-in-callback-passed-to-jsx.tsx b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-mutating-ref-property-in-callback-passed-to-jsx.tsx new file mode 100644 index 0000000000000..b9b7a2dd8e67b --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/allow-mutating-ref-property-in-callback-passed-to-jsx.tsx @@ -0,0 +1,24 @@ +// @validateRefAccessDuringRender +import { useRef } from "react"; + +function Component() { + const ref = useRef(null); + + const onClick = () => { + if (ref.current !== null) { + ref.current.value = ""; + } + }; + + return ( + <> + +