From 1ce952e687b7d2031bf3d6aa69f917506bc2c2cb Mon Sep 17 00:00:00 2001 From: Lauren Tan Date: Thu, 21 Sep 2023 17:00:04 -0400 Subject: [PATCH] Support hoisting const variable declarations This PR adds preliminary support for hoisting const variable declarations. We do this via BuildHIR when lowering top level statements in a BlockStatement, by first checking which bindings are in scope to be hoistable if referenced before they are declared. The declarations are then hoisted to their earliest point where they are referenced (ie the top level statement just before) as context variables. Later, prior to codegen, we restore the original source by removing the DeclareContexts and transforming their associated StoreContexts back. Support for hoisting other kinds of declarations will come in future PRs! --- .../src/Entrypoint/Pipeline.ts | 8 ++ .../src/HIR/BuildHIR.ts | 112 +++++++++++++++++- .../src/HIR/Environment.ts | 12 ++ .../babel-plugin-react-forget/src/HIR/HIR.ts | 8 +- .../src/HIR/PrintHIR.ts | 3 + .../ReactiveScopes/CodegenReactiveFunction.ts | 16 +++ .../ReactiveScopes/PruneHoistedContexts.ts | 64 ++++++++++ .../src/ReactiveScopes/index.ts | 1 + ...ing-simple-function-declaration.expect.md} | 0 ...r.hoisting-simple-function-declaration.js} | 0 ...sting-simple-function-expression.expect.md | 31 ----- ...ing-simple-let-const-declaration.expect.md | 29 ----- ...sting-computed-member-expression.expect.md | 67 +++++++++++ .../hoisting-computed-member-expression.js | 18 +++ .../hoisting-member-expression.expect.md | 56 +++++++++ .../compiler/hoisting-member-expression.js | 14 +++ ...sting-nested-const-declaration-2.expect.md | 64 ++++++++++ .../hoisting-nested-const-declaration-2.js | 17 +++ ...oisting-nested-const-declaration.expect.md | 61 ++++++++++ .../hoisting-nested-const-declaration.js | 21 ++++ ...oisting-simple-const-declaration.expect.md | 55 +++++++++ ...s => hoisting-simple-const-declaration.js} | 2 +- ...sting-simple-function-expression.expect.md | 57 +++++++++ ...=> hoisting-simple-function-expression.js} | 0 24 files changed, 650 insertions(+), 66 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/PruneHoistedContexts.ts rename compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/{error.todo-hoisting-simple-function-declaration.expect.md => error.hoisting-simple-function-declaration.expect.md} (100%) rename compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/{error.todo-hoisting-simple-function-declaration.js => error.hoisting-simple-function-declaration.js} (100%) delete mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.todo-hoisting-simple-function-expression.expect.md delete mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.todo-hoisting-simple-let-const-declaration.expect.md create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/hoisting-computed-member-expression.expect.md create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/hoisting-computed-member-expression.js create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/hoisting-member-expression.expect.md create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/hoisting-member-expression.js create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/hoisting-nested-const-declaration-2.expect.md create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/hoisting-nested-const-declaration-2.js create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/hoisting-nested-const-declaration.expect.md create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/hoisting-nested-const-declaration.js create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/hoisting-simple-const-declaration.expect.md rename compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/{error.todo-hoisting-simple-let-const-declaration.js => hoisting-simple-const-declaration.js} (93%) create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/hoisting-simple-function-expression.expect.md rename compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/{error.todo-hoisting-simple-function-expression.js => hoisting-simple-function-expression.js} (100%) diff --git a/compiler/packages/babel-plugin-react-forget/src/Entrypoint/Pipeline.ts b/compiler/packages/babel-plugin-react-forget/src/Entrypoint/Pipeline.ts index bb61d34c7a..c6612a49da 100644 --- a/compiler/packages/babel-plugin-react-forget/src/Entrypoint/Pipeline.ts +++ b/compiler/packages/babel-plugin-react-forget/src/Entrypoint/Pipeline.ts @@ -46,6 +46,7 @@ import { promoteUsedTemporaries, propagateScopeDependencies, pruneAllReactiveScopes, + pruneHoistedContexts, pruneNonEscapingScopes, pruneNonReactiveDependencies, pruneUnusedLValues, @@ -299,6 +300,13 @@ export function* run( value: reactiveFunction, }); + pruneHoistedContexts(reactiveFunction); + yield log({ + kind: "reactive", + name: "PruneHoistedContexts", + value: reactiveFunction, + }); + const ast = codegenReactiveFunction(reactiveFunction).unwrap(); yield log({ kind: "ast", name: "Codegen", value: ast }); diff --git a/compiler/packages/babel-plugin-react-forget/src/HIR/BuildHIR.ts b/compiler/packages/babel-plugin-react-forget/src/HIR/BuildHIR.ts index 65679cfa04..ebaf2e9a5b 100644 --- a/compiler/packages/babel-plugin-react-forget/src/HIR/BuildHIR.ts +++ b/compiler/packages/babel-plugin-react-forget/src/HIR/BuildHIR.ts @@ -5,7 +5,7 @@ * LICENSE file in the root directory of this source tree. */ -import { NodePath, Scope } from "@babel/traverse"; +import { Binding, NodePath, Scope } from "@babel/traverse"; import * as t from "@babel/types"; import { Expression } from "@babel/types"; import invariant from "invariant"; @@ -298,7 +298,108 @@ function lowerStatement( } case "BlockStatement": { const stmt = stmtPath as NodePath; - stmt.get("body").forEach((s) => lowerStatement(builder, s)); + const statements = stmt.get("body"); + const hoistableBindings: Set = new Set(); + + const recordDeclaration = (lval: NodePath): void => { + // TODO: support other kinds of declarations that might need to be hoisted + switch (lval.type) { + case "Identifier": { + const lv = lval as NodePath; + const binding = stmt.scope.getBinding(lv.node.name); + if (binding != null) { + hoistableBindings.delete(binding); + } + break; + } + } + }; + + for (const [, binding] of Object.entries(stmt.scope.bindings)) { + // TODO: support other kinds of bindings + if (binding.kind === "const") { + if ( + binding.path.isVariableDeclarator() && + binding.path.get("id").isIdentifier() + ) { + hoistableBindings.add(binding); + } + } + } + + for (const s of statements) { + const hoistableIdentifiers = new Set>(); + // After visiting the declaration, hoisting is no longer required + // TODO: support other kinds of declarations + if (s.isVariableDeclaration()) { + for (const decl of s.get("declarations")) { + recordDeclaration(decl.get("id")); + } + } + + // If we see a hoistable identifier before its declaration, it should be hoisted just + // before the statement that references it + s.traverse({ + Identifier(id: NodePath) { + const binding = stmt.scope.getBinding(id.node.name); + if (binding != null && hoistableBindings.has(binding)) { + if ( + id.parentPath.isVariableDeclarator() || + // don't hoist MemberExpr `property`s, only their `object` + (id.parentPath.isMemberExpression() && + id.parentPath.get("property") === id && + id.parentPath.node.computed === false) + ) { + return; + } + hoistableIdentifiers.add(id); + } + }, + }); + + // Hoist declarations that need it to the earliest point where they are needed + for (const id of hoistableIdentifiers) { + const binding = stmt.scope.getBinding(id.node.name); + CompilerError.invariant(binding != null, { + reason: "Expected to find binding for hoisted identifier", + description: `Could not find a binding for ${id.node.name}`, + suggestions: null, + loc: id.node.loc ?? GeneratedSource, + }); + if (builder.environment.isHoistedIdentifier(binding.identifier)) { + // Already hoisted + continue; + } + if (!binding.path.isVariableDeclarator()) { + builder.errors.push({ + severity: ErrorSeverity.Todo, + reason: "Unsupported declaration type for hoisting", + description: `${id.parentPath.type}`, + suggestions: null, + loc: id.parentPath.node.loc ?? GeneratedSource, + }); + continue; + } + const identifier = builder.resolveIdentifier(id)!; + const place: Place = { + effect: Effect.Unknown, + identifier, + kind: "Identifier", + loc: id.node.loc ?? GeneratedSource, + }; + lowerValueToTemporary(builder, { + kind: "DeclareContext", + lvalue: { + kind: InstructionKind.HoistedConst, + place, + }, + loc: id.node.loc ?? GeneratedSource, + }); + builder.environment.addHoistedIdentifier(binding.identifier); + } + lowerStatement(builder, s); + } + return; } case "BreakStatement": { @@ -2894,13 +2995,16 @@ function lowerAssignment( node: lvalue.node, }; } + const isHoistedIdentifier = builder.environment.isHoistedIdentifier( + lvalue.node + ); let temporary; if (builder.isContextIdentifier(lvalue)) { - if (kind !== InstructionKind.Reassign) { + if (kind !== InstructionKind.Reassign && !isHoistedIdentifier) { if (kind === InstructionKind.Const) { builder.errors.push({ - reason: `Invalid declaration kind (const), this variable is reassigned later`, + reason: `[lowerAssignment] Invalid declaration kind (const), this variable is reassigned later`, severity: ErrorSeverity.InvalidJS, loc: lvalue.node.loc ?? null, suggestions: null, diff --git a/compiler/packages/babel-plugin-react-forget/src/HIR/Environment.ts b/compiler/packages/babel-plugin-react-forget/src/HIR/Environment.ts index ec82bbfc34..c1bf9bbb76 100644 --- a/compiler/packages/babel-plugin-react-forget/src/HIR/Environment.ts +++ b/compiler/packages/babel-plugin-react-forget/src/HIR/Environment.ts @@ -235,6 +235,7 @@ export class Environment { enableForest: boolean; #contextIdentifiers: Set; + #hoistedIdentifiers: Set; constructor( config: EnvironmentConfig | null, @@ -289,6 +290,7 @@ export class Environment { this.enableForest = config?.enableForest ?? false; this.#contextIdentifiers = contextIdentifiers; + this.#hoistedIdentifiers = new Set(); } get nextIdentifierId(): IdentifierId { @@ -298,10 +300,15 @@ export class Environment { get nextBlockId(): BlockId { return makeBlockId(this.#nextBlock++); } + isContextIdentifier(node: t.Identifier): boolean { return this.#contextIdentifiers.has(node); } + isHoistedIdentifier(node: t.Identifier): boolean { + return this.#hoistedIdentifiers.has(node); + } + getGlobalDeclaration(name: string): Global | null { let resolvedGlobal: Global | null = this.#globals.get(name) ?? null; if (resolvedGlobal === null) { @@ -356,6 +363,11 @@ export class Environment { } return null; } + + addHoistedIdentifier(node: t.Identifier): void { + this.#contextIdentifiers.add(node); + this.#hoistedIdentifiers.add(node); + } } // From https://github.com/facebook/react/blob/main/packages/eslint-plugin-react-hooks/src/RulesOfHooks.js#LL18C1-L23C2 diff --git a/compiler/packages/babel-plugin-react-forget/src/HIR/HIR.ts b/compiler/packages/babel-plugin-react-forget/src/HIR/HIR.ts index 2a203c9ba2..2d9772dd58 100644 --- a/compiler/packages/babel-plugin-react-forget/src/HIR/HIR.ts +++ b/compiler/packages/babel-plugin-react-forget/src/HIR/HIR.ts @@ -586,6 +586,11 @@ export enum InstructionKind { * catch clause binding */ Catch = "Catch", + + /** + * hoisted const declarations + */ + HoistedConst = "HoistedConst", } function _staticInvariantInstructionValueHasLocation( @@ -641,6 +646,7 @@ export type CallExpression = { * * Operands are therefore always a Place. */ + export type InstructionValue = | { kind: "LoadLocal"; @@ -660,7 +666,7 @@ export type InstructionValue = | { kind: "DeclareContext"; lvalue: { - kind: InstructionKind.Let; + kind: InstructionKind.Let | InstructionKind.HoistedConst; place: Place; }; loc: SourceLocation; diff --git a/compiler/packages/babel-plugin-react-forget/src/HIR/PrintHIR.ts b/compiler/packages/babel-plugin-react-forget/src/HIR/PrintHIR.ts index 9f1e4468a1..dbd494ec75 100644 --- a/compiler/packages/babel-plugin-react-forget/src/HIR/PrintHIR.ts +++ b/compiler/packages/babel-plugin-react-forget/src/HIR/PrintHIR.ts @@ -602,6 +602,9 @@ export function printLValue(lval: LValue): string { case InstructionKind.Catch: { return `Catch ${lvalue}`; } + case InstructionKind.HoistedConst: { + return `HoistedConst ${lvalue}$`; + } default: { assertExhaustive(lval.kind, `Unexpected lvalue kind '${lval.kind}'`); } diff --git a/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/CodegenReactiveFunction.ts b/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/CodegenReactiveFunction.ts index d99cd74f43..2bcaf28a2b 100644 --- a/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/CodegenReactiveFunction.ts +++ b/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/CodegenReactiveFunction.ts @@ -467,6 +467,13 @@ function codegenTerminal( loc: iterableItem.loc, suggestions: null, }); + case InstructionKind.HoistedConst: + CompilerError.invariant(false, { + reason: "Unexpected HoistedConst variable in for-of collection", + description: null, + loc: iterableItem.loc, + suggestions: null, + }); default: assertExhaustive( iterableItem.value.lvalue.kind, @@ -671,6 +678,15 @@ function codegenInstructionNullable( case InstructionKind.Catch: { return t.emptyStatement(); } + case InstructionKind.HoistedConst: { + CompilerError.invariant(false, { + reason: + "Expected HoistedConsts to have been pruned in PruneHoistedContexts", + description: null, + loc: instr.loc, + suggestions: null, + }); + } default: { assertExhaustive(kind, `Unexpected instruction kind '${kind}'`); } diff --git a/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/PruneHoistedContexts.ts b/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/PruneHoistedContexts.ts new file mode 100644 index 0000000000..451badc528 --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/PruneHoistedContexts.ts @@ -0,0 +1,64 @@ +import { + Identifier, + InstructionKind, + ReactiveFunction, + ReactiveInstruction, + ReactiveStatement, +} from "../HIR"; +import { + ReactiveFunctionTransform, + Transformed, + visitReactiveFunction, +} from "./visitors"; + +/** + * Prunes DeclareContexts lowered for HoistedConsts, and transforms any references back to its + * original instruction kind. + */ +export function pruneHoistedContexts(fn: ReactiveFunction): void { + const hoistedIdentifiers: HoistedIdentifiers = new Set(); + visitReactiveFunction(fn, new Visitor(), hoistedIdentifiers); +} + +type HoistedIdentifiers = Set; + +class Visitor extends ReactiveFunctionTransform { + override transformInstruction( + instruction: ReactiveInstruction, + state: HoistedIdentifiers + ): Transformed { + this.visitInstruction(instruction, state); + if ( + instruction.value.kind === "DeclareContext" && + instruction.value.lvalue.kind === "HoistedConst" + ) { + state.add(instruction.value.lvalue.place.identifier); + return { kind: "remove" }; + } + + if ( + instruction.value.kind === "StoreContext" && + state.has(instruction.value.lvalue.place.identifier) + ) { + return { + kind: "replace", + value: { + kind: "instruction", + instruction: { + ...instruction, + value: { + ...instruction.value, + lvalue: { + ...instruction.value.lvalue, + kind: InstructionKind.Const, + }, + kind: "StoreLocal", + }, + }, + }, + }; + } + + return { kind: "keep" }; + } +} diff --git a/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/index.ts b/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/index.ts index 5de4cc9424..f44ad5acce 100644 --- a/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/index.ts +++ b/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/index.ts @@ -22,6 +22,7 @@ export { printReactiveFunction } from "./PrintReactiveFunction"; export { promoteUsedTemporaries } from "./PromoteUsedTemporaries"; export { propagateScopeDependencies } from "./PropagateScopeDependencies"; export { pruneAllReactiveScopes } from "./PruneAllReactiveScopes"; +export { pruneHoistedContexts } from "./PruneHoistedContexts"; export { pruneNonEscapingScopes } from "./PruneNonEscapingScopes"; export { pruneNonReactiveDependencies } from "./PruneNonReactiveDependencies"; export { pruneTemporaryLValues as pruneUnusedLValues } from "./PruneTemporaryLValues"; diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.todo-hoisting-simple-function-declaration.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.hoisting-simple-function-declaration.expect.md similarity index 100% rename from compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.todo-hoisting-simple-function-declaration.expect.md rename to compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.hoisting-simple-function-declaration.expect.md diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.todo-hoisting-simple-function-declaration.js b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.hoisting-simple-function-declaration.js similarity index 100% rename from compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.todo-hoisting-simple-function-declaration.js rename to compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.hoisting-simple-function-declaration.js diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.todo-hoisting-simple-function-expression.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.todo-hoisting-simple-function-expression.expect.md deleted file mode 100644 index 4f5c4c0c41..0000000000 --- a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.todo-hoisting-simple-function-expression.expect.md +++ /dev/null @@ -1,31 +0,0 @@ - -## Input - -```javascript -function hoisting() { - const foo = () => { - return bar(); - }; - const bar = () => { - return 1; - }; - - return foo(); // OK: bar's value is only accessed outside of its TDZ -} - -export const FIXTURE_ENTRYPOINT = { - fn: hoisting, - params: [], - isComponent: false, -}; - -``` - - -## Error - -``` -[ReactForget] Todo: EnterSSA: Expected identifier to be defined before being used. Identifier bar$0 is undefined (5:7) -``` - - \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.todo-hoisting-simple-let-const-declaration.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.todo-hoisting-simple-let-const-declaration.expect.md deleted file mode 100644 index cecf6d6fc6..0000000000 --- a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/error.todo-hoisting-simple-let-const-declaration.expect.md +++ /dev/null @@ -1,29 +0,0 @@ - -## Input - -```javascript -function hoisting() { - const foo = () => { - return bar + baz; - }; - let bar = 3; - const baz = 2; - return foo(); // OK: called outside of TDZ for bar/baz -} - -export const FIXTURE_ENTRYPOINT = { - fn: hoisting, - params: [], - isComponent: false, -}; - -``` - - -## Error - -``` -[ReactForget] Todo: EnterSSA: Expected identifier to be defined before being used. Identifier bar$0 is undefined (5:5) -``` - - \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/hoisting-computed-member-expression.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/hoisting-computed-member-expression.expect.md new file mode 100644 index 0000000000..30861e8497 --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/hoisting-computed-member-expression.expect.md @@ -0,0 +1,67 @@ + +## Input + +```javascript +function hoisting() { + function onClick(x) { + return x + bar["baz"]; + } + function onClick2(x) { + return x + bar[baz]; + } + const baz = "baz"; + const bar = { baz: 1 }; + + return