[be] Clarify naming in scope merging pass
Anytime we have a nested `.scope` in code my brain hurts. For example `scope.scope.dependencies`. This PR updates the scope merging pass to use the name `scopeBlock` for a ReactiveScopeBlock and `scope` only for ReactiveScope values, to make things a bit more clear.
Joe Savona committed
Nov 9, 2023 at 16:33 UTC
4742d27f9e80aa2ffcc11b94978e4d4eb0936769
1 file changed
+38
-30
compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/MergeReactiveScopesThatInvalidateTogether.ts
+38
-30
@@ -110,15 +110,15 @@ class Transform extends ReactiveFunctionTransform<ReactiveScopeDependencies | nu
110
}
111
112
override transformScope(
113
- scope: ReactiveScopeBlock,
113
+ scopeBlock: ReactiveScopeBlock,
114
state: ReactiveScopeDependencies | null
115
): Transformed<ReactiveStatement> {
116
- this.visitScope(scope, scope.scope.dependencies);
116
+ this.visitScope(scopeBlock, scopeBlock.scope.dependencies);
117
if (
118
state !== null &&
119
- areEqualDependencies(state, scope.scope.dependencies)
119
+ areEqualDependencies(state, scopeBlock.scope.dependencies)
120
) {
121
- return { kind: "replace-many", value: scope.instructions };
121
+ return { kind: "replace-many", value: scopeBlock.instructions };
122
} else {
123
return { kind: "keep" };
124
}
@@ -133,7 +133,7 @@ class Transform extends ReactiveFunctionTransform<ReactiveScopeDependencies | nu
133
134
// Pass 2: identify scopes for merging
135
type MergedScope = {
136
- scope: ReactiveScopeBlock;
136
+ block: ReactiveScopeBlock;
137
from: number;
138
to: number;
139
lvalues: Set<IdentifierId>;
@@ -160,7 +160,7 @@ class Transform extends ReactiveFunctionTransform<ReactiveScopeDependencies | nu
160
// For now we don't merge across terminals
161
if (current !== null) {
162
log(
163
- `Reset scope @${current.scope.scope.id} from terminal [${instr.terminal.id}]`
163
+ `Reset scope @${current.block.scope.id} from terminal [${instr.terminal.id}]`
164
);
165
reset();
166
}
@@ -190,7 +190,7 @@ class Transform extends ReactiveFunctionTransform<ReactiveScopeDependencies | nu
190
// Other instructions are known to prevent merging, so we reset the scope if present
191
if (current !== null) {
192
log(
193
- `Reset scope @${current.scope.scope.id} from instruction [${instr.instruction.id}]`
193
+ `Reset scope @${current.block.scope.id} from instruction [${instr.instruction.id}]`
194
);
195
reset();
196
}
@@ -201,7 +201,7 @@ class Transform extends ReactiveFunctionTransform<ReactiveScopeDependencies | nu
201
case "scope": {
202
if (
203
current !== null &&
204
- canMergeScopes(current.scope, instr) &&
204
+ canMergeScopes(current.block, instr) &&
205
areLValuesLastUsedByScope(
206
instr.scope,
207
current.lvalues,
@@ -210,21 +210,21 @@ class Transform extends ReactiveFunctionTransform<ReactiveScopeDependencies | nu
210
) {
211
// The current and next scopes can merge!
212
log(
213
- `Can merge scope @${current.scope.scope.id} with @${instr.scope.id}`
213
+ `Can merge scope @${current.block.scope.id} with @${instr.scope.id}`
214
);
215
// Update the merged scope's range
216
- current.scope.scope.range.end = makeInstructionId(
217
- Math.max(current.scope.scope.range.end, instr.scope.range.end)
216
+ current.block.scope.range.end = makeInstructionId(
217
+ Math.max(current.block.scope.range.end, instr.scope.range.end)
218
);
219
// Add declarations
220
for (const [key, value] of instr.scope.declarations) {
221
- current.scope.scope.declarations.set(key, value);
221
+ current.block.scope.declarations.set(key, value);
222
}
223
/*
224
* Then prune declarations - this removes declarations from the earlier
225
* scope that are last-used at or before the newly merged subsequent scope
226
*/
227
- updateScopeDeclarations(current.scope.scope, this.lastUsage);
227
+ updateScopeDeclarations(current.block.scope, this.lastUsage);
228
current.to = i + 1;
229
/*
230
* We already checked that intermediate values were used at-or-before the merged
@@ -247,14 +247,14 @@ class Transform extends ReactiveFunctionTransform<ReactiveScopeDependencies | nu
247
if (current !== null) {
248
// Reset if necessary
249
log(
250
- `Reset scope @${current.scope.scope.id}, not mergeable with subsequent scope @${instr.scope.id}`
250
+ `Reset scope @${current.block.scope.id}, not mergeable with subsequent scope @${instr.scope.id}`
251
);
252
reset();
253
}
254
// Only set a new merge candidate if the scope is guaranteed to invalidate on changes
255
if (scopeIsEligibleForMerging(instr)) {
256
current = {
257
- scope: instr,
257
+ block: instr,
258
from: i,
259
to: i + 1,
260
lvalues: new Set(),
@@ -282,7 +282,7 @@ class Transform extends ReactiveFunctionTransform<ReactiveScopeDependencies | nu
282
log(`merged ${merged.length} scopes:`);
283
for (const entry of merged) {
284
log(
285
- printReactiveScopeSummary(entry.scope.scope) +
285
+ printReactiveScopeSummary(entry.block.scope) +
286
` from=${entry.from} to=${entry.to}`
287
);
288
}
@@ -363,14 +363,22 @@ function areLValuesLastUsedByScope(
363
return true;
364
}
365
366
-function canMergeScopes(a: ReactiveScopeBlock, b: ReactiveScopeBlock): boolean {
366
+function canMergeScopes(
367
+ current: ReactiveScopeBlock,
368
+ next: ReactiveScopeBlock
369
+): boolean {
370
// Don't merge scopes with reassignments
368
- if (a.scope.reassignments.size !== 0 || b.scope.reassignments.size !== 0) {
371
+ if (
372
+ current.scope.reassignments.size !== 0 ||
373
+ next.scope.reassignments.size !== 0
374
+ ) {
375
log(` cannot merge, has reassignments`);
376
return false;
377
}
378
// Merge scopes whose dependencies are identical
373
- if (areEqualDependencies(a.scope.dependencies, b.scope.dependencies)) {
379
+ if (
380
+ areEqualDependencies(current.scope.dependencies, next.scope.dependencies)
381
+ ) {
382
log(` canMergeScopes: dependencies are equal`);
383
return true;
384
}
@@ -386,20 +394,20 @@ function canMergeScopes(a: ReactiveScopeBlock, b: ReactiveScopeBlock): boolean {
394
if (
395
areEqualDependencies(
396
new Set(
389
- [...a.scope.declarations.values()].map((declaration) => ({
397
+ [...current.scope.declarations.values()].map((declaration) => ({
398
identifier: declaration.identifier,
399
path: [],
400
}))
401
),
394
- b.scope.dependencies
402
+ next.scope.dependencies
403
)
404
) {
405
log(` outputs of prev are input to current`);
406
return true;
407
}
408
log(` cannot merge scopes:`);
401
- log(` ${printReactiveScopeSummary(a.scope)}`);
402
- log(` ${printReactiveScopeSummary(b.scope)}`);
409
+ log(` ${printReactiveScopeSummary(current.scope)}`);
410
+ log(` ${printReactiveScopeSummary(next.scope)}`);
411
return false;
412
}
413
@@ -442,16 +450,16 @@ function areEqualPaths(a: Array<string>, b: Array<string>): boolean {
450
* A special-case is if the scope has no dependencies, then its output will
451
* *never* change and it's also eligible for merging.
452
*/
445
-function scopeIsEligibleForMerging(scope: ReactiveScopeBlock): boolean {
446
- if (scope.scope.dependencies.size === 0) {
453
+function scopeIsEligibleForMerging(scopeBlock: ReactiveScopeBlock): boolean {
454
+ if (scopeBlock.scope.dependencies.size === 0) {
455
/*
456
* Regardless of the type of value produced, if the scope has no dependencies
457
* then its value will never change.
458
*/
459
return true;
460
}
453
- const visitor = new DeclarationTypeVisitor(scope.scope);
454
- visitor.visitScope(scope, undefined);
461
+ const visitor = new DeclarationTypeVisitor(scopeBlock.scope);
462
+ visitor.visitScope(scopeBlock, undefined);
463
return visitor.alwaysInvalidatesOnInputChange;
464
}
465
@@ -464,11 +472,11 @@ class DeclarationTypeVisitor extends ReactiveFunctionVisitor<void> {
472
this.scope = scope;
473
}
474
467
- override visitScope(scope: ReactiveScopeBlock, state: void): void {
468
- if (scope.scope.id !== this.scope.id) {
475
+ override visitScope(scopeBlock: ReactiveScopeBlock, state: void): void {
476
+ if (scopeBlock.scope.id !== this.scope.id) {
477
return;
478
}
471
- this.traverseScope(scope, state);
479
+ this.traverseScope(scopeBlock, state);
480
}
481
482
override visitInstruction(