Skip to content

Commit 4759161

Browse files
committed
[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: c5d38d7 Pull Request resolved: #30812
1 parent 5e51d76 commit 4759161

File tree

9 files changed

+40
-94
lines changed

9 files changed

+40
-94
lines changed

compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -776,7 +776,7 @@ export type ManualMemoDependency = {
776776
value: Place;
777777
}
778778
| {kind: 'Global'; identifierName: string};
779-
path: Array<string>;
779+
path: DependencyPath;
780780
};
781781

782782
export type StartMemoize = {
@@ -1494,9 +1494,17 @@ export type ReactiveScopeDeclaration = {
14941494

14951495
export type ReactiveScopeDependency = {
14961496
identifier: Identifier;
1497-
path: Array<string>;
1497+
path: DependencyPath;
14981498
};
14991499

1500+
export function areEqualPaths(a: DependencyPath, b: DependencyPath): boolean {
1501+
return (
1502+
a.length === b.length &&
1503+
a.every((item, ix) => item.property === b[ix].property)
1504+
);
1505+
}
1506+
export type DependencyPath = Array<{property: string}>;
1507+
15001508
/*
15011509
* Simulated opaque type for BlockIds to prevent using normal numbers as block ids
15021510
* accidentally.

compiler/packages/babel-plugin-react-compiler/src/Inference/DropManualMemoization.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ export function collectMaybeMemoDependencies(
6868
if (object != null) {
6969
return {
7070
root: object.root,
71-
path: [...object.path, value.property],
71+
path: [...object.path, {property: value.property}],
7272
};
7373
}
7474
break;

compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1411,7 +1411,7 @@ function printDependencyComment(dependency: ReactiveScopeDependency): string {
14111411
let name = identifier.name;
14121412
if (dependency.path !== null) {
14131413
for (const path of dependency.path) {
1414-
name += `.${path}`;
1414+
name += `.${path.property}`;
14151415
}
14161416
}
14171417
return name;
@@ -1448,7 +1448,7 @@ function codegenDependency(
14481448
let object: t.Expression = convertIdentifier(dependency.identifier);
14491449
if (dependency.path !== null) {
14501450
for (const path of dependency.path) {
1451-
object = t.memberExpression(object, t.identifier(path));
1451+
object = t.memberExpression(object, t.identifier(path.property));
14521452
}
14531453
}
14541454
return object;

compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/DeriveMinimalDependencies.ts

Lines changed: 14 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -14,20 +14,8 @@ import {assertExhaustive} from '../Utils/utils';
1414
* We need to understand optional member expressions only when determining
1515
* dependencies of a ReactiveScope (i.e. in {@link PropagateScopeDependencies}),
1616
* hence why this type lives here (not in HIR.ts)
17-
*
18-
* {@link ReactiveScopePropertyDependency.optionalPath} is populated only if the Property
19-
* represents an optional member expression, and it represents the property path
20-
* loaded conditionally.
21-
* e.g. the member expr a.b.c?.d.e?.f is represented as
22-
* {
23-
* identifier: 'a';
24-
* path: ['b', 'c'],
25-
* optionalPath: ['d', 'e', 'f'].
26-
* }
2717
*/
28-
export type ReactiveScopePropertyDependency = ReactiveScopeDependency & {
29-
optionalPath: Array<string>;
30-
};
18+
export type ReactiveScopePropertyDependency = ReactiveScopeDependency;
3119

3220
/*
3321
* Finalizes a set of ReactiveScopeDependencies to produce a set of minimal unconditional
@@ -69,59 +57,29 @@ export class ReactiveScopeDependencyTree {
6957
}
7058

7159
add(dep: ReactiveScopePropertyDependency, inConditional: boolean): void {
72-
const {path, optionalPath} = dep;
60+
const {path} = dep;
7361
let currNode = this.#getOrCreateRoot(dep.identifier);
7462

7563
const accessType = inConditional
7664
? PropertyAccessType.ConditionalAccess
7765
: PropertyAccessType.UnconditionalAccess;
7866

79-
for (const property of path) {
67+
for (const item of path) {
8068
// all properties read 'on the way' to a dependency are marked as 'access'
81-
let currChild = getOrMakeProperty(currNode, property);
69+
let currChild = getOrMakeProperty(currNode, item.property);
8270
currChild.accessType = merge(currChild.accessType, accessType);
8371
currNode = currChild;
8472
}
8573

86-
if (optionalPath.length === 0) {
87-
/*
88-
* If this property does not have a conditional path (i.e. a.b.c), the
89-
* final property node should be marked as an conditional/unconditional
90-
* `dependency` as based on control flow.
91-
*/
92-
const depType = inConditional
93-
? PropertyAccessType.ConditionalDependency
94-
: PropertyAccessType.UnconditionalDependency;
95-
96-
currNode.accessType = merge(currNode.accessType, depType);
97-
} else {
98-
/*
99-
* Technically, we only depend on whether unconditional path `dep.path`
100-
* is nullish (not its actual value). As long as we preserve the nullthrows
101-
* behavior of `dep.path`, we can keep it as an access (and not promote
102-
* to a dependency).
103-
* See test `reduce-reactive-cond-memberexpr-join` for example.
104-
*/
105-
106-
/*
107-
* If this property has an optional path (i.e. a?.b.c), all optional
108-
* nodes should be marked accordingly.
109-
*/
110-
for (const property of optionalPath) {
111-
let currChild = getOrMakeProperty(currNode, property);
112-
currChild.accessType = merge(
113-
currChild.accessType,
114-
PropertyAccessType.ConditionalAccess,
115-
);
116-
currNode = currChild;
117-
}
74+
/**
75+
* The final property node should be marked as an conditional/unconditional
76+
* `dependency` as based on control flow.
77+
*/
78+
const depType = inConditional
79+
? PropertyAccessType.ConditionalDependency
80+
: PropertyAccessType.UnconditionalDependency;
11881

119-
// The final node should be marked as a conditional dependency.
120-
currNode.accessType = merge(
121-
currNode.accessType,
122-
PropertyAccessType.ConditionalDependency,
123-
);
124-
}
82+
currNode.accessType = merge(currNode.accessType, depType);
12583
}
12684

12785
deriveMinimalDependencies(): Set<ReactiveScopeDependency> {
@@ -294,7 +252,7 @@ type DependencyNode = {
294252
};
295253

296254
type ReduceResultNode = {
297-
relativePath: Array<string>;
255+
relativePath: Array<{property: string}>;
298256
accessType: PropertyAccessType;
299257
};
300258

@@ -325,7 +283,7 @@ function deriveMinimalDependenciesInSubtree(
325283
const childResult = deriveMinimalDependenciesInSubtree(childNode).map(
326284
({relativePath, accessType}) => {
327285
return {
328-
relativePath: [childName, ...relativePath],
286+
relativePath: [{property: childName}, ...relativePath],
329287
accessType,
330288
};
331289
},

compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/MergeReactiveScopesThatInvalidateTogether.ts

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import {
1919
ReactiveScopeDependency,
2020
ReactiveStatement,
2121
Type,
22+
areEqualPaths,
2223
makeInstructionId,
2324
} from '../HIR';
2425
import {
@@ -525,10 +526,6 @@ function areEqualDependencies(
525526
return true;
526527
}
527528

528-
export function areEqualPaths(a: Array<string>, b: Array<string>): boolean {
529-
return a.length === b.length && a.every((item, ix) => item === b[ix]);
530-
}
531-
532529
/**
533530
* Is this scope eligible for merging with subsequent scopes? In general this
534531
* is only true if the scope's output values are guaranteed to change when its

compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PrintReactiveFunction.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,7 @@ export function printDependency(dependency: ReactiveScopeDependency): string {
113113
const identifier =
114114
printIdentifier(dependency.identifier) +
115115
printType(dependency.identifier.type);
116-
return `${identifier}${dependency.path.map(prop => `.${prop}`).join('')}`;
116+
return `${identifier}${dependency.path.map(token => `.${token.property}`).join('')}`;
117117
}
118118

119119
export function printReactiveInstructions(

compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PropagateScopeDependencies.ts

Lines changed: 5 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77

88
import {CompilerError} from '../CompilerError';
99
import {
10+
areEqualPaths,
1011
BlockId,
1112
DeclarationId,
1213
GeneratedSource,
@@ -35,7 +36,6 @@ import {
3536
ReactiveScopeDependencyTree,
3637
ReactiveScopePropertyDependency,
3738
} from './DeriveMinimalDependencies';
38-
import {areEqualPaths} from './MergeReactiveScopesThatInvalidateTogether';
3939
import {ReactiveFunctionVisitor, visitReactiveFunction} from './visitors';
4040

4141
/*
@@ -465,7 +465,6 @@ class Context {
465465
#getProperty(
466466
object: Place,
467467
property: string,
468-
isConditional: boolean,
469468
): ReactiveScopePropertyDependency {
470469
const resolvedObject = this.resolveTemporary(object);
471470
const resolvedDependency = this.#properties.get(resolvedObject.identifier);
@@ -478,36 +477,21 @@ class Context {
478477
objectDependency = {
479478
identifier: resolvedObject.identifier,
480479
path: [],
481-
optionalPath: [],
482480
};
483481
} else {
484482
objectDependency = {
485483
identifier: resolvedDependency.identifier,
486484
path: [...resolvedDependency.path],
487-
optionalPath: [...resolvedDependency.optionalPath],
488485
};
489486
}
490487

491-
// (2) Determine whether property is an optional access
492-
if (objectDependency.optionalPath.length > 0) {
493-
/*
494-
* If the base property dependency represents a optional member expression,
495-
* property is on the optionalPath (regardless of whether this PropertyLoad
496-
* itself was conditional)
497-
* e.g. for `a.b?.c.d`, `d` should be added to optionalPath
498-
*/
499-
objectDependency.optionalPath.push(property);
500-
} else if (isConditional) {
501-
objectDependency.optionalPath.push(property);
502-
} else {
503-
objectDependency.path.push(property);
504-
}
488+
objectDependency.path.push({property});
505489

506490
return objectDependency;
507491
}
508492

509493
declareProperty(lvalue: Place, object: Place, property: string): void {
510-
const nextDependency = this.#getProperty(object, property, false);
494+
const nextDependency = this.#getProperty(object, property);
511495
this.#properties.set(lvalue.identifier, nextDependency);
512496
}
513497

@@ -516,7 +500,7 @@ class Context {
516500
// ref.current access is not a valid dep
517501
if (
518502
isUseRefType(maybeDependency.identifier) &&
519-
maybeDependency.path.at(0) === 'current'
503+
maybeDependency.path.at(0)?.property === 'current'
520504
) {
521505
return false;
522506
}
@@ -577,7 +561,6 @@ class Context {
577561
let dependency: ReactiveScopePropertyDependency = {
578562
identifier: resolved.identifier,
579563
path: [],
580-
optionalPath: [],
581564
};
582565
if (resolved.identifier.name === null) {
583566
const propertyDependency = this.#properties.get(resolved.identifier);
@@ -589,7 +572,7 @@ class Context {
589572
}
590573

591574
visitProperty(object: Place, property: string): void {
592-
const nextDependency = this.#getProperty(object, property, false);
575+
const nextDependency = this.#getProperty(object, property);
593576
this.visitDependency(nextDependency);
594577
}
595578

compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneInitializationDependencies.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -180,8 +180,8 @@ class Visitor extends ReactiveFunctionVisitor<CreateUpdate> {
180180
[...scope.scope.dependencies].forEach(ident => {
181181
let target: undefined | IdentifierId =
182182
this.aliases.find(ident.identifier.id) ?? ident.identifier.id;
183-
ident.path.forEach(key => {
184-
target &&= this.paths.get(target)?.get(key);
183+
ident.path.forEach(token => {
184+
target &&= this.paths.get(target)?.get(token.property);
185185
});
186186
if (target && this.map.get(target) === 'Create') {
187187
scope.scope.dependencies.delete(ident);

compiler/packages/babel-plugin-react-compiler/src/Validation/ValidatePreservedManualMemoization.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -167,7 +167,7 @@ function compareDeps(
167167

168168
let isSubpath = true;
169169
for (let i = 0; i < Math.min(inferred.path.length, source.path.length); i++) {
170-
if (inferred.path[i] !== source.path[i]) {
170+
if (inferred.path[i].property !== source.path[i].property) {
171171
isSubpath = false;
172172
break;
173173
}
@@ -177,14 +177,14 @@ function compareDeps(
177177
isSubpath &&
178178
(source.path.length === inferred.path.length ||
179179
(inferred.path.length >= source.path.length &&
180-
!inferred.path.includes('current')))
180+
!inferred.path.some(token => token.property === 'current')))
181181
) {
182182
return CompareDependencyResult.Ok;
183183
} else {
184184
if (isSubpath) {
185185
if (
186-
source.path.includes('current') ||
187-
inferred.path.includes('current')
186+
source.path.some(token => token.property === 'current') ||
187+
inferred.path.some(token => token.property === 'current')
188188
) {
189189
return CompareDependencyResult.RefAccessDifference;
190190
} else {

0 commit comments

Comments
 (0)