From fa992fdc1032c33b58d5d82c6bd4618713dedcae Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Wed, 6 Aug 2025 23:48:57 -0700 Subject: [PATCH 1/2] [compiler] remove use of inspect module --- .../packages/babel-plugin-react-compiler/src/Flood/Types.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/compiler/packages/babel-plugin-react-compiler/src/Flood/Types.ts b/compiler/packages/babel-plugin-react-compiler/src/Flood/Types.ts index b4f91ab478e..21391b197b7 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Flood/Types.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Flood/Types.ts @@ -9,6 +9,7 @@ import { import * as t from '@babel/types'; import * as TypeErrors from './TypeErrors'; import {assertExhaustive} from '../Utils/utils'; +import {FlowType} from './FlowTypes'; export const DEBUG = false; @@ -196,8 +197,6 @@ export function makeVariableId(id: number): VariableId { return id as VariableId; } -import {inspect} from 'util'; -import {FlowType} from './FlowTypes'; export function printConcrete( type: ConcreteType, printType: (_: T) => string, @@ -241,7 +240,7 @@ export function printConcrete( case 'Generic': return `T${type.id}`; case 'Object': { - const name = `Object ${inspect([...type.members.keys()])}`; + const name = `Object [${[...type.members.keys()].map(key => JSON.stringify(key)).join(', ')}]`; return `${name}`; } case 'Tuple': { From 61b605f764e701a3cc293650f4fdbd864d18cd28 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Thu, 7 Aug 2025 00:10:40 -0700 Subject: [PATCH 2/2] [compiler] Add hint to name variables with "Ref" suffix If you have a ref that the compiler doesn't know is a ref (say, a value returned from a custom hook) and try to assign its `.current = ...`, we currently fail with a generic error that hook return values are not mutable. However, an assignment to `.current` specifically is a very strong hint that the value is likely to be a ref. So in this PR, we track the reason for the mutation and if it ends up being an error, we use it to show an additional hint to the user. See the fixture for an example of the message. --- .../src/Inference/AliasingEffects.ts | 4 +- .../Inference/InferMutationAliasingEffects.ts | 75 ++++++++++++++----- ...-assing-to-ref-current-in-render.expect.md | 42 +++++++++++ ...invalid-assing-to-ref-current-in-render.js | 7 ++ 4 files changed, 107 insertions(+), 21 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-assing-to-ref-current-in-render.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-assing-to-ref-current-in-render.js diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/AliasingEffects.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/AliasingEffects.ts index c492e283546..80694a7a784 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/AliasingEffects.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/AliasingEffects.ts @@ -50,7 +50,7 @@ export type AliasingEffect = /** * Mutate the value and any direct aliases (not captures). Errors if the value is not mutable. */ - | {kind: 'Mutate'; value: Place} + | {kind: 'Mutate'; value: Place; reason?: MutationReason | null} /** * Mutate the value and any direct aliases (not captures), but only if the value is known mutable. * This should be rare. @@ -174,6 +174,8 @@ export type AliasingEffect = place: Place; }; +export type MutationReason = {kind: 'AssignCurrentProperty'}; + export function hashEffect(effect: AliasingEffect): string { switch (effect.kind) { case 'Apply': { 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 2adf78fe058..44bbaa463d6 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts @@ -68,7 +68,12 @@ import {FunctionSignature} from '../HIR/ObjectShape'; import {getWriteErrorReason} from './InferFunctionEffects'; import prettyFormat from 'pretty-format'; import {createTemporaryPlace} from '../HIR/HIRBuilder'; -import {AliasingEffect, AliasingSignature, hashEffect} from './AliasingEffects'; +import { + AliasingEffect, + AliasingSignature, + hashEffect, + MutationReason, +} from './AliasingEffects'; const DEBUG = false; @@ -452,18 +457,29 @@ function applySignature( effect.value.identifier.name.kind === 'named' ? `\`${effect.value.identifier.name.value}\`` : 'value'; + const diagnostic = CompilerDiagnostic.create({ + severity: ErrorSeverity.InvalidReact, + category: 'This value cannot be modified', + description: `${reason}.`, + }).withDetail({ + kind: 'error', + loc: effect.value.loc, + message: `${variable} cannot be modified`, + }); + if ( + effect.kind === 'Mutate' && + effect.reason?.kind === 'AssignCurrentProperty' + ) { + diagnostic.withDetail({ + kind: 'error', + loc: effect.value.loc, + message: `Hint: If this value is a Ref (value returned by \`useRef()\`), rename the variable to end in "Ref".`, + }); + } effects.push({ kind: 'MutateFrozen', place: effect.value, - error: CompilerDiagnostic.create({ - severity: ErrorSeverity.InvalidReact, - category: 'This value cannot be modified', - description: `${reason}.`, - }).withDetail({ - kind: 'error', - loc: effect.value.loc, - message: `${variable} cannot be modified`, - }), + error: diagnostic, }); } } @@ -1066,6 +1082,25 @@ function applyEffect( effect.value.identifier.name.kind === 'named' ? `\`${effect.value.identifier.name.value}\`` : 'value'; + const diagnostic = CompilerDiagnostic.create({ + severity: ErrorSeverity.InvalidReact, + category: 'This value cannot be modified', + description: `${reason}.`, + }).withDetail({ + kind: 'error', + loc: effect.value.loc, + message: `${variable} cannot be modified`, + }); + if ( + effect.kind === 'Mutate' && + effect.reason?.kind === 'AssignCurrentProperty' + ) { + diagnostic.withDetail({ + kind: 'error', + loc: effect.value.loc, + message: `Hint: If this value is a Ref (value returned by \`useRef()\`), rename the variable to end in "Ref".`, + }); + } applyEffect( context, state, @@ -1075,15 +1110,7 @@ function applyEffect( ? 'MutateFrozen' : 'MutateGlobal', place: effect.value, - error: CompilerDiagnostic.create({ - severity: ErrorSeverity.InvalidReact, - category: 'This value cannot be modified', - description: `${reason}.`, - }).withDetail({ - kind: 'error', - loc: effect.value.loc, - message: `${variable} cannot be modified`, - }), + error: diagnostic, }, initialized, effects, @@ -1680,7 +1707,15 @@ function computeSignatureForInstruction( } case 'PropertyStore': case 'ComputedStore': { - effects.push({kind: 'Mutate', value: value.object}); + const mutationReason: MutationReason | null = + value.kind === 'PropertyStore' && value.property === 'current' + ? {kind: 'AssignCurrentProperty'} + : null; + effects.push({ + kind: 'Mutate', + value: value.object, + reason: mutationReason, + }); effects.push({ kind: 'Capture', from: value.value, diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-assing-to-ref-current-in-render.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-assing-to-ref-current-in-render.expect.md new file mode 100644 index 00000000000..af94ba06973 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-assing-to-ref-current-in-render.expect.md @@ -0,0 +1,42 @@ + +## Input + +```javascript +// @flow + +component Foo() { + const foo = useFoo(); + foo.current = true; + return
; +} + +``` + + +## Error + +``` +Found 1 error: + +Error: This value cannot be modified + +Modifying a value returned from a hook is not allowed. Consider moving the modification into the hook where the value is constructed. + + 3 | component Foo() { + 4 | const foo = useFoo(); +> 5 | foo.current = true; + | ^^^ value cannot be modified + 6 | return
; + 7 | } + 8 | + + 3 | component Foo() { + 4 | const foo = useFoo(); +> 5 | foo.current = true; + | ^^^ Hint: If this value is a Ref (value returned by `useRef()`), rename the variable to end in "Ref". + 6 | return
; + 7 | } + 8 | +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-assing-to-ref-current-in-render.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-assing-to-ref-current-in-render.js new file mode 100644 index 00000000000..efe92bc034e --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-assing-to-ref-current-in-render.js @@ -0,0 +1,7 @@ +// @flow + +component Foo() { + const foo = useFoo(); + foo.current = true; + return
; +}