From af6e837b60fcdf1ae77e119f63afc4a6cff41459 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Mon, 5 Feb 2024 22:29:53 -0800 Subject: [PATCH] Fix destructuring with mixed local/scope declarations Fixes T176436488. The logic for rewriting Destructure instructions was correct, but the visitor implementation was accidentally dropping subsequent Destructure instructions within a block after encountering one that needed a rewrite. Switching to use the transform infra (added after this pass was written) fixes it. --- ...tractScopeDeclarationsFromDestructuring.ts | 60 ++++---- ...assignment-to-scope-declarations.expect.md | 129 +++++++++++++++++ ...turing-assignment-to-scope-declarations.js | 30 ++++ ...ixed-local-and-scope-declaration.expect.md | 134 ++++++++++++++++++ ...-both-mixed-local-and-scope-declaration.js | 33 +++++ 5 files changed, 352 insertions(+), 34 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/sequential-destructuring-assignment-to-scope-declarations.expect.md create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/sequential-destructuring-assignment-to-scope-declarations.js create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/sequential-destructuring-both-mixed-local-and-scope-declaration.expect.md create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/sequential-destructuring-both-mixed-local-and-scope-declaration.js diff --git a/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/ExtractScopeDeclarationsFromDestructuring.ts b/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/ExtractScopeDeclarationsFromDestructuring.ts index ff0366f32a..7bcd3d3c06 100644 --- a/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/ExtractScopeDeclarationsFromDestructuring.ts +++ b/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/ExtractScopeDeclarationsFromDestructuring.ts @@ -11,13 +11,17 @@ import { IdentifierId, InstructionKind, Place, - ReactiveBlock, ReactiveFunction, ReactiveInstruction, ReactiveScopeBlock, + ReactiveStatement, } from "../HIR"; import { eachPatternOperand, mapPatternOperands } from "../HIR/visitors"; -import { ReactiveFunctionTransform, visitReactiveFunction } from "./visitors"; +import { + ReactiveFunctionTransform, + Transformed, + visitReactiveFunction, +} from "./visitors"; /* * Destructuring statements may sometimes define some variables which are declared by the scope, @@ -92,41 +96,29 @@ class Visitor extends ReactiveFunctionTransform { this.traverseScope(scope, state); } - override visitBlock(block: ReactiveBlock, state: State): void { - // Traverse first to transform inner items - this.traverseBlock(block, state); + override transformInstruction( + instruction: ReactiveInstruction, + state: State + ): Transformed { + this.visitInstruction(instruction, 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 (instruction.value.kind === "Destructure") { + const transformed = transformDestructuring( + state, + instruction, + instruction.value + ); + if (transformed) { + return { + kind: "replace-many", + value: transformed.map((instruction) => ({ + kind: "instruction", + instruction, + })), + }; } } - if (nextBlock !== null) { - block.length = 0; - block.push(...nextBlock); - } + return { kind: "keep" }; } } diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/sequential-destructuring-assignment-to-scope-declarations.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/sequential-destructuring-assignment-to-scope-declarations.expect.md new file mode 100644 index 0000000000..c27aea1fee --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/sequential-destructuring-assignment-to-scope-declarations.expect.md @@ -0,0 +1,129 @@ + +## Input + +```javascript +import { identity } from "shared-runtime"; + +function Component(statusName) { + const { status, text } = foo(statusName); + const { bg, color } = getStyles(status); + return ( +
+ {[text]} +
+ ); +} + +function foo(name) { + return { + status: ``, + text: `${name}!`, + }; +} + +function getStyles(status) { + return { + bg: "#eee8d5", + color: "#657b83", + }; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: ["Mofei"], +}; + +``` + +## Code + +```javascript +import { unstable_useMemoCache as useMemoCache } from "react"; +import { identity } from "shared-runtime"; + +function Component(statusName) { + const $ = useMemoCache(12); + let text; + let t0; + let t1; + if ($[0] !== statusName) { + const { status, text: t47 } = foo(statusName); + text = t47; + const { bg, color } = getStyles(status); + + t1 = identity(bg); + t0 = identity(color); + $[0] = statusName; + $[1] = text; + $[2] = t0; + $[3] = t1; + } else { + text = $[1]; + t0 = $[2]; + t1 = $[3]; + } + let t2; + if ($[4] !== text) { + t2 = [text]; + $[4] = text; + $[5] = t2; + } else { + t2 = $[5]; + } + let t3; + if ($[6] !== t0 || $[7] !== t2) { + t3 = {t2}; + $[6] = t0; + $[7] = t2; + $[8] = t3; + } else { + t3 = $[8]; + } + let t4; + if ($[9] !== t1 || $[10] !== t3) { + t4 =
{t3}
; + $[9] = t1; + $[10] = t3; + $[11] = t4; + } else { + t4 = $[11]; + } + return t4; +} + +function foo(name) { + const $ = useMemoCache(2); + + const t0 = `${name}!`; + let t1; + if ($[0] !== t0) { + t1 = { status: ``, text: t0 }; + $[0] = t0; + $[1] = t1; + } else { + t1 = $[1]; + } + return t1; +} + +function getStyles(status) { + const $ = useMemoCache(1); + let t0; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + t0 = { bg: "#eee8d5", color: "#657b83" }; + $[0] = t0; + } else { + t0 = $[0]; + } + return t0; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: ["Mofei"], +}; + +``` + +### Eval output +(kind: ok)
Mofei!
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/sequential-destructuring-assignment-to-scope-declarations.js b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/sequential-destructuring-assignment-to-scope-declarations.js new file mode 100644 index 0000000000..3aa1030f0b --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/sequential-destructuring-assignment-to-scope-declarations.js @@ -0,0 +1,30 @@ +import { identity } from "shared-runtime"; + +function Component(statusName) { + const { status, text } = foo(statusName); + const { bg, color } = getStyles(status); + return ( +
+ {[text]} +
+ ); +} + +function foo(name) { + return { + status: ``, + text: `${name}!`, + }; +} + +function getStyles(status) { + return { + bg: "#eee8d5", + color: "#657b83", + }; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: ["Mofei"], +}; diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/sequential-destructuring-both-mixed-local-and-scope-declaration.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/sequential-destructuring-both-mixed-local-and-scope-declaration.expect.md new file mode 100644 index 0000000000..a184412caa --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/sequential-destructuring-both-mixed-local-and-scope-declaration.expect.md @@ -0,0 +1,134 @@ + +## Input + +```javascript +import { identity } from "shared-runtime"; + +function Component(statusName) { + // status is local, text is a scope declaration + const { status, text } = foo(statusName); + // color is local, font is a scope declaration + const { color, font } = getStyles(status); + // bg is a declaration + const bg = identity(color); + return ( +
+ {[text]} +
+ ); +} +function foo(name) { + return { + status: ``, + text: `${name}!`, + }; +} + +function getStyles(status) { + return { + font: "comic-sans", + color: "#657b83", + }; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: ["Sathya"], +}; + +``` + +## Code + +```javascript +import { unstable_useMemoCache as useMemoCache } from "react"; +import { identity } from "shared-runtime"; + +function Component(statusName) { + const $ = useMemoCache(12); + let t0; + let text; + let font; + if ($[0] !== statusName) { + const { status, text: t49 } = foo(statusName); + text = t49; + + const { color, font: t50 } = getStyles(status); + font = t50; + + t0 = identity(color); + $[0] = statusName; + $[1] = t0; + $[2] = text; + $[3] = font; + } else { + t0 = $[1]; + text = $[2]; + font = $[3]; + } + const bg = t0; + let t1; + if ($[4] !== text) { + t1 = [text]; + $[4] = text; + $[5] = t1; + } else { + t1 = $[5]; + } + let t2; + if ($[6] !== font || $[7] !== t1) { + t2 = {t1}; + $[6] = font; + $[7] = t1; + $[8] = t2; + } else { + t2 = $[8]; + } + let t3; + if ($[9] !== bg || $[10] !== t2) { + t3 =
{t2}
; + $[9] = bg; + $[10] = t2; + $[11] = t3; + } else { + t3 = $[11]; + } + return t3; +} + +function foo(name) { + const $ = useMemoCache(2); + + const t0 = `${name}!`; + let t1; + if ($[0] !== t0) { + t1 = { status: ``, text: t0 }; + $[0] = t0; + $[1] = t1; + } else { + t1 = $[1]; + } + return t1; +} + +function getStyles(status) { + const $ = useMemoCache(1); + let t0; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + t0 = { font: "comic-sans", color: "#657b83" }; + $[0] = t0; + } else { + t0 = $[0]; + } + return t0; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: ["Sathya"], +}; + +``` + +### Eval output +(kind: ok)
Sathya!
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/sequential-destructuring-both-mixed-local-and-scope-declaration.js b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/sequential-destructuring-both-mixed-local-and-scope-declaration.js new file mode 100644 index 0000000000..62efc5251d --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/sequential-destructuring-both-mixed-local-and-scope-declaration.js @@ -0,0 +1,33 @@ +import { identity } from "shared-runtime"; + +function Component(statusName) { + // status is local, text is a scope declaration + const { status, text } = foo(statusName); + // color is local, font is a scope declaration + const { color, font } = getStyles(status); + // bg is a declaration + const bg = identity(color); + return ( +
+ {[text]} +
+ ); +} +function foo(name) { + return { + status: ``, + text: `${name}!`, + }; +} + +function getStyles(status) { + return { + font: "comic-sans", + color: "#657b83", + }; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: ["Sathya"], +};