Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[compiler] Wrap ReactiveScopeDep path tokens in object #30812

Merged
merged 4 commits into from
Aug 28, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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