[compiler] Wrap ReactiveScopeDep path tokens in object

Previously the path of a ReactiveScopeDependency was `Array<string>`. 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-source-id: 3b547d32f3b6bcaf71dca7b0de02265e6f1e3ada Pull Request resolved: facebook/react#30812
nivemula · Aug 28, 2024 · e9356cc · e9356cc
1 parent 4472e16
commit e9356cc
Show file tree

Hide file tree

Showing 9 changed files with 41 additions and 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
@@ -776,7 +776,7 @@ export type ManualMemoDependency = {
         value: Place;
       }
     | {kind: 'Global'; identifierName: string};
-  path: Array<string>;
+  path: DependencyPath;
 };
 
 export type StartMemoize = {
@@ -1494,9 +1494,17 @@ export type ReactiveScopeDeclaration = {
 
 export type ReactiveScopeDependency = {
   identifier: Identifier;
-  path: Array<string>;
+  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
@@ -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
@@ -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/...iler/packages/babel-plugin-react-compiler/src/ReactiveScopes/DeriveMinimalDependencies.ts b/...iler/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<string>;
-};
+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<ReactiveScopeDependency> {
@@ -294,7 +252,7 @@ type DependencyNode = {
 };
 
 type ReduceResultNode = {
-  relativePath: Array<string>;
+  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/...bel-plugin-react-compiler/src/ReactiveScopes/MergeReactiveScopesThatInvalidateTogether.ts b/...bel-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<string>, b: Array<string>): 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
@@ -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/...ler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PropagateScopeDependencies.ts b/...ler/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/...ackages/babel-plugin-react-compiler/src/ReactiveScopes/PruneInitializationDependencies.ts b/...ackages/babel-plugin-react-compiler/src/ReactiveScopes/PruneInitializationDependencies.ts
@@ -180,8 +180,8 @@ class Visitor extends ReactiveFunctionVisitor<CreateUpdate> {
     [...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/...packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts b/...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 {