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 d889fa2a23605..ea90ccaf285a7 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/CollectHoistablePropertyLoads.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/CollectHoistablePropertyLoads.ts @@ -74,10 +74,10 @@ export function collectHoistablePropertyLoads( temporaries: ReadonlyMap, optionals: ReadonlyMap, ): ReadonlyMap { - const tree = new Tree(); + const registry = new PropertyPathRegistry(); - const nodes = collectNonNullsInBlocks(fn, temporaries, optionals, tree); - propagateNonNull(fn, nodes, tree); + const nodes = collectNonNullsInBlocks(fn, temporaries, optionals, registry); + propagateNonNull(fn, nodes, registry); const nodesKeyedByScopeId = new Map(); for (const [_, block] of fn.body.blocks) { @@ -94,16 +94,16 @@ export function collectHoistablePropertyLoads( export type BlockInfo = { block: BasicBlock; - assumedNonNullObjects: ReadonlySet; + assumedNonNullObjects: ReadonlySet; }; /** - * Tree data structure to dedupe property loads (e.g. a.b.c) + * PropertyLoadRegistry data structure to dedupe property loads (e.g. a.b.c) * and make computing sets intersections simpler. */ type RootNode = { - properties: Map; - optionalProperties: Map; + properties: Map; + optionalProperties: Map; parent: null; // Recorded to make later computations simpler fullPath: ReactiveScopeDependency; @@ -111,20 +111,20 @@ type RootNode = { root: IdentifierId; }; -type PropertyLoadNode = +type PropertyPathNode = | { - properties: Map; - optionalProperties: Map; - parent: PropertyLoadNode; + properties: Map; + optionalProperties: Map; + parent: PropertyPathNode; fullPath: ReactiveScopeDependency; hasOptional: boolean; } | RootNode; -class Tree { +class PropertyPathRegistry { roots: Map = new Map(); - getOrCreateIdentifier(identifier: Identifier): PropertyLoadNode { + getOrCreateIdentifier(identifier: Identifier): PropertyPathNode { /** * Reads from a statically scoped variable are always safe in JS, * with the exception of TDZ (not addressed by this pass). @@ -149,9 +149,9 @@ class Tree { } static getOrCreatePropertyEntry( - parent: PropertyLoadNode, + parent: PropertyPathNode, entry: DependencyPathEntry, - ): PropertyLoadNode { + ): PropertyPathNode { const map = entry.optional ? parent.optionalProperties : parent.properties; let child = map.get(entry.property); if (child == null) { @@ -170,7 +170,7 @@ class Tree { return child; } - getOrCreateProperty(n: ReactiveScopeDependency): PropertyLoadNode { + getOrCreateProperty(n: ReactiveScopeDependency): PropertyPathNode { /** * We add ReactiveScopeDependencies according to instruction ordering, * so all subpaths of a PropertyLoad should already exist @@ -181,18 +181,24 @@ class Tree { return currNode; } for (let i = 0; i < n.path.length - 1; i++) { - currNode = Tree.getOrCreatePropertyEntry(currNode, n.path[i]); + currNode = PropertyPathRegistry.getOrCreatePropertyEntry( + currNode, + n.path[i], + ); } - return Tree.getOrCreatePropertyEntry(currNode, n.path.at(-1)!); + return PropertyPathRegistry.getOrCreatePropertyEntry( + currNode, + n.path.at(-1)!, + ); } } -function pushPropertyLoadNode( - node: PropertyLoadNode, +function addNonNullPropertyPath( + node: PropertyPathNode, instrId: InstructionId, knownImmutableIdentifiers: Set, - result: Set, + result: Set, ): void { const object = node.fullPath.identifier; /** @@ -221,7 +227,7 @@ function collectNonNullsInBlocks( fn: HIRFunction, temporaries: ReadonlyMap, optionals: ReadonlyMap, - tree: Tree, + registry: PropertyPathRegistry, ): ReadonlyMap { /** * Due to current limitations of mutable range inference, there are edge cases in @@ -243,7 +249,7 @@ function collectNonNullsInBlocks( * Known non-null objects such as functional component props can be safely * read from any block. */ - const knownNonNullIdentifiers = new Set(); + const knownNonNullIdentifiers = new Set(); if ( fn.env.config.enablePropagateDepsInHIR === 'enabled_with_optimizations' && fn.fnType === 'Component' && @@ -251,11 +257,11 @@ function collectNonNullsInBlocks( fn.params[0].kind === 'Identifier' ) { const identifier = fn.params[0].identifier; - knownNonNullIdentifiers.add(tree.getOrCreateIdentifier(identifier)); + knownNonNullIdentifiers.add(registry.getOrCreateIdentifier(identifier)); } const nodes = new Map(); for (const [_, block] of fn.body.blocks) { - const assumedNonNullObjects = new Set( + const assumedNonNullObjects = new Set( knownNonNullIdentifiers, ); @@ -265,7 +271,9 @@ function collectNonNullsInBlocks( }); const maybeOptionalChain = optionals.get(block.id); if (maybeOptionalChain != null) { - assumedNonNullObjects.add(tree.getOrCreateProperty(maybeOptionalChain)); + assumedNonNullObjects.add( + registry.getOrCreateProperty(maybeOptionalChain), + ); continue; } for (const instr of block.instructions) { @@ -274,8 +282,8 @@ function collectNonNullsInBlocks( identifier: instr.value.object.identifier, path: [], }; - const propertyNode = tree.getOrCreateProperty(source); - pushPropertyLoadNode( + const propertyNode = registry.getOrCreateProperty(source); + addNonNullPropertyPath( propertyNode, instr.id, knownImmutableIdentifiers, @@ -288,8 +296,8 @@ function collectNonNullsInBlocks( const source = instr.value.value.identifier.id; const sourceNode = temporaries.get(source); if (sourceNode != null) { - pushPropertyLoadNode( - tree.getOrCreateProperty(sourceNode), + addNonNullPropertyPath( + registry.getOrCreateProperty(sourceNode), instr.id, knownImmutableIdentifiers, assumedNonNullObjects, @@ -302,8 +310,8 @@ function collectNonNullsInBlocks( const source = instr.value.object.identifier.id; const sourceNode = temporaries.get(source); if (sourceNode != null) { - pushPropertyLoadNode( - tree.getOrCreateProperty(sourceNode), + addNonNullPropertyPath( + registry.getOrCreateProperty(sourceNode), instr.id, knownImmutableIdentifiers, assumedNonNullObjects, @@ -318,7 +326,7 @@ function collectNonNullsInBlocks( function propagateNonNull( fn: HIRFunction, nodes: ReadonlyMap, - tree: Tree, + registry: PropertyPathRegistry, ): void { const blockSuccessors = new Map>(); const terminalPreds = new Set(); @@ -404,7 +412,7 @@ function propagateNonNull( const prevObjects = assertNonNull(nodes.get(nodeId)).assumedNonNullObjects; const mergedObjects = Set_union(prevObjects, neighborAccesses); - reduceMaybeOptionalChains(mergedObjects, tree); + reduceMaybeOptionalChains(mergedObjects, registry); assertNonNull(nodes.get(nodeId)).assumedNonNullObjects = mergedObjects; traversalState.set(nodeId, 'done'); @@ -471,8 +479,8 @@ export function assertNonNull, U>( * with .PROPERTY_STRING */ function reduceMaybeOptionalChains( - nodes: Set, - tree: Tree, + nodes: Set, + registry: PropertyPathRegistry, ): void { let optionalChainNodes = Set_filter(nodes, n => n.hasOptional); if (optionalChainNodes.size === 0) { @@ -485,7 +493,8 @@ function reduceMaybeOptionalChains( for (const original of optionalChainNodes) { let {identifier, path: origPath} = original.fullPath; - let currNode: PropertyLoadNode = tree.getOrCreateIdentifier(identifier); + let currNode: PropertyPathNode = + registry.getOrCreateIdentifier(identifier); for (let i = 0; i < origPath.length; i++) { const entry = origPath[i]; // If the base is known to be non-null, replace with a non-optional load @@ -493,7 +502,10 @@ function reduceMaybeOptionalChains( entry.optional && knownNonNulls.has(currNode) ? {property: entry.property, optional: false} : entry; - currNode = Tree.getOrCreatePropertyEntry(currNode, nextEntry); + currNode = PropertyPathRegistry.getOrCreatePropertyEntry( + currNode, + nextEntry, + ); } if (currNode !== original) { changed = true; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reduce-reactive-deps/jump-unpoisoned/throw-before-scope-starts.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reduce-reactive-deps/jump-unpoisoned/throw-before-scope-starts.expect.md index 4f04ef7fdc1ba..dd7509c48c4c4 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reduce-reactive-deps/jump-unpoisoned/throw-before-scope-starts.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reduce-reactive-deps/jump-unpoisoned/throw-before-scope-starts.expect.md @@ -75,7 +75,7 @@ export const FIXTURE_ENTRYPOINT = { ### Eval output (kind: ok) [[ (exception in render) Error: throw with error! ]] -[[ (exception in render) Error: throw with error! ]] +[2] [[ (exception in render) Error: throw with error! ]] [[ (exception in render) TypeError: Cannot read properties of undefined (reading 'b') ]] [null] diff --git a/compiler/packages/snap/src/sprout/evaluator.ts b/compiler/packages/snap/src/sprout/evaluator.ts index 27cbe55858f60..77e8044fb5ed5 100644 --- a/compiler/packages/snap/src/sprout/evaluator.ts +++ b/compiler/packages/snap/src/sprout/evaluator.ts @@ -60,6 +60,7 @@ const ExportSchema = z.object({ FIXTURE_ENTRYPOINT: EntrypointSchema, }); +const NO_ERROR_SENTINEL = Symbol(); /** * Wraps WrapperTestComponent in an error boundary to simplify re-rendering * when an exception is thrown. @@ -67,31 +68,47 @@ const ExportSchema = z.object({ */ class WrapperTestComponentWithErrorBoundary extends React.Component< {fn: any; params: Array}, - {hasError: boolean; error: any} + {errorFromLastRender: any} > { - propsErrorMap: MutableRefObject>; + /** + * Limit retries of the child component by caching seen errors. + */ + propsErrorMap: Map; + lastProps: any | null; + // lastProps: object | null; constructor(props: any) { super(props); - this.state = {hasError: false, error: null}; - this.propsErrorMap = React.createRef() as MutableRefObject>; - this.propsErrorMap.current = new Map(); + this.lastProps = null; + this.propsErrorMap = new Map(); + this.state = { + errorFromLastRender: NO_ERROR_SENTINEL, + }; } static getDerivedStateFromError(error: any) { - return {hasError: true, error: error}; + // Reschedule a second render that immediately returns the cached error + return {errorFromLastRender: error}; } override componentDidUpdate() { - if (this.state.hasError) { - this.setState({hasError: false, error: null}); + if (this.state.errorFromLastRender !== NO_ERROR_SENTINEL) { + // Reschedule a third render that immediately returns the cached error + this.setState({errorFromLastRender: NO_ERROR_SENTINEL}); } } override render() { - if (this.state.hasError) { - this.propsErrorMap.current!.set( - this.props, - `[[ (exception in render) ${this.state.error?.toString()} ]]`, - ); + if ( + this.state.errorFromLastRender !== NO_ERROR_SENTINEL && + this.props === this.lastProps + ) { + /** + * The last render errored, cache the error message to avoid running the + * test fixture more than once + */ + const errorMsg = `[[ (exception in render) ${this.state.errorFromLastRender?.toString()} ]]`; + this.propsErrorMap.set(this.lastProps, errorMsg); + return errorMsg; } - const cachedError = this.propsErrorMap.current!.get(this.props); + this.lastProps = this.props; + const cachedError = this.propsErrorMap.get(this.props); if (cachedError != null) { return cachedError; }