Skip to content

Commit

Permalink
[compiler] Wrap ReactiveScopeDep path tokens in object
Browse files Browse the repository at this point in the history
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: c5d38d72f6b9d084a5df69ad23178794468f5f8b
Pull Request resolved: #30812
  • Loading branch information
josephsavona committed Aug 28, 2024
1 parent 5e51d76 commit 4759161
Show file tree
Hide file tree
Showing 9 changed files with 40 additions and 94 deletions.
12 changes: 10 additions & 2 deletions compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts
Original file line number Diff line number Diff line change
Expand Up @@ -776,7 +776,7 @@ export type ManualMemoDependency = {
value: Place;
}
| {kind: 'Global'; identifierName: string};
path: Array<string>;
path: DependencyPath;
};

export type StartMemoize = {
Expand Down Expand Up @@ -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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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> {
Expand Down Expand Up @@ -294,7 +252,7 @@ type DependencyNode = {
};

type ReduceResultNode = {
relativePath: Array<string>;
relativePath: Array<{property: string}>;
accessType: PropertyAccessType;
};

Expand Down Expand Up @@ -325,7 +283,7 @@ function deriveMinimalDependenciesInSubtree(
const childResult = deriveMinimalDependenciesInSubtree(childNode).map(
({relativePath, accessType}) => {
return {
relativePath: [childName, ...relativePath],
relativePath: [{property: childName}, ...relativePath],
accessType,
};
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import {
ReactiveScopeDependency,
ReactiveStatement,
Type,
areEqualPaths,
makeInstructionId,
} from '../HIR';
import {
Expand Down Expand Up @@ -525,10 +526,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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

import {CompilerError} from '../CompilerError';
import {
areEqualPaths,
BlockId,
DeclarationId,
GeneratedSource,
Expand Down Expand Up @@ -35,7 +36,6 @@ import {
ReactiveScopeDependencyTree,
ReactiveScopePropertyDependency,
} from './DeriveMinimalDependencies';
import {areEqualPaths} from './MergeReactiveScopesThatInvalidateTogether';
import {ReactiveFunctionVisitor, visitReactiveFunction} from './visitors';

/*
Expand Down Expand Up @@ -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);
Expand All @@ -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);
}

Expand All @@ -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;
}
Expand Down Expand Up @@ -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);
Expand All @@ -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);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand All @@ -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 {
Expand Down

0 comments on commit 4759161

Please sign in to comment.