From 4a67d539dc723e3353d8aaca3ff065f83a448d8b Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Fri, 21 Apr 2023 08:03:37 -0700 Subject: [PATCH] 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. --- compiler/forget/src/HIR/HIR.ts | 7 ++- .../ReactiveScopes/CodegenReactiveFunction.ts | 12 +++--- .../ReactiveScopes/PrintReactiveFunction.ts | 4 +- .../ReactiveScopes/PromoteUsedTemporaries.ts | 6 +-- .../PropagateScopeDependencies.ts | 13 +++--- .../PruneNonReactiveDependencies.ts | 4 +- .../src/ReactiveScopes/PruneUnusedScopes.ts | 20 ++++++++- .../fixtures/compiler/dependencies.expect.md | 43 +++++++------------ 8 files changed, 62 insertions(+), 47 deletions(-) diff --git a/compiler/forget/src/HIR/HIR.ts b/compiler/forget/src/HIR/HIR.ts index 6a08bf6d4f..5f5e5b9db4 100644 --- a/compiler/forget/src/HIR/HIR.ts +++ b/compiler/forget/src/HIR/HIR.ts @@ -827,10 +827,15 @@ export type ReactiveScope = { id: ScopeId; range: MutableRange; dependencies: Set; - declarations: Map; + declarations: Map; reassignments: Set; }; +export type ReactiveScopeDeclaration = { + identifier: Identifier; + scope: ReactiveScope; // the scope in which the variable was originally declared +}; + export type ReactiveScopeDependency = { identifier: Identifier; path: Array; diff --git a/compiler/forget/src/ReactiveScopes/CodegenReactiveFunction.ts b/compiler/forget/src/ReactiveScopes/CodegenReactiveFunction.ts index d78ec70946..4d6f152c91 100644 --- a/compiler/forget/src/ReactiveScopes/CodegenReactiveFunction.ts +++ b/compiler/forget/src/ReactiveScopes/CodegenReactiveFunction.ts @@ -191,20 +191,20 @@ function codegenReactiveScope( ); } let firstOutputIndex: number | null = null; - for (const [, declaration] of scope.declarations) { + for (const [, { identifier }] of scope.declarations) { const index = cx.nextCacheIndex; if (firstOutputIndex === null) { firstOutputIndex = index; } invariant( - declaration.name != null, + identifier.name != null, "Expected identifier '@%s' to be named", - declaration.id + identifier.id ); - const name = convertIdentifier(declaration); - if (!cx.hasDeclared(declaration)) { + const name = convertIdentifier(identifier); + if (!cx.hasDeclared(identifier)) { statements.push( t.variableDeclaration("let", [t.variableDeclarator(name)]) ); @@ -227,7 +227,7 @@ function codegenReactiveScope( ) ) ); - cx.declare(declaration); + cx.declare(identifier); } for (const reassignment of scope.reassignments) { const index = cx.nextCacheIndex; diff --git a/compiler/forget/src/ReactiveScopes/PrintReactiveFunction.ts b/compiler/forget/src/ReactiveScopes/PrintReactiveFunction.ts index ec4785f5c8..ee3c490f69 100644 --- a/compiler/forget/src/ReactiveScopes/PrintReactiveFunction.ts +++ b/compiler/forget/src/ReactiveScopes/PrintReactiveFunction.ts @@ -47,7 +47,9 @@ export function printReactiveBlock( }] dependencies=[${Array.from(block.scope.dependencies) .map((dep) => printDependency(dep)) .join(", ")}] declarations=[${Array.from(block.scope.declarations) - .map(([, decl]) => printIdentifier(decl)) + .map(([, decl]) => + printIdentifier({ ...decl.identifier, scope: decl.scope }) + ) .join(", ")}] reassignments=[${Array.from(block.scope.reassignments).map( (reassign) => printIdentifier(reassign) )}] {` diff --git a/compiler/forget/src/ReactiveScopes/PromoteUsedTemporaries.ts b/compiler/forget/src/ReactiveScopes/PromoteUsedTemporaries.ts index e5578622f8..fd32580943 100644 --- a/compiler/forget/src/ReactiveScopes/PromoteUsedTemporaries.ts +++ b/compiler/forget/src/ReactiveScopes/PromoteUsedTemporaries.ts @@ -29,9 +29,9 @@ class Visitor extends ReactiveFunctionVisitor { // value. // Many of our current test fixtures do not return a value, so // it is better for now to promote (and memoize) every output. - for (const [, identifier] of block.scope.declarations) { - if (identifier.name == null) { - identifier.name = `t${state.nextId++}`; + for (const [, declaration] of block.scope.declarations) { + if (declaration.identifier.name == null) { + declaration.identifier.name = `t${state.nextId++}`; } } } diff --git a/compiler/forget/src/ReactiveScopes/PropagateScopeDependencies.ts b/compiler/forget/src/ReactiveScopes/PropagateScopeDependencies.ts index 21448a06b6..60d15876f3 100644 --- a/compiler/forget/src/ReactiveScopes/PropagateScopeDependencies.ts +++ b/compiler/forget/src/ReactiveScopes/PropagateScopeDependencies.ts @@ -317,13 +317,16 @@ class Context { const originalDeclaration = this.#declarations.get( maybeDependency.identifier.id ); - if (originalDeclaration !== undefined) { + if ( + originalDeclaration !== undefined && + originalDeclaration.scope.value !== null + ) { originalDeclaration.scope.each((scope) => { if (!this.#isScopeActive(scope)) { - scope.declarations.set( - maybeDependency.identifier.id, - maybeDependency.identifier - ); + scope.declarations.set(maybeDependency.identifier.id, { + identifier: maybeDependency.identifier, + scope: originalDeclaration.scope.value!, // checked above + }); } }); } diff --git a/compiler/forget/src/ReactiveScopes/PruneNonReactiveDependencies.ts b/compiler/forget/src/ReactiveScopes/PruneNonReactiveDependencies.ts index df482b3cc8..91b7db4cf6 100644 --- a/compiler/forget/src/ReactiveScopes/PruneNonReactiveDependencies.ts +++ b/compiler/forget/src/ReactiveScopes/PruneNonReactiveDependencies.ts @@ -34,12 +34,12 @@ class Visitor extends ReactiveFunctionVisitor { if (scope.scope.dependencies.size === 0) { // If a scope has no dependencies, then its declarations are all non-reactive for (const [, declaration] of scope.scope.declarations) { - state.delete(declaration.id); + state.delete(declaration.identifier.id); } } else { // otherwise, all the scope's declarations are reactive for (const [, declaration] of scope.scope.declarations) { - state.add(declaration.id); + state.add(declaration.identifier.id); } } } diff --git a/compiler/forget/src/ReactiveScopes/PruneUnusedScopes.ts b/compiler/forget/src/ReactiveScopes/PruneUnusedScopes.ts index 6e2787c11f..a4fa2e7fd1 100644 --- a/compiler/forget/src/ReactiveScopes/PruneUnusedScopes.ts +++ b/compiler/forget/src/ReactiveScopes/PruneUnusedScopes.ts @@ -30,8 +30,11 @@ class Transform extends ReactiveFunctionTransform { ): Transformed { this.visitScope(scopeBlock, state); if ( - scopeBlock.scope.declarations.size === 0 && - scopeBlock.scope.reassignments.size === 0 + scopeBlock.scope.reassignments.size === 0 && + (scopeBlock.scope.declarations.size === 0 || + // Can prune scopes where all declarations bubbled up from inner + // scopes + !hasOwnDeclaration(scopeBlock)) ) { return { kind: "replace-many", value: scopeBlock.instructions }; } else { @@ -39,3 +42,16 @@ class Transform extends ReactiveFunctionTransform { } } } + +/** + * Does the scope block declare any values of its own? This can return + * false if all the block's declarations are propagated from nested scopes. + */ +function hasOwnDeclaration(block: ReactiveScopeBlock): boolean { + for (const declaration of block.scope.declarations.values()) { + if (declaration.scope.id === block.scope.id) { + return true; + } + } + return false; +} diff --git a/compiler/forget/src/__tests__/fixtures/compiler/dependencies.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/dependencies.expect.md index f9e0a011c2..6ad952df97 100644 --- a/compiler/forget/src/__tests__/fixtures/compiler/dependencies.expect.md +++ b/compiler/forget/src/__tests__/fixtures/compiler/dependencies.expect.md @@ -25,36 +25,25 @@ function foo(x, y, z) { ```javascript import * as React from "react"; function foo(x, y, z) { - const $ = React.unstable_useMemoCache(7); - const c_0 = $[0] !== z; - const c_1 = $[1] !== x; - const c_2 = $[2] !== y; + const $ = React.unstable_useMemoCache(3); + const items = [z]; + items.push(x); + const c_0 = $[0] !== x; + const c_1 = $[1] !== y; let items2; - if (c_0 || c_1 || c_2) { - const items = [z]; - items.push(x); - const c_4 = $[4] !== x; - const c_5 = $[5] !== y; - if (c_4 || c_5) { - items2 = []; - if (x) { - items2.push(y); - } - $[4] = x; - $[5] = y; - $[6] = items2; - } else { - items2 = $[6]; + if (c_0 || c_1) { + items2 = []; + if (x) { + items2.push(y); } - if (y) { - items.push(x); - } - $[0] = z; - $[1] = x; - $[2] = y; - $[3] = items2; + $[0] = x; + $[1] = y; + $[2] = items2; } else { - items2 = $[3]; + items2 = $[2]; + } + if (y) { + items.push(x); } return items2; }