From 69cb23bbcd486737cbb12db51afd49599e034bb6 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Wed, 11 Jun 2025 13:21:27 -0700 Subject: [PATCH] [compiler] Improve error message for mutating hook args/return The previous error message was generic, because the old style function signature didn't support a way to specify a reason alongside a freeze effect. This meant we could only say why a value was frozen for instructions, but not hooks which use function signatures. By defining a new aliasing signature for custom hooks we can specify a reason and provide a better error message. --- .../src/HIR/HIR.ts | 10 +++++ .../src/HIR/ObjectShape.ts | 28 ++++++++++++ .../src/Inference/InferFunctionEffects.ts | 4 ++ .../Inference/InferMutationAliasingEffects.ts | 43 ++++++++++--------- ...call-freezes-captured-identifier.expect.md | 2 +- ...call-freezes-captured-memberexpr.expect.md | 2 +- ...d-reanimated-shared-value-writes.expect.md | 2 +- .../no-emit/retry-no-emit.expect.md | 2 +- .../error.mutate-frozen-value.expect.md | 2 +- .../new-mutability/retry-no-emit.expect.md | 2 +- 10 files changed, 71 insertions(+), 26 deletions(-) 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 252721765a..33c978c145 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts @@ -1388,6 +1388,16 @@ export enum ValueReason { */ JsxCaptured = 'jsx-captured', + /** + * Argument to a hook + */ + HookCaptured = 'hook-captured', + + /** + * Return value of a hook + */ + HookReturn = 'hook-return', + /** * Passed to an effect */ diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/ObjectShape.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/ObjectShape.ts index e47d561231..eb80e5c59d 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/ObjectShape.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/ObjectShape.ts @@ -1302,6 +1302,34 @@ export const DefaultNonmutatingHook = addHook( calleeEffect: Effect.Read, hookKind: 'Custom', returnValueKind: ValueKind.Frozen, + aliasing: { + receiver: makeIdentifierId(0), + params: [], + rest: makeIdentifierId(1), + returns: makeIdentifierId(2), + temporaries: [], + effects: [ + // Freeze the arguments + { + kind: 'Freeze', + value: signatureArgument(1), + reason: ValueReason.HookCaptured, + }, + // Returns a frozen value + { + kind: 'Create', + into: signatureArgument(2), + value: ValueKind.Frozen, + reason: ValueReason.HookReturn, + }, + // May alias any arguments into the return + { + kind: 'Alias', + from: signatureArgument(1), + into: signatureArgument(2), + }, + ], + }, }, 'DefaultNonmutatingHook', ); diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferFunctionEffects.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferFunctionEffects.ts index 4a27885095..9b347ebb6c 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferFunctionEffects.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferFunctionEffects.ts @@ -341,6 +341,10 @@ export function getWriteErrorReason(abstractValue: AbstractValue): string { return "Mutating a value returned from 'useReducer()', which should not be mutated. Use the dispatch function to update instead"; } else if (abstractValue.reason.has(ValueReason.Effect)) { return 'Updating a value used previously in an effect function or as an effect dependency is not allowed. Consider moving the mutation before calling useEffect()'; + } else if (abstractValue.reason.has(ValueReason.HookCaptured)) { + return 'Updating a value previously passed as an argument to a hook is not allowed. Consider moving the mutation before calling the hook'; + } else if (abstractValue.reason.has(ValueReason.HookReturn)) { + return 'Updating a value returned from a hook is not allowed. Consider moving the mutation into the hook where the value is constructed'; } else { return 'This mutates a variable that React considers immutable'; } 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 f0ecb67546..521f55026e 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts @@ -1980,28 +1980,17 @@ function computeEffectsForLegacySignature( break; } case Effect.ConditionallyMutateIterator: { - if ( - isArrayType(place.identifier) || - isSetType(place.identifier) || - isMapType(place.identifier) - ) { - effects.push({ - kind: 'Capture', - from: place, - into: lvalue, - }); - } else { - effects.push({ - kind: 'Capture', - from: place, - into: lvalue, - }); + const mutateIterator = conditionallyMutateIterator(place); + if (mutateIterator != null) { + effects.push(mutateIterator); + // TODO: should we always push to captures? captures.push(place); - effects.push({ - kind: 'MutateTransitiveConditionally', - value: place, - }); } + effects.push({ + kind: 'Capture', + from: place, + into: lvalue, + }); break; } case Effect.Freeze: { @@ -2191,6 +2180,7 @@ function computeEffectsForSignature( return null; } // Build substitutions + const mutableSpreads = new Set(); const substitutions: Map> = new Map(); substitutions.set(signature.receiver, [receiver]); substitutions.set(signature.returns, [lvalue]); @@ -2208,6 +2198,13 @@ function computeEffectsForSignature( } const place = arg.kind === 'Identifier' ? arg : arg.place; getOrInsertWith(substitutions, signature.rest, () => []).push(place); + + if (arg.kind === 'Spread') { + const mutateIterator = conditionallyMutateIterator(arg.place); + if (mutateIterator != null) { + mutableSpreads.add(arg.place.identifier.id); + } + } } else { const param = params[i]; substitutions.set(param, [arg]); @@ -2279,6 +2276,12 @@ function computeEffectsForSignature( case 'Freeze': { const values = substitutions.get(effect.value.identifier.id) ?? []; for (const value of values) { + if (mutableSpreads.has(value.identifier.id)) { + CompilerError.throwTodo({ + reason: 'Support spread syntax for hook arguments', + loc: value.loc, + }); + } effects.push({kind: 'Freeze', value, reason: effect.reason}); } break; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.hook-call-freezes-captured-identifier.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.hook-call-freezes-captured-identifier.expect.md index 7babe57b00..5e0a988627 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.hook-call-freezes-captured-identifier.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.hook-call-freezes-captured-identifier.expect.md @@ -32,7 +32,7 @@ export const FIXTURE_ENTRYPOINT = { 11 | }); 12 | > 13 | x.value += count; - | ^ InvalidReact: This mutates a variable that React considers immutable (13:13) + | ^ InvalidReact: Updating a value previously passed as an argument to a hook is not allowed. Consider moving the mutation before calling the hook (13:13) 14 | return ; 15 | } 16 | diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.hook-call-freezes-captured-memberexpr.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.hook-call-freezes-captured-memberexpr.expect.md index fcc47ddc2b..c5af59d642 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.hook-call-freezes-captured-memberexpr.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.hook-call-freezes-captured-memberexpr.expect.md @@ -32,7 +32,7 @@ export const FIXTURE_ENTRYPOINT = { 11 | }); 12 | > 13 | x.value += count; - | ^ InvalidReact: This mutates a variable that React considers immutable (13:13) + | ^ InvalidReact: Updating a value previously passed as an argument to a hook is not allowed. Consider moving the mutation before calling the hook (13:13) 14 | return ; 15 | } 16 | diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-non-imported-reanimated-shared-value-writes.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-non-imported-reanimated-shared-value-writes.expect.md index d3bb7f4136..ce278dda62 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-non-imported-reanimated-shared-value-writes.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-non-imported-reanimated-shared-value-writes.expect.md @@ -27,7 +27,7 @@ function SomeComponent() { 9 | return ( 10 |