From a0777212df7009cc26e47ffd46444276d0be923d Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Tue, 18 Apr 2023 21:27:11 -0700 Subject: [PATCH] Propagate scope declarations to parent scopes Fix for bug demonstrated in #1506. When we add variables as output of their defining scope, we need to propagate this information upwards to all parent scopes which are not current active. --- .../PropagateScopeDependencies.ts | 54 +++++++------ compiler/forget/src/Utils/Stack.ts | 79 ++++++++++++++++++ .../fixtures/compiler/dependencies.expect.md | 45 +++++++---- ...promoted-to-outer-scope-dynamic.expect.md} | 80 ++++++++++--------- ...ue-not-promoted-to-outer-scope-dynamic.js} | 0 ...-promoted-to-outer-scope-static.expect.md} | 42 +++++----- ...lue-not-promoted-to-outer-scope-static.js} | 0 ...eps-join-uncond-scopes-cond-deps.expect.md | 34 ++++---- 8 files changed, 217 insertions(+), 117 deletions(-) create mode 100644 compiler/forget/src/Utils/Stack.ts rename compiler/forget/src/__tests__/fixtures/compiler/{_bug.inner-memo-value-not-promoted-to-outer-scope-dynamic.expect.md => inner-memo-value-not-promoted-to-outer-scope-dynamic.expect.md} (57%) rename compiler/forget/src/__tests__/fixtures/compiler/{_bug.inner-memo-value-not-promoted-to-outer-scope-dynamic.js => inner-memo-value-not-promoted-to-outer-scope-dynamic.js} (100%) rename compiler/forget/src/__tests__/fixtures/compiler/{_bug.inner-memo-value-not-promoted-to-outer-scope-static.expect.md => inner-memo-value-not-promoted-to-outer-scope-static.expect.md} (74%) rename compiler/forget/src/__tests__/fixtures/compiler/{_bug.inner-memo-value-not-promoted-to-outer-scope-static.js => inner-memo-value-not-promoted-to-outer-scope-static.js} (100%) diff --git a/compiler/forget/src/ReactiveScopes/PropagateScopeDependencies.ts b/compiler/forget/src/ReactiveScopes/PropagateScopeDependencies.ts index 1ded68ab50..eb135753ba 100644 --- a/compiler/forget/src/ReactiveScopes/PropagateScopeDependencies.ts +++ b/compiler/forget/src/ReactiveScopes/PropagateScopeDependencies.ts @@ -23,6 +23,7 @@ import { eachInstructionValueOperand, eachPatternOperand, } from "../HIR/visitors"; +import { empty, Stack } from "../Utils/Stack"; import { assertExhaustive } from "../Utils/utils"; import { ReactiveScopeDependencyTree, @@ -40,13 +41,13 @@ export function propagateScopeDependencies(fn: ReactiveFunction): void { if (fn.id !== null) { context.declare(fn.id, { id: makeInstructionId(0), - scope: null, + scope: empty(), }); } for (const param of fn.params) { context.declare(param.identifier, { id: makeInstructionId(0), - scope: null, + scope: empty(), }); } visit(context, fn.body); @@ -55,11 +56,9 @@ export function propagateScopeDependencies(fn: ReactiveFunction): void { type DeclMap = Map; type Decl = { id: InstructionId; - scope: ReactiveScope | null; + scope: Stack; }; -type Scopes = Array; - class Context { #declarations: DeclMap = new Map(); #reassignments: Map = new Map(); @@ -80,7 +79,7 @@ class Context { // - accessed by all cfg branches (added through promoteDeps) #depsInCurrentConditional: ReactiveScopeDependencyTree = new ReactiveScopeDependencyTree(); - #scopes: Scopes = []; + #scopes: Stack = empty(); enter(scope: ReactiveScope, fn: () => void): Set { // Save context of previous scope @@ -94,12 +93,12 @@ class Context { const scopedDependencies = new ReactiveScopeDependencyTree(); this.#inConditionalWithinScope = false; this.#dependencies = scopedDependencies; - this.#scopes.push(scope); + this.#scopes = this.#scopes.push(scope); fn(); // Restore context of previous scope - this.#scopes.pop(); + this.#scopes = this.#scopes.pop(); this.#dependencies = previousDependencies; this.#inConditionalWithinScope = prevInConditional; @@ -246,22 +245,25 @@ class Context { const currentDeclaration = this.#reassignments.get(identifier) ?? this.#declarations.get(identifier.id); - const currentScope = this.currentScope; + const currentScope = this.#scopes !== null ? this.#scopes.value : null; return ( currentScope != null && currentDeclaration !== undefined && currentDeclaration.id < currentScope.range.start && (currentDeclaration.scope == null || - currentDeclaration.scope !== currentScope) + currentDeclaration.scope.value !== currentScope) ); } #isScopeActive(scope: ReactiveScope): boolean { - return this.#scopes.indexOf(scope) !== -1; + if (this.#scopes === null) { + return false; + } + return this.#scopes.contains(scope); } - get currentScope(): ReactiveScope | null { - return this.#scopes.at(-1) ?? null; + get currentScope(): Stack { + return this.#scopes; } visitOperand(place: Place): void { @@ -300,15 +302,15 @@ class Context { const originalDeclaration = this.#declarations.get( maybeDependency.identifier.id ); - if ( - originalDeclaration !== undefined && - originalDeclaration.scope !== null && - !this.#isScopeActive(originalDeclaration.scope) - ) { - originalDeclaration.scope.declarations.set( - maybeDependency.identifier.id, - maybeDependency.identifier - ); + if (originalDeclaration !== undefined) { + originalDeclaration.scope.each((scope) => { + if (!this.#isScopeActive(scope)) { + scope.declarations.set( + maybeDependency.identifier.id, + maybeDependency.identifier + ); + } + }); } if (this.#checkValidDependencyId(maybeDependency.identifier)) { @@ -326,15 +328,15 @@ class Context { visitReassignment(place: Place): void { const declaration = this.#declarations.get(place.identifier.id); if ( - this.currentScope != null && + this.currentScope.value != null && place.identifier.scope != null && declaration !== undefined && - declaration.scope !== place.identifier.scope && - !Array.from(this.currentScope.reassignments).some( + declaration.scope.value !== place.identifier.scope && + !Array.from(this.currentScope.value.reassignments).some( (ident) => ident.id === place.identifier.id ) ) { - this.currentScope.reassignments.add(place.identifier); + this.currentScope.value.reassignments.add(place.identifier); } } } diff --git a/compiler/forget/src/Utils/Stack.ts b/compiler/forget/src/Utils/Stack.ts new file mode 100644 index 0000000000..68b2df059d --- /dev/null +++ b/compiler/forget/src/Utils/Stack.ts @@ -0,0 +1,79 @@ +/** + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +export interface Stack { + push(value: T): Stack; + + pop(): Stack; + + contains(value: T): boolean; + + each(fn: (value: T) => void): void; + + get value(): T | null; +} + +export function create(value: T): Stack { + return new Node(value); +} + +export function empty(): Stack { + return EMPTY as any; +} + +class Node implements Stack { + #value: T; + #next: Stack; + + constructor(value: T, next: Stack = EMPTY as any) { + this.#value = value; + this.#next = next; + } + + push(value: T): Node { + return new Node(value, this); + } + + pop(): Stack { + return this.#next; + } + + contains(value: T): boolean { + return ( + value === this.#value || + (this.#next !== null && this.#next.contains(value)) + ); + } + each(fn: (value: T) => void): void { + fn(this.#value); + this.#next.each(fn); + } + + get value(): T { + return this.#value; + } +} + +class Empty implements Stack { + push(value: T): Stack { + return new Node(value, this); + } + pop(): Stack { + return this; + } + contains(_value: T): boolean { + return false; + } + each(_fn: (value: T) => void): void { + return; + } + get value(): T | null { + return null; + } +} + +const EMPTY: Stack = new Empty(); diff --git a/compiler/forget/src/__tests__/fixtures/compiler/dependencies.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/dependencies.expect.md index 6ad952df97..f9e0a011c2 100644 --- a/compiler/forget/src/__tests__/fixtures/compiler/dependencies.expect.md +++ b/compiler/forget/src/__tests__/fixtures/compiler/dependencies.expect.md @@ -25,25 +25,36 @@ function foo(x, y, z) { ```javascript import * as React from "react"; function foo(x, y, z) { - const $ = React.unstable_useMemoCache(3); - const items = [z]; - items.push(x); - const c_0 = $[0] !== x; - const c_1 = $[1] !== y; + const $ = React.unstable_useMemoCache(7); + const c_0 = $[0] !== z; + const c_1 = $[1] !== x; + const c_2 = $[2] !== y; let items2; - if (c_0 || c_1) { - items2 = []; - if (x) { - items2.push(y); - } - $[0] = x; - $[1] = y; - $[2] = items2; - } else { - items2 = $[2]; - } - if (y) { + 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 (y) { + items.push(x); + } + $[0] = z; + $[1] = x; + $[2] = y; + $[3] = items2; + } else { + items2 = $[3]; } return items2; } diff --git a/compiler/forget/src/__tests__/fixtures/compiler/_bug.inner-memo-value-not-promoted-to-outer-scope-dynamic.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/inner-memo-value-not-promoted-to-outer-scope-dynamic.expect.md similarity index 57% rename from compiler/forget/src/__tests__/fixtures/compiler/_bug.inner-memo-value-not-promoted-to-outer-scope-dynamic.expect.md rename to compiler/forget/src/__tests__/fixtures/compiler/inner-memo-value-not-promoted-to-outer-scope-dynamic.expect.md index 03af913e61..5449ffe010 100644 --- a/compiler/forget/src/__tests__/fixtures/compiler/_bug.inner-memo-value-not-promoted-to-outer-scope-dynamic.expect.md +++ b/compiler/forget/src/__tests__/fixtures/compiler/inner-memo-value-not-promoted-to-outer-scope-dynamic.expect.md @@ -24,7 +24,7 @@ function Component(props) { ```javascript import * as React from "react"; function Component(props) { - const $ = React.unstable_useMemoCache(21); + const $ = React.unstable_useMemoCache(23); const item = useFragment(FRAGMENT, props.item); useFreeze(item); const c_0 = $[0] !== item; @@ -32,6 +32,7 @@ function Component(props) { let t2; let t3; let t4; + let t0; let t5; let t6; let t7; @@ -42,12 +43,11 @@ function Component(props) { t7 = "\n "; t3 = View; t4 = "\n "; - let t0; - if ($[8] === Symbol.for("react.memo_cache_sentinel")) { + if ($[9] === Symbol.for("react.memo_cache_sentinel")) { t0 = Text; - $[8] = t0; + $[9] = t0; } else { - t0 = $[8]; + t0 = $[9]; } t5 = "\n "; t1 = "span"; @@ -57,35 +57,38 @@ function Component(props) { $[2] = t2; $[3] = t3; $[4] = t4; - $[5] = t5; - $[6] = t6; - $[7] = t7; + $[5] = t0; + $[6] = t5; + $[7] = t6; + $[8] = t7; } else { t1 = $[1]; t2 = $[2]; t3 = $[3]; t4 = $[4]; - t5 = $[5]; - t6 = $[6]; - t7 = $[7]; + t0 = $[5]; + t5 = $[6]; + t6 = $[7]; + t7 = $[8]; } - const c_9 = $[9] !== t1; - const c_10 = $[10] !== t2; + const c_10 = $[10] !== t1; + const c_11 = $[11] !== t2; let t8; - if (c_9 || c_10) { + if (c_10 || c_11) { t8 = {t2}; - $[9] = t1; - $[10] = t2; - $[11] = t8; + $[10] = t1; + $[11] = t2; + $[12] = t8; } else { - t8 = $[11]; + t8 = $[12]; } - const c_12 = $[12] !== t3; - const c_13 = $[13] !== t4; - const c_14 = $[14] !== t5; - const c_15 = $[15] !== t8; + const c_13 = $[13] !== t3; + const c_14 = $[14] !== t4; + const c_15 = $[15] !== t0; + const c_16 = $[16] !== t5; + const c_17 = $[17] !== t8; let t9; - if (c_12 || c_13 || c_14 || c_15) { + if (c_13 || c_14 || c_15 || c_16 || c_17) { t9 = ( {t4} @@ -94,31 +97,32 @@ function Component(props) { {t8} ); - $[12] = t3; - $[13] = t4; - $[14] = t5; - $[15] = t8; - $[16] = t9; + $[13] = t3; + $[14] = t4; + $[15] = t0; + $[16] = t5; + $[17] = t8; + $[18] = t9; } else { - t9 = $[16]; + t9 = $[18]; } - const c_17 = $[17] !== t6; - const c_18 = $[18] !== t7; - const c_19 = $[19] !== t9; + const c_19 = $[19] !== t6; + const c_20 = $[20] !== t7; + const c_21 = $[21] !== t9; let t10; - if (c_17 || c_18 || c_19) { + if (c_19 || c_20 || c_21) { t10 = ( {t7} {t9} ); - $[17] = t6; - $[18] = t7; - $[19] = t9; - $[20] = t10; + $[19] = t6; + $[20] = t7; + $[21] = t9; + $[22] = t10; } else { - t10 = $[20]; + t10 = $[22]; } return t10; } diff --git a/compiler/forget/src/__tests__/fixtures/compiler/_bug.inner-memo-value-not-promoted-to-outer-scope-dynamic.js b/compiler/forget/src/__tests__/fixtures/compiler/inner-memo-value-not-promoted-to-outer-scope-dynamic.js similarity index 100% rename from compiler/forget/src/__tests__/fixtures/compiler/_bug.inner-memo-value-not-promoted-to-outer-scope-dynamic.js rename to compiler/forget/src/__tests__/fixtures/compiler/inner-memo-value-not-promoted-to-outer-scope-dynamic.js diff --git a/compiler/forget/src/__tests__/fixtures/compiler/_bug.inner-memo-value-not-promoted-to-outer-scope-static.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/inner-memo-value-not-promoted-to-outer-scope-static.expect.md similarity index 74% rename from compiler/forget/src/__tests__/fixtures/compiler/_bug.inner-memo-value-not-promoted-to-outer-scope-static.expect.md rename to compiler/forget/src/__tests__/fixtures/compiler/inner-memo-value-not-promoted-to-outer-scope-static.expect.md index 193962c826..ea89b86a49 100644 --- a/compiler/forget/src/__tests__/fixtures/compiler/_bug.inner-memo-value-not-promoted-to-outer-scope-static.expect.md +++ b/compiler/forget/src/__tests__/fixtures/compiler/inner-memo-value-not-promoted-to-outer-scope-static.expect.md @@ -21,11 +21,12 @@ function Component(props) { ```javascript import * as React from "react"; function Component(props) { - const $ = React.unstable_useMemoCache(11); + const $ = React.unstable_useMemoCache(12); let t1; let t2; let t3; let t4; + let t0; let t5; let t6; let t7; @@ -36,12 +37,11 @@ function Component(props) { t7 = "\n "; t3 = View; t4 = "\n "; - let t0; - if ($[7] === Symbol.for("react.memo_cache_sentinel")) { + if ($[8] === Symbol.for("react.memo_cache_sentinel")) { t0 = Text; - $[7] = t0; + $[8] = t0; } else { - t0 = $[7]; + t0 = $[8]; } t5 = "\n "; t1 = "span"; @@ -50,27 +50,29 @@ function Component(props) { $[1] = t2; $[2] = t3; $[3] = t4; - $[4] = t5; - $[5] = t6; - $[6] = t7; + $[4] = t0; + $[5] = t5; + $[6] = t6; + $[7] = t7; } else { t1 = $[0]; t2 = $[1]; t3 = $[2]; t4 = $[3]; - t5 = $[4]; - t6 = $[5]; - t7 = $[6]; + t0 = $[4]; + t5 = $[5]; + t6 = $[6]; + t7 = $[7]; } let t8; - if ($[8] === Symbol.for("react.memo_cache_sentinel")) { + if ($[9] === Symbol.for("react.memo_cache_sentinel")) { t8 = {t2}; - $[8] = t8; + $[9] = t8; } else { - t8 = $[8]; + t8 = $[9]; } let t9; - if ($[9] === Symbol.for("react.memo_cache_sentinel")) { + if ($[10] === Symbol.for("react.memo_cache_sentinel")) { t9 = ( {t4} @@ -79,21 +81,21 @@ function Component(props) { {t8} ); - $[9] = t9; + $[10] = t9; } else { - t9 = $[9]; + t9 = $[10]; } let t10; - if ($[10] === Symbol.for("react.memo_cache_sentinel")) { + if ($[11] === Symbol.for("react.memo_cache_sentinel")) { t10 = ( {t7} {t9} ); - $[10] = t10; + $[11] = t10; } else { - t10 = $[10]; + t10 = $[11]; } return t10; } diff --git a/compiler/forget/src/__tests__/fixtures/compiler/_bug.inner-memo-value-not-promoted-to-outer-scope-static.js b/compiler/forget/src/__tests__/fixtures/compiler/inner-memo-value-not-promoted-to-outer-scope-static.js similarity index 100% rename from compiler/forget/src/__tests__/fixtures/compiler/_bug.inner-memo-value-not-promoted-to-outer-scope-static.js rename to compiler/forget/src/__tests__/fixtures/compiler/inner-memo-value-not-promoted-to-outer-scope-static.js diff --git a/compiler/forget/src/__tests__/fixtures/compiler/reduce-reactive-deps-join-uncond-scopes-cond-deps.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/reduce-reactive-deps-join-uncond-scopes-cond-deps.expect.md index 3a294d07f0..76e3f89f03 100644 --- a/compiler/forget/src/__tests__/fixtures/compiler/reduce-reactive-deps-join-uncond-scopes-cond-deps.expect.md +++ b/compiler/forget/src/__tests__/fixtures/compiler/reduce-reactive-deps-join-uncond-scopes-cond-deps.expect.md @@ -52,40 +52,42 @@ import * as React from "react"; // This tests an optimization, NOT a correctness // } function TestJoinCondDepsInUncondScopes(props) { - const $ = React.unstable_useMemoCache(7); + const $ = React.unstable_useMemoCache(8); const c_0 = $[0] !== props.a.b; + let x; let y; if (c_0) { y = {}; - const c_2 = $[2] !== props; - let x; - if (c_2) { + const c_3 = $[3] !== props; + if (c_3) { x = {}; if (foo) { mutate1(x, props.a.b); } - $[2] = props; - $[3] = x; + $[3] = props; + $[4] = x; } else { - x = $[3]; + x = $[4]; } mutate2(y, props.a.b); $[0] = props.a.b; - $[1] = y; + $[1] = x; + $[2] = y; } else { - y = $[1]; + x = $[1]; + y = $[2]; } - const c_4 = $[4] !== x; - const c_5 = $[5] !== y; + const c_5 = $[5] !== x; + const c_6 = $[6] !== y; let t0; - if (c_4 || c_5) { + if (c_5 || c_6) { t0 = [x, y]; - $[4] = x; - $[5] = y; - $[6] = t0; + $[5] = x; + $[6] = y; + $[7] = t0; } else { - t0 = $[6]; + t0 = $[7]; } return t0; }