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 4482d17890196..ba2f4e8c4cc79 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/ObjectShape.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/ObjectShape.ts @@ -545,8 +545,16 @@ addObject(BUILTIN_SHAPES, BuiltInMixedReadonlyId, [ [ 'map', addFunction(BUILTIN_SHAPES, [], { + /** + * Note `map`'s arguments are annotated as Effect.ConditionallyMutate as + * calling `.map(fn)` might invoke `fn`, which means replaying its + * effects. + * + * (Note that Effect.Read / Effect.Capture on a function type means + * potential data dependency or aliasing respectively.) + */ positionalParams: [], - restParam: Effect.Read, + restParam: Effect.ConditionallyMutate, returnType: {kind: 'Object', shapeId: BuiltInArrayId}, calleeEffect: Effect.ConditionallyMutate, returnValueKind: ValueKind.Mutable, 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 0ae54839b6fa3..fe209924e4e08 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferFunctionEffects.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferFunctionEffects.ts @@ -41,11 +41,16 @@ function inferOperandEffect(state: State, place: Place): null | FunctionEffect { if (isRefOrRefValue(place.identifier)) { break; } else if (value.kind === ValueKind.Context) { + CompilerError.invariant(value.context.size > 0, { + reason: + "[InferFunctionEffects] Expected Context-kind value's capture list to be non-empty.", + loc: place.loc, + }); return { kind: 'ContextMutation', loc: place.loc, effect: place.effect, - places: value.context.size === 0 ? new Set([place]) : value.context, + places: value.context, }; } else if ( value.kind !== ValueKind.Mutable && 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 8cf30a9666e25..70395e58d930e 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferReferenceEffects.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferReferenceEffects.ts @@ -857,17 +857,19 @@ function inferBlock( break; } case 'ArrayExpression': { - const valueKind: AbstractValue = hasContextRefOperand(state, instrValue) - ? { - kind: ValueKind.Context, - reason: new Set([ValueReason.Other]), - context: new Set(), - } - : { - kind: ValueKind.Mutable, - reason: new Set([ValueReason.Other]), - context: new Set(), - }; + const contextRefOperands = getContextRefOperand(state, instrValue); + const valueKind: AbstractValue = + contextRefOperands.length > 0 + ? { + kind: ValueKind.Context, + reason: new Set([ValueReason.Other]), + context: new Set(contextRefOperands), + } + : { + kind: ValueKind.Mutable, + reason: new Set([ValueReason.Other]), + context: new Set(), + }; continuation = { kind: 'initialize', valueKind, @@ -918,17 +920,19 @@ function inferBlock( break; } case 'ObjectExpression': { - const valueKind: AbstractValue = hasContextRefOperand(state, instrValue) - ? { - kind: ValueKind.Context, - reason: new Set([ValueReason.Other]), - context: new Set(), - } - : { - kind: ValueKind.Mutable, - reason: new Set([ValueReason.Other]), - context: new Set(), - }; + const contextRefOperands = getContextRefOperand(state, instrValue); + const valueKind: AbstractValue = + contextRefOperands.length > 0 + ? { + kind: ValueKind.Context, + reason: new Set([ValueReason.Other]), + context: new Set(contextRefOperands), + } + : { + kind: ValueKind.Mutable, + reason: new Set([ValueReason.Other]), + context: new Set(), + }; for (const property of instrValue.properties) { switch (property.kind) { @@ -1593,15 +1597,21 @@ function inferBlock( } case 'LoadLocal': { const lvalue = instr.lvalue; - const effect = - state.isDefined(lvalue) && - state.kind(lvalue).kind === ValueKind.Context - ? Effect.ConditionallyMutate - : Effect.Capture; + CompilerError.invariant( + !( + state.isDefined(lvalue) && + state.kind(lvalue).kind === ValueKind.Context + ), + { + reason: + '[InferReferenceEffects] Unexpected LoadLocal with context kind', + loc: lvalue.loc, + }, + ); state.referenceAndRecordEffects( freezeActions, instrValue.place, - effect, + Effect.Capture, ValueReason.Other, ); lvalue.effect = Effect.ConditionallyMutate; @@ -1932,19 +1942,20 @@ function inferBlock( ); } -function hasContextRefOperand( +function getContextRefOperand( state: InferenceState, instrValue: InstructionValue, -): boolean { +): Array { + const result = []; for (const place of eachInstructionValueOperand(instrValue)) { if ( state.isDefined(place) && state.kind(place).kind === ValueKind.Context ) { - return true; + result.push(place); } } - return false; + return result; } export function getFunctionCallSignature( diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/mixedreadonly-mutating-map.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/mixedreadonly-mutating-map.expect.md new file mode 100644 index 0000000000000..4867388a864d2 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/mixedreadonly-mutating-map.expect.md @@ -0,0 +1,156 @@ + +## Input + +```javascript +import { + arrayPush, + identity, + makeArray, + Stringify, + useFragment, +} from 'shared-runtime'; + +/** + * Bug repro showing why it's invalid for function references to be annotated + * with a `Read` effect when that reference might lead to the function being + * invoked. + * + * Note that currently, `Array.map` is annotated to have `Read` effects on its + * operands. This is incorrect as function effects must be replayed when `map` + * is called + * - Read: non-aliasing data dependency + * - Capture: maybe-aliasing data dependency + * - ConditionallyMutate: maybe-aliasing data dependency; maybe-write / invoke + * but only if the value is mutable + * + * Invalid evaluator result: Found differences in evaluator results Non-forget + * (expected): (kind: ok) + *
{"x":[2,2,2],"count":3}
{"item":1}
+ *
{"x":[2,2,2],"count":4}
{"item":1}
+ * Forget: + * (kind: ok) + *
{"x":[2,2,2],"count":3}
{"item":1}
+ *
{"x":[2,2,2,2,2,2],"count":4}
{"item":1}
+ */ + +function Component({extraJsx}) { + const x = makeArray(); + const items = useFragment(); + // This closure has the following effects that must be replayed: + // - MaybeFreeze / Capture of `items` + // - ConditionalMutate of x + const jsx = items.a.map((item, i) => { + arrayPush(x, 2); + return ; + }); + const offset = jsx.length; + for (let i = 0; i < extraJsx; i++) { + jsx.push(); + } + const count = jsx.length; + identity(count); + return ( + <> + + {jsx[0]} + + ); +} +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{extraJsx: 0}], + sequentialRenders: [{extraJsx: 0}, {extraJsx: 1}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import { + arrayPush, + identity, + makeArray, + Stringify, + useFragment, +} from "shared-runtime"; + +/** + * Bug repro showing why it's invalid for function references to be annotated + * with a `Read` effect when that reference might lead to the function being + * invoked. + * + * Note that currently, `Array.map` is annotated to have `Read` effects on its + * operands. This is incorrect as function effects must be replayed when `map` + * is called + * - Read: non-aliasing data dependency + * - Capture: maybe-aliasing data dependency + * - ConditionallyMutate: maybe-aliasing data dependency; maybe-write / invoke + * but only if the value is mutable + * + * Invalid evaluator result: Found differences in evaluator results Non-forget + * (expected): (kind: ok) + *
{"x":[2,2,2],"count":3}
{"item":1}
+ *
{"x":[2,2,2],"count":4}
{"item":1}
+ * Forget: + * (kind: ok) + *
{"x":[2,2,2],"count":3}
{"item":1}
+ *
{"x":[2,2,2,2,2,2],"count":4}
{"item":1}
+ */ + +function Component(t0) { + const $ = _c(6); + const { extraJsx } = t0; + const x = makeArray(); + const items = useFragment(); + + const jsx = items.a.map((item, i) => { + arrayPush(x, 2); + return ; + }); + const offset = jsx.length; + for (let i_0 = 0; i_0 < extraJsx; i_0++) { + jsx.push(); + } + + const count = jsx.length; + identity(count); + let t1; + if ($[0] !== count || $[1] !== x) { + t1 = ; + $[0] = count; + $[1] = x; + $[2] = t1; + } else { + t1 = $[2]; + } + const t2 = jsx[0]; + let t3; + if ($[3] !== t1 || $[4] !== t2) { + t3 = ( + <> + {t1} + {t2} + + ); + $[3] = t1; + $[4] = t2; + $[5] = t3; + } else { + t3 = $[5]; + } + return t3; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ extraJsx: 0 }], + sequentialRenders: [{ extraJsx: 0 }, { extraJsx: 1 }], +}; + +``` + +### Eval output +(kind: ok)
{"x":[2,2,2],"count":3}
{"item":1}
+
{"x":[2,2,2],"count":4}
{"item":1}
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/mixedreadonly-mutating-map.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/mixedreadonly-mutating-map.js new file mode 100644 index 0000000000000..858a4ab3dc693 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/mixedreadonly-mutating-map.js @@ -0,0 +1,59 @@ +import { + arrayPush, + identity, + makeArray, + Stringify, + useFragment, +} from 'shared-runtime'; + +/** + * Bug repro showing why it's invalid for function references to be annotated + * with a `Read` effect when that reference might lead to the function being + * invoked. + * + * Note that currently, `Array.map` is annotated to have `Read` effects on its + * operands. This is incorrect as function effects must be replayed when `map` + * is called + * - Read: non-aliasing data dependency + * - Capture: maybe-aliasing data dependency + * - ConditionallyMutate: maybe-aliasing data dependency; maybe-write / invoke + * but only if the value is mutable + * + * Invalid evaluator result: Found differences in evaluator results Non-forget + * (expected): (kind: ok) + *
{"x":[2,2,2],"count":3}
{"item":1}
+ *
{"x":[2,2,2],"count":4}
{"item":1}
+ * Forget: + * (kind: ok) + *
{"x":[2,2,2],"count":3}
{"item":1}
+ *
{"x":[2,2,2,2,2,2],"count":4}
{"item":1}
+ */ + +function Component({extraJsx}) { + const x = makeArray(); + const items = useFragment(); + // This closure has the following effects that must be replayed: + // - MaybeFreeze / Capture of `items` + // - ConditionalMutate of x + const jsx = items.a.map((item, i) => { + arrayPush(x, 2); + return ; + }); + const offset = jsx.length; + for (let i = 0; i < extraJsx; i++) { + jsx.push(); + } + const count = jsx.length; + identity(count); + return ( + <> + + {jsx[0]} + + ); +} +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{extraJsx: 0}], + sequentialRenders: [{extraJsx: 0}, {extraJsx: 1}], +};