From af91a7ab863c1990be28b7b0db8bc1b0227c82b9 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Tue, 20 Dec 2022 11:06:08 -0800 Subject: [PATCH] Ensure member path assignments memoize independently Assignment expressions to a member path are a special case because they're the only place where a value isn't assigned to a (possibly temporary) variable, which is our unit of memoization. #901 demonstrated how this can lead to values that can't be independently memoized: ```javascript const x = {a: a} x.y = [b, c]; // array recomputed w `x`, even if only `a` changed ``` This PR ensures that assignment expressions where the LHS is a member path lower the RHS to a Place. That means the above example is handled as if you wrote: ```javascript const x = {a: a}; const tmp1 = [b, c]; x.y = tmp1; ``` And we independently memoize the temporary. --- compiler/forget/src/HIR/BuildHIR.ts | 5 +- ...endently-memoize-object-property.expect.md | 131 ------------------ ...g_independently-memoize-object-property.js | 13 -- .../hir/assignment-variations.expect.md | 26 ++-- ...endently-memoize-object-property.expect.md | 84 +++++++++++ .../independently-memoize-object-property.js | 7 + 6 files changed, 109 insertions(+), 157 deletions(-) delete mode 100644 compiler/forget/src/__tests__/fixtures/hir/_bug_independently-memoize-object-property.expect.md delete mode 100644 compiler/forget/src/__tests__/fixtures/hir/_bug_independently-memoize-object-property.js create mode 100644 compiler/forget/src/__tests__/fixtures/hir/independently-memoize-object-property.expect.md create mode 100644 compiler/forget/src/__tests__/fixtures/hir/independently-memoize-object-property.js diff --git a/compiler/forget/src/HIR/BuildHIR.ts b/compiler/forget/src/HIR/BuildHIR.ts index 2ff8d420c4..6d79b759d4 100644 --- a/compiler/forget/src/HIR/BuildHIR.ts +++ b/compiler/forget/src/HIR/BuildHIR.ts @@ -946,7 +946,10 @@ function lowerExpression( const operator = expr.node.operator; if (operator === "=") { - const right = lowerExpression(builder, expr.get("right")); + const right = + left.memberPath === null + ? lowerExpression(builder, expr.get("right")) + : lowerExpressionToPlace(builder, expr.get("right")); builder.push({ id: makeInstructionId(0), lvalue: { place: left, kind: InstructionKind.Reassign }, diff --git a/compiler/forget/src/__tests__/fixtures/hir/_bug_independently-memoize-object-property.expect.md b/compiler/forget/src/__tests__/fixtures/hir/_bug_independently-memoize-object-property.expect.md deleted file mode 100644 index 9bb7e57af5..0000000000 --- a/compiler/forget/src/__tests__/fixtures/hir/_bug_independently-memoize-object-property.expect.md +++ /dev/null @@ -1,131 +0,0 @@ - -## Input - -```javascript -function foo(a, b, c) { - const x = { a: a }; - // TODO @josephsavona: this array *should* be memoized independently from `x`, - // similar to the behavior if we extract into a variable as with `z` below: - x.y = [b, c]; - - const y = { a: a }; - // this array correctly memoizes independently - const z = [b, c]; - y.y = z; - - return [x, y]; -} - -``` - -## HIR - -``` -bb0: - [1] Const mutate x$11_@0:TObject[1:3] = Object { a: read a$8 } - [2] Reassign mutate x$11_@0.y[1:3] = Array [read b$9, read c$10] - [3] Const mutate y$12_@1:TObject[3:6] = Object { a: read a$8 } - [4] Const mutate z$13_@2 = Array [read b$9, read c$10] - [5] Reassign mutate y$12_@1.y[3:6] = read z$13_@2 - [6] Const mutate t13$14_@3 = Array [read x$11_@0:TObject, read y$12_@1:TObject] - [7] Return freeze t13$14_@3 -``` - -## Reactive Scopes - -``` -function foo( - a, - b, - c, -) { - scope @0 [1:3] deps=[read a$8, read b$9, read c$10] out=[x$11_@0] { - [1] Const mutate x$11_@0:TObject[1:3] = Object { a: read a$8 } - [2] Reassign mutate x$11_@0.y[1:3] = Array [read b$9, read c$10] - } - scope @1 [3:6] deps=[read a$8, read b$9, read c$10] out=[y$12_@1] { - [3] Const mutate y$12_@1:TObject[3:6] = Object { a: read a$8 } - scope @2 [4:5] deps=[read b$9, read c$10] out=[z$13_@2] { - [4] Const mutate z$13_@2 = Array [read b$9, read c$10] - } - [5] Reassign mutate y$12_@1.y[3:6] = read z$13_@2 - } - scope @3 [6:7] deps=[read x$11_@0:TObject, read y$12_@1:TObject] out=[$14_@3] { - [6] Const mutate $14_@3 = Array [read x$11_@0:TObject, read y$12_@1:TObject] - } - return freeze $14_@3 -} - -``` - -## Code - -```javascript -function foo$0(a$8, b$9, c$10) { - const $ = React.useMemoCache(); - const c_0 = $[0] !== a$8; - const c_1 = $[1] !== b$9; - const c_2 = $[2] !== c$10; - let x$11; - if (c_0 || c_1 || c_2) { - x$11 = { - a: a$8, - }; - x$11.y = [b$9, c$10]; - $[0] = a$8; - $[1] = b$9; - $[2] = c$10; - $[3] = x$11; - } else { - x$11 = $[3]; - } - - const c_4 = $[4] !== a$8; - const c_5 = $[5] !== b$9; - const c_6 = $[6] !== c$10; - let y$12; - - if (c_4 || c_5 || c_6) { - y$12 = { - a: a$8, - }; - const c_8 = $[8] !== b$9; - const c_9 = $[9] !== c$10; - let z$13; - - if (c_8 || c_9) { - z$13 = [b$9, c$10]; - $[8] = b$9; - $[9] = c$10; - $[10] = z$13; - } else { - z$13 = $[10]; - } - - y$12.y = z$13; - $[4] = a$8; - $[5] = b$9; - $[6] = c$10; - $[7] = y$12; - } else { - y$12 = $[7]; - } - - const c_11 = $[11] !== x$11; - const c_12 = $[12] !== y$12; - let t13$14; - - if (c_11 || c_12) { - t13$14 = [x$11, y$12]; - $[11] = x$11; - $[12] = y$12; - $[13] = t13$14; - } else { - t13$14 = $[13]; - } - - return t13$14; -} - -``` - \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/hir/_bug_independently-memoize-object-property.js b/compiler/forget/src/__tests__/fixtures/hir/_bug_independently-memoize-object-property.js deleted file mode 100644 index a8cf1a12f2..0000000000 --- a/compiler/forget/src/__tests__/fixtures/hir/_bug_independently-memoize-object-property.js +++ /dev/null @@ -1,13 +0,0 @@ -function foo(a, b, c) { - const x = { a: a }; - // TODO @josephsavona: this array *should* be memoized independently from `x`, - // similar to the behavior if we extract into a variable as with `z` below: - x.y = [b, c]; - - const y = { a: a }; - // this array correctly memoizes independently - const z = [b, c]; - y.y = z; - - return [x, y]; -} diff --git a/compiler/forget/src/__tests__/fixtures/hir/assignment-variations.expect.md b/compiler/forget/src/__tests__/fixtures/hir/assignment-variations.expect.md index e391fd7876..a65ad7961e 100644 --- a/compiler/forget/src/__tests__/fixtures/hir/assignment-variations.expect.md +++ b/compiler/forget/src/__tests__/fixtures/hir/assignment-variations.expect.md @@ -62,11 +62,12 @@ function f$0() { ``` bb0: - [1] Const mutate $5:TPrimitive = 1 - [2] Reassign mutate a$4_@0.b.c[0:5] = Binary read a$4_@0.b.c + read $5:TPrimitive - [3] Const mutate $6:TPrimitive = 2 - [4] Reassign mutate a$4_@0.b.c[0:5] = Binary read a$4_@0.b.c * read $6:TPrimitive - [5] Return + [1] Const mutate $6:TPrimitive = 1 + [2] Const mutate $7:TPrimitive = Binary read a$5_@0.b.c + read $6:TPrimitive + [3] Reassign read a$5_@0.b.c[0:6] = read $7:TPrimitive + [4] Const mutate $8:TPrimitive = 2 + [5] Reassign mutate a$5_@0.b.c[0:6] = Binary read a$5_@0.b.c * read $8:TPrimitive + [6] Return ``` ## Reactive Scopes @@ -75,10 +76,11 @@ bb0: function g( a, ) { - [1] Const mutate $5:TPrimitive = 1 - [2] Reassign mutate a$4_@0.b.c[0:5] = Binary read a$4_@0.b.c + read $5:TPrimitive - [3] Const mutate $6:TPrimitive = 2 - [4] Reassign mutate a$4_@0.b.c[0:5] = Binary read a$4_@0.b.c * read $6:TPrimitive + [1] Const mutate $6:TPrimitive = 1 + [2] Const mutate $7:TPrimitive = Binary read a$5_@0.b.c + read $6:TPrimitive + [3] Reassign read a$5_@0.b.c[0:6] = read $7:TPrimitive + [4] Const mutate $8:TPrimitive = 2 + [5] Reassign mutate a$5_@0.b.c[0:6] = Binary read a$5_@0.b.c * read $8:TPrimitive return } @@ -87,9 +89,9 @@ function g( ## Code ```javascript -function g$0(a$4) { - a$4.c.b = a$4.b.c + 1; - a$4.c.b = a$4.b.c * 2; +function g$0(a$5) { + a$5.c.b = a$5.b.c + 1; + a$5.c.b = a$5.b.c * 2; } ``` diff --git a/compiler/forget/src/__tests__/fixtures/hir/independently-memoize-object-property.expect.md b/compiler/forget/src/__tests__/fixtures/hir/independently-memoize-object-property.expect.md new file mode 100644 index 0000000000..825db2fe3d --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/hir/independently-memoize-object-property.expect.md @@ -0,0 +1,84 @@ + +## Input + +```javascript +function foo(a, b, c) { + const x = { a: a }; + // NOTE: this array should memoize independently from x, w only b,c as deps + x.y = [b, c]; + + return x; +} + +``` + +## HIR + +``` +bb0: + [1] Const mutate x$9_@0:TObject[1:4] = Object { a: read a$6 } + [2] Const mutate t6$10_@1 = Array [read b$7, read c$8] + [3] Reassign mutate x$9_@0.y[1:4] = read t6$10_@1 + [4] Return freeze x$9_@0:TObject +``` + +## Reactive Scopes + +``` +function foo( + a, + b, + c, +) { + scope @0 [1:4] deps=[read a$6, read b$7, read c$8] out=[x$9_@0] { + [1] Const mutate x$9_@0:TObject[1:4] = Object { a: read a$6 } + scope @1 [2:3] deps=[read b$7, read c$8] out=[$10_@1] { + [2] Const mutate $10_@1 = Array [read b$7, read c$8] + } + [3] Reassign mutate x$9_@0.y[1:4] = read $10_@1 + } + return freeze x$9_@0:TObject +} + +``` + +## Code + +```javascript +function foo$0(a$6, b$7, c$8) { + const $ = React.useMemoCache(); + const c_0 = $[0] !== a$6; + const c_1 = $[1] !== b$7; + const c_2 = $[2] !== c$8; + let x$9; + if (c_0 || c_1 || c_2) { + x$9 = { + a: a$6, + }; + const c_4 = $[4] !== b$7; + const c_5 = $[5] !== c$8; + let t6$10; + + if (c_4 || c_5) { + t6$10 = [b$7, c$8]; + $[4] = b$7; + $[5] = c$8; + $[6] = t6$10; + } else { + t6$10 = $[6]; + } + + x$9.y = t6$10; + $[0] = a$6; + $[1] = b$7; + $[2] = c$8; + $[3] = x$9; + } else { + x$9 = $[3]; + } + + return x$9; +} + +``` + \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/hir/independently-memoize-object-property.js b/compiler/forget/src/__tests__/fixtures/hir/independently-memoize-object-property.js new file mode 100644 index 0000000000..e76dce6c91 --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/hir/independently-memoize-object-property.js @@ -0,0 +1,7 @@ +function foo(a, b, c) { + const x = { a: a }; + // NOTE: this array should memoize independently from x, w only b,c as deps + x.y = [b, c]; + + return x; +}