From 294fa2c42234fbbabf2afadae50914e3abf9574d Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Thu, 24 Jul 2025 16:01:40 -0700 Subject: [PATCH 1/2] [compiler] Fix for edge cases of mutation of potentially frozen values Fixes two related cases of mutation of potentially frozen values. The first is method calls on frozen values. Previously, we modeled unknown function calls as potentially aliasing their receiver+args into the return value. If the receiver or argument were known to be frozen, then we would downgrade the `Alias` effect into an `ImmutableCapture`. However, within a function expression it's possible to call a function using a frozen value as an argument (that gets `Alias`-ed into the return) but where we don't have the context locally to know that the value is frozen. This results in cases like this: ```js const frozen = useContext(...); useEffect(() => { frozen.method().property = true; ^^^^^^^^^^^^^^^^^^^^^^^^ cannot mutate frozen value }, [...]); ``` Within the function we would infer: ``` t0 = MethodCall ... Create t0 = mutable Alias t0 <- frozen t1 = PropertyStore ... Mutate t0 ``` And then transitively infer the function expression as having a `Mutate 'frozen'` effect, which when evaluated against the outer context (`frozen` is frozen) is an error. The fix is to model unknown function calls as _maybe_ aliasing their receiver/args in the return, and then considering mutations of a maybe-aliased value to only be a conditional mutation of the source: ``` t0 = MethodCall ... Create t0 = mutable MaybeAlias t0 <- frozen // maybe alias now t1 = PropertyStore ... Mutate t0 ``` Then, the `Mutate t0` turns into a `MutateConditional 'frozen'`, which just gets ignored when we process the outer context. The second, related fix is for known mutation of phis that may be a frozen value. The previous inference model correctly recorded these as errors, the new model does not. We now correctly report a validation error for this case in the new model. --- .../src/HIR/PrintHIR.ts | 5 +- .../src/Inference/AliasingEffects.ts | 20 ++++- .../src/Inference/AnalyseFunctions.ts | 3 +- .../Inference/InferMutationAliasingEffects.ts | 6 +- .../Inference/InferMutationAliasingRanges.ts | 75 +++++++++++++--- .../Inference/MUTABILITY_ALIASING_MODEL.md | 15 ++++ ...mutate-phi-which-could-be-frozen.expect.md | 39 ++++++++ ...nvalid-mutate-phi-which-could-be-frozen.js | 12 +++ ...p-with-context-variable-iterator.expect.md | 61 +++++++++++++ ...or-loop-with-context-variable-iterator.js} | 5 ++ ...p-with-context-variable-iterator.expect.md | 89 ------------------- ...-argument-in-function-expression.expect.md | 77 ++++++++++++++++ ...-frozen-argument-in-function-expression.js | 18 ++++ ...zen-value-in-function-expression.expect.md | 77 ++++++++++++++++ ...-on-frozen-value-in-function-expression.js | 18 ++++ ...-call-on-frozen-value-is-allowed.expect.md | 57 ++++++++++++ ...-method-call-on-frozen-value-is-allowed.js | 12 +++ 17 files changed, 483 insertions(+), 106 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-mutate-phi-which-could-be-frozen.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-mutate-phi-which-could-be-frozen.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-for-loop-with-context-variable-iterator.expect.md rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{for-loop-with-context-variable-iterator.js => error.todo-for-loop-with-context-variable-iterator.js} (63%) delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/for-loop-with-context-variable-iterator.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-mutate-result-of-function-call-with-frozen-argument-in-function-expression.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-mutate-result-of-function-call-with-frozen-argument-in-function-expression.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-mutate-result-of-method-call-on-frozen-value-in-function-expression.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-mutate-result-of-method-call-on-frozen-value-in-function-expression.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-mutate-result-of-method-call-on-frozen-value-is-allowed.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-mutate-result-of-method-call-on-frozen-value-is-allowed.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 cecbb49e4436..869799073e49 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts @@ -943,7 +943,10 @@ export function printAliasingEffect(effect: AliasingEffect): string { return `Assign ${printPlaceForAliasEffect(effect.into)} = ${printPlaceForAliasEffect(effect.from)}`; } case 'Alias': { - return `Alias ${printPlaceForAliasEffect(effect.into)} = ${printPlaceForAliasEffect(effect.from)}`; + return `Alias ${printPlaceForAliasEffect(effect.into)} <- ${printPlaceForAliasEffect(effect.from)}`; + } + case 'MaybeAlias': { + return `MaybeAlias ${printPlaceForAliasEffect(effect.into)} <- ${printPlaceForAliasEffect(effect.from)}`; } case 'Capture': { return `Capture ${printPlaceForAliasEffect(effect.into)} <- ${printPlaceForAliasEffect(effect.from)}`; 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 95f4e0f5bb1a..c492e2835460 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/AliasingEffects.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/AliasingEffects.ts @@ -90,6 +90,23 @@ export type AliasingEffect = * c could be mutating a. */ | {kind: 'Alias'; from: Place; into: Place} + + /** + * Indicates the potential for information flow from `from` to `into`. This is used for a specific + * case: functions with unknown signatures. If the compiler sees a call such as `foo(x)`, it has to + * consider several possibilities (which may depend on the arguments): + * - foo(x) returns a new mutable value that does not capture any information from x. + * - foo(x) returns a new mutable value that *does* capture information from x. + * - foo(x) returns x itself, ie foo is the identity function + * + * The same is true of functions that take multiple arguments: `cond(a, b, c)` could conditionally + * return b or c depending on the value of a. + * + * To represent this case, MaybeAlias represents the fact that an aliasing relationship could exist. + * Any mutations that flow through this relationship automatically become conditional. + */ + | {kind: 'MaybeAlias'; from: Place; into: Place} + /** * Records direct assignment: `into = from`. */ @@ -183,7 +200,8 @@ export function hashEffect(effect: AliasingEffect): string { case 'ImmutableCapture': case 'Assign': case 'Alias': - case 'Capture': { + case 'Capture': + case 'MaybeAlias': { return [ effect.kind, effect.from.identifier.id, diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/AnalyseFunctions.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/AnalyseFunctions.ts index 7052cb2dd8a5..78d82c0c461d 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/AnalyseFunctions.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/AnalyseFunctions.ts @@ -85,7 +85,8 @@ function lowerWithMutationAliasing(fn: HIRFunction): void { case 'Assign': case 'Alias': case 'Capture': - case 'CreateFrom': { + case 'CreateFrom': + case 'MaybeAlias': { capturedOrMutated.add(effect.from.identifier.id); break; } 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 ca6c6ebae96f..2adf78fe058e 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts @@ -691,6 +691,7 @@ function applyEffect( } break; } + case 'MaybeAlias': case 'Alias': case 'Capture': { CompilerError.invariant( @@ -955,7 +956,7 @@ function applyEffect( context, state, // OK: recording information flow - {kind: 'Alias', from: operand, into: effect.into}, + {kind: 'MaybeAlias', from: operand, into: effect.into}, initialized, effects, ); @@ -1323,7 +1324,7 @@ class InferenceState { return 'mutate-global'; } case ValueKind.MaybeFrozen: { - return 'none'; + return 'mutate-frozen'; } default: { assertExhaustive(kind, `Unexpected kind ${kind}`); @@ -2376,6 +2377,7 @@ function computeEffectsForSignature( // Apply substitutions for (const effect of signature.effects) { switch (effect.kind) { + case 'MaybeAlias': case 'Assign': case 'ImmutableCapture': case 'Alias': 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 cd344342225c..135e802c82d1 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingRanges.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingRanges.ts @@ -160,6 +160,8 @@ export function inferMutationAliasingRanges( state.assign(index++, effect.from, effect.into); } else if (effect.kind === 'Alias') { state.assign(index++, effect.from, effect.into); + } else if (effect.kind === 'MaybeAlias') { + state.maybeAlias(index++, effect.from, effect.into); } else if (effect.kind === 'Capture') { state.capture(index++, effect.from, effect.into); } else if ( @@ -346,7 +348,8 @@ export function inferMutationAliasingRanges( case 'Assign': case 'Alias': case 'Capture': - case 'CreateFrom': { + case 'CreateFrom': + case 'MaybeAlias': { const isMutatedOrReassigned = effect.into.identifier.mutableRange.end > instr.id; if (isMutatedOrReassigned) { @@ -567,7 +570,12 @@ type Node = { createdFrom: Map; captures: Map; aliases: Map; - edges: Array<{index: number; node: Identifier; kind: 'capture' | 'alias'}>; + maybeAliases: Map; + edges: Array<{ + index: number; + node: Identifier; + kind: 'capture' | 'alias' | 'maybeAlias'; + }>; transitive: {kind: MutationKind; loc: SourceLocation} | null; local: {kind: MutationKind; loc: SourceLocation} | null; lastMutated: number; @@ -585,6 +593,7 @@ class AliasingState { createdFrom: new Map(), captures: new Map(), aliases: new Map(), + maybeAliases: new Map(), edges: [], transitive: null, local: null, @@ -630,6 +639,18 @@ class AliasingState { } } + maybeAlias(index: number, from: Place, into: Place): void { + const fromNode = this.nodes.get(from.identifier); + const toNode = this.nodes.get(into.identifier); + if (fromNode == null || toNode == null) { + return; + } + fromNode.edges.push({index, node: into.identifier, kind: 'maybeAlias'}); + if (!toNode.maybeAliases.has(from.identifier)) { + toNode.maybeAliases.set(from.identifier, index); + } + } + render(index: number, start: Identifier, errors: CompilerError): void { const seen = new Set(); const queue: Array = [start]; @@ -673,22 +694,24 @@ class AliasingState { // Null is used for simulated mutations end: InstructionId | null, transitive: boolean, - kind: MutationKind, + startKind: MutationKind, loc: SourceLocation, errors: CompilerError, ): void { - const seen = new Set(); + const seen = new Map(); const queue: Array<{ place: Identifier; transitive: boolean; direction: 'backwards' | 'forwards'; - }> = [{place: start, transitive, direction: 'backwards'}]; + kind: MutationKind; + }> = [{place: start, transitive, direction: 'backwards', kind: startKind}]; while (queue.length !== 0) { - const {place: current, transitive, direction} = queue.pop()!; - if (seen.has(current)) { + const {place: current, transitive, direction, kind} = queue.pop()!; + const previousKind = seen.get(current); + if (previousKind != null && previousKind >= kind) { continue; } - seen.add(current); + seen.set(current, kind); const node = this.nodes.get(current); if (node == null) { continue; @@ -724,13 +747,18 @@ class AliasingState { if (edge.index >= index) { break; } - queue.push({place: edge.node, transitive, direction: 'forwards'}); + queue.push({place: edge.node, transitive, direction: 'forwards', kind}); } for (const [alias, when] of node.createdFrom) { if (when >= index) { continue; } - queue.push({place: alias, transitive: true, direction: 'backwards'}); + queue.push({ + place: alias, + transitive: true, + direction: 'backwards', + kind, + }); } if (direction === 'backwards' || node.value.kind !== 'Phi') { /** @@ -747,7 +775,25 @@ class AliasingState { if (when >= index) { continue; } - queue.push({place: alias, transitive, direction: 'backwards'}); + queue.push({place: alias, transitive, direction: 'backwards', kind}); + } + /** + * MaybeAlias indicates potential data flow from unknown function calls, + * so we downgrade mutations through these aliases to consider them + * conditional. This means we'll consider them for mutation *range* + * purposes but not report validation errors for mutations, since + * we aren't sure that the `from` value could actually be aliased. + */ + for (const [alias, when] of node.maybeAliases) { + if (when >= index) { + continue; + } + queue.push({ + place: alias, + transitive, + direction: 'backwards', + kind: MutationKind.Conditional, + }); } } /** @@ -758,7 +804,12 @@ class AliasingState { if (when >= index) { continue; } - queue.push({place: capture, transitive, direction: 'backwards'}); + queue.push({ + place: capture, + transitive, + direction: 'backwards', + kind, + }); } } } diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/MUTABILITY_ALIASING_MODEL.md b/compiler/packages/babel-plugin-react-compiler/src/Inference/MUTABILITY_ALIASING_MODEL.md index 60ef16a6245f..ab327c255b10 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/MUTABILITY_ALIASING_MODEL.md +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/MUTABILITY_ALIASING_MODEL.md @@ -153,6 +153,10 @@ This is somewhat the inverse of `Capture`. The `CreateFrom` effect describes tha Describes immutable data flow from one value to another. This is not currently used for anything, but is intended to eventually power a more sophisticated escape analysis. +### MaybeAlias + +Describes potential data flow that the compiler knows may occur behind a function call, but cannot be sure about. For example, `foo(x)` _may_ be the identity function and return `x`, or `cond(a, b, c)` may conditionally return `b` or `c` depending on the value of `a`, but those functions could just as easily return new mutable values and not capture any information from their arguments. MaybeAlias represents that we have to consider the potential for data flow when deciding mutable ranges, but should be conservative about reporting errors. For example, `foo(someFrozenValue).property = true` should not error since we don't know for certain that foo returns its input. + ### State-Changing Effects The following effects describe state changes to specific values, not data flow. In many cases, JavaScript semantics will involve a combination of both data-flow effects *and* state-change effects. For example, `object.property = value` has data flow (`Capture object <- value`) and mutation (`Mutate object`). @@ -347,6 +351,17 @@ a.b = b; // capture mutate(a); // can transitively mutate b ``` +### MaybeAlias makes mutation conditional + +Because we don't know for certain that the aliasing occurs, we consider the mutation conditional against the source. + +``` +MaybeAlias a <- b +Mutate a +=> +MutateConditional b +``` + ### Freeze Does Not Freeze the Value Freeze does not freeze the value itself: diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-mutate-phi-which-could-be-frozen.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-mutate-phi-which-could-be-frozen.expect.md new file mode 100644 index 000000000000..3ea3d017179f --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-mutate-phi-which-could-be-frozen.expect.md @@ -0,0 +1,39 @@ + +## Input + +```javascript +import {useHook} from 'shared-runtime'; + +function Component(props) { + const frozen = useHook(); + let x; + if (props.cond) { + x = frozen; + } else { + x = {}; + } + x.property = true; +} + +``` + + +## 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. + +error.invalid-mutate-phi-which-could-be-frozen.ts:11:2 + 9 | x = {}; + 10 | } +> 11 | x.property = true; + | ^ value cannot be modified + 12 | } + 13 | +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-mutate-phi-which-could-be-frozen.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-mutate-phi-which-could-be-frozen.js new file mode 100644 index 000000000000..f1f4691006b2 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-mutate-phi-which-could-be-frozen.js @@ -0,0 +1,12 @@ +import {useHook} from 'shared-runtime'; + +function Component(props) { + const frozen = useHook(); + let x; + if (props.cond) { + x = frozen; + } else { + x = {}; + } + x.property = true; +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-for-loop-with-context-variable-iterator.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-for-loop-with-context-variable-iterator.expect.md new file mode 100644 index 000000000000..e56f70b29eb4 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-for-loop-with-context-variable-iterator.expect.md @@ -0,0 +1,61 @@ + +## Input + +```javascript +import {Stringify, useIdentity} from 'shared-runtime'; + +function Component() { + const data = useIdentity( + new Map([ + [0, 'value0'], + [1, 'value1'], + ]) + ); + const items = []; + // NOTE: `i` is a context variable because it's reassigned and also referenced + // within a closure, the `onClick` handler of each item + // TODO: for loops create a unique environment on each iteration, which means + // that if the iteration variable is only updated in the updater, the variable + // is effectively const within the body and the "update" acts more like + // a re-initialization than a reassignment. + // Until we model this "new environment" semantic, we allow this case to error + for (let i = MIN; i <= MAX; i += INCREMENT) { + items.push( + data.get(i)} shouldInvokeFns={true} /> + ); + } + return <>{items}; +} + +const MIN = 0; +const MAX = 3; +const INCREMENT = 1; + +export const FIXTURE_ENTRYPOINT = { + params: [], + fn: Component, +}; + +``` + + +## Error + +``` +Found 1 error: + +Error: This value cannot be modified + +Modifying a value used previously in JSX is not allowed. Consider moving the modification before the JSX. + +error.todo-for-loop-with-context-variable-iterator.ts:18:30 + 16 | // a re-initialization than a reassignment. + 17 | // Until we model this "new environment" semantic, we allow this case to error +> 18 | for (let i = MIN; i <= MAX; i += INCREMENT) { + | ^ `i` cannot be modified + 19 | items.push( + 20 | data.get(i)} shouldInvokeFns={true} /> + 21 | ); +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/for-loop-with-context-variable-iterator.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-for-loop-with-context-variable-iterator.js similarity index 63% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/for-loop-with-context-variable-iterator.js rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-for-loop-with-context-variable-iterator.js index f7dbd76a6449..5942071d6c19 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/for-loop-with-context-variable-iterator.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-for-loop-with-context-variable-iterator.js @@ -10,6 +10,11 @@ function Component() { const items = []; // NOTE: `i` is a context variable because it's reassigned and also referenced // within a closure, the `onClick` handler of each item + // TODO: for loops create a unique environment on each iteration, which means + // that if the iteration variable is only updated in the updater, the variable + // is effectively const within the body and the "update" acts more like + // a re-initialization than a reassignment. + // Until we model this "new environment" semantic, we allow this case to error for (let i = MIN; i <= MAX; i += INCREMENT) { items.push( data.get(i)} shouldInvokeFns={true} /> diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/for-loop-with-context-variable-iterator.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/for-loop-with-context-variable-iterator.expect.md deleted file mode 100644 index db9ea1d5b628..000000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/for-loop-with-context-variable-iterator.expect.md +++ /dev/null @@ -1,89 +0,0 @@ - -## Input - -```javascript -import {Stringify, useIdentity} from 'shared-runtime'; - -function Component() { - const data = useIdentity( - new Map([ - [0, 'value0'], - [1, 'value1'], - ]) - ); - const items = []; - // NOTE: `i` is a context variable because it's reassigned and also referenced - // within a closure, the `onClick` handler of each item - for (let i = MIN; i <= MAX; i += INCREMENT) { - items.push( - data.get(i)} shouldInvokeFns={true} /> - ); - } - return <>{items}; -} - -const MIN = 0; -const MAX = 3; -const INCREMENT = 1; - -export const FIXTURE_ENTRYPOINT = { - params: [], - fn: Component, -}; - -``` - -## Code - -```javascript -import { c as _c } from "react/compiler-runtime"; -import { Stringify, useIdentity } from "shared-runtime"; - -function Component() { - const $ = _c(3); - let t0; - if ($[0] === Symbol.for("react.memo_cache_sentinel")) { - t0 = new Map([ - [0, "value0"], - [1, "value1"], - ]); - $[0] = t0; - } else { - t0 = $[0]; - } - const data = useIdentity(t0); - let t1; - if ($[1] !== data) { - const items = []; - for (let i = MIN; i <= MAX; i = i + INCREMENT, i) { - items.push( - data.get(i)} - shouldInvokeFns={true} - />, - ); - } - - t1 = <>{items}; - $[1] = data; - $[2] = t1; - } else { - t1 = $[2]; - } - return t1; -} - -const MIN = 0; -const MAX = 3; -const INCREMENT = 1; - -export const FIXTURE_ENTRYPOINT = { - params: [], - fn: Component, -}; - -``` - -### Eval output -(kind: ok)
{"onClick":{"kind":"Function","result":"value0"},"shouldInvokeFns":true}
{"onClick":{"kind":"Function","result":"value1"},"shouldInvokeFns":true}
{"onClick":{"kind":"Function"},"shouldInvokeFns":true}
{"onClick":{"kind":"Function"},"shouldInvokeFns":true}
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-mutate-result-of-function-call-with-frozen-argument-in-function-expression.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-mutate-result-of-function-call-with-frozen-argument-in-function-expression.expect.md new file mode 100644 index 000000000000..320b252bb5cb --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-mutate-result-of-function-call-with-frozen-argument-in-function-expression.expect.md @@ -0,0 +1,77 @@ + +## Input + +```javascript +import {identity, makeObject_Primitives, Stringify} from 'shared-runtime'; + +function Example(props) { + const object = props.object; + const f = () => { + // The argument maybe-aliases into the return + const obj = identity(object); + obj.property = props.value; + return obj; + }; + const obj = f(); + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Example, + params: [{object: makeObject_Primitives(), value: 42}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import { identity, makeObject_Primitives, Stringify } from "shared-runtime"; + +function Example(props) { + const $ = _c(7); + const object = props.object; + let t0; + if ($[0] !== object || $[1] !== props.value) { + t0 = () => { + const obj = identity(object); + obj.property = props.value; + return obj; + }; + $[0] = object; + $[1] = props.value; + $[2] = t0; + } else { + t0 = $[2]; + } + const f = t0; + let t1; + if ($[3] !== f) { + t1 = f(); + $[3] = f; + $[4] = t1; + } else { + t1 = $[4]; + } + const obj_0 = t1; + let t2; + if ($[5] !== obj_0) { + t2 = ; + $[5] = obj_0; + $[6] = t2; + } else { + t2 = $[6]; + } + return t2; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Example, + params: [{ object: makeObject_Primitives(), value: 42 }], +}; + +``` + +### Eval output +(kind: ok)
{"obj":{"a":0,"b":"value1","c":true,"property":42}}
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-mutate-result-of-function-call-with-frozen-argument-in-function-expression.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-mutate-result-of-function-call-with-frozen-argument-in-function-expression.js new file mode 100644 index 000000000000..88070806119a --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-mutate-result-of-function-call-with-frozen-argument-in-function-expression.js @@ -0,0 +1,18 @@ +import {identity, makeObject_Primitives, Stringify} from 'shared-runtime'; + +function Example(props) { + const object = props.object; + const f = () => { + // The argument maybe-aliases into the return + const obj = identity(object); + obj.property = props.value; + return obj; + }; + const obj = f(); + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Example, + params: [{object: makeObject_Primitives(), value: 42}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-mutate-result-of-method-call-on-frozen-value-in-function-expression.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-mutate-result-of-method-call-on-frozen-value-in-function-expression.expect.md new file mode 100644 index 000000000000..d3dbb86711a7 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-mutate-result-of-method-call-on-frozen-value-in-function-expression.expect.md @@ -0,0 +1,77 @@ + +## Input + +```javascript +import {makeObject_Primitives, Stringify} from 'shared-runtime'; + +function Example(props) { + const object = props.object; + const f = () => { + // The receiver maybe-aliases into the return + const obj = object.makeObject(); + obj.property = props.value; + return obj; + }; + const obj = f(); + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Example, + params: [{object: {makeObject: makeObject_Primitives}, value: 42}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import { makeObject_Primitives, Stringify } from "shared-runtime"; + +function Example(props) { + const $ = _c(7); + const object = props.object; + let t0; + if ($[0] !== object || $[1] !== props.value) { + t0 = () => { + const obj = object.makeObject(); + obj.property = props.value; + return obj; + }; + $[0] = object; + $[1] = props.value; + $[2] = t0; + } else { + t0 = $[2]; + } + const f = t0; + let t1; + if ($[3] !== f) { + t1 = f(); + $[3] = f; + $[4] = t1; + } else { + t1 = $[4]; + } + const obj_0 = t1; + let t2; + if ($[5] !== obj_0) { + t2 = ; + $[5] = obj_0; + $[6] = t2; + } else { + t2 = $[6]; + } + return t2; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Example, + params: [{ object: { makeObject: makeObject_Primitives }, value: 42 }], +}; + +``` + +### Eval output +(kind: ok)
{"obj":{"a":0,"b":"value1","c":true,"property":42}}
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-mutate-result-of-method-call-on-frozen-value-in-function-expression.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-mutate-result-of-method-call-on-frozen-value-in-function-expression.js new file mode 100644 index 000000000000..92834df138d3 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-mutate-result-of-method-call-on-frozen-value-in-function-expression.js @@ -0,0 +1,18 @@ +import {makeObject_Primitives, Stringify} from 'shared-runtime'; + +function Example(props) { + const object = props.object; + const f = () => { + // The receiver maybe-aliases into the return + const obj = object.makeObject(); + obj.property = props.value; + return obj; + }; + const obj = f(); + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Example, + params: [{object: {makeObject: makeObject_Primitives}, value: 42}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-mutate-result-of-method-call-on-frozen-value-is-allowed.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-mutate-result-of-method-call-on-frozen-value-is-allowed.expect.md new file mode 100644 index 000000000000..0c91a935ed7d --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-mutate-result-of-method-call-on-frozen-value-is-allowed.expect.md @@ -0,0 +1,57 @@ + +## Input + +```javascript +import {makeObject_Primitives, Stringify} from 'shared-runtime'; + +function Example(props) { + const obj = props.object.makeObject(); + obj.property = props.value; + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Example, + params: [{object: {makeObject: makeObject_Primitives}, value: 42}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import { makeObject_Primitives, Stringify } from "shared-runtime"; + +function Example(props) { + const $ = _c(5); + let obj; + if ($[0] !== props.object || $[1] !== props.value) { + obj = props.object.makeObject(); + obj.property = props.value; + $[0] = props.object; + $[1] = props.value; + $[2] = obj; + } else { + obj = $[2]; + } + let t0; + if ($[3] !== obj) { + t0 = ; + $[3] = obj; + $[4] = t0; + } else { + t0 = $[4]; + } + return t0; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Example, + params: [{ object: { makeObject: makeObject_Primitives }, value: 42 }], +}; + +``` + +### Eval output +(kind: ok)
{"obj":{"a":0,"b":"value1","c":true,"property":42}}
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-mutate-result-of-method-call-on-frozen-value-is-allowed.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-mutate-result-of-method-call-on-frozen-value-is-allowed.js new file mode 100644 index 000000000000..d5ed97e159b2 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-mutate-result-of-method-call-on-frozen-value-is-allowed.js @@ -0,0 +1,12 @@ +import {makeObject_Primitives, Stringify} from 'shared-runtime'; + +function Example(props) { + const obj = props.object.makeObject(); + obj.property = props.value; + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Example, + params: [{object: {makeObject: makeObject_Primitives}, value: 42}], +}; From 6cd727ccbdeb604fc0e29e1c9b8775fc197483a4 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Thu, 24 Jul 2025 16:11:39 -0700 Subject: [PATCH 2/2] [compiler] clarify text for setState-in-effect error --- .../src/Validation/ValidateNoSetStateInEffects.ts | 11 ++++++++--- ...invalid-setState-in-useEffect-transitive.expect.md | 2 +- .../compiler/invalid-setState-in-useEffect.expect.md | 2 +- 3 files changed, 10 insertions(+), 5 deletions(-) diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoSetStateInEffects.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoSetStateInEffects.ts index 3c810025a7e5..9c4efa380cd4 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoSetStateInEffects.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoSetStateInEffects.ts @@ -97,16 +97,21 @@ export function validateNoSetStateInEffects( errors.pushDiagnostic( CompilerDiagnostic.create({ category: - 'Calling setState within an effect can trigger cascading renders', + 'Calling setState synchronously within an effect can trigger cascading renders', description: - 'Calling setState directly within a useEffect causes cascading renders that can hurt performance, and is not recommended. Consider alternatives to useEffect. (https://react.dev/learn/you-might-not-need-an-effect)', + 'Effects are intended to synchronize state between React and external systems such as manually updating the DOM, state management libraries, or other platform APIs. ' + + 'In general, the body of an effect should do one or both of the following:\n' + + '* Update external systems with the latest state from React.\n' + + '* Subscribe for updates from some external system, calling setState in a callback function when external state changes.\n\n' + + 'Calling setState synchronously within an effect body causes cascading renders that can hurt performance, and is not recommended. ' + + '(https://react.dev/learn/you-might-not-need-an-effect)', severity: ErrorSeverity.InvalidReact, suggestions: null, }).withDetail({ kind: 'error', loc: setState.loc, message: - 'Avoid calling setState() in the top-level of an effect', + 'Avoid calling setState() directly within an effect', }), ); } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-setState-in-useEffect-transitive.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-setState-in-useEffect-transitive.expect.md index 4e9e1f2c3a79..b629bfef2736 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-setState-in-useEffect-transitive.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-setState-in-useEffect-transitive.expect.md @@ -65,7 +65,7 @@ function _temp(s) { ## Logs ``` -{"kind":"CompileError","detail":{"options":{"category":"Calling setState within an effect can trigger cascading renders","description":"Calling setState directly within a useEffect causes cascading renders that can hurt performance, and is not recommended. Consider alternatives to useEffect. (https://react.dev/learn/you-might-not-need-an-effect)","severity":"InvalidReact","suggestions":null,"details":[{"kind":"error","loc":{"start":{"line":13,"column":4,"index":265},"end":{"line":13,"column":5,"index":266},"filename":"invalid-setState-in-useEffect-transitive.ts","identifierName":"g"},"message":"Avoid calling setState() in the top-level of an effect"}]}},"fnLoc":null} +{"kind":"CompileError","detail":{"options":{"category":"Calling setState synchronously within an effect can trigger cascading renders","description":"Effects are intended to synchronize state between React and external systems such as manually updating the DOM, state management libraries, or other platform APIs. In general, the body of an effect should do one or both of the following:\n* Update external systems with the latest state from React.\n* Subscribe for updates from some external system, calling setState in a callback function when external state changes.\n\nCalling setState synchronously within an effect body causes cascading renders that can hurt performance, and is not recommended. (https://react.dev/learn/you-might-not-need-an-effect)","severity":"InvalidReact","suggestions":null,"details":[{"kind":"error","loc":{"start":{"line":13,"column":4,"index":265},"end":{"line":13,"column":5,"index":266},"filename":"invalid-setState-in-useEffect-transitive.ts","identifierName":"g"},"message":"Avoid calling setState() directly within an effect"}]}},"fnLoc":null} {"kind":"CompileSuccess","fnLoc":{"start":{"line":4,"column":0,"index":92},"end":{"line":16,"column":1,"index":293},"filename":"invalid-setState-in-useEffect-transitive.ts"},"fnName":"Component","memoSlots":2,"memoBlocks":2,"memoValues":2,"prunedMemoBlocks":0,"prunedMemoValues":0} ``` diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-setState-in-useEffect.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-setState-in-useEffect.expect.md index 8d902c2c7d64..c16890e52e5f 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-setState-in-useEffect.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-setState-in-useEffect.expect.md @@ -45,7 +45,7 @@ function _temp(s) { ## Logs ``` -{"kind":"CompileError","detail":{"options":{"category":"Calling setState within an effect can trigger cascading renders","description":"Calling setState directly within a useEffect causes cascading renders that can hurt performance, and is not recommended. Consider alternatives to useEffect. (https://react.dev/learn/you-might-not-need-an-effect)","severity":"InvalidReact","suggestions":null,"details":[{"kind":"error","loc":{"start":{"line":7,"column":4,"index":180},"end":{"line":7,"column":12,"index":188},"filename":"invalid-setState-in-useEffect.ts","identifierName":"setState"},"message":"Avoid calling setState() in the top-level of an effect"}]}},"fnLoc":null} +{"kind":"CompileError","detail":{"options":{"category":"Calling setState synchronously within an effect can trigger cascading renders","description":"Effects are intended to synchronize state between React and external systems such as manually updating the DOM, state management libraries, or other platform APIs. In general, the body of an effect should do one or both of the following:\n* Update external systems with the latest state from React.\n* Subscribe for updates from some external system, calling setState in a callback function when external state changes.\n\nCalling setState synchronously within an effect body causes cascading renders that can hurt performance, and is not recommended. (https://react.dev/learn/you-might-not-need-an-effect)","severity":"InvalidReact","suggestions":null,"details":[{"kind":"error","loc":{"start":{"line":7,"column":4,"index":180},"end":{"line":7,"column":12,"index":188},"filename":"invalid-setState-in-useEffect.ts","identifierName":"setState"},"message":"Avoid calling setState() directly within an effect"}]}},"fnLoc":null} {"kind":"CompileSuccess","fnLoc":{"start":{"line":4,"column":0,"index":92},"end":{"line":10,"column":1,"index":225},"filename":"invalid-setState-in-useEffect.ts"},"fnName":"Component","memoSlots":1,"memoBlocks":1,"memoValues":1,"prunedMemoBlocks":0,"prunedMemoValues":0} ```