From cee83fe2aaef6f0898d00b85aae6c775ce7146f3 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Mon, 26 Aug 2024 16:18:49 -0700 Subject: [PATCH] [wip] PropagateScopeDeps understands optionals [ghstack-poisoned] --- .../src/HIR/PrintHIR.ts | 2 +- .../DeriveMinimalDependencies.ts | 22 +++ .../ReactiveScopes/PrintReactiveFunction.ts | 2 +- .../PropagateScopeDependencies.ts | 176 +++++++++++++----- ...-optional-member-expression-as-memo-dep.js | 3 +- 5 files changed, 160 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 ecf0b5f0c6041..c2db20c5099a1 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts @@ -191,7 +191,7 @@ export function printTerminal(terminal: Terminal): Array | string { case 'branch': { value = `[${terminal.id}] Branch (${printPlace(terminal.test)}) then:bb${ terminal.consequent - } else:bb${terminal.alternate}`; + } else:bb${terminal.alternate} fallthrough:bb${terminal.fallthrough}`; break; } case 'logical': { diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/DeriveMinimalDependencies.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/DeriveMinimalDependencies.ts index dcf67a36e5c6b..0f0d7f192d5a4 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/DeriveMinimalDependencies.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/DeriveMinimalDependencies.ts @@ -9,6 +9,7 @@ import {CompilerError} from '../CompilerError'; import {DependencyPath, Identifier, ReactiveScopeDependency} from '../HIR'; import {printIdentifier} from '../HIR/PrintHIR'; import {assertExhaustive} from '../Utils/utils'; +import {printDependency} from './PrintReactiveFunction'; /* * We need to understand optional member expressions only when determining @@ -173,6 +174,27 @@ export class ReactiveScopeDependencyTree { } return res.flat().join('\n'); } + + debug(): string { + const buf: Array = [`tree() [`]; + for (const [rootId, rootNode] of this.#roots) { + buf.push(`${printIdentifier(rootId)} (${rootNode.accessType}):`); + this.#debugImpl(buf, rootNode, 1); + } + buf.push(']'); + return buf.length > 2 ? buf.join('\n') : buf.join(''); + } + + #debugImpl( + buf: Array, + node: DependencyNode, + depth: number = 0, + ): void { + for (const [property, childNode] of node.properties) { + buf.push(`${' '.repeat(depth)}.${property} (${childNode.accessType}):`); + this.#debugImpl(buf, childNode, depth + 1); + } + } } /* 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 80395a2e0ea41..b5aa44ead095d 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PrintReactiveFunction.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PrintReactiveFunction.ts @@ -113,7 +113,7 @@ export function printDependency(dependency: ReactiveScopeDependency): string { const identifier = printIdentifier(dependency.identifier) + printType(dependency.identifier.type); - return `${identifier}${dependency.path.map(token => `.${token.property}`).join('')}`; + return `${identifier}${dependency.path.map(token => `${token.optional ? '?.' : '.'}${token.property}`).join('')}`; } export function printReactiveInstructions( diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PropagateScopeDependencies.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PropagateScopeDependencies.ts index 8fd324ae2c9aa..7539c0dc87bdb 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PropagateScopeDependencies.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PropagateScopeDependencies.ts @@ -5,6 +5,7 @@ * LICENSE file in the root directory of this source tree. */ +import prettyFormat from 'pretty-format'; import {CompilerError} from '../CompilerError'; import { areEqualPaths, @@ -17,11 +18,13 @@ import { isObjectMethodType, isRefValueType, isUseRefType, + LoadLocal, makeInstructionId, Place, PrunedReactiveScopeBlock, ReactiveFunction, ReactiveInstruction, + ReactiveOptionalCallValue, ReactiveScope, ReactiveScopeBlock, ReactiveScopeDependency, @@ -29,6 +32,7 @@ import { ReactiveValue, ScopeId, } from '../HIR/HIR'; +import {printIdentifier, printPlace} from '../HIR/PrintHIR'; import {eachInstructionValueOperand, eachPatternOperand} from '../HIR/visitors'; import {empty, Stack} from '../Utils/Stack'; import {assertExhaustive, Iterable_some} from '../Utils/utils'; @@ -36,6 +40,7 @@ import { ReactiveScopeDependencyTree, ReactiveScopePropertyDependency, } from './DeriveMinimalDependencies'; +import {printDependency, printReactiveFunction} from './PrintReactiveFunction'; import {ReactiveFunctionVisitor, visitReactiveFunction} from './visitors'; /* @@ -302,6 +307,7 @@ class Context { #properties: Map = new Map(); #temporaries: Map = new Map(); #inConditionalWithinScope: boolean = false; + inOptional: boolean = false; /* * Reactive dependencies used unconditionally in the current conditional. * Composed of dependencies: @@ -465,6 +471,7 @@ class Context { #getProperty( object: Place, property: string, + optional: boolean, ): ReactiveScopePropertyDependency { const resolvedObject = this.resolveTemporary(object); const resolvedDependency = this.#properties.get(resolvedObject.identifier); @@ -485,13 +492,18 @@ class Context { }; } - objectDependency.path.push({property, optional: false}); + objectDependency.path.push({property, optional}); return objectDependency; } - declareProperty(lvalue: Place, object: Place, property: string): void { - const nextDependency = this.#getProperty(object, property); + declareProperty( + lvalue: Place, + object: Place, + property: string, + optional: boolean, + ): void { + const nextDependency = this.#getProperty(object, property, optional); this.#properties.set(lvalue.identifier, nextDependency); } @@ -571,8 +583,8 @@ class Context { this.visitDependency(dependency); } - visitProperty(object: Place, property: string): void { - const nextDependency = this.#getProperty(object, property); + visitProperty(object: Place, property: string, optional: boolean): void { + const nextDependency = this.#getProperty(object, property, optional); this.visitDependency(nextDependency); } @@ -744,51 +756,102 @@ class PropagationVisitor extends ReactiveFunctionVisitor { }); } + visitOptionalExpression( + context: Context, + id: InstructionId, + value: ReactiveOptionalCallValue, + lvalue: Place | null, + ): void { + const wasWithinOptional = context.inOptional; + const isOptional = wasWithinOptional || value.optional; + const inner = value.value; + /* + * OptionalExpression value is a SequenceExpression where the instructions + * represent the code prior to the `?` and the final value represents the + * conditional code that follows. + */ + CompilerError.invariant(inner.kind === 'SequenceExpression', { + reason: 'Expected OptionalExpression value to be a SequenceExpression', + description: `Found a \`${value.kind}\``, + loc: value.loc, + }); + // Instructions are the unconditionally executed portion before the `?` + context.inOptional = isOptional; + for (const instr of inner.instructions) { + this.visitInstruction(instr, context); + } + context.inOptional = wasWithinOptional; + const innerValue = inner.value; + if ( + innerValue.kind !== 'SequenceExpression' || + innerValue.value.kind !== 'LoadLocal' + ) { + CompilerError.invariant(false, { + reason: + 'Expected OptionalExpression inner value to be a SequenceExpression', + description: `Found a \`${innerValue.kind}\``, + loc: innerValue.loc, + }); + } + if ( + isOptional && + innerValue.instructions.length === 1 && + innerValue.instructions[0].value.kind === 'PropertyLoad' && + innerValue.instructions[0].lvalue !== null && + innerValue.instructions[0].lvalue.identifier.id === + innerValue.value.place.identifier.id + ) { + const propertyLoad = innerValue.instructions[0].value; + if (lvalue !== null && !context.isUsedOutsideDeclaringScope(lvalue)) { + context.declareProperty( + lvalue, + propertyLoad.object, + propertyLoad.property, + isOptional, + ); + } else { + context.visitProperty( + propertyLoad.object, + propertyLoad.property, + isOptional, + ); + } + if (isOptional && !wasWithinOptional && lvalue !== null) { + context.visitOperand(lvalue); + } + return; + } + context.enterConditional(() => { + this.visitReactiveValue(context, id, innerValue, lvalue); + }); + } + visitReactiveValue( context: Context, id: InstructionId, value: ReactiveValue, + lvalue: Place | null, ): void { switch (value.kind) { case 'OptionalExpression': { - const inner = value.value; - /* - * OptionalExpression value is a SequenceExpression where the instructions - * represent the code prior to the `?` and the final value represents the - * conditional code that follows. - */ - CompilerError.invariant(inner.kind === 'SequenceExpression', { - reason: - 'Expected OptionalExpression value to be a SequenceExpression', - description: `Found a \`${value.kind}\``, - loc: value.loc, - suggestions: null, - }); - // Instructions are the unconditionally executed portion before the `?` - for (const instr of inner.instructions) { - this.visitInstruction(instr, context); - } - // The final value is the conditional portion following the `?` - context.enterConditional(() => { - this.visitReactiveValue(context, id, inner.value); - }); + this.visitOptionalExpression(context, id, value, lvalue); break; } case 'LogicalExpression': { - this.visitReactiveValue(context, id, value.left); + this.visitReactiveValue(context, id, value.left, null); context.enterConditional(() => { - this.visitReactiveValue(context, id, value.right); + this.visitReactiveValue(context, id, value.right, lvalue); }); break; } case 'ConditionalExpression': { - this.visitReactiveValue(context, id, value.test); + this.visitReactiveValue(context, id, value.test, null); const consequentDeps = context.enterConditional(() => { - this.visitReactiveValue(context, id, value.consequent); + this.visitReactiveValue(context, id, value.consequent, null); }); const alternateDeps = context.enterConditional(() => { - this.visitReactiveValue(context, id, value.alternate); + this.visitReactiveValue(context, id, value.alternate, null); }); context.promoteDepsFromExhaustiveConditionals([ consequentDeps, @@ -797,10 +860,34 @@ class PropagationVisitor extends ReactiveFunctionVisitor { break; } case 'SequenceExpression': { + /** + * Sequence + * = Sequence <-- we're here + * = ... + * LoadLocal + */ + if ( + lvalue !== null && + value.instructions.length === 1 && + value.value.kind === 'LoadLocal' && + value.value.place.identifier.id === lvalue.identifier.id && + value.instructions[0].lvalue !== null && + value.instructions[0].lvalue.identifier.id === + value.value.place.identifier.id + ) { + this.visitInstructionValue( + context, + value.instructions[0].id, + value.instructions[0].value, + lvalue, + ); + break; + } + for (const instr of value.instructions) { this.visitInstruction(instr, context); } - this.visitInstructionValue(context, id, value.value, null); + this.visitInstructionValue(context, id, value.value, lvalue); break; } case 'FunctionExpression': { @@ -851,9 +938,9 @@ class PropagationVisitor extends ReactiveFunctionVisitor { } } else if (value.kind === 'PropertyLoad') { if (lvalue !== null && !context.isUsedOutsideDeclaringScope(lvalue)) { - context.declareProperty(lvalue, value.object, value.property); + context.declareProperty(lvalue, value.object, value.property, false); } else { - context.visitProperty(value.object, value.property); + context.visitProperty(value.object, value.property, false); } } else if (value.kind === 'StoreLocal') { context.visitOperand(value.value); @@ -896,7 +983,7 @@ class PropagationVisitor extends ReactiveFunctionVisitor { }); } } else { - this.visitReactiveValue(context, id, value); + this.visitReactiveValue(context, id, value, lvalue); } } @@ -947,25 +1034,30 @@ class PropagationVisitor extends ReactiveFunctionVisitor { break; } case 'for': { - this.visitReactiveValue(context, terminal.id, terminal.init); - this.visitReactiveValue(context, terminal.id, terminal.test); + this.visitReactiveValue(context, terminal.id, terminal.init, null); + this.visitReactiveValue(context, terminal.id, terminal.test, null); context.enterConditional(() => { this.visitBlock(terminal.loop, context); if (terminal.update !== null) { - this.visitReactiveValue(context, terminal.id, terminal.update); + this.visitReactiveValue( + context, + terminal.id, + terminal.update, + null, + ); } }); break; } case 'for-of': { - this.visitReactiveValue(context, terminal.id, terminal.init); + this.visitReactiveValue(context, terminal.id, terminal.init, null); context.enterConditional(() => { this.visitBlock(terminal.loop, context); }); break; } case 'for-in': { - this.visitReactiveValue(context, terminal.id, terminal.init); + this.visitReactiveValue(context, terminal.id, terminal.init, null); context.enterConditional(() => { this.visitBlock(terminal.loop, context); }); @@ -974,12 +1066,12 @@ class PropagationVisitor extends ReactiveFunctionVisitor { case 'do-while': { this.visitBlock(terminal.loop, context); context.enterConditional(() => { - this.visitReactiveValue(context, terminal.id, terminal.test); + this.visitReactiveValue(context, terminal.id, terminal.test, null); }); break; } case 'while': { - this.visitReactiveValue(context, terminal.id, terminal.test); + this.visitReactiveValue(context, terminal.id, terminal.test, null); context.enterConditional(() => { this.visitBlock(terminal.loop, context); }); diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-optional-member-expression-as-memo-dep.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-optional-member-expression-as-memo-dep.js index fd8cf0214c87b..641704d71cfda 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-optional-member-expression-as-memo-dep.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-optional-member-expression-as-memo-dep.js @@ -1,7 +1,8 @@ // @validatePreserveExistingMemoizationGuarantees function Component(props) { const data = useMemo(() => { - return props.items?.edges?.nodes ?? []; + return props?.items.edges?.nodes.map(); }, [props.items?.edges?.nodes]); + // const data = props?.item.edges?.nodes.map(); return ; }