From 46417a4ed77513d8e385ccd32e2103d5504bce4e Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Thu, 18 May 2023 09:25:28 -0700 Subject: [PATCH] Fix bug with partially memoized destructuring --- compiler/forget/src/CompilerPipeline.ts | 8 + compiler/forget/src/HIR/HIR.ts | 15 +- ...tractScopeDeclarationsFromDestructuring.ts | 184 ++++++++++++++++++ compiler/forget/src/ReactiveScopes/index.ts | 1 + ...and-local-variables-with-default.expect.md | 101 ++++++++++ ...scope-and-local-variables-with-default.js} | 17 +- ...ed-scope-declarations-and-locals.expect.md | 81 ++++++++ ...ng-mixed-scope-declarations-and-locals.js} | 0 ...alysis-destructured-rest-element.expect.md | 22 --- ....unused-object-element-with-rest.expect.md | 20 -- ...alysis-destructured-rest-element.expect.md | 56 ++++++ ...ape-analysis-destructured-rest-element.js} | 0 .../unused-object-element-with-rest.expect.md | 33 ++++ ....js => unused-object-element-with-rest.js} | 0 14 files changed, 474 insertions(+), 64 deletions(-) create mode 100644 compiler/forget/src/ReactiveScopes/ExtractScopeDeclarationsFromDestructuring.ts create mode 100644 compiler/forget/src/__tests__/fixtures/compiler/destructuring-mixed-scope-and-local-variables-with-default.expect.md rename compiler/forget/src/__tests__/fixtures/compiler/{error.destructuring-mixed-scope-declarations-and-locals.expect.md => destructuring-mixed-scope-and-local-variables-with-default.js} (73%) create mode 100644 compiler/forget/src/__tests__/fixtures/compiler/destructuring-mixed-scope-declarations-and-locals.expect.md rename compiler/forget/src/__tests__/fixtures/compiler/{error.destructuring-mixed-scope-declarations-and-locals.js => destructuring-mixed-scope-declarations-and-locals.js} (100%) delete mode 100644 compiler/forget/src/__tests__/fixtures/compiler/error.escape-analysis-destructured-rest-element.expect.md delete mode 100644 compiler/forget/src/__tests__/fixtures/compiler/error.unused-object-element-with-rest.expect.md create mode 100644 compiler/forget/src/__tests__/fixtures/compiler/escape-analysis-destructured-rest-element.expect.md rename compiler/forget/src/__tests__/fixtures/compiler/{error.escape-analysis-destructured-rest-element.js => escape-analysis-destructured-rest-element.js} (100%) create mode 100644 compiler/forget/src/__tests__/fixtures/compiler/unused-object-element-with-rest.expect.md rename compiler/forget/src/__tests__/fixtures/compiler/{error.unused-object-element-with-rest.js => unused-object-element-with-rest.js} (100%) diff --git a/compiler/forget/src/CompilerPipeline.ts b/compiler/forget/src/CompilerPipeline.ts index 983bc6bf19..96cc2b7f45 100644 --- a/compiler/forget/src/CompilerPipeline.ts +++ b/compiler/forget/src/CompilerPipeline.ts @@ -32,6 +32,7 @@ import { buildReactiveBlocks, buildReactiveFunction, codegenReactiveFunction, + extractScopeDeclarationsFromDestructuring, flattenReactiveLoops, flattenScopesWithHooks, inferReactiveScopeVariables, @@ -223,6 +224,13 @@ export function* run( value: reactiveFunction, }); + extractScopeDeclarationsFromDestructuring(reactiveFunction); + yield log({ + kind: "reactive", + name: "ExtractScopeDeclarationsFromDestructuring", + value: reactiveFunction, + }); + renameVariables(reactiveFunction); yield log({ kind: "reactive", diff --git a/compiler/forget/src/HIR/HIR.ts b/compiler/forget/src/HIR/HIR.ts index e181431211..89538ca2a3 100644 --- a/compiler/forget/src/HIR/HIR.ts +++ b/compiler/forget/src/HIR/HIR.ts @@ -585,12 +585,7 @@ export type InstructionValue = value: Place; loc: SourceLocation; } - | { - kind: "Destructure"; - lvalue: LValuePattern; - value: Place; - loc: SourceLocation; - } + | Destructure | { kind: "Primitive"; value: number | boolean | string | null | undefined; @@ -756,6 +751,14 @@ export type FunctionExpression = { expr: t.ArrowFunctionExpression | t.FunctionExpression; loc: SourceLocation; }; + +export type Destructure = { + kind: "Destructure"; + lvalue: LValuePattern; + value: Place; + loc: SourceLocation; +}; + /** * A place where data may be read from / written to: * - a variable (identifier) diff --git a/compiler/forget/src/ReactiveScopes/ExtractScopeDeclarationsFromDestructuring.ts b/compiler/forget/src/ReactiveScopes/ExtractScopeDeclarationsFromDestructuring.ts new file mode 100644 index 0000000000..22eae8e306 --- /dev/null +++ b/compiler/forget/src/ReactiveScopes/ExtractScopeDeclarationsFromDestructuring.ts @@ -0,0 +1,184 @@ +/** + * 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. + */ + +import { + Destructure, + Environment, + IdentifierId, + InstructionKind, + Place, + ReactiveBlock, + ReactiveFunction, + ReactiveInstruction, + ReactiveScopeBlock, +} from "../HIR"; +import { eachPatternOperand, mapPatternOperands } from "../HIR/visitors"; +import { ReactiveFunctionTransform, visitReactiveFunction } from "./visitors"; + +/** + * Destructuring statements may sometimes define some variables which are declared by the scope, + * and others that are only used locally within the scope, for example: + * + * ``` + * const {x, ...rest} = value; + * return rest; + * ``` + * + * Here the scope structure turns into: + * + * ``` + * let c_0 = $[0] !== value; + * let rest; + * if (c_0) { + * // OOPS! we want to reassign `rest` here, but + * // `x` isn't declared anywhere! + * {x, ...rest} = value; + * $[0] = value; + * $[1] = rest; + * } else { + * rest = $[1]; + * } + * return rest; + * ``` + * + * Note that because `rest` is declared by the scope, we can't redeclare it in the + * destructuring statement. But we have to declare `x`! + * + * This pass finds destructuring instructions that contain mixed values such as this, + * and rewrites them to ensure that any scope variable assignments are extracted first + * to a temporary and reassigned in a separate instruction. For example, the output + * for the above would be along the lines of: + * + * ``` + * let c_0 = $[0] !== value; + * let rest; + * if (c_0) { + * const {x, ...t0} = value; <-- replace `rest` with a temporary + * rest = t0; // <-- and create a separate instruction to assign that to `rest` + * $[0] = value; + * $[1] = rest; + * } else { + * rest = $[1]; + * } + * return rest; + * ``` + * + */ +export function extractScopeDeclarationsFromDestructuring( + fn: ReactiveFunction +): void { + const state = new State(fn.env); + visitReactiveFunction(fn, new Visitor(), state); +} + +class State { + env: Environment; + declared: Set = new Set(); + + constructor(env: Environment) { + this.env = env; + } +} + +class Visitor extends ReactiveFunctionTransform { + override visitScope(scope: ReactiveScopeBlock, state: State): void { + for (const [, declaration] of scope.scope.declarations) { + state.declared.add(declaration.identifier.id); + } + this.traverseScope(scope, state); + } + + override visitBlock(block: ReactiveBlock, state: State): void { + // Traverse first to transform inner items + this.traverseBlock(block, state); + + // Then transform any mixed destructuring instructions + let nextBlock: ReactiveBlock | null = null; + for (let i = 0; i < block.length; i++) { + const instr = block[i]; + if ( + instr.kind === "instruction" && + instr.instruction.value.kind === "Destructure" + ) { + const transformed = transformDestructuring( + state, + instr.instruction, + instr.instruction.value + ); + if (transformed) { + nextBlock ??= block.slice(0, i); + transformed.forEach((instruction) => { + nextBlock?.push({ + kind: "instruction", + instruction, + }); + }); + continue; + } + } else if (nextBlock !== null) { + nextBlock.push(instr); + } + } + if (nextBlock !== null) { + block.length = 0; + block.push(...nextBlock); + } + } +} + +function transformDestructuring( + state: State, + instr: ReactiveInstruction, + destructure: Destructure +): null | Array { + let reassigned: Set = new Set(); + let hasDeclaration = false; + for (const place of eachPatternOperand(destructure.lvalue.pattern)) { + const isDeclared = state.declared.has(place.identifier.id); + if (isDeclared) { + reassigned.add(place.identifier.id); + } + hasDeclaration ||= !isDeclared; + } + if (reassigned.size === 0 || !hasDeclaration) { + return null; + } + // Else it's a mix, replace the reassigned items in the destructuring with temporary + // variables and emit separate assignment statements for them + const instructions: Array = []; + const renamed: Map = new Map(); + mapPatternOperands(destructure.lvalue.pattern, (place) => { + if (!reassigned.has(place.identifier.id)) { + return place; + } + const tempId = state.env.nextIdentifierId; + const temporary = { + ...place, + identifier: { ...place.identifier, id: tempId, name: `t${tempId}` }, + }; + renamed.set(place, temporary); + return temporary; + }); + instructions.push(instr); + for (const [original, temporary] of renamed) { + instructions.push({ + id: instr.id, + lvalue: null, + value: { + kind: "StoreLocal", + lvalue: { + kind: InstructionKind.Reassign, + place: original, + }, + value: temporary, + loc: destructure.loc, + }, + loc: instr.loc, + }); + } + return instructions; +} diff --git a/compiler/forget/src/ReactiveScopes/index.ts b/compiler/forget/src/ReactiveScopes/index.ts index 8d042ee6e0..f3da369604 100644 --- a/compiler/forget/src/ReactiveScopes/index.ts +++ b/compiler/forget/src/ReactiveScopes/index.ts @@ -9,6 +9,7 @@ export { alignReactiveScopesToBlockScopes } from "./AlignReactiveScopesToBlockSc export { buildReactiveBlocks } from "./BuildReactiveBlocks"; export { buildReactiveFunction } from "./BuildReactiveFunction"; export { codegenReactiveFunction } from "./CodegenReactiveFunction"; +export { extractScopeDeclarationsFromDestructuring } from "./ExtractScopeDeclarationsFromDestructuring"; export { flattenReactiveLoops } from "./FlattenReactiveLoops"; export { flattenScopesWithHooks } from "./FlattenScopesWithHooks"; export { inferReactiveScopeVariables } from "./InferReactiveScopeVariables"; diff --git a/compiler/forget/src/__tests__/fixtures/compiler/destructuring-mixed-scope-and-local-variables-with-default.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/destructuring-mixed-scope-and-local-variables-with-default.expect.md new file mode 100644 index 0000000000..9cc3238b14 --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/compiler/destructuring-mixed-scope-and-local-variables-with-default.expect.md @@ -0,0 +1,101 @@ + +## Input + +```javascript +function Component(props) { + const post = useFragment(graphql`...`, props.post); + const allUrls = []; + // `media` and `urls` are exported from the scope that will wrap this code, + // but `comments` is not (it doesn't need to be memoized, bc the callback + // only checks `comments.length`) + // because of the scope, the let declaration for media and urls are lifted + // out of the scope, and the destructure statement ends up turning into + // a reassignment, instead of a const declaration. this means we try to + // reassign `comments` when there's no declaration for it. + const { media = null, comments = [], urls = [] } = post; + const onClick = (e) => { + if (!comments.length) { + return; + } + log(comments.length); + }; + allUrls.push(...urls); + return ; +} + +``` + +## Code + +```javascript +import { unstable_useMemoCache as useMemoCache } from "react"; +function Component(props) { + const $ = useMemoCache(12); + const post = useFragment(graphql`...`, props.post); + const c_0 = $[0] !== post; + let media; + let onClick; + if (c_0) { + const allUrls = []; + + const { media: t0, comments: t2, urls: t81 } = post; + const c_3 = $[3] !== t0; + let t1; + if (c_3) { + t1 = t0 === undefined ? null : t0; + $[3] = t0; + $[4] = t1; + } else { + t1 = $[4]; + } + media = t1; + const c_5 = $[5] !== t2; + let t3; + if (c_5) { + t3 = t2 === undefined ? [] : t2; + $[5] = t2; + $[6] = t3; + } else { + t3 = $[6]; + } + const comments = t3; + const urls = t81 === undefined ? [] : t81; + const c_7 = $[7] !== comments.length; + let t4; + if (c_7) { + t4 = (e) => { + if (!comments.length) { + return; + } + log(comments.length); + }; + $[7] = comments.length; + $[8] = t4; + } else { + t4 = $[8]; + } + onClick = t4; + allUrls.push(...urls); + $[0] = post; + $[1] = media; + $[2] = onClick; + } else { + media = $[1]; + onClick = $[2]; + } + const c_9 = $[9] !== media; + const c_10 = $[10] !== onClick; + let t5; + if (c_9 || c_10) { + t5 = ; + $[9] = media; + $[10] = onClick; + $[11] = t5; + } else { + t5 = $[11]; + } + return t5; +} + +``` + \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/compiler/error.destructuring-mixed-scope-declarations-and-locals.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/destructuring-mixed-scope-and-local-variables-with-default.js similarity index 73% rename from compiler/forget/src/__tests__/fixtures/compiler/error.destructuring-mixed-scope-declarations-and-locals.expect.md rename to compiler/forget/src/__tests__/fixtures/compiler/destructuring-mixed-scope-and-local-variables-with-default.js index f583d24bbd..6067b285f0 100644 --- a/compiler/forget/src/__tests__/fixtures/compiler/error.destructuring-mixed-scope-declarations-and-locals.expect.md +++ b/compiler/forget/src/__tests__/fixtures/compiler/destructuring-mixed-scope-and-local-variables-with-default.js @@ -1,7 +1,3 @@ - -## Input - -```javascript function Component(props) { const post = useFragment(graphql`...`, props.post); const allUrls = []; @@ -12,7 +8,7 @@ function Component(props) { // out of the scope, and the destructure statement ends up turning into // a reassignment, instead of a const declaration. this means we try to // reassign `comments` when there's no declaration for it. - const { media, comments, urls } = post; + const { media = null, comments = [], urls = [] } = post; const onClick = (e) => { if (!comments.length) { return; @@ -22,14 +18,3 @@ function Component(props) { allUrls.push(...urls); return ; } - -``` - - -## Error - -``` -[ReactForget] Invariant: Encountered a destructuring operation where some identifiers are already declared (reassignments) but others are not (declarations) (11:11) -``` - - \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/compiler/destructuring-mixed-scope-declarations-and-locals.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/destructuring-mixed-scope-declarations-and-locals.expect.md new file mode 100644 index 0000000000..237f6c1024 --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/compiler/destructuring-mixed-scope-declarations-and-locals.expect.md @@ -0,0 +1,81 @@ + +## Input + +```javascript +function Component(props) { + const post = useFragment(graphql`...`, props.post); + const allUrls = []; + // `media` and `urls` are exported from the scope that will wrap this code, + // but `comments` is not (it doesn't need to be memoized, bc the callback + // only checks `comments.length`) + // because of the scope, the let declaration for media and urls are lifted + // out of the scope, and the destructure statement ends up turning into + // a reassignment, instead of a const declaration. this means we try to + // reassign `comments` when there's no declaration for it. + const { media, comments, urls } = post; + const onClick = (e) => { + if (!comments.length) { + return; + } + log(comments.length); + }; + allUrls.push(...urls); + return ; +} + +``` + +## Code + +```javascript +import { unstable_useMemoCache as useMemoCache } from "react"; +function Component(props) { + const $ = useMemoCache(8); + const post = useFragment(graphql`...`, props.post); + const c_0 = $[0] !== post; + let media; + let onClick; + if (c_0) { + const allUrls = []; + + const { media: t83, comments, urls } = post; + media = t83; + const c_3 = $[3] !== comments.length; + let t0; + if (c_3) { + t0 = (e) => { + if (!comments.length) { + return; + } + log(comments.length); + }; + $[3] = comments.length; + $[4] = t0; + } else { + t0 = $[4]; + } + onClick = t0; + allUrls.push(...urls); + $[0] = post; + $[1] = media; + $[2] = onClick; + } else { + media = $[1]; + onClick = $[2]; + } + const c_5 = $[5] !== media; + const c_6 = $[6] !== onClick; + let t1; + if (c_5 || c_6) { + t1 = ; + $[5] = media; + $[6] = onClick; + $[7] = t1; + } else { + t1 = $[7]; + } + return t1; +} + +``` + \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/compiler/error.destructuring-mixed-scope-declarations-and-locals.js b/compiler/forget/src/__tests__/fixtures/compiler/destructuring-mixed-scope-declarations-and-locals.js similarity index 100% rename from compiler/forget/src/__tests__/fixtures/compiler/error.destructuring-mixed-scope-declarations-and-locals.js rename to compiler/forget/src/__tests__/fixtures/compiler/destructuring-mixed-scope-declarations-and-locals.js diff --git a/compiler/forget/src/__tests__/fixtures/compiler/error.escape-analysis-destructured-rest-element.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/error.escape-analysis-destructured-rest-element.expect.md deleted file mode 100644 index 6403b6bb22..0000000000 --- a/compiler/forget/src/__tests__/fixtures/compiler/error.escape-analysis-destructured-rest-element.expect.md +++ /dev/null @@ -1,22 +0,0 @@ - -## Input - -```javascript -function Component(props) { - // b is an object, must be memoized even though the input is not memoized - const { a, ...b } = props.a; - // d is an array, mut be memoized even though the input is not memoized - const [c, ...d] = props.c; - return
; -} - -``` - - -## Error - -``` -[ReactForget] Invariant: Encountered a destructuring operation where some identifiers are already declared (reassignments) but others are not (declarations) (3:3) -``` - - \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/compiler/error.unused-object-element-with-rest.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/error.unused-object-element-with-rest.expect.md deleted file mode 100644 index 3f7f2ee810..0000000000 --- a/compiler/forget/src/__tests__/fixtures/compiler/error.unused-object-element-with-rest.expect.md +++ /dev/null @@ -1,20 +0,0 @@ - -## Input - -```javascript -function Foo(props) { - // can't remove `unused` since it affects which properties are copied into `rest` - const { unused, ...rest } = props.a; - return rest; -} - -``` - - -## Error - -``` -[ReactForget] Invariant: Encountered a destructuring operation where some identifiers are already declared (reassignments) but others are not (declarations) (3:3) -``` - - \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/compiler/escape-analysis-destructured-rest-element.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/escape-analysis-destructured-rest-element.expect.md new file mode 100644 index 0000000000..b80443d5f4 --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/compiler/escape-analysis-destructured-rest-element.expect.md @@ -0,0 +1,56 @@ + +## Input + +```javascript +function Component(props) { + // b is an object, must be memoized even though the input is not memoized + const { a, ...b } = props.a; + // d is an array, mut be memoized even though the input is not memoized + const [c, ...d] = props.c; + return
; +} + +``` + +## Code + +```javascript +import { unstable_useMemoCache as useMemoCache } from "react"; +function Component(props) { + const $ = useMemoCache(7); + const c_0 = $[0] !== props.a; + let b; + if (c_0) { + const { a, ...t30 } = props.a; + b = t30; + $[0] = props.a; + $[1] = b; + } else { + b = $[1]; + } + const c_2 = $[2] !== props.c; + let d; + if (c_2) { + const [c, ...t31] = props.c; + d = t31; + $[2] = props.c; + $[3] = d; + } else { + d = $[3]; + } + const c_4 = $[4] !== b; + const c_5 = $[5] !== d; + let t0; + if (c_4 || c_5) { + t0 =
; + $[4] = b; + $[5] = d; + $[6] = t0; + } else { + t0 = $[6]; + } + return t0; +} + +``` + \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/compiler/error.escape-analysis-destructured-rest-element.js b/compiler/forget/src/__tests__/fixtures/compiler/escape-analysis-destructured-rest-element.js similarity index 100% rename from compiler/forget/src/__tests__/fixtures/compiler/error.escape-analysis-destructured-rest-element.js rename to compiler/forget/src/__tests__/fixtures/compiler/escape-analysis-destructured-rest-element.js diff --git a/compiler/forget/src/__tests__/fixtures/compiler/unused-object-element-with-rest.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/unused-object-element-with-rest.expect.md new file mode 100644 index 0000000000..9a3643b9d4 --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/compiler/unused-object-element-with-rest.expect.md @@ -0,0 +1,33 @@ + +## Input + +```javascript +function Foo(props) { + // can't remove `unused` since it affects which properties are copied into `rest` + const { unused, ...rest } = props.a; + return rest; +} + +``` + +## Code + +```javascript +import { unstable_useMemoCache as useMemoCache } from "react"; +function Foo(props) { + const $ = useMemoCache(2); + const c_0 = $[0] !== props.a; + let rest; + if (c_0) { + const { unused, ...t16 } = props.a; + rest = t16; + $[0] = props.a; + $[1] = rest; + } else { + rest = $[1]; + } + return rest; +} + +``` + \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/compiler/error.unused-object-element-with-rest.js b/compiler/forget/src/__tests__/fixtures/compiler/unused-object-element-with-rest.js similarity index 100% rename from compiler/forget/src/__tests__/fixtures/compiler/error.unused-object-element-with-rest.js rename to compiler/forget/src/__tests__/fixtures/compiler/unused-object-element-with-rest.js