From 661665425326e5fb7f9adce983e6a9f04d4325e7 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Wed, 27 Aug 2025 09:44:51 -0700 Subject: [PATCH] [compiler] Show a ref name hint when assigning to non-ref in a callback In #34125 I added a hint where if you assign to the .current property of a frozen object, we suggest naming the variable as `ref` or `-Ref`. However, the tracking for mutations that assign to .current specifically wasn't propagated past function expression boundaries, which meant that the hint only showed up if you mutated the ref in the main body of the component/hook. That's less likely to happen since most folks know not to access refs in render. What's more likely is that you'll (correctly) assign a ref in an effect or callback, but the compiler will throw an error. By showing a hint in this case we can help people understand the naming pattern. --- .../src/HIR/PrintHIR.ts | 2 +- .../Inference/InferMutationAliasingRanges.ts | 12 +++++- .../error.assign-ref-in-effect-hint.expect.md | 37 +++++++++++++++++++ .../error.assign-ref-in-effect-hint.js | 7 ++++ ...valid-mutate-context-in-callback.expect.md | 2 + 5 files changed, 58 insertions(+), 2 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.assign-ref-in-effect-hint.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.assign-ref-in-effect-hint.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts index 7d2b0b234c7f..fd757730901a 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts @@ -983,7 +983,7 @@ export function printAliasingEffect(effect: AliasingEffect): string { case 'MutateConditionally': case 'MutateTransitive': case 'MutateTransitiveConditionally': { - return `${effect.kind} ${printPlaceForAliasEffect(effect.value)}`; + return `${effect.kind} ${printPlaceForAliasEffect(effect.value)}${effect.kind === 'Mutate' && effect.reason?.kind === 'AssignCurrentProperty' ? ' (assign `.current`)' : ''}`; } case 'MutateFrozen': { return `MutateFrozen ${printPlaceForAliasEffect(effect.place)} reason=${JSON.stringify(effect.error.reason)}`; 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 9d1733c77a9a..b53026a4d4b8 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingRanges.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingRanges.ts @@ -27,7 +27,7 @@ import { } from '../HIR/visitors'; import {assertExhaustive, getOrInsertWith} from '../Utils/utils'; import {Err, Ok, Result} from '../Utils/Result'; -import {AliasingEffect} from './AliasingEffects'; +import {AliasingEffect, MutationReason} from './AliasingEffects'; /** * This pass builds an abstract model of the heap and interprets the effects of the @@ -101,6 +101,7 @@ export function inferMutationAliasingRanges( transitive: boolean; kind: MutationKind; place: Place; + reason: MutationReason | null; }> = []; const renders: Array<{index: number; place: Place}> = []; @@ -176,6 +177,7 @@ export function inferMutationAliasingRanges( effect.kind === 'MutateTransitive' ? MutationKind.Definite : MutationKind.Conditional, + reason: null, place: effect.value, }); } else if ( @@ -190,6 +192,7 @@ export function inferMutationAliasingRanges( effect.kind === 'Mutate' ? MutationKind.Definite : MutationKind.Conditional, + reason: effect.kind === 'Mutate' ? (effect.reason ?? null) : null, place: effect.value, }); } else if ( @@ -241,6 +244,7 @@ export function inferMutationAliasingRanges( mutation.transitive, mutation.kind, mutation.place.loc, + mutation.reason, errors, ); } @@ -267,6 +271,7 @@ export function inferMutationAliasingRanges( functionEffects.push({ kind: 'Mutate', value: {...place, loc: node.local.loc}, + reason: node.mutationReason, }); } } @@ -507,6 +512,7 @@ export function inferMutationAliasingRanges( true, MutationKind.Conditional, into.loc, + null, ignoredErrors, ); for (const from of tracked) { @@ -580,6 +586,7 @@ type Node = { transitive: {kind: MutationKind; loc: SourceLocation} | null; local: {kind: MutationKind; loc: SourceLocation} | null; lastMutated: number; + mutationReason: MutationReason | null; value: | {kind: 'Object'} | {kind: 'Phi'} @@ -599,6 +606,7 @@ class AliasingState { transitive: null, local: null, lastMutated: 0, + mutationReason: null, value, }); } @@ -697,6 +705,7 @@ class AliasingState { transitive: boolean, startKind: MutationKind, loc: SourceLocation, + reason: MutationReason | null, errors: CompilerError, ): void { const seen = new Map(); @@ -717,6 +726,7 @@ class AliasingState { if (node == null) { continue; } + node.mutationReason ??= reason; node.lastMutated = Math.max(node.lastMutated, index); if (end != null) { node.id.mutableRange.end = makeInstructionId( diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.assign-ref-in-effect-hint.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.assign-ref-in-effect-hint.expect.md new file mode 100644 index 000000000000..c89e773f3212 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.assign-ref-in-effect-hint.expect.md @@ -0,0 +1,37 @@ + +## Input + +```javascript +// Fixture to test that we show a hint to name as `ref` or `-Ref` when attempting +// to assign .current inside an effect +function Component({foo}) { + useEffect(() => { + foo.current = true; + }, [foo]); +} + +``` + + +## Error + +``` +Found 1 error: + +Error: This value cannot be modified + +Modifying component props or hook arguments is not allowed. Consider using a local variable instead. + +error.assign-ref-in-effect-hint.ts:5:4 + 3 | function Component({foo}) { + 4 | useEffect(() => { +> 5 | foo.current = true; + | ^^^ `foo` cannot be modified + 6 | }, [foo]); + 7 | } + 8 | + +Hint: If this value is a Ref (value returned by `useRef()`), rename the variable to end in "Ref". +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.assign-ref-in-effect-hint.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.assign-ref-in-effect-hint.js new file mode 100644 index 000000000000..154673495964 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.assign-ref-in-effect-hint.js @@ -0,0 +1,7 @@ +// Fixture to test that we show a hint to name as `ref` or `-Ref` when attempting +// to assign .current inside an effect +function Component({foo}) { + useEffect(() => { + foo.current = true; + }, [foo]); +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-mutate-context-in-callback.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-mutate-context-in-callback.expect.md index 9467aa21e031..142f538a0dd3 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-mutate-context-in-callback.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-mutate-context-in-callback.expect.md @@ -38,6 +38,8 @@ error.invalid-mutate-context-in-callback.ts:12:4 13 | }; 14 | return
; 15 | } + +Hint: If this value is a Ref (value returned by `useRef()`), rename the variable to end in "Ref". ``` \ No newline at end of file