Prune scopes wo own outputs
#1507 Ensured that declarations of reactive scopes were propagated to parent reactive scopes as necessary to ensure that those declarations would be available at the appropriate block scope. This meant that some scopes that were previously pruned would no longer be pruned. Specifically, an outer scope wo any declarations, but which contained a nested scope _with_ a propagated declaration, would now end up with non-empty declarations and not be pruned. This PR changes to track the declaring scope of each declaration, so we still prune scopes that don't have any of their own declarations.
Joe Savona committed
Apr 21, 2023 at 08:03 UTC
4a67d539dc723e3353d8aaca3ff065f83a448d8b
8 files changed
+62
-47
compiler/forget/src/HIR/HIR.ts
+6
-1
@@ -827,10 +827,15 @@ export type ReactiveScope = {
827
id: ScopeId;
828
range: MutableRange;
829
dependencies: Set<ReactiveScopeDependency>;
830
- declarations: Map<IdentifierId, Identifier>;
830
+ declarations: Map<IdentifierId, ReactiveScopeDeclaration>;
831
reassignments: Set<Identifier>;
832
};
833
834
+export type ReactiveScopeDeclaration = {
835
+ identifier: Identifier;
836
+ scope: ReactiveScope; // the scope in which the variable was originally declared
837
+};
838
+
839
export type ReactiveScopeDependency = {
840
identifier: Identifier;
841
path: Array<string>;
compiler/forget/src/ReactiveScopes/CodegenReactiveFunction.ts
+6
-6
@@ -191,20 +191,20 @@ function codegenReactiveScope(
191
);
192
}
193
let firstOutputIndex: number | null = null;
194
- for (const [, declaration] of scope.declarations) {
194
+ for (const [, { identifier }] of scope.declarations) {
195
const index = cx.nextCacheIndex;
196
if (firstOutputIndex === null) {
197
firstOutputIndex = index;
198
}
199
200
invariant(
201
- declaration.name != null,
201
+ identifier.name != null,
202
"Expected identifier '@%s' to be named",
203
- declaration.id
203
+ identifier.id
204
);
205
206
- const name = convertIdentifier(declaration);
207
- if (!cx.hasDeclared(declaration)) {
206
+ const name = convertIdentifier(identifier);
207
+ if (!cx.hasDeclared(identifier)) {
208
statements.push(
209
t.variableDeclaration("let", [t.variableDeclarator(name)])
210
);
@@ -227,7 +227,7 @@ function codegenReactiveScope(
227
)
228
)
229
);
230
- cx.declare(declaration);
230
+ cx.declare(identifier);
231
}
232
for (const reassignment of scope.reassignments) {
233
const index = cx.nextCacheIndex;
compiler/forget/src/ReactiveScopes/PrintReactiveFunction.ts
+3
-1
@@ -47,7 +47,9 @@ export function printReactiveBlock(
47
}] dependencies=[${Array.from(block.scope.dependencies)
48
.map((dep) => printDependency(dep))
49
.join(", ")}] declarations=[${Array.from(block.scope.declarations)
50
- .map(([, decl]) => printIdentifier(decl))
50
+ .map(([, decl]) =>
51
+ printIdentifier({ ...decl.identifier, scope: decl.scope })
52
+ )
53
.join(", ")}] reassignments=[${Array.from(block.scope.reassignments).map(
54
(reassign) => printIdentifier(reassign)
55
)}] {`
compiler/forget/src/ReactiveScopes/PromoteUsedTemporaries.ts
+3
-3
@@ -29,9 +29,9 @@ class Visitor extends ReactiveFunctionVisitor<VisitorState> {
29
// value.
30
// Many of our current test fixtures do not return a value, so
31
// it is better for now to promote (and memoize) every output.
32
- for (const [, identifier] of block.scope.declarations) {
33
- if (identifier.name == null) {
34
- identifier.name = `t${state.nextId++}`;
32
+ for (const [, declaration] of block.scope.declarations) {
33
+ if (declaration.identifier.name == null) {
34
+ declaration.identifier.name = `t${state.nextId++}`;
35
}
36
}
37
}
compiler/forget/src/ReactiveScopes/PropagateScopeDependencies.ts
+8
-5
@@ -317,13 +317,16 @@ class Context {
317
const originalDeclaration = this.#declarations.get(
318
maybeDependency.identifier.id
319
);
320
- if (originalDeclaration !== undefined) {
320
+ if (
321
+ originalDeclaration !== undefined &&
322
+ originalDeclaration.scope.value !== null
323
+ ) {
324
originalDeclaration.scope.each((scope) => {
325
if (!this.#isScopeActive(scope)) {
323
- scope.declarations.set(
324
- maybeDependency.identifier.id,
325
- maybeDependency.identifier
326
- );
326
+ scope.declarations.set(maybeDependency.identifier.id, {
327
+ identifier: maybeDependency.identifier,
328
+ scope: originalDeclaration.scope.value!, // checked above
329
+ });
330
}
331
});
332
}
compiler/forget/src/ReactiveScopes/PruneNonReactiveDependencies.ts
+2
-2
@@ -34,12 +34,12 @@ class Visitor extends ReactiveFunctionVisitor<State> {
34
if (scope.scope.dependencies.size === 0) {
35
// If a scope has no dependencies, then its declarations are all non-reactive
36
for (const [, declaration] of scope.scope.declarations) {
37
- state.delete(declaration.id);
37
+ state.delete(declaration.identifier.id);
38
}
39
} else {
40
// otherwise, all the scope's declarations are reactive
41
for (const [, declaration] of scope.scope.declarations) {
42
- state.add(declaration.id);
42
+ state.add(declaration.identifier.id);
43
}
44
}
45
}
compiler/forget/src/ReactiveScopes/PruneUnusedScopes.ts
+18
-2
@@ -30,8 +30,11 @@ class Transform extends ReactiveFunctionTransform<void> {
30
): Transformed<ReactiveStatement> {
31
this.visitScope(scopeBlock, state);
32
if (
33
- scopeBlock.scope.declarations.size === 0 &&
34
- scopeBlock.scope.reassignments.size === 0
33
+ scopeBlock.scope.reassignments.size === 0 &&
34
+ (scopeBlock.scope.declarations.size === 0 ||
35
+ // Can prune scopes where all declarations bubbled up from inner
36
+ // scopes
37
+ !hasOwnDeclaration(scopeBlock))
38
) {
39
return { kind: "replace-many", value: scopeBlock.instructions };
40
} else {
@@ -39,3 +42,16 @@ class Transform extends ReactiveFunctionTransform<void> {
42
}
43
}
44
}
45
+
46
+/**
47
+ * Does the scope block declare any values of its own? This can return
48
+ * false if all the block's declarations are propagated from nested scopes.
49
+ */
50
+function hasOwnDeclaration(block: ReactiveScopeBlock): boolean {
51
+ for (const declaration of block.scope.declarations.values()) {
52
+ if (declaration.scope.id === block.scope.id) {
53
+ return true;
54
+ }
55
+ }
56
+ return false;
57
+}
compiler/forget/src/__tests__/fixtures/compiler/dependencies.expect.md
+16
-27
@@ -25,36 +25,25 @@ function foo(x, y, z) {
25
```javascript
26
import * as React from "react";
27
function foo(x, y, z) {
28
- const $ = React.unstable_useMemoCache(7);
29
- const c_0 = $[0] !== z;
30
- const c_1 = $[1] !== x;
31
- const c_2 = $[2] !== y;
28
+ const $ = React.unstable_useMemoCache(3);
29
+ const items = [z];
30
+ items.push(x);
31
+ const c_0 = $[0] !== x;
32
+ const c_1 = $[1] !== y;
33
let items2;
33
- if (c_0 || c_1 || c_2) {
34
- const items = [z];
35
- items.push(x);
36
- const c_4 = $[4] !== x;
37
- const c_5 = $[5] !== y;
38
- if (c_4 || c_5) {
39
- items2 = [];
40
- if (x) {
41
- items2.push(y);
42
- }
43
- $[4] = x;
44
- $[5] = y;
45
- $[6] = items2;
46
- } else {
47
- items2 = $[6];
48
- }
49
- if (y) {
50
- items.push(x);
34
+ if (c_0 || c_1) {
35
+ items2 = [];
36
+ if (x) {
37
+ items2.push(y);
38
}
52
- $[0] = z;
53
- $[1] = x;
54
- $[2] = y;
55
- $[3] = items2;
39
+ $[0] = x;
40
+ $[1] = y;
41
+ $[2] = items2;
42
} else {
57
- items2 = $[3];
43
+ items2 = $[2];
44
+ }
45
+ if (y) {
46
+ items.push(x);
47
}
48
return items2;
49
}