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