diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/CollectHoistablePropertyLoads.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/CollectHoistablePropertyLoads.ts index 4f79b2d163ead..d01fa3e2280d1 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/CollectHoistablePropertyLoads.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/CollectHoistablePropertyLoads.ts @@ -21,9 +21,9 @@ import { } from './HIR'; /** - * Helper function for `PropagateScopeDependencies`. - * Uses control flow graph analysis to determine which `Identifier`s can - * be assumed to be non-null objects, on a per-block basis. + * Helper function for `PropagateScopeDependencies`. Uses control flow graph + * analysis to determine which `Identifier`s can be assumed to be non-null + * objects, on a per-block basis. * * Here is an example: * ```js @@ -48,15 +48,16 @@ import { * } * ``` * - * Note that we currently do NOT account for mutable / declaration range - * when doing the CFG-based traversal, producing results that are technically + * Note that we currently do NOT account for mutable / declaration range when + * doing the CFG-based traversal, producing results that are technically * incorrect but filtered by PropagateScopeDeps (which only takes dependencies * on constructed value -- i.e. a scope's dependencies must have mutable ranges * ending earlier than the scope start). * - * Take this example, this function will infer x.foo.bar as non-nullable for bb0, - * via the intersection of bb1 & bb2 which in turn comes from bb3. This is technically - * incorrect bb0 is before / during x's mutable range. + * Take this example, this function will infer x.foo.bar as non-nullable for + * bb0, via the intersection of bb1 & bb2 which in turn comes from bb3. This is + * technically incorrect bb0 is before / during x's mutable range. + * ``` * bb0: * const x = ...; * if cond then bb1 else bb2 @@ -68,15 +69,29 @@ import { * goto bb3: * bb3: * x.foo.bar + * ``` + * + * @param fn + * @param temporaries sidemap of identifier -> baseObject.a.b paths. Does not + * contain optional chains. + * @param hoistableFromOptionals sidemap of optionalBlock -> baseObject?.a + * optional paths for which it's safe to evaluate non-optional loads (see + * CollectOptionalChainDependencies). + * @returns */ export function collectHoistablePropertyLoads( fn: HIRFunction, temporaries: ReadonlyMap, - optionals: ReadonlyMap, + hoistableFromOptionals: ReadonlyMap, ): ReadonlyMap { const registry = new PropertyPathRegistry(); - const nodes = collectNonNullsInBlocks(fn, temporaries, optionals, registry); + const nodes = collectNonNullsInBlocks( + fn, + temporaries, + hoistableFromOptionals, + registry, + ); propagateNonNull(fn, nodes, registry); const nodesKeyedByScopeId = new Map(); @@ -216,7 +231,7 @@ function getMaybeNonNullInInstruction( function collectNonNullsInBlocks( fn: HIRFunction, temporaries: ReadonlyMap, - optionals: ReadonlyMap, + hoistableFromOptionals: ReadonlyMap, registry: PropertyPathRegistry, ): ReadonlyMap { /** @@ -258,7 +273,7 @@ function collectNonNullsInBlocks( block, assumedNonNullObjects, }); - const maybeOptionalChain = optionals.get(block.id); + const maybeOptionalChain = hoistableFromOptionals.get(block.id); if (maybeOptionalChain != null) { assumedNonNullObjects.add( registry.getOrCreateProperty(maybeOptionalChain), @@ -399,8 +414,10 @@ function propagateNonNull( assertNonNull(nodes.get(nodeId)).assumedNonNullObjects = mergedObjects; traversalState.set(nodeId, 'done'); /** - * Note that it might not sufficient to compare set sizes since reduceMaybeOptionalChains - * may replace optional-chain loads with unconditional loads + * Note that it's not sufficient to compare set sizes since + * reduceMaybeOptionalChains may replace optional-chain loads with + * unconditional loads. This could in turn change `assumedNonNullObjects` of + * downstream blocks and backedges. */ changed ||= !Set_equal(prevObjects, mergedObjects); return changed; @@ -457,8 +474,8 @@ export function assertNonNull, U>( * property strings paths de-duplicates. * * Intuitively: given ?.b, we know to be either hoistable or not. - * If is hoistable, we can replace all ?.PROPERTY_STRING subpaths - * with .PROPERTY_STRING + * If unconditional reads from are hoistable, we can replace all + * ?.PROPERTY_STRING subpaths with .PROPERTY_STRING */ function reduceMaybeOptionalChains( nodes: Set, @@ -468,7 +485,6 @@ function reduceMaybeOptionalChains( if (optionalChainNodes.size === 0) { return; } - const knownNonNulls = new Set(nodes); let changed: boolean; do { changed = false; @@ -481,7 +497,7 @@ function reduceMaybeOptionalChains( const entry = origPath[i]; // If the base is known to be non-null, replace with a non-optional load const nextEntry: DependencyPathEntry = - entry.optional && knownNonNulls.has(currNode) + entry.optional && nodes.has(currNode) ? {property: entry.property, optional: false} : entry; currNode = PropertyPathRegistry.getOrCreatePropertyEntry( @@ -495,7 +511,6 @@ function reduceMaybeOptionalChains( optionalChainNodes.add(currNode); nodes.delete(original); nodes.add(currNode); - knownNonNulls.add(currNode); } } } while (changed); diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/CollectOptionalChainDependencies.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/CollectOptionalChainDependencies.ts index c4b1e47ec784b..2f63b41cfdcae 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/CollectOptionalChainDependencies.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/CollectOptionalChainDependencies.ts @@ -16,7 +16,8 @@ import { OptionalTerminal, HIRFunction, } from './HIR'; -import {printIdentifier, printInstruction} from './PrintHIR'; +import {printIdentifier} from './PrintHIR'; + export function collectOptionalChainSidemap( fn: HIRFunction, ): OptionalChainSidemap { @@ -49,12 +50,14 @@ export function collectOptionalChainSidemap( export type OptionalChainSidemap = { /** * Stores the correct property mapping (e.g. `a?.b` instead of `a.b`) for - * dependency calculation. Note that we currently do not store anything on phi nodes (e.g. the outer ) + * dependency calculation. Note that we currently do not store anything on + * outer phi nodes. */ temporariesReadInOptional: ReadonlyMap; /** - * When extracting dependencies in PropagateScopeDependencies, skip instructions already - * processed in this pass. + * Records instructions (PropertyLoads, StoreLocals, and test terminals) + * processed in this pass. When extracting dependencies in + * PropagateScopeDependencies, these instructions are skipped. * * E.g. given a?.b * ``` @@ -96,9 +99,9 @@ export type OptionalChainSidemap = { */ processedInstrsInOptional: ReadonlySet; /** - * Optional-chains that can be hoisted to the start of optional chains. e.g. - * given `a?.b.c`, we can evaluate any PropertyLoad from `a?.b` at the - * optional terminal in bb1 + * Records optional chains for which we can safely evaluate non-optional + * PropertyLoads. e.g. given `a?.b.c`, we can evaluate any load from `a?.b` at + * the optional terminal in bb1. * ```js * bb1: * ... @@ -196,11 +199,12 @@ function assertOptionalAlternateBlock( /** * Traverse into the optional block and all transitively referenced blocks to - * collect a sidemaps identifier and block ids -> optional chain dependencies. + * collect sidemaps of optional chain dependencies. * * @returns the IdentifierId representing the optional block if the block and - * all transitively referenced optionals precisely represent a chain of property - * loads. If any part of the optional chain is not hoistable, returns null. + * all transitively referenced optional blocks precisely represent a chain of + * property loads. If any part of the optional chain is not hoistable, returns + * null. */ function traverseOptionalBlock( optional: TBasicBlock, @@ -224,20 +228,16 @@ function traverseOptionalBlock( * blocks, but this would require changes to HIR. */ CompilerError.invariant(optional.terminal.optional, { - reason: - '[OptionalChainDeps] Expect base optional case to be always optional', + reason: '[OptionalChainDeps] Expect base case to be always optional', loc: optional.terminal.loc, }); - CompilerError.invariant(maybeTest.instructions.length >= 1, { - reason: - '[OptionalChainDeps] Expected direct optional test branch (base case) to have at least one instruction', - loc: maybeTest.terminal.loc, - }); - /** * Only match base expressions that are straightforward PropertyLoad chains */ - if (maybeTest.instructions[0].value.kind !== 'LoadLocal') { + if ( + maybeTest.instructions.length === 0 || + maybeTest.instructions[0].value.kind !== 'LoadLocal' + ) { return null; } const path = maybeTest.instructions.slice(1).map((entry, i) => { @@ -271,7 +271,9 @@ function traverseOptionalBlock( } else if (maybeTest.terminal.kind === 'optional') { /** * This is either - * - a chained optional i.e. base=?.b or .b + * - ?.property (optional=true) + * - .property (optional=false) + * - * - a optional base block with a separate nested optional-chain (e.g. a(c?.d)?.d) */ const testBlock = context.blocks.get(maybeTest.terminal.fallthrough)!; @@ -337,19 +339,18 @@ function traverseOptionalBlock( context.temporariesReadInOptional.get(innerOptional), ); test = testBlock.terminal; - if (test.alternate === outerAlternate) { - CompilerError.invariant(optional.instructions.length === 0, { - reason: - '[OptionalChainDeps] Unexpected instructions an inner optional block. ' + - 'This indicates that the compiler may be incorrectly concatenating two unrelated optional chains', - description: `bb${optional.id}\n ${testBlock.id}\n${optional.instructions.map(printInstruction).join('\n')}`, - loc: optional.terminal.loc, - }); - } } else { return null; } + if (test.alternate === outerAlternate) { + CompilerError.invariant(optional.instructions.length === 0, { + reason: + '[OptionalChainDeps] Unexpected instructions an inner optional block. ' + + 'This indicates that the compiler may be incorrectly concatenating two unrelated optional chains', + loc: optional.terminal.loc, + }); + } const matchConsequentResult = matchOptionalTestBlock(test, context.blocks); if (!matchConsequentResult) { // Optional chain consequent is not hoistable e.g. a?.[computed()]