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 ea7268c573..a11822538f 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/CollectHoistablePropertyLoads.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/CollectHoistablePropertyLoads.ts @@ -241,7 +241,10 @@ type PropertyPathNode = class PropertyPathRegistry { roots: Map = new Map(); - getOrCreateIdentifier(identifier: Identifier): PropertyPathNode { + getOrCreateIdentifier( + identifier: Identifier, + reactive: boolean, + ): PropertyPathNode { /** * Reads from a statically scoped variable are always safe in JS, * with the exception of TDZ (not addressed by this pass). @@ -255,12 +258,19 @@ class PropertyPathRegistry { optionalProperties: new Map(), fullPath: { identifier, + reactive, path: [], }, hasOptional: false, parent: null, }; this.roots.set(identifier.id, rootNode); + } else { + CompilerError.invariant(reactive === rootNode.fullPath.reactive, { + reason: + '[HoistablePropertyLoads] Found inconsistencies in `reactive` flag when deduping identifier reads within the same scope', + loc: identifier.loc, + }); } return rootNode; } @@ -278,6 +288,7 @@ class PropertyPathRegistry { parent: parent, fullPath: { identifier: parent.fullPath.identifier, + reactive: parent.fullPath.reactive, path: parent.fullPath.path.concat(entry), }, hasOptional: parent.hasOptional || entry.optional, @@ -293,7 +304,7 @@ class PropertyPathRegistry { * so all subpaths of a PropertyLoad should already exist * (e.g. a.b is added before a.b.c), */ - let currNode = this.getOrCreateIdentifier(n.identifier); + let currNode = this.getOrCreateIdentifier(n.identifier, n.reactive); if (n.path.length === 0) { return currNode; } @@ -315,10 +326,11 @@ function getMaybeNonNullInInstruction( instr: InstructionValue, context: CollectHoistablePropertyLoadsContext, ): PropertyPathNode | null { - let path = null; + let path: ReactiveScopeDependency | null = null; if (instr.kind === 'PropertyLoad') { path = context.temporaries.get(instr.object.identifier.id) ?? { identifier: instr.object.identifier, + reactive: instr.object.reactive, path: [], }; } else if (instr.kind === 'Destructure') { @@ -381,7 +393,7 @@ function collectNonNullsInBlocks( ) { const identifier = fn.params[0].identifier; knownNonNullIdentifiers.add( - context.registry.getOrCreateIdentifier(identifier), + context.registry.getOrCreateIdentifier(identifier, true), ); } const nodes = new Map< @@ -616,9 +628,11 @@ function reduceMaybeOptionalChains( changed = false; for (const original of optionalChainNodes) { - let {identifier, path: origPath} = original.fullPath; - let currNode: PropertyPathNode = - registry.getOrCreateIdentifier(identifier); + let {identifier, path: origPath, reactive} = original.fullPath; + let currNode: PropertyPathNode = registry.getOrCreateIdentifier( + identifier, + reactive, + ); 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 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 cb787d04d0..75dad4c1bf 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/CollectOptionalChainDependencies.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/CollectOptionalChainDependencies.ts @@ -290,6 +290,7 @@ function traverseOptionalBlock( ); baseObject = { identifier: maybeTest.instructions[0].value.place.identifier, + reactive: maybeTest.instructions[0].value.place.reactive, path, }; test = maybeTest.terminal; @@ -391,6 +392,7 @@ function traverseOptionalBlock( ); const load = { identifier: baseObject.identifier, + reactive: baseObject.reactive, path: [ ...baseObject.path, { diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/DeriveMinimalDependenciesHIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/DeriveMinimalDependenciesHIR.ts index 7f6fb9e88f..7819ab39b2 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/DeriveMinimalDependenciesHIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/DeriveMinimalDependenciesHIR.ts @@ -25,8 +25,9 @@ export class ReactiveScopeDependencyTreeHIR { * `identifier.path`, or `identifier?.path` is in this map, it is safe to * evaluate (non-optional) PropertyLoads from. */ - #hoistableObjects: Map = new Map(); - #deps: Map = new Map(); + #hoistableObjects: Map = + new Map(); + #deps: Map = new Map(); /** * @param hoistableObjects a set of paths from which we can safely evaluate @@ -35,9 +36,10 @@ export class ReactiveScopeDependencyTreeHIR { * duplicates when traversing the CFG. */ constructor(hoistableObjects: Iterable) { - for (const {path, identifier} of hoistableObjects) { + for (const {path, identifier, reactive} of hoistableObjects) { let currNode = ReactiveScopeDependencyTreeHIR.#getOrCreateRoot( identifier, + reactive, this.#hoistableObjects, path.length > 0 && path[0].optional ? 'Optional' : 'NonNull', ); @@ -70,7 +72,8 @@ export class ReactiveScopeDependencyTreeHIR { static #getOrCreateRoot( identifier: Identifier, - roots: Map>, + reactive: boolean, + roots: Map & {reactive: boolean}>, defaultAccessType: T, ): TreeNode { // roots can always be accessed unconditionally in JS @@ -79,9 +82,16 @@ export class ReactiveScopeDependencyTreeHIR { if (rootNode === undefined) { rootNode = { properties: new Map(), + reactive, accessType: defaultAccessType, }; roots.set(identifier, rootNode); + } else { + CompilerError.invariant(reactive === rootNode.reactive, { + reason: '[DeriveMinimalDependenciesHIR] Conflicting reactive root flag', + description: `Identifier ${printIdentifier(identifier)}`, + loc: GeneratedSource, + }); } return rootNode; } @@ -92,9 +102,10 @@ export class ReactiveScopeDependencyTreeHIR { * safe-to-evaluate subpath */ addDependency(dep: ReactiveScopeDependency): void { - const {identifier, path} = dep; + const {identifier, reactive, path} = dep; let depCursor = ReactiveScopeDependencyTreeHIR.#getOrCreateRoot( identifier, + reactive, this.#deps, PropertyAccessType.UnconditionalAccess, ); @@ -172,7 +183,13 @@ export class ReactiveScopeDependencyTreeHIR { deriveMinimalDependencies(): Set { const results = new Set(); for (const [rootId, rootNode] of this.#deps.entries()) { - collectMinimalDependenciesInSubtree(rootNode, rootId, [], results); + collectMinimalDependenciesInSubtree( + rootNode, + rootNode.reactive, + rootId, + [], + results, + ); } return results; @@ -294,25 +311,24 @@ type HoistableNode = TreeNode<'Optional' | 'NonNull'>; type DependencyNode = TreeNode; /** - * TODO: this is directly pasted from DeriveMinimalDependencies. Since we no - * longer have conditionally accessed nodes, we can simplify - * * Recursively calculates minimal dependencies in a subtree. * @param node DependencyNode representing a dependency subtree. * @returns a minimal list of dependencies in this subtree. */ function collectMinimalDependenciesInSubtree( node: DependencyNode, + reactive: boolean, rootIdentifier: Identifier, path: Array, results: Set, ): void { if (isDependency(node.accessType)) { - results.add({identifier: rootIdentifier, path}); + results.add({identifier: rootIdentifier, reactive, path}); } else { for (const [childName, childNode] of node.properties) { collectMinimalDependenciesInSubtree( childNode, + reactive, rootIdentifier, [ ...path, diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts index 99b8c189ee..1699a0fc3d 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts @@ -1568,6 +1568,18 @@ export type DependencyPathEntry = { export type DependencyPath = Array; export type ReactiveScopeDependency = { identifier: Identifier; + /** + * Reflects whether the base identifier is reactive. Note that some reactive + * objects may have non-reactive properties, but we do not currently track + * this. + * + * ```js + * // Technically, result[0] is reactive and result[1] is not. + * // Currently, both dependencies would be marked as reactive. + * const result = useState(); + * ``` + */ + reactive: boolean; path: DependencyPath; }; diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/PropagateScopeDependenciesHIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/PropagateScopeDependenciesHIR.ts index 96b9e51710..91b7712b88 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/PropagateScopeDependenciesHIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/PropagateScopeDependenciesHIR.ts @@ -316,6 +316,7 @@ function collectTemporariesSidemapImpl( ) { temporaries.set(lvalue.identifier.id, { identifier: value.place.identifier, + reactive: value.place.reactive, path: [], }); } @@ -369,11 +370,13 @@ function getProperty( if (resolvedDependency == null) { property = { identifier: object.identifier, + reactive: object.reactive, path: [{property: propertyName, optional}], }; } else { property = { identifier: resolvedDependency.identifier, + reactive: resolvedDependency.reactive, path: [...resolvedDependency.path, {property: propertyName, optional}], }; } @@ -532,6 +535,7 @@ export class DependencyCollectionContext { this.visitDependency( this.#temporaries.get(place.identifier.id) ?? { identifier: place.identifier, + reactive: place.reactive, path: [], }, ); @@ -596,6 +600,7 @@ export class DependencyCollectionContext { ) { maybeDependency = { identifier: maybeDependency.identifier, + reactive: maybeDependency.reactive, path: [], }; } @@ -617,7 +622,11 @@ export class DependencyCollectionContext { identifier => identifier.declarationId === place.identifier.declarationId, ) && - this.#checkValidDependency({identifier: place.identifier, path: []}) + this.#checkValidDependency({ + identifier: place.identifier, + reactive: place.reactive, + path: [], + }) ) { currentScope.reassignments.add(place.identifier); } diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferReactivePlaces.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferReactivePlaces.ts index b05b292124..88faccd8cf 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferReactivePlaces.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferReactivePlaces.ts @@ -26,6 +26,7 @@ import { import {PostDominator} from '../HIR/Dominator'; import { eachInstructionLValue, + eachInstructionOperand, eachInstructionValueOperand, eachTerminalOperand, } from '../HIR/visitors'; @@ -292,7 +293,7 @@ export function inferReactivePlaces(fn: HIRFunction): void { let hasReactiveInput = false; /* * NOTE: we want to mark all operands as reactive or not, so we - * avoid short-circuting here + * avoid short-circuiting here */ for (const operand of eachInstructionValueOperand(value)) { const reactive = reactiveIdentifiers.isReactive(operand); @@ -375,6 +376,41 @@ export function inferReactivePlaces(fn: HIRFunction): void { } } } while (reactiveIdentifiers.snapshot()); + + function propagateReactivityToInnerFunctions( + fn: HIRFunction, + isOutermost: boolean, + ): void { + for (const [, block] of fn.body.blocks) { + for (const instr of block.instructions) { + if (!isOutermost) { + for (const operand of eachInstructionOperand(instr)) { + reactiveIdentifiers.isReactive(operand); + } + } + if ( + instr.value.kind === 'ObjectMethod' || + instr.value.kind === 'FunctionExpression' + ) { + propagateReactivityToInnerFunctions( + instr.value.loweredFunc.func, + false, + ); + } + } + if (!isOutermost) { + for (const operand of eachTerminalOperand(block.terminal)) { + reactiveIdentifiers.isReactive(operand); + } + } + } + } + + /** + * Propagate reactivity for inner functions, as we eventually hoist and dedupe + * dependency instructions for scopes. + */ + propagateReactivityToInnerFunctions(fn, true); } /* diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/MergeReactiveScopesThatInvalidateTogether.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/MergeReactiveScopesThatInvalidateTogether.ts index 08d2212d86..6f2d97ff8e 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/MergeReactiveScopesThatInvalidateTogether.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/MergeReactiveScopesThatInvalidateTogether.ts @@ -456,6 +456,7 @@ function canMergeScopes( new Set( [...current.scope.declarations.values()].map(declaration => ({ identifier: declaration.identifier, + reactive: true, path: [], })), ),