From aa2df52e576951f1010948fae1668b03635efa8c Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Mon, 26 Aug 2024 12:20:11 -0700 Subject: [PATCH] [compiler] Wrap ReactiveScopeDep path tokens in object Previously the path of a ReactiveScopeDependency was `Array`. We need to track whether each property access is optional or not, so as a first step we change this to `Array<{property: string}>`, making space for an additional property in a subsequent PR. [ghstack-poisoned] --- .../src/HIR/HIR.ts | 12 +++- .../src/Inference/DropManualMemoization.ts | 2 +- .../ReactiveScopes/CodegenReactiveFunction.ts | 4 +- .../DeriveMinimalDependencies.ts | 70 ++++--------------- ...rgeReactiveScopesThatInvalidateTogether.ts | 6 +- .../ReactiveScopes/PrintReactiveFunction.ts | 2 +- .../PropagateScopeDependencies.ts | 27 ++----- .../PruneInitializationDependencies.ts | 4 +- .../ValidatePreservedManualMemoization.ts | 8 +-- 9 files changed, 41 insertions(+), 94 deletions(-) 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 06fa48a73656c..e74ec52203bab 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts @@ -776,7 +776,7 @@ export type ManualMemoDependency = { value: Place; } | {kind: 'Global'; identifierName: string}; - path: Array; + path: DependencyPath; }; export type StartMemoize = { @@ -1494,9 +1494,17 @@ export type ReactiveScopeDeclaration = { export type ReactiveScopeDependency = { identifier: Identifier; - path: Array; + path: DependencyPath; }; +export function areEqualPaths(a: DependencyPath, b: DependencyPath): boolean { + return ( + a.length === b.length && + a.every((item, ix) => item.property === b[ix].property) + ); +} +export type DependencyPath = Array<{property: string}>; + /* * Simulated opaque type for BlockIds to prevent using normal numbers as block ids * accidentally. diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/DropManualMemoization.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/DropManualMemoization.ts index c9d2a7e1412c3..e60d8a9914583 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/DropManualMemoization.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/DropManualMemoization.ts @@ -68,7 +68,7 @@ export function collectMaybeMemoDependencies( if (object != null) { return { root: object.root, - path: [...object.path, value.property], + path: [...object.path, {property: value.property}], }; } break; diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts index 624a4b604d66f..73330b959e018 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts @@ -1411,7 +1411,7 @@ function printDependencyComment(dependency: ReactiveScopeDependency): string { let name = identifier.name; if (dependency.path !== null) { for (const path of dependency.path) { - name += `.${path}`; + name += `.${path.property}`; } } return name; @@ -1448,7 +1448,7 @@ function codegenDependency( let object: t.Expression = convertIdentifier(dependency.identifier); if (dependency.path !== null) { for (const path of dependency.path) { - object = t.memberExpression(object, t.identifier(path)); + object = t.memberExpression(object, t.identifier(path.property)); } } return object; 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 8c2e31fa9666e..13420ee140e63 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/DeriveMinimalDependencies.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/DeriveMinimalDependencies.ts @@ -14,20 +14,8 @@ import {assertExhaustive} from '../Utils/utils'; * We need to understand optional member expressions only when determining * dependencies of a ReactiveScope (i.e. in {@link PropagateScopeDependencies}), * hence why this type lives here (not in HIR.ts) - * - * {@link ReactiveScopePropertyDependency.optionalPath} is populated only if the Property - * represents an optional member expression, and it represents the property path - * loaded conditionally. - * e.g. the member expr a.b.c?.d.e?.f is represented as - * { - * identifier: 'a'; - * path: ['b', 'c'], - * optionalPath: ['d', 'e', 'f']. - * } */ -export type ReactiveScopePropertyDependency = ReactiveScopeDependency & { - optionalPath: Array; -}; +export type ReactiveScopePropertyDependency = ReactiveScopeDependency; /* * Finalizes a set of ReactiveScopeDependencies to produce a set of minimal unconditional @@ -69,59 +57,29 @@ export class ReactiveScopeDependencyTree { } add(dep: ReactiveScopePropertyDependency, inConditional: boolean): void { - const {path, optionalPath} = dep; + const {path} = dep; let currNode = this.#getOrCreateRoot(dep.identifier); const accessType = inConditional ? PropertyAccessType.ConditionalAccess : PropertyAccessType.UnconditionalAccess; - for (const property of path) { + for (const item of path) { // all properties read 'on the way' to a dependency are marked as 'access' - let currChild = getOrMakeProperty(currNode, property); + let currChild = getOrMakeProperty(currNode, item.property); currChild.accessType = merge(currChild.accessType, accessType); currNode = currChild; } - if (optionalPath.length === 0) { - /* - * If this property does not have a conditional path (i.e. a.b.c), the - * final property node should be marked as an conditional/unconditional - * `dependency` as based on control flow. - */ - const depType = inConditional - ? PropertyAccessType.ConditionalDependency - : PropertyAccessType.UnconditionalDependency; - - currNode.accessType = merge(currNode.accessType, depType); - } else { - /* - * Technically, we only depend on whether unconditional path `dep.path` - * is nullish (not its actual value). As long as we preserve the nullthrows - * behavior of `dep.path`, we can keep it as an access (and not promote - * to a dependency). - * See test `reduce-reactive-cond-memberexpr-join` for example. - */ - - /* - * If this property has an optional path (i.e. a?.b.c), all optional - * nodes should be marked accordingly. - */ - for (const property of optionalPath) { - let currChild = getOrMakeProperty(currNode, property); - currChild.accessType = merge( - currChild.accessType, - PropertyAccessType.ConditionalAccess, - ); - currNode = currChild; - } + /** + * The final property node should be marked as an conditional/unconditional + * `dependency` as based on control flow. + */ + const depType = inConditional + ? PropertyAccessType.ConditionalDependency + : PropertyAccessType.UnconditionalDependency; - // The final node should be marked as a conditional dependency. - currNode.accessType = merge( - currNode.accessType, - PropertyAccessType.ConditionalDependency, - ); - } + currNode.accessType = merge(currNode.accessType, depType); } deriveMinimalDependencies(): Set { @@ -294,7 +252,7 @@ type DependencyNode = { }; type ReduceResultNode = { - relativePath: Array; + relativePath: Array<{property: string}>; accessType: PropertyAccessType; }; @@ -325,7 +283,7 @@ function deriveMinimalDependenciesInSubtree( const childResult = deriveMinimalDependenciesInSubtree(childNode).map( ({relativePath, accessType}) => { return { - relativePath: [childName, ...relativePath], + relativePath: [{property: childName}, ...relativePath], accessType, }; }, 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 1e73697783f0b..21f2140f4c137 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/MergeReactiveScopesThatInvalidateTogether.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/MergeReactiveScopesThatInvalidateTogether.ts @@ -8,6 +8,7 @@ import {CompilerError} from '..'; import { DeclarationId, + DependencyPath, InstructionId, InstructionKind, Place, @@ -19,6 +20,7 @@ import { ReactiveScopeDependency, ReactiveStatement, Type, + areEqualPaths, makeInstructionId, } from '../HIR'; import { @@ -525,10 +527,6 @@ function areEqualDependencies( return true; } -export function areEqualPaths(a: Array, b: Array): boolean { - return a.length === b.length && a.every((item, ix) => item === b[ix]); -} - /** * Is this scope eligible for merging with subsequent scopes? In general this * is only true if the scope's output values are guaranteed to change when its 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 f85f7071f1c49..80395a2e0ea41 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(prop => `.${prop}`).join('')}`; + return `${identifier}${dependency.path.map(token => `.${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 690bdb758839d..0e47a8dcedfb2 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PropagateScopeDependencies.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PropagateScopeDependencies.ts @@ -7,6 +7,7 @@ import {CompilerError} from '../CompilerError'; import { + areEqualPaths, BlockId, DeclarationId, GeneratedSource, @@ -35,7 +36,6 @@ import { ReactiveScopeDependencyTree, ReactiveScopePropertyDependency, } from './DeriveMinimalDependencies'; -import {areEqualPaths} from './MergeReactiveScopesThatInvalidateTogether'; import {ReactiveFunctionVisitor, visitReactiveFunction} from './visitors'; /* @@ -465,7 +465,6 @@ class Context { #getProperty( object: Place, property: string, - isConditional: boolean, ): ReactiveScopePropertyDependency { const resolvedObject = this.resolveTemporary(object); const resolvedDependency = this.#properties.get(resolvedObject.identifier); @@ -478,36 +477,21 @@ class Context { objectDependency = { identifier: resolvedObject.identifier, path: [], - optionalPath: [], }; } else { objectDependency = { identifier: resolvedDependency.identifier, path: [...resolvedDependency.path], - optionalPath: [...resolvedDependency.optionalPath], }; } - // (2) Determine whether property is an optional access - if (objectDependency.optionalPath.length > 0) { - /* - * If the base property dependency represents a optional member expression, - * property is on the optionalPath (regardless of whether this PropertyLoad - * itself was conditional) - * e.g. for `a.b?.c.d`, `d` should be added to optionalPath - */ - objectDependency.optionalPath.push(property); - } else if (isConditional) { - objectDependency.optionalPath.push(property); - } else { - objectDependency.path.push(property); - } + objectDependency.path.push({property}); return objectDependency; } declareProperty(lvalue: Place, object: Place, property: string): void { - const nextDependency = this.#getProperty(object, property, false); + const nextDependency = this.#getProperty(object, property); this.#properties.set(lvalue.identifier, nextDependency); } @@ -516,7 +500,7 @@ class Context { // ref.current access is not a valid dep if ( isUseRefType(maybeDependency.identifier) && - maybeDependency.path.at(0) === 'current' + maybeDependency.path.at(0)?.property === 'current' ) { return false; } @@ -577,7 +561,6 @@ class Context { let dependency: ReactiveScopePropertyDependency = { identifier: resolved.identifier, path: [], - optionalPath: [], }; if (resolved.identifier.name === null) { const propertyDependency = this.#properties.get(resolved.identifier); @@ -589,7 +572,7 @@ class Context { } visitProperty(object: Place, property: string): void { - const nextDependency = this.#getProperty(object, property, false); + const nextDependency = this.#getProperty(object, property); this.visitDependency(nextDependency); } diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneInitializationDependencies.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneInitializationDependencies.ts index 721fa7b0ec65d..2a9d0b9793d9f 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneInitializationDependencies.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneInitializationDependencies.ts @@ -180,8 +180,8 @@ class Visitor extends ReactiveFunctionVisitor { [...scope.scope.dependencies].forEach(ident => { let target: undefined | IdentifierId = this.aliases.find(ident.identifier.id) ?? ident.identifier.id; - ident.path.forEach(key => { - target &&= this.paths.get(target)?.get(key); + ident.path.forEach(token => { + target &&= this.paths.get(target)?.get(token.property); }); if (target && this.map.get(target) === 'Create') { scope.scope.dependencies.delete(ident); diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts index 7af4aaaccd7ab..43a477f3418e6 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts @@ -167,7 +167,7 @@ function compareDeps( let isSubpath = true; for (let i = 0; i < Math.min(inferred.path.length, source.path.length); i++) { - if (inferred.path[i] !== source.path[i]) { + if (inferred.path[i].property !== source.path[i].property) { isSubpath = false; break; } @@ -177,14 +177,14 @@ function compareDeps( isSubpath && (source.path.length === inferred.path.length || (inferred.path.length >= source.path.length && - !inferred.path.includes('current'))) + !inferred.path.some(token => token.property === 'current'))) ) { return CompareDependencyResult.Ok; } else { if (isSubpath) { if ( - source.path.includes('current') || - inferred.path.includes('current') + source.path.some(token => token.property === 'current') || + inferred.path.some(token => token.property === 'current') ) { return CompareDependencyResult.RefAccessDifference; } else {