From f4896b45b2ff4c7f10bf710c9c9d2b87d6352e0d Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Thu, 20 Apr 2023 18:52:39 -0700 Subject: [PATCH] Fix temporaries accessed outside of their defining scope Fixed a bug identified in repro cases earlier in the stack. The case is where some later value is composed of several values, say A and B, where A is an identifier that is reassigned within B. Also, the mutable range of B surrounds the evaluation of A. In this case, the reference to A gets lowered to a temporary (say a t0 = LoadLocal A), and that temporary is created within the reactive scope for B. PropagateScopeDependencies bypasses LoadLocal indirections, and considers the reference to the temporary (t0) as if it was a reference to the identifier (A). That breaks the whole reason we lower Identifiers to temporaries - to preserve evaluation order. This PR fixes the bug by promoting temporaries to names values if they are referenced outside their defining scope. So, the reference to t0 stays a reference to t0, which correctly preserves the value of A at the right point in time. This is all much easier to see in the new test case. --- .../PropagateScopeDependencies.ts | 25 ++++-- .../error.jsx-tag-evaluation-order.expect.md | 28 ------ ...-tag-evaluation-order-non-global.expect.md | 87 +++++++++++++++++++ ...=> jsx-tag-evaluation-order-non-global.js} | 0 ...temporary-accessed-outside-scope.expect.md | 49 +++++++++++ ...signed-temporary-accessed-outside-scope.js | 5 ++ 6 files changed, 161 insertions(+), 33 deletions(-) delete mode 100644 compiler/forget/src/__tests__/fixtures/compiler/error.jsx-tag-evaluation-order.expect.md create mode 100644 compiler/forget/src/__tests__/fixtures/compiler/jsx-tag-evaluation-order-non-global.expect.md rename compiler/forget/src/__tests__/fixtures/compiler/{error.jsx-tag-evaluation-order.js => jsx-tag-evaluation-order-non-global.js} (100%) create mode 100644 compiler/forget/src/__tests__/fixtures/compiler/reassigned-temporary-accessed-outside-scope.expect.md create mode 100644 compiler/forget/src/__tests__/fixtures/compiler/reassigned-temporary-accessed-outside-scope.js diff --git a/compiler/forget/src/ReactiveScopes/PropagateScopeDependencies.ts b/compiler/forget/src/ReactiveScopes/PropagateScopeDependencies.ts index eb135753ba..21448a06b6 100644 --- a/compiler/forget/src/ReactiveScopes/PropagateScopeDependencies.ts +++ b/compiler/forget/src/ReactiveScopes/PropagateScopeDependencies.ts @@ -71,7 +71,8 @@ class Context { // ReactiveScope (B) that uses the produced temporary. // - codegen will inline these PropertyLoads back into scope (B) #properties: Map = new Map(); - #temporaries: Map = new Map(); + #temporaries: Map = + new Map(); #inConditionalWithinScope: boolean = false; // Reactive dependencies used unconditionally in the current conditional. // Composed of dependencies: @@ -184,8 +185,22 @@ class Context { this.#reassignments.set(identifier, decl); } - declareTemporary(lvalue: Place, value: Place): void { - this.#temporaries.set(lvalue.identifier, value); + declareTemporary(lvalue: Place, place: Place): void { + this.#temporaries.set(lvalue.identifier, { + place, + scope: this.currentScope.value, + }); + } + + resolveTemporary(place: Place): Place { + const temporary = this.#temporaries.get(place.identifier); + if ( + temporary !== undefined && + (temporary.scope === null || this.#isScopeActive(temporary.scope)) + ) { + return temporary.place; + } + return place; } #getProperty( @@ -193,7 +208,7 @@ class Context { property: string, isConditional: boolean ): ReactiveScopePropertyDependency { - const resolvedObject = this.#temporaries.get(object.identifier) ?? object; + const resolvedObject = this.resolveTemporary(object); const resolvedDependency = this.#properties.get(resolvedObject.identifier); let objectDependency: ReactiveScopePropertyDependency; // (1) Create the base property dependency as either a LoadLocal (from a temporary) @@ -267,7 +282,7 @@ class Context { } visitOperand(place: Place): void { - const resolved = this.#temporaries.get(place.identifier) ?? place; + const resolved = this.resolveTemporary(place); // if this operand is a temporary created for a property load, try to resolve it to // the expanded Place. Fall back to using the operand as-is. diff --git a/compiler/forget/src/__tests__/fixtures/compiler/error.jsx-tag-evaluation-order.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/error.jsx-tag-evaluation-order.expect.md deleted file mode 100644 index 81e5631cf7..0000000000 --- a/compiler/forget/src/__tests__/fixtures/compiler/error.jsx-tag-evaluation-order.expect.md +++ /dev/null @@ -1,28 +0,0 @@ - -## Input - -```javascript -function Component(props) { - const maybeMutable = new MaybeMutable(); - let Tag = props.component; - // NOTE: the order of evaluation in the lowering is incorrect: - // the jsx element's tag observes `Tag` after reassignment, but should observe - // it before the reassignment. - return ( - - {((Tag = props.alternateComponent), maybeMutate(maybeMutable))} - - - ); -} - -``` - - -## Error - -``` -[ReactForget] Invariant: [Codegen] No value found for temporary. Value for 'read $33' was not set in the codegen context (8:8) -``` - - \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/compiler/jsx-tag-evaluation-order-non-global.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/jsx-tag-evaluation-order-non-global.expect.md new file mode 100644 index 0000000000..01e001bd57 --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/compiler/jsx-tag-evaluation-order-non-global.expect.md @@ -0,0 +1,87 @@ + +## Input + +```javascript +function Component(props) { + const maybeMutable = new MaybeMutable(); + let Tag = props.component; + // NOTE: the order of evaluation in the lowering is incorrect: + // the jsx element's tag observes `Tag` after reassignment, but should observe + // it before the reassignment. + return ( + + {((Tag = props.alternateComponent), maybeMutate(maybeMutable))} + + + ); +} + +``` + +## Code + +```javascript +import * as React from "react"; +function Component(props) { + const $ = React.unstable_useMemoCache(13); + const c_0 = $[0] !== props.component; + const c_1 = $[1] !== props.alternateComponent; + let Tag; + let t0; + let t1; + let t2; + if (c_0 || c_1) { + const maybeMutable = new MaybeMutable(); + Tag = props.component; + + t0 = Tag; + t1 = "\n "; + Tag = props.alternateComponent; + t2 = maybeMutate(maybeMutable); + $[0] = props.component; + $[1] = props.alternateComponent; + $[2] = Tag; + $[3] = t0; + $[4] = t1; + $[5] = t2; + } else { + Tag = $[2]; + t0 = $[3]; + t1 = $[4]; + t2 = $[5]; + } + const c_6 = $[6] !== Tag; + let t3; + if (c_6) { + t3 = ; + $[6] = Tag; + $[7] = t3; + } else { + t3 = $[7]; + } + const c_8 = $[8] !== t0; + const c_9 = $[9] !== t1; + const c_10 = $[10] !== t2; + const c_11 = $[11] !== t3; + let t4; + if (c_8 || c_9 || c_10 || c_11) { + t4 = ( + + {t1} + {t2} + {t3} + + ); + $[8] = t0; + $[9] = t1; + $[10] = t2; + $[11] = t3; + $[12] = t4; + } else { + t4 = $[12]; + } + return t4; +} + +``` + \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/compiler/error.jsx-tag-evaluation-order.js b/compiler/forget/src/__tests__/fixtures/compiler/jsx-tag-evaluation-order-non-global.js similarity index 100% rename from compiler/forget/src/__tests__/fixtures/compiler/error.jsx-tag-evaluation-order.js rename to compiler/forget/src/__tests__/fixtures/compiler/jsx-tag-evaluation-order-non-global.js diff --git a/compiler/forget/src/__tests__/fixtures/compiler/reassigned-temporary-accessed-outside-scope.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/reassigned-temporary-accessed-outside-scope.expect.md new file mode 100644 index 0000000000..4f8ecbcb72 --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/compiler/reassigned-temporary-accessed-outside-scope.expect.md @@ -0,0 +1,49 @@ + +## Input + +```javascript +function Component(props) { + const maybeMutable = new MaybeMutable(); + let x = props.value; + return [x, maybeMutate(maybeMutable)]; +} + +``` + +## Code + +```javascript +import * as React from "react"; +function Component(props) { + const $ = React.unstable_useMemoCache(6); + const c_0 = $[0] !== props.value; + let t0; + let t1; + if (c_0) { + const maybeMutable = new MaybeMutable(); + const x = props.value; + t0 = x; + t1 = maybeMutate(maybeMutable); + $[0] = props.value; + $[1] = t0; + $[2] = t1; + } else { + t0 = $[1]; + t1 = $[2]; + } + const c_3 = $[3] !== t0; + const c_4 = $[4] !== t1; + let t2; + if (c_3 || c_4) { + t2 = [t0, t1]; + $[3] = t0; + $[4] = t1; + $[5] = t2; + } else { + t2 = $[5]; + } + return t2; +} + +``` + \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/compiler/reassigned-temporary-accessed-outside-scope.js b/compiler/forget/src/__tests__/fixtures/compiler/reassigned-temporary-accessed-outside-scope.js new file mode 100644 index 0000000000..54eb92d540 --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/compiler/reassigned-temporary-accessed-outside-scope.js @@ -0,0 +1,5 @@ +function Component(props) { + const maybeMutable = new MaybeMutable(); + let x = props.value; + return [x, maybeMutate(maybeMutable)]; +}