From 124c683245bde2ff7391fea1cd7eb70e31a50dc9 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Wed, 30 Apr 2025 14:19:18 +0900 Subject: [PATCH 1/4] [compiler] PruneNonEscapingScopes understands terminal operands We weren't treating terminal operands as eligible for memoization in PruneNonEscapingScopes, which meant that they could end up un-memoized. Terminal operands can also be compound ReactiveValues like SequenceExpressions, so part of the fix is to make sure we don't just recurse into compound values but record the full aliasing information we would for top-level instructions. Still WIP, this needs to handle terminals other than for..of. [ghstack-poisoned] --- .../src/HIR/PrintHIR.ts | 2 +- .../ReactiveScopes/PrintReactiveFunction.ts | 6 + .../ReactiveScopes/PruneNonEscapingScopes.ts | 982 +++++++++--------- ...rror.dont-hoist-inline-reference.expect.md | 2 +- ...on-with-shadowed-local-same-name.expect.md | 2 +- ...ollection-when-loop-body-returns.expect.md | 67 ++ ...or-of-collection-when-loop-body-returns.js | 11 + ...-that-produce-memoizeable-values.expect.md | 55 + ...e-loops-that-produce-memoizeable-values.js | 16 + 9 files changed, 676 insertions(+), 467 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-memoize-for-of-collection-when-loop-body-returns.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-memoize-for-of-collection-when-loop-body-returns.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo.memoize-loops-that-produce-memoizeable-values.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo.memoize-loops-that-produce-memoizeable-values.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 c8182c9e72a7c..2485403d3ac7d 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts @@ -839,7 +839,7 @@ export function printPlace(place: Place): string { } export function printIdentifier(id: Identifier): string { - return `${printName(id.name)}\$${id.id}${printScope(id.scope)}`; + return `${printName(id.name)}\$${id.id}${printScope(id.scope)}_d${id.declarationId}`; } function printName(name: IdentifierName | null): string { diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PrintReactiveFunction.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PrintReactiveFunction.ts index 259fc06486888..f44b031bf8db6 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PrintReactiveFunction.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PrintReactiveFunction.ts @@ -255,6 +255,12 @@ function writeReactiveValue(writer: Writer, value: ReactiveValue): void { } } +export function printReactiveTerminal(terminal: ReactiveTerminal): string { + const writer = new Writer(); + writeTerminal(writer, terminal); + return writer.complete(); +} + function writeTerminal(writer: Writer, terminal: ReactiveTerminal): void { switch (terminal.kind) { case 'break': { diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneNonEscapingScopes.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneNonEscapingScopes.ts index 441d529b39616..6c92584b89271 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneNonEscapingScopes.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneNonEscapingScopes.ts @@ -26,7 +26,7 @@ import { } from '../HIR'; import {getFunctionCallSignature} from '../Inference/InferReferenceEffects'; import {assertExhaustive, getOrInsertDefault} from '../Utils/utils'; -import {getPlaceScope} from '../HIR/HIR'; +import {getPlaceScope, ReactiveScope} from '../HIR/HIR'; import { ReactiveFunctionTransform, ReactiveFunctionVisitor, @@ -34,6 +34,13 @@ import { eachReactiveValueOperand, visitReactiveFunction, } from './visitors'; +import { + printReactiveFunction, + printReactiveTerminal, + printReactiveValue, +} from './PrintReactiveFunction'; +import {printPlace} from '../HIR/PrintHIR'; +import prettyFormat from 'pretty-format'; /* * This pass prunes reactive scopes that are not necessary to bound downstream computation. @@ -121,9 +128,17 @@ export function pruneNonEscapingScopes(fn: ReactiveFunction): void { state.declare(param.place.identifier.declarationId); } } - visitReactiveFunction(fn, new CollectDependenciesVisitor(fn.env), state); + visitReactiveFunction(fn, new CollectDependenciesVisitor(fn.env, state), []); - // log(() => prettyFormat(state)); + // console.log('collect dependencies'); + // console.log( + // prettyFormat({ + // definitions: state.definitions, + // escapingValues: state.escapingValues, + // identifiers: state.identifiers, + // scopes: state.scopes, + // }), + // ); /* * Then walk outward from the returned values and find all captured operands. @@ -131,7 +146,16 @@ export function pruneNonEscapingScopes(fn: ReactiveFunction): void { */ const memoized = computeMemoizedIdentifiers(state); - // log(() => prettyFormat(memoized)); + // console.log('compute memoized identifiers'); + // console.log( + // prettyFormat({ + // definitions: state.definitions, + // escapingValues: state.escapingValues, + // identifiers: state.identifiers, + // scopes: state.scopes, + // }), + // ); + // console.log(prettyFormat(memoized)); // log(() => printReactiveFunction(fn)); @@ -268,7 +292,7 @@ class State { const identifierNode = this.identifiers.get(identifier); CompilerError.invariant(identifierNode !== undefined, { reason: 'Expected identifier to be initialized', - description: null, + description: `[${id}] operand=${printPlace(place)} for identifier declaration ${identifier}`, loc: place.loc, suggestions: null, }); @@ -360,435 +384,6 @@ type LValueMemoization = { level: MemoizationLevel; }; -/* - * Given a value, returns a description of how it should be memoized: - * - lvalues: optional extra places that are lvalue-like in the sense of - * aliasing the rvalues - * - rvalues: places that are aliased by the instruction's lvalues. - * - level: the level of memoization to apply to this value - */ -function computeMemoizationInputs( - env: Environment, - value: ReactiveValue, - lvalue: Place | null, - options: MemoizationOptions, -): { - // can optionally return a custom set of lvalues per instruction - lvalues: Array; - rvalues: Array; -} { - switch (value.kind) { - case 'ConditionalExpression': { - return { - // Only need to memoize if the rvalues are memoized - lvalues: - lvalue !== null - ? [{place: lvalue, level: MemoizationLevel.Conditional}] - : [], - rvalues: [ - // Conditionals do not alias their test value. - ...computeMemoizationInputs(env, value.consequent, null, options) - .rvalues, - ...computeMemoizationInputs(env, value.alternate, null, options) - .rvalues, - ], - }; - } - case 'LogicalExpression': { - return { - // Only need to memoize if the rvalues are memoized - lvalues: - lvalue !== null - ? [{place: lvalue, level: MemoizationLevel.Conditional}] - : [], - rvalues: [ - ...computeMemoizationInputs(env, value.left, null, options).rvalues, - ...computeMemoizationInputs(env, value.right, null, options).rvalues, - ], - }; - } - case 'SequenceExpression': { - return { - // Only need to memoize if the rvalues are memoized - lvalues: - lvalue !== null - ? [{place: lvalue, level: MemoizationLevel.Conditional}] - : [], - /* - * Only the final value of the sequence is a true rvalue: - * values from the sequence's instructions are evaluated - * as separate nodes - */ - rvalues: computeMemoizationInputs(env, value.value, null, options) - .rvalues, - }; - } - case 'JsxExpression': { - const operands: Array = []; - if (value.tag.kind === 'Identifier') { - operands.push(value.tag); - } - for (const prop of value.props) { - if (prop.kind === 'JsxAttribute') { - operands.push(prop.place); - } else { - operands.push(prop.argument); - } - } - if (value.children !== null) { - for (const child of value.children) { - operands.push(child); - } - } - const level = options.memoizeJsxElements - ? MemoizationLevel.Memoized - : MemoizationLevel.Unmemoized; - return { - /* - * JSX elements themselves are not memoized unless forced to - * avoid breaking downstream memoization - */ - lvalues: lvalue !== null ? [{place: lvalue, level}] : [], - rvalues: operands, - }; - } - case 'JsxFragment': { - const level = options.memoizeJsxElements - ? MemoizationLevel.Memoized - : MemoizationLevel.Unmemoized; - return { - /* - * JSX elements themselves are not memoized unless forced to - * avoid breaking downstream memoization - */ - lvalues: lvalue !== null ? [{place: lvalue, level}] : [], - rvalues: value.children, - }; - } - case 'NextPropertyOf': - case 'StartMemoize': - case 'FinishMemoize': - case 'Debugger': - case 'ComputedDelete': - case 'PropertyDelete': - case 'LoadGlobal': - case 'MetaProperty': - case 'TemplateLiteral': - case 'Primitive': - case 'JSXText': - case 'BinaryExpression': - case 'UnaryExpression': { - const level = options.forceMemoizePrimitives - ? MemoizationLevel.Memoized - : MemoizationLevel.Never; - return { - // All of these instructions return a primitive value and never need to be memoized - lvalues: lvalue !== null ? [{place: lvalue, level}] : [], - rvalues: [], - }; - } - case 'Await': - case 'TypeCastExpression': { - return { - // Indirection for the inner value, memoized if the value is - lvalues: - lvalue !== null - ? [{place: lvalue, level: MemoizationLevel.Conditional}] - : [], - rvalues: [value.value], - }; - } - case 'IteratorNext': { - return { - // Indirection for the inner value, memoized if the value is - lvalues: - lvalue !== null - ? [{place: lvalue, level: MemoizationLevel.Conditional}] - : [], - rvalues: [value.iterator, value.collection], - }; - } - case 'GetIterator': { - return { - // Indirection for the inner value, memoized if the value is - lvalues: - lvalue !== null - ? [{place: lvalue, level: MemoizationLevel.Conditional}] - : [], - rvalues: [value.collection], - }; - } - case 'LoadLocal': { - return { - // Indirection for the inner value, memoized if the value is - lvalues: - lvalue !== null - ? [{place: lvalue, level: MemoizationLevel.Conditional}] - : [], - rvalues: [value.place], - }; - } - case 'LoadContext': { - return { - // Should never be pruned - lvalues: - lvalue !== null - ? [{place: lvalue, level: MemoizationLevel.Conditional}] - : [], - rvalues: [value.place], - }; - } - case 'DeclareContext': { - const lvalues = [ - {place: value.lvalue.place, level: MemoizationLevel.Memoized}, - ]; - if (lvalue !== null) { - lvalues.push({place: lvalue, level: MemoizationLevel.Unmemoized}); - } - return { - lvalues, - rvalues: [], - }; - } - - case 'DeclareLocal': { - const lvalues = [ - {place: value.lvalue.place, level: MemoizationLevel.Unmemoized}, - ]; - if (lvalue !== null) { - lvalues.push({place: lvalue, level: MemoizationLevel.Unmemoized}); - } - return { - lvalues, - rvalues: [], - }; - } - case 'PrefixUpdate': - case 'PostfixUpdate': { - const lvalues = [ - {place: value.lvalue, level: MemoizationLevel.Conditional}, - ]; - if (lvalue !== null) { - lvalues.push({place: lvalue, level: MemoizationLevel.Conditional}); - } - return { - // Indirection for the inner value, memoized if the value is - lvalues, - rvalues: [value.value], - }; - } - case 'StoreLocal': { - const lvalues = [ - {place: value.lvalue.place, level: MemoizationLevel.Conditional}, - ]; - if (lvalue !== null) { - lvalues.push({place: lvalue, level: MemoizationLevel.Conditional}); - } - return { - // Indirection for the inner value, memoized if the value is - lvalues, - rvalues: [value.value], - }; - } - case 'StoreContext': { - // Should never be pruned - const lvalues = [ - {place: value.lvalue.place, level: MemoizationLevel.Memoized}, - ]; - if (lvalue !== null) { - lvalues.push({place: lvalue, level: MemoizationLevel.Conditional}); - } - - return { - lvalues, - rvalues: [value.value], - }; - } - case 'StoreGlobal': { - const lvalues = []; - if (lvalue !== null) { - lvalues.push({place: lvalue, level: MemoizationLevel.Unmemoized}); - } - - return { - lvalues, - rvalues: [value.value], - }; - } - case 'Destructure': { - // Indirection for the inner value, memoized if the value is - const lvalues = []; - if (lvalue !== null) { - lvalues.push({place: lvalue, level: MemoizationLevel.Conditional}); - } - lvalues.push(...computePatternLValues(value.lvalue.pattern)); - return { - lvalues: lvalues, - rvalues: [value.value], - }; - } - case 'ComputedLoad': - case 'PropertyLoad': { - const level = options.forceMemoizePrimitives - ? MemoizationLevel.Memoized - : MemoizationLevel.Conditional; - return { - // Indirection for the inner value, memoized if the value is - lvalues: lvalue !== null ? [{place: lvalue, level}] : [], - /* - * Only the object is aliased to the result, and the result only needs to be - * memoized if the object is - */ - rvalues: [value.object], - }; - } - case 'ComputedStore': { - /* - * The object being stored to acts as an lvalue (it aliases the value), but - * the computed key is not aliased - */ - const lvalues = [ - {place: value.object, level: MemoizationLevel.Conditional}, - ]; - if (lvalue !== null) { - lvalues.push({place: lvalue, level: MemoizationLevel.Conditional}); - } - return { - lvalues, - rvalues: [value.value], - }; - } - case 'OptionalExpression': { - // Indirection for the inner value, memoized if the value is - const lvalues = []; - if (lvalue !== null) { - lvalues.push({place: lvalue, level: MemoizationLevel.Conditional}); - } - return { - lvalues: lvalues, - rvalues: [ - ...computeMemoizationInputs(env, value.value, null, options).rvalues, - ], - }; - } - case 'TaggedTemplateExpression': { - const signature = getFunctionCallSignature( - env, - value.tag.identifier.type, - ); - let lvalues = []; - if (lvalue !== null) { - lvalues.push({place: lvalue, level: MemoizationLevel.Memoized}); - } - if (signature?.noAlias === true) { - return { - lvalues, - rvalues: [], - }; - } - const operands = [...eachReactiveValueOperand(value)]; - lvalues.push( - ...operands - .filter(operand => isMutableEffect(operand.effect, operand.loc)) - .map(place => ({place, level: MemoizationLevel.Memoized})), - ); - return { - lvalues, - rvalues: operands, - }; - } - case 'CallExpression': { - const signature = getFunctionCallSignature( - env, - value.callee.identifier.type, - ); - let lvalues = []; - if (lvalue !== null) { - lvalues.push({place: lvalue, level: MemoizationLevel.Memoized}); - } - if (signature?.noAlias === true) { - return { - lvalues, - rvalues: [], - }; - } - const operands = [...eachReactiveValueOperand(value)]; - lvalues.push( - ...operands - .filter(operand => isMutableEffect(operand.effect, operand.loc)) - .map(place => ({place, level: MemoizationLevel.Memoized})), - ); - return { - lvalues, - rvalues: operands, - }; - } - case 'MethodCall': { - const signature = getFunctionCallSignature( - env, - value.property.identifier.type, - ); - let lvalues = []; - if (lvalue !== null) { - lvalues.push({place: lvalue, level: MemoizationLevel.Memoized}); - } - if (signature?.noAlias === true) { - return { - lvalues, - rvalues: [], - }; - } - const operands = [...eachReactiveValueOperand(value)]; - lvalues.push( - ...operands - .filter(operand => isMutableEffect(operand.effect, operand.loc)) - .map(place => ({place, level: MemoizationLevel.Memoized})), - ); - return { - lvalues, - rvalues: operands, - }; - } - case 'RegExpLiteral': - case 'ObjectMethod': - case 'FunctionExpression': - case 'ArrayExpression': - case 'NewExpression': - case 'ObjectExpression': - case 'PropertyStore': { - /* - * All of these instructions may produce new values which must be memoized if - * reachable from a return value. Any mutable rvalue may alias any other rvalue - */ - const operands = [...eachReactiveValueOperand(value)]; - const lvalues = operands - .filter(operand => isMutableEffect(operand.effect, operand.loc)) - .map(place => ({place, level: MemoizationLevel.Memoized})); - if (lvalue !== null) { - lvalues.push({place: lvalue, level: MemoizationLevel.Memoized}); - } - return { - lvalues, - rvalues: operands, - }; - } - case 'UnsupportedNode': { - CompilerError.invariant(false, { - reason: `Unexpected unsupported node`, - description: null, - loc: value.loc, - suggestions: null, - }); - } - default: { - assertExhaustive( - value, - `Unexpected value kind \`${(value as any).kind}\``, - ); - } - } -} - function computePatternLValues(pattern: Pattern): Array { const lvalues: Array = []; switch (pattern.kind) { @@ -832,39 +427,468 @@ function computePatternLValues(pattern: Pattern): Array { * Populates the input state with the set of returned identifiers and information about each * identifier's and scope's dependencies. */ -class CollectDependenciesVisitor extends ReactiveFunctionVisitor { +class CollectDependenciesVisitor extends ReactiveFunctionVisitor< + Array +> { env: Environment; + state: State; options: MemoizationOptions; - constructor(env: Environment) { + constructor(env: Environment, state: State) { super(); this.env = env; + this.state = state; this.options = { memoizeJsxElements: !this.env.config.enableForest, forceMemoizePrimitives: this.env.config.enableForest, }; } - override visitInstruction( - instruction: ReactiveInstruction, - state: State, - ): void { - this.traverseInstruction(instruction, state); + /* + * Given a value, returns a description of how it should be memoized: + * - lvalues: optional extra places that are lvalue-like in the sense of + * aliasing the rvalues + * - rvalues: places that are aliased by the instruction's lvalues. + * - level: the level of memoization to apply to this value + */ + computeMemoizationInputs( + value: ReactiveValue, + lvalue: Place | null, + ): { + // can optionally return a custom set of lvalues per instruction + lvalues: Array; + rvalues: Array; + } { + const env = this.env; + const options = this.options; + switch (value.kind) { + case 'ConditionalExpression': { + return { + // Only need to memoize if the rvalues are memoized + lvalues: + lvalue !== null + ? [{place: lvalue, level: MemoizationLevel.Conditional}] + : [], + rvalues: [ + // Conditionals do not alias their test value. + ...this.computeMemoizationInputs(value.consequent, null).rvalues, + ...this.computeMemoizationInputs(value.alternate, null).rvalues, + ], + }; + } + case 'LogicalExpression': { + return { + // Only need to memoize if the rvalues are memoized + lvalues: + lvalue !== null + ? [{place: lvalue, level: MemoizationLevel.Conditional}] + : [], + rvalues: [ + ...this.computeMemoizationInputs(value.left, null).rvalues, + ...this.computeMemoizationInputs(value.right, null).rvalues, + ], + }; + } + case 'SequenceExpression': { + for (const instr of value.instructions) { + this.visitValueForMemoization(instr.id, instr.value, instr.lvalue); + } + return { + // Only need to memoize if the rvalues are memoized + lvalues: + lvalue !== null + ? [{place: lvalue, level: MemoizationLevel.Conditional}] + : [], + /* + * Only the final value of the sequence is a true rvalue: + * values from the sequence's instructions are evaluated + * as separate nodes + */ + rvalues: this.computeMemoizationInputs(value.value, null).rvalues, + }; + } + case 'JsxExpression': { + const operands: Array = []; + if (value.tag.kind === 'Identifier') { + operands.push(value.tag); + } + for (const prop of value.props) { + if (prop.kind === 'JsxAttribute') { + operands.push(prop.place); + } else { + operands.push(prop.argument); + } + } + if (value.children !== null) { + for (const child of value.children) { + operands.push(child); + } + } + const level = options.memoizeJsxElements + ? MemoizationLevel.Memoized + : MemoizationLevel.Unmemoized; + return { + /* + * JSX elements themselves are not memoized unless forced to + * avoid breaking downstream memoization + */ + lvalues: lvalue !== null ? [{place: lvalue, level}] : [], + rvalues: operands, + }; + } + case 'JsxFragment': { + const level = options.memoizeJsxElements + ? MemoizationLevel.Memoized + : MemoizationLevel.Unmemoized; + return { + /* + * JSX elements themselves are not memoized unless forced to + * avoid breaking downstream memoization + */ + lvalues: lvalue !== null ? [{place: lvalue, level}] : [], + rvalues: value.children, + }; + } + case 'NextPropertyOf': + case 'StartMemoize': + case 'FinishMemoize': + case 'Debugger': + case 'ComputedDelete': + case 'PropertyDelete': + case 'LoadGlobal': + case 'MetaProperty': + case 'TemplateLiteral': + case 'Primitive': + case 'JSXText': + case 'BinaryExpression': + case 'UnaryExpression': { + const level = options.forceMemoizePrimitives + ? MemoizationLevel.Memoized + : MemoizationLevel.Never; + return { + // All of these instructions return a primitive value and never need to be memoized + lvalues: lvalue !== null ? [{place: lvalue, level}] : [], + rvalues: [], + }; + } + case 'Await': + case 'TypeCastExpression': { + return { + // Indirection for the inner value, memoized if the value is + lvalues: + lvalue !== null + ? [{place: lvalue, level: MemoizationLevel.Conditional}] + : [], + rvalues: [value.value], + }; + } + case 'IteratorNext': { + return { + // Indirection for the inner value, memoized if the value is + lvalues: + lvalue !== null + ? [{place: lvalue, level: MemoizationLevel.Conditional}] + : [], + rvalues: [value.iterator, value.collection], + }; + } + case 'GetIterator': { + return { + // Indirection for the inner value, memoized if the value is + lvalues: + lvalue !== null + ? [{place: lvalue, level: MemoizationLevel.Conditional}] + : [], + rvalues: [value.collection], + }; + } + case 'LoadLocal': { + return { + // Indirection for the inner value, memoized if the value is + lvalues: + lvalue !== null + ? [{place: lvalue, level: MemoizationLevel.Conditional}] + : [], + rvalues: [value.place], + }; + } + case 'LoadContext': { + return { + // Should never be pruned + lvalues: + lvalue !== null + ? [{place: lvalue, level: MemoizationLevel.Conditional}] + : [], + rvalues: [value.place], + }; + } + case 'DeclareContext': { + const lvalues = [ + {place: value.lvalue.place, level: MemoizationLevel.Memoized}, + ]; + if (lvalue !== null) { + lvalues.push({place: lvalue, level: MemoizationLevel.Unmemoized}); + } + return { + lvalues, + rvalues: [], + }; + } + + case 'DeclareLocal': { + const lvalues = [ + {place: value.lvalue.place, level: MemoizationLevel.Unmemoized}, + ]; + if (lvalue !== null) { + lvalues.push({place: lvalue, level: MemoizationLevel.Unmemoized}); + } + return { + lvalues, + rvalues: [], + }; + } + case 'PrefixUpdate': + case 'PostfixUpdate': { + const lvalues = [ + {place: value.lvalue, level: MemoizationLevel.Conditional}, + ]; + if (lvalue !== null) { + lvalues.push({place: lvalue, level: MemoizationLevel.Conditional}); + } + return { + // Indirection for the inner value, memoized if the value is + lvalues, + rvalues: [value.value], + }; + } + case 'StoreLocal': { + const lvalues = [ + {place: value.lvalue.place, level: MemoizationLevel.Conditional}, + ]; + if (lvalue !== null) { + lvalues.push({place: lvalue, level: MemoizationLevel.Conditional}); + } + return { + // Indirection for the inner value, memoized if the value is + lvalues, + rvalues: [value.value], + }; + } + case 'StoreContext': { + // Should never be pruned + const lvalues = [ + {place: value.lvalue.place, level: MemoizationLevel.Memoized}, + ]; + if (lvalue !== null) { + lvalues.push({place: lvalue, level: MemoizationLevel.Conditional}); + } + + return { + lvalues, + rvalues: [value.value], + }; + } + case 'StoreGlobal': { + const lvalues = []; + if (lvalue !== null) { + lvalues.push({place: lvalue, level: MemoizationLevel.Unmemoized}); + } + + return { + lvalues, + rvalues: [value.value], + }; + } + case 'Destructure': { + // Indirection for the inner value, memoized if the value is + const lvalues = []; + if (lvalue !== null) { + lvalues.push({place: lvalue, level: MemoizationLevel.Conditional}); + } + lvalues.push(...computePatternLValues(value.lvalue.pattern)); + return { + lvalues: lvalues, + rvalues: [value.value], + }; + } + case 'ComputedLoad': + case 'PropertyLoad': { + const level = options.forceMemoizePrimitives + ? MemoizationLevel.Memoized + : MemoizationLevel.Conditional; + return { + // Indirection for the inner value, memoized if the value is + lvalues: lvalue !== null ? [{place: lvalue, level}] : [], + /* + * Only the object is aliased to the result, and the result only needs to be + * memoized if the object is + */ + rvalues: [value.object], + }; + } + case 'ComputedStore': { + /* + * The object being stored to acts as an lvalue (it aliases the value), but + * the computed key is not aliased + */ + const lvalues = [ + {place: value.object, level: MemoizationLevel.Conditional}, + ]; + if (lvalue !== null) { + lvalues.push({place: lvalue, level: MemoizationLevel.Conditional}); + } + return { + lvalues, + rvalues: [value.value], + }; + } + case 'OptionalExpression': { + // Indirection for the inner value, memoized if the value is + const lvalues = []; + if (lvalue !== null) { + lvalues.push({place: lvalue, level: MemoizationLevel.Conditional}); + } + return { + lvalues: lvalues, + rvalues: [ + ...this.computeMemoizationInputs(value.value, null).rvalues, + ], + }; + } + case 'TaggedTemplateExpression': { + const signature = getFunctionCallSignature( + env, + value.tag.identifier.type, + ); + let lvalues = []; + if (lvalue !== null) { + lvalues.push({place: lvalue, level: MemoizationLevel.Memoized}); + } + if (signature?.noAlias === true) { + return { + lvalues, + rvalues: [], + }; + } + const operands = [...eachReactiveValueOperand(value)]; + lvalues.push( + ...operands + .filter(operand => isMutableEffect(operand.effect, operand.loc)) + .map(place => ({place, level: MemoizationLevel.Memoized})), + ); + return { + lvalues, + rvalues: operands, + }; + } + case 'CallExpression': { + const signature = getFunctionCallSignature( + env, + value.callee.identifier.type, + ); + let lvalues = []; + if (lvalue !== null) { + lvalues.push({place: lvalue, level: MemoizationLevel.Memoized}); + } + if (signature?.noAlias === true) { + return { + lvalues, + rvalues: [], + }; + } + const operands = [...eachReactiveValueOperand(value)]; + lvalues.push( + ...operands + .filter(operand => isMutableEffect(operand.effect, operand.loc)) + .map(place => ({place, level: MemoizationLevel.Memoized})), + ); + return { + lvalues, + rvalues: operands, + }; + } + case 'MethodCall': { + const signature = getFunctionCallSignature( + env, + value.property.identifier.type, + ); + let lvalues = []; + if (lvalue !== null) { + lvalues.push({place: lvalue, level: MemoizationLevel.Memoized}); + } + if (signature?.noAlias === true) { + return { + lvalues, + rvalues: [], + }; + } + const operands = [...eachReactiveValueOperand(value)]; + lvalues.push( + ...operands + .filter(operand => isMutableEffect(operand.effect, operand.loc)) + .map(place => ({place, level: MemoizationLevel.Memoized})), + ); + return { + lvalues, + rvalues: operands, + }; + } + case 'RegExpLiteral': + case 'ObjectMethod': + case 'FunctionExpression': + case 'ArrayExpression': + case 'NewExpression': + case 'ObjectExpression': + case 'PropertyStore': { + /* + * All of these instructions may produce new values which must be memoized if + * reachable from a return value. Any mutable rvalue may alias any other rvalue + */ + const operands = [...eachReactiveValueOperand(value)]; + const lvalues = operands + .filter(operand => isMutableEffect(operand.effect, operand.loc)) + .map(place => ({place, level: MemoizationLevel.Memoized})); + if (lvalue !== null) { + lvalues.push({place: lvalue, level: MemoizationLevel.Memoized}); + } + return { + lvalues, + rvalues: operands, + }; + } + case 'UnsupportedNode': { + CompilerError.invariant(false, { + reason: `Unexpected unsupported node`, + description: null, + loc: value.loc, + suggestions: null, + }); + } + default: { + assertExhaustive( + value, + `Unexpected value kind \`${(value as any).kind}\``, + ); + } + } + } + + visitValueForMemoization( + id: InstructionId, + value: ReactiveValue, + lvalue: Place | null, + ): void { + const state = this.state; // Determe the level of memoization for this value and the lvalues/rvalues - const aliasing = computeMemoizationInputs( - this.env, - instruction.value, - instruction.lvalue, - this.options, - ); + const aliasing = this.computeMemoizationInputs(value, lvalue); // Associate all the rvalues with the instruction's scope if it has one for (const operand of aliasing.rvalues) { const operandId = state.definitions.get(operand.identifier.declarationId) ?? operand.identifier.declarationId; - state.visitOperand(instruction.id, operand, operandId); + state.visitOperand(id, operand, operandId); } // Add the operands as dependencies of all lvalues. @@ -898,22 +922,17 @@ class CollectDependenciesVisitor extends ReactiveFunctionVisitor { node.dependencies.add(operandId); } - state.visitOperand(instruction.id, lvalue, lvalueId); + state.visitOperand(id, lvalue, lvalueId); } - if (instruction.value.kind === 'LoadLocal' && instruction.lvalue !== null) { + if (value.kind === 'LoadLocal' && lvalue !== null) { state.definitions.set( - instruction.lvalue.identifier.declarationId, - instruction.value.place.identifier.declarationId, + lvalue.identifier.declarationId, + value.place.identifier.declarationId, ); - } else if ( - instruction.value.kind === 'CallExpression' || - instruction.value.kind === 'MethodCall' - ) { + } else if (value.kind === 'CallExpression' || value.kind === 'MethodCall') { let callee = - instruction.value.kind === 'CallExpression' - ? instruction.value.callee - : instruction.value.property; + value.kind === 'CallExpression' ? value.callee : value.property; if (getHookKind(state.env, callee.identifier) != null) { const signature = getFunctionCallSignature( this.env, @@ -928,7 +947,7 @@ class CollectDependenciesVisitor extends ReactiveFunctionVisitor { if (signature && signature.noAlias === true) { return; } - for (const operand of instruction.value.args) { + for (const operand of value.args) { const place = operand.kind === 'Spread' ? operand.place : operand; state.escapingValues.add(place.identifier.declarationId); } @@ -936,16 +955,51 @@ class CollectDependenciesVisitor extends ReactiveFunctionVisitor { } } + override visitInstruction( + instruction: ReactiveInstruction, + scopes: Array, + ): void { + this.traverseInstruction(instruction, scopes); + this.visitValueForMemoization( + instruction.id, + instruction.value, + instruction.lvalue, + ); + } + override visitTerminal( stmt: ReactiveTerminalStatement, - state: State, + scopes: Array, ): void { - this.traverseTerminal(stmt, state); - + this.traverseTerminal(stmt, scopes); if (stmt.terminal.kind === 'return') { - state.escapingValues.add(stmt.terminal.value.identifier.declarationId); + this.state.escapingValues.add( + stmt.terminal.value.identifier.declarationId, + ); + const identifierNode = this.state.identifiers.get( + stmt.terminal.value.identifier.declarationId, + ); + CompilerError.invariant(identifierNode !== undefined, { + reason: 'Expected identifier to be initialized', + description: null, + loc: stmt.terminal.loc, + suggestions: null, + }); + for (const scope of scopes) { + identifierNode.scopes.add(scope.id); + } + } else if (stmt.terminal.kind === 'for-of') { + this.visitValueForMemoization(stmt.terminal.id, stmt.terminal.init, null); + this.visitValueForMemoization(stmt.terminal.id, stmt.terminal.test, null); } } + + override visitScope( + scope: ReactiveScopeBlock, + scopes: Array, + ): void { + this.traverseScope(scope, [...scopes, scope.scope]); + } } // Prune reactive scopes that do not have any memoized outputs diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.dont-hoist-inline-reference.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.dont-hoist-inline-reference.expect.md index b08d151be64c4..23c833c3d94ce 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.dont-hoist-inline-reference.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.dont-hoist-inline-reference.expect.md @@ -22,7 +22,7 @@ export const FIXTURE_ENTRYPOINT = { 1 | import {identity} from 'shared-runtime'; 2 | function useInvalid() { > 3 | const x = identity(x); - | ^^^^^^^^^^^^^^^^^^^^^^ Todo: [hoisting] EnterSSA: Expected identifier to be defined before being used. Identifier x$1 is undefined (3:3) + | ^^^^^^^^^^^^^^^^^^^^^^ Todo: [hoisting] EnterSSA: Expected identifier to be defined before being used. Identifier x$1_d1 is undefined (3:3) 4 | return x; 5 | } 6 | diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-repro-named-function-with-shadowed-local-same-name.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-repro-named-function-with-shadowed-local-same-name.expect.md index f66b970f00dd4..6188459e01c6b 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-repro-named-function-with-shadowed-local-same-name.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-repro-named-function-with-shadowed-local-same-name.expect.md @@ -22,7 +22,7 @@ function Component(props) { 7 | return hasErrors; 8 | } > 9 | return hasErrors(); - | ^^^^^^^^^ Invariant: [hoisting] Expected value for identifier to be initialized. hasErrors_0$14 (9:9) + | ^^^^^^^^^ Invariant: [hoisting] Expected value for identifier to be initialized. hasErrors_0$14_d14 (9:9) 10 | } 11 | ``` diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-memoize-for-of-collection-when-loop-body-returns.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-memoize-for-of-collection-when-loop-body-returns.expect.md new file mode 100644 index 0000000000000..bc124ff96af5e --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-memoize-for-of-collection-when-loop-body-returns.expect.md @@ -0,0 +1,67 @@ + +## Input + +```javascript +function useHook(nodeID, condition) { + const graph = useContext(GraphContext); + const node = nodeID != null ? graph[nodeID] : null; + + for (const key of Object.keys(node?.fields ?? {})) { + if (condition) { + return new Class(node.fields?.[field]); + } + } + return new Class(); +} + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +function useHook(nodeID, condition) { + const $ = _c(7); + const graph = useContext(GraphContext); + const node = nodeID != null ? graph[nodeID] : null; + let t0; + if ($[0] !== node?.fields) { + t0 = Object.keys(node?.fields ?? {}); + $[0] = node?.fields; + $[1] = t0; + } else { + t0 = $[1]; + } + let t1; + if ($[2] !== condition || $[3] !== node || $[4] !== t0) { + t1 = Symbol.for("react.early_return_sentinel"); + bb0: for (const key of t0) { + if (condition) { + t1 = new Class(node.fields?.[field]); + break bb0; + } + } + $[2] = condition; + $[3] = node; + $[4] = t0; + $[5] = t1; + } else { + t1 = $[5]; + } + if (t1 !== Symbol.for("react.early_return_sentinel")) { + return t1; + } + let t2; + if ($[6] === Symbol.for("react.memo_cache_sentinel")) { + t2 = new Class(); + $[6] = t2; + } else { + t2 = $[6]; + } + return t2; +} + +``` + +### Eval output +(kind: exception) Fixture not implemented \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-memoize-for-of-collection-when-loop-body-returns.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-memoize-for-of-collection-when-loop-body-returns.js new file mode 100644 index 0000000000000..a010f3f28ccbe --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-memoize-for-of-collection-when-loop-body-returns.js @@ -0,0 +1,11 @@ +function useHook(nodeID, condition) { + const graph = useContext(GraphContext); + const node = nodeID != null ? graph[nodeID] : null; + + for (const key of Object.keys(node?.fields ?? {})) { + if (condition) { + return new Class(node.fields?.[field]); + } + } + return new Class(); +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo.memoize-loops-that-produce-memoizeable-values.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo.memoize-loops-that-produce-memoizeable-values.expect.md new file mode 100644 index 0000000000000..ea52c0ef31cb0 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo.memoize-loops-that-produce-memoizeable-values.expect.md @@ -0,0 +1,55 @@ + +## Input + +```javascript +function useHook(nodeID, condition) { + const graph = useContext(GraphContext); + const node = nodeID != null ? graph[nodeID] : null; + + // (2) Instead we can create a scope around the loop since the loop produces an escapinng value + let value; + for (const key of Object.keys(node?.fields ?? {})) { + if (condition) { + // (1) We currently create a scope just for this instruction, then later prune the scope because + // it's inside a loop + value = new Class(node.fields?.[field]); + break; + } + } + return value; +} + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +function useHook(nodeID, condition) { + const $ = _c(4); + const graph = useContext(GraphContext); + const node = nodeID != null ? graph[nodeID] : null; + + let value; + const t0 = Object.keys(node?.fields ?? {}); + if ($[0] !== condition || $[1] !== node || $[2] !== t0) { + for (const key of t0) { + if (condition) { + value = new Class(node.fields?.[field]); + break; + } + } + $[0] = condition; + $[1] = node; + $[2] = t0; + $[3] = value; + } else { + value = $[3]; + } + return value; +} + +``` + +### Eval output +(kind: exception) Fixture not implemented \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo.memoize-loops-that-produce-memoizeable-values.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo.memoize-loops-that-produce-memoizeable-values.js new file mode 100644 index 0000000000000..9ec8f6253b062 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo.memoize-loops-that-produce-memoizeable-values.js @@ -0,0 +1,16 @@ +function useHook(nodeID, condition) { + const graph = useContext(GraphContext); + const node = nodeID != null ? graph[nodeID] : null; + + // (2) Instead we can create a scope around the loop since the loop produces an escapinng value + let value; + for (const key of Object.keys(node?.fields ?? {})) { + if (condition) { + // (1) We currently create a scope just for this instruction, then later prune the scope because + // it's inside a loop + value = new Class(node.fields?.[field]); + break; + } + } + return value; +} From 6bf63378de91a4a66e59e8dfd1d47c0a923905bf Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Wed, 30 Apr 2025 15:46:24 +0900 Subject: [PATCH 2/4] Update on "[compiler] PruneNonEscapingScopes understands terminal operands" We weren't treating terminal operands as eligible for memoization in PruneNonEscapingScopes, which meant that they could end up un-memoized. Terminal operands can also be compound ReactiveValues like SequenceExpressions, so part of the fix is to make sure we don't just recurse into compound values but record the full aliasing information we would for top-level instructions. Still WIP, this needs to handle terminals other than for..of. [ghstack-poisoned] --- .../src/HIR/PrintHIR.ts | 2 +- .../ReactiveScopes/PruneNonEscapingScopes.ts | 50 ++++++------------- ...rror.dont-hoist-inline-reference.expect.md | 2 +- ...on-with-shadowed-local-same-name.expect.md | 2 +- ...-that-produce-memoizeable-values.expect.md | 23 ++++++--- 5 files changed, 34 insertions(+), 45 deletions(-) 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 2485403d3ac7d..c8182c9e72a7c 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts @@ -839,7 +839,7 @@ export function printPlace(place: Place): string { } export function printIdentifier(id: Identifier): string { - return `${printName(id.name)}\$${id.id}${printScope(id.scope)}_d${id.declarationId}`; + return `${printName(id.name)}\$${id.id}${printScope(id.scope)}`; } function printName(name: IdentifierName | null): string { diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneNonEscapingScopes.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneNonEscapingScopes.ts index 6c92584b89271..a7107b8ee786c 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneNonEscapingScopes.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneNonEscapingScopes.ts @@ -34,13 +34,7 @@ import { eachReactiveValueOperand, visitReactiveFunction, } from './visitors'; -import { - printReactiveFunction, - printReactiveTerminal, - printReactiveValue, -} from './PrintReactiveFunction'; import {printPlace} from '../HIR/PrintHIR'; -import prettyFormat from 'pretty-format'; /* * This pass prunes reactive scopes that are not necessary to bound downstream computation. @@ -130,35 +124,12 @@ export function pruneNonEscapingScopes(fn: ReactiveFunction): void { } visitReactiveFunction(fn, new CollectDependenciesVisitor(fn.env, state), []); - // console.log('collect dependencies'); - // console.log( - // prettyFormat({ - // definitions: state.definitions, - // escapingValues: state.escapingValues, - // identifiers: state.identifiers, - // scopes: state.scopes, - // }), - // ); - /* * Then walk outward from the returned values and find all captured operands. * This forms the set of identifiers which should be memoized. */ const memoized = computeMemoizedIdentifiers(state); - // console.log('compute memoized identifiers'); - // console.log( - // prettyFormat({ - // definitions: state.definitions, - // escapingValues: state.escapingValues, - // identifiers: state.identifiers, - // scopes: state.scopes, - // }), - // ); - // console.log(prettyFormat(memoized)); - - // log(() => printReactiveFunction(fn)); - // Prune scopes that do not declare/reassign any escaping values visitReactiveFunction(fn, new PruneScopesTransform(), memoized); } @@ -957,9 +928,8 @@ class CollectDependenciesVisitor extends ReactiveFunctionVisitor< override visitInstruction( instruction: ReactiveInstruction, - scopes: Array, + _scopes: Array, ): void { - this.traverseInstruction(instruction, scopes); this.visitValueForMemoization( instruction.id, instruction.value, @@ -988,9 +958,6 @@ class CollectDependenciesVisitor extends ReactiveFunctionVisitor< for (const scope of scopes) { identifierNode.scopes.add(scope.id); } - } else if (stmt.terminal.kind === 'for-of') { - this.visitValueForMemoization(stmt.terminal.id, stmt.terminal.init, null); - this.visitValueForMemoization(stmt.terminal.id, stmt.terminal.test, null); } } @@ -998,6 +965,21 @@ class CollectDependenciesVisitor extends ReactiveFunctionVisitor< scope: ReactiveScopeBlock, scopes: Array, ): void { + for (const reassignment of scope.scope.reassignments) { + const identifierNode = this.state.identifiers.get( + reassignment.declarationId, + ); + CompilerError.invariant(identifierNode !== undefined, { + reason: 'Expected identifier to be initialized', + description: null, + loc: reassignment.loc, + suggestions: null, + }); + for (const scope of scopes) { + identifierNode.scopes.add(scope.id); + } + identifierNode.scopes.add(scope.scope.id); + } this.traverseScope(scope, [...scopes, scope.scope]); } } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.dont-hoist-inline-reference.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.dont-hoist-inline-reference.expect.md index 23c833c3d94ce..b08d151be64c4 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.dont-hoist-inline-reference.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.dont-hoist-inline-reference.expect.md @@ -22,7 +22,7 @@ export const FIXTURE_ENTRYPOINT = { 1 | import {identity} from 'shared-runtime'; 2 | function useInvalid() { > 3 | const x = identity(x); - | ^^^^^^^^^^^^^^^^^^^^^^ Todo: [hoisting] EnterSSA: Expected identifier to be defined before being used. Identifier x$1_d1 is undefined (3:3) + | ^^^^^^^^^^^^^^^^^^^^^^ Todo: [hoisting] EnterSSA: Expected identifier to be defined before being used. Identifier x$1 is undefined (3:3) 4 | return x; 5 | } 6 | diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-repro-named-function-with-shadowed-local-same-name.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-repro-named-function-with-shadowed-local-same-name.expect.md index 6188459e01c6b..f66b970f00dd4 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-repro-named-function-with-shadowed-local-same-name.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-repro-named-function-with-shadowed-local-same-name.expect.md @@ -22,7 +22,7 @@ function Component(props) { 7 | return hasErrors; 8 | } > 9 | return hasErrors(); - | ^^^^^^^^^ Invariant: [hoisting] Expected value for identifier to be initialized. hasErrors_0$14_d14 (9:9) + | ^^^^^^^^^ Invariant: [hoisting] Expected value for identifier to be initialized. hasErrors_0$14 (9:9) 10 | } 11 | ``` diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo.memoize-loops-that-produce-memoizeable-values.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo.memoize-loops-that-produce-memoizeable-values.expect.md index ea52c0ef31cb0..36e4a10ab2449 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo.memoize-loops-that-produce-memoizeable-values.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo.memoize-loops-that-produce-memoizeable-values.expect.md @@ -26,25 +26,32 @@ function useHook(nodeID, condition) { ```javascript import { c as _c } from "react/compiler-runtime"; function useHook(nodeID, condition) { - const $ = _c(4); + const $ = _c(6); const graph = useContext(GraphContext); const node = nodeID != null ? graph[nodeID] : null; let value; - const t0 = Object.keys(node?.fields ?? {}); - if ($[0] !== condition || $[1] !== node || $[2] !== t0) { + let t0; + if ($[0] !== node?.fields) { + t0 = Object.keys(node?.fields ?? {}); + $[0] = node?.fields; + $[1] = t0; + } else { + t0 = $[1]; + } + if ($[2] !== condition || $[3] !== node || $[4] !== t0) { for (const key of t0) { if (condition) { value = new Class(node.fields?.[field]); break; } } - $[0] = condition; - $[1] = node; - $[2] = t0; - $[3] = value; + $[2] = condition; + $[3] = node; + $[4] = t0; + $[5] = value; } else { - value = $[3]; + value = $[5]; } return value; } From 4c44a286d02d7d4ee861add1c927a8e88571d86f Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Wed, 30 Apr 2025 15:56:07 +0900 Subject: [PATCH 3/4] Update on "[compiler] PruneNonEscapingScopes understands terminal operands" We weren't treating terminal operands as eligible for memoization in PruneNonEscapingScopes, which meant that they could end up un-memoized. Terminal operands can also be compound ReactiveValues like SequenceExpressions, so part of the fix is to make sure we don't just recurse into compound values but record the full aliasing information we would for top-level instructions. Still WIP, this needs to handle terminals other than for..of. [ghstack-poisoned] --- .../src/ReactiveScopes/PruneNonEscapingScopes.ts | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneNonEscapingScopes.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneNonEscapingScopes.ts index a7107b8ee786c..5ae4c7dfc72f9 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneNonEscapingScopes.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneNonEscapingScopes.ts @@ -946,6 +946,15 @@ class CollectDependenciesVisitor extends ReactiveFunctionVisitor< this.state.escapingValues.add( stmt.terminal.value.identifier.declarationId, ); + + /* + * If the return is within a scope, then those scopes must be evaluated + * with the return and should be considered dependencies of the returned + * value. + * + * This ensures that if those scopes have dependencies that those deps + * are also memoized. + */ const identifierNode = this.state.identifiers.get( stmt.terminal.value.identifier.declarationId, ); @@ -965,6 +974,11 @@ class CollectDependenciesVisitor extends ReactiveFunctionVisitor< scope: ReactiveScopeBlock, scopes: Array, ): void { + /* + * If a scope reassigns any variables, set the chain of active scopes as a dependency + * of those variables. This ensures that if the variable escapes that we treat the + * reassignment scopes — and importantly their dependencies — as needing memoization. + */ for (const reassignment of scope.scope.reassignments) { const identifierNode = this.state.identifiers.get( reassignment.declarationId, @@ -980,6 +994,7 @@ class CollectDependenciesVisitor extends ReactiveFunctionVisitor< } identifierNode.scopes.add(scope.scope.id); } + this.traverseScope(scope, [...scopes, scope.scope]); } } From 2790dd44c297fc29131f294e01267820cb4a149f Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Thu, 1 May 2025 12:40:25 +0900 Subject: [PATCH 4/4] Update on "[compiler] PruneNonEscapingScopes understands terminal operands" Handles a few edge cases relating to for..of terminals, which have a compound `init` value block: * For..of with a return statement inside would not memoize the rvalue of the init expression (the collection). This is because there was no direct dependency relationship between the value being returned and the outer scope. The fix is to track the set of active scopes and make return values depend on all the active scopes at the point of the return. * For..of that reassigns a variable which later escapes were also not memoizing the collection. This is similar: the escape analysis aliasing didn't consider scopes that reassign, so there was no dependency between the returned variable and the scope that did the reassigning. The fix is to establish that dependency relationship. In both cases, adding the additional scopes as a dependency of the right variable ensures that the scope's dependencies are treated as escaping (needing memoization), ensuring the collection is memoized. [ghstack-poisoned] --- ...todo.memoize-loops-that-produce-memoizeable-values.expect.md | 2 +- .../todo.memoize-loops-that-produce-memoizeable-values.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo.memoize-loops-that-produce-memoizeable-values.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo.memoize-loops-that-produce-memoizeable-values.expect.md index 36e4a10ab2449..c7c8dfff90a20 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo.memoize-loops-that-produce-memoizeable-values.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo.memoize-loops-that-produce-memoizeable-values.expect.md @@ -6,7 +6,7 @@ function useHook(nodeID, condition) { const graph = useContext(GraphContext); const node = nodeID != null ? graph[nodeID] : null; - // (2) Instead we can create a scope around the loop since the loop produces an escapinng value + // (2) Instead we can create a scope around the loop since the loop produces an escaping value let value; for (const key of Object.keys(node?.fields ?? {})) { if (condition) { diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo.memoize-loops-that-produce-memoizeable-values.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo.memoize-loops-that-produce-memoizeable-values.js index 9ec8f6253b062..0a70976a08404 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo.memoize-loops-that-produce-memoizeable-values.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/todo.memoize-loops-that-produce-memoizeable-values.js @@ -2,7 +2,7 @@ function useHook(nodeID, condition) { const graph = useContext(GraphContext); const node = nodeID != null ? graph[nodeID] : null; - // (2) Instead we can create a scope around the loop since the loop produces an escapinng value + // (2) Instead we can create a scope around the loop since the loop produces an escaping value let value; for (const key of Object.keys(node?.fields ?? {})) { if (condition) {