From 7ed187e3a3f2e5d00eb1ab4e19a8adad37094b6a Mon Sep 17 00:00:00 2001 From: Mike Vitousek Date: Mon, 9 Sep 2024 10:24:03 -0700 Subject: [PATCH] [compiler] Implement support for hoisted and recursive functions Summary: Introduces a new binding kind for functions that allows them to be hoisted. Also has the result of causing all nested function declarations to be outputted as function declarations, not as let bindings. ghstack-source-id: 597e5478208fb10d8d09b3d5b217c18cf9e74de5 Pull Request resolved: https://github.com/facebook/react/pull/30922 --- .../src/HIR/BuildHIR.ts | 30 ++++--- .../src/HIR/HIR.ts | 6 +- .../src/HIR/PrintHIR.ts | 6 ++ .../ReactiveScopes/CodegenReactiveFunction.ts | 85 +++++++++---------- .../ReactiveScopes/PruneHoistedContexts.ts | 11 +++ ...ror.hoisted-function-declaration.expect.md | 29 ------- .../error.hoisted-function-declaration.js | 8 -- ...ting-simple-function-declaration.expect.md | 14 +-- .../error.todo-hoist-function-decls.expect.md | 14 +-- ...do-recursive-function-expression.expect.md | 30 ------- .../hoisted-function-declaration.expect.md | 63 ++++++++++++++ .../compiler/hoisted-function-declaration.js | 19 +++++ .../recursive-function-expression.expect.md | 53 ++++++++++++ ...on.js => recursive-function-expression.js} | 5 ++ 14 files changed, 234 insertions(+), 139 deletions(-) delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.hoisted-function-declaration.expect.md delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.hoisted-function-declaration.js delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-recursive-function-expression.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisted-function-declaration.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisted-function-declaration.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/recursive-function-expression.expect.md rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/{error.todo-recursive-function-expression.js => recursive-function-expression.js} (67%) diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/BuildHIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/BuildHIR.ts index 7fb12d4624c10..a179224a7705f 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/BuildHIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/BuildHIR.ts @@ -420,7 +420,19 @@ function lowerStatement( // Already hoisted continue; } - if (!binding.path.isVariableDeclarator()) { + + let kind: + | InstructionKind.Let + | InstructionKind.HoistedConst + | InstructionKind.HoistedLet + | InstructionKind.HoistedFunction; + if (binding.kind === 'const' || binding.kind === 'var') { + kind = InstructionKind.HoistedConst; + } else if (binding.kind === 'let') { + kind = InstructionKind.HoistedLet; + } else if (binding.path.isFunctionDeclaration()) { + kind = InstructionKind.HoistedFunction; + } else if (!binding.path.isVariableDeclarator()) { builder.errors.push({ severity: ErrorSeverity.Todo, reason: 'Unsupported declaration type for hoisting', @@ -429,11 +441,7 @@ function lowerStatement( loc: id.parentPath.node.loc ?? GeneratedSource, }); continue; - } else if ( - binding.kind !== 'const' && - binding.kind !== 'var' && - binding.kind !== 'let' - ) { + } else { builder.errors.push({ severity: ErrorSeverity.Todo, reason: 'Handle non-const declarations for hoisting', @@ -443,6 +451,7 @@ function lowerStatement( }); continue; } + const identifier = builder.resolveIdentifier(id); CompilerError.invariant(identifier.kind === 'Identifier', { reason: @@ -456,13 +465,6 @@ function lowerStatement( reactive: false, loc: id.node.loc ?? GeneratedSource, }; - const kind = - // Avoid double errors on var declarations, which we do not plan to support anyways - binding.kind === 'const' || binding.kind === 'var' - ? InstructionKind.HoistedConst - : binding.kind === 'let' - ? InstructionKind.HoistedLet - : assertExhaustive(binding.kind, 'Unexpected binding kind'); lowerValueToTemporary(builder, { kind: 'DeclareContext', lvalue: { @@ -999,7 +1001,7 @@ function lowerStatement( lowerAssignment( builder, stmt.node.loc ?? GeneratedSource, - InstructionKind.Let, + InstructionKind.Function, id, fn, 'Assignment', diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts index 930dd79f2fd59..30408ab032b35 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts @@ -746,6 +746,9 @@ export enum InstructionKind { // hoisted const declarations HoistedLet = 'HoistedLet', + + HoistedFunction = 'HoistedFunction', + Function = 'Function', } function _staticInvariantInstructionValueHasLocation( @@ -865,7 +868,8 @@ export type InstructionValue = kind: | InstructionKind.Let | InstructionKind.HoistedConst - | InstructionKind.HoistedLet; + | InstructionKind.HoistedLet + | InstructionKind.HoistedFunction; place: Place; }; loc: SourceLocation; diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts index c2db20c5099a1..c88d3bf773898 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts @@ -765,6 +765,12 @@ export function printLValue(lval: LValue): string { case InstructionKind.HoistedLet: { return `HoistedLet ${lvalue}$`; } + case InstructionKind.Function: { + return `Function ${lvalue}$`; + } + case InstructionKind.HoistedFunction: { + return `HoistedFunction ${lvalue}$`; + } default: { assertExhaustive(lval.kind, `Unexpected lvalue kind \`${lval.kind}\``); } diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts index 2df7b5ed1c7fd..297c7712546c9 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts @@ -981,22 +981,12 @@ function codegenTerminal( suggestions: null, }); case InstructionKind.Catch: - CompilerError.invariant(false, { - reason: 'Unexpected catch variable as for..in collection', - description: null, - loc: iterableItem.loc, - suggestions: null, - }); case InstructionKind.HoistedConst: - CompilerError.invariant(false, { - reason: 'Unexpected HoistedConst variable in for..in collection', - description: null, - loc: iterableItem.loc, - suggestions: null, - }); case InstructionKind.HoistedLet: + case InstructionKind.HoistedFunction: + case InstructionKind.Function: CompilerError.invariant(false, { - reason: 'Unexpected HoistedLet variable in for..in collection', + reason: `Unexpected ${iterableItem.value.lvalue.kind} variable in for..in collection`, description: null, loc: iterableItem.loc, suggestions: null, @@ -1075,30 +1065,13 @@ function codegenTerminal( varDeclKind = 'let' as const; break; case InstructionKind.Reassign: - CompilerError.invariant(false, { - reason: - 'Destructure should never be Reassign as it would be an Object/ArrayPattern', - description: null, - loc: iterableItem.loc, - suggestions: null, - }); case InstructionKind.Catch: - CompilerError.invariant(false, { - reason: 'Unexpected catch variable as for..of collection', - description: null, - 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, - }); case InstructionKind.HoistedLet: + case InstructionKind.HoistedFunction: + case InstructionKind.Function: CompilerError.invariant(false, { - reason: 'Unexpected HoistedLet variable in for..of collection', + reason: `Unexpected ${iterableItem.value.lvalue.kind} variable in for..of collection`, description: null, loc: iterableItem.loc, suggestions: null, @@ -1261,6 +1234,35 @@ function codegenInstructionNullable( t.variableDeclarator(codegenLValue(cx, lvalue), value), ]); } + case InstructionKind.Function: { + CompilerError.invariant(instr.lvalue === null, { + reason: `Function declaration cannot be referenced as an expression`, + description: null, + loc: instr.value.loc, + suggestions: null, + }); + const genLvalue = codegenLValue(cx, lvalue); + CompilerError.invariant(genLvalue.type === 'Identifier', { + reason: 'Expected an identifier as a function declaration lvalue', + description: null, + loc: instr.value.loc, + suggestions: null, + }); + CompilerError.invariant(value?.type === 'FunctionExpression', { + reason: 'Expected a function as a function declaration value', + description: null, + loc: instr.value.loc, + suggestions: null, + }); + return createFunctionDeclaration( + instr.loc, + genLvalue, + value.params, + value.body, + value.generator, + value.async, + ); + } case InstructionKind.Let: { CompilerError.invariant(instr.lvalue === null, { reason: `Const declaration cannot be referenced as an expression`, @@ -1303,19 +1305,11 @@ function codegenInstructionNullable( case InstructionKind.Catch: { return t.emptyStatement(); } - case InstructionKind.HoistedLet: { - CompilerError.invariant(false, { - reason: - 'Expected HoistedLet to have been pruned in PruneHoistedContexts', - description: null, - loc: instr.loc, - suggestions: null, - }); - } - case InstructionKind.HoistedConst: { + case InstructionKind.HoistedLet: + case InstructionKind.HoistedConst: + case InstructionKind.HoistedFunction: { CompilerError.invariant(false, { - reason: - 'Expected HoistedConsts to have been pruned in PruneHoistedContexts', + reason: `Expected ${kind} to have been pruned in PruneHoistedContexts`, description: null, loc: instr.loc, suggestions: null, @@ -1486,6 +1480,7 @@ const createBinaryExpression = withLoc(t.binaryExpression); const createExpressionStatement = withLoc(t.expressionStatement); const _createLabelledStatement = withLoc(t.labeledStatement); const createVariableDeclaration = withLoc(t.variableDeclaration); +const createFunctionDeclaration = withLoc(t.functionDeclaration); const _createWhileStatement = withLoc(t.whileStatement); const createTaggedTemplateExpression = withLoc(t.taggedTemplateExpression); const createLogicalExpression = withLoc(t.logicalExpression); diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneHoistedContexts.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneHoistedContexts.ts index 1df211afc3ae4..07b099c2ea5fe 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneHoistedContexts.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneHoistedContexts.ts @@ -57,6 +57,17 @@ class Visitor extends ReactiveFunctionTransform { return {kind: 'remove'}; } + if ( + instruction.value.kind === 'DeclareContext' && + instruction.value.lvalue.kind === 'HoistedFunction' + ) { + state.set( + instruction.value.lvalue.place.identifier.declarationId, + InstructionKind.Function, + ); + return {kind: 'remove'}; + } + if ( instruction.value.kind === 'StoreContext' && state.has(instruction.value.lvalue.place.identifier.declarationId) diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.hoisted-function-declaration.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.hoisted-function-declaration.expect.md deleted file mode 100644 index d2e51d07256e6..0000000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.hoisted-function-declaration.expect.md +++ /dev/null @@ -1,29 +0,0 @@ - -## Input - -```javascript -function component(a) { - let t = {a}; - x(t); // hoisted call - function x(p) { - p.foo(); - } - return t; -} - -``` - - -## Error - -``` - 1 | function component(a) { - 2 | let t = {a}; -> 3 | x(t); // hoisted call - | ^^^^ Todo: Unsupported declaration type for hoisting. variable "x" declared with FunctionDeclaration (3:3) - 4 | function x(p) { - 5 | p.foo(); - 6 | } -``` - - \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.hoisted-function-declaration.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.hoisted-function-declaration.js deleted file mode 100644 index 3b0586d4376e6..0000000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.hoisted-function-declaration.js +++ /dev/null @@ -1,8 +0,0 @@ -function component(a) { - let t = {a}; - x(t); // hoisted call - function x(p) { - p.foo(); - } - return t; -} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.hoisting-simple-function-declaration.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.hoisting-simple-function-declaration.expect.md index 91f8e67d0c698..2045ee7901e96 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.hoisting-simple-function-declaration.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.hoisting-simple-function-declaration.expect.md @@ -24,13 +24,13 @@ export const FIXTURE_ENTRYPOINT = { ## Error ``` - 3 | return x; - 4 | } -> 5 | return baz(); // OK: FuncDecls are HoistableDeclarations that have both declaration and value hoisting - | ^^^^^ Todo: Unsupported declaration type for hoisting. variable "baz" declared with FunctionDeclaration (5:5) - 6 | function baz() { - 7 | return bar(); - 8 | } + 5 | return baz(); // OK: FuncDecls are HoistableDeclarations that have both declaration and value hoisting + 6 | function baz() { +> 7 | return bar(); + | ^^^ Todo: Support functions with unreachable code that may contain hoisted declarations (7:7) + 8 | } + 9 | } + 10 | ``` \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-hoist-function-decls.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-hoist-function-decls.expect.md index 02b2060e84e7e..b3aa848f1c745 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-hoist-function-decls.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-hoist-function-decls.expect.md @@ -16,11 +16,15 @@ function Component() { ``` 1 | function Component() { -> 2 | return get2(); - | ^^^^^^ Todo: Unsupported declaration type for hoisting. variable "get2" declared with FunctionDeclaration (2:2) - 3 | function get2() { - 4 | return 2; - 5 | } + 2 | return get2(); +> 3 | function get2() { + | ^^^^^^^^^^^^^^^^^ +> 4 | return 2; + | ^^^^^^^^^^^^^ +> 5 | } + | ^^^^ Todo: Support functions with unreachable code that may contain hoisted declarations (3:5) + 6 | } + 7 | ``` \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-recursive-function-expression.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-recursive-function-expression.expect.md deleted file mode 100644 index d4d09dfeae9b0..0000000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-recursive-function-expression.expect.md +++ /dev/null @@ -1,30 +0,0 @@ - -## Input - -```javascript -function Component() { - function callback(x) { - if (x == 0) { - return null; - } - return callback(x - 1); - } - return callback(10); -} - -``` - - -## Error - -``` - 4 | return null; - 5 | } -> 6 | return callback(x - 1); - | ^^^^^^^^^^^^^^^ Todo: Unsupported declaration type for hoisting. variable "callback" declared with FunctionDeclaration (6:6) - 7 | } - 8 | return callback(10); - 9 | } -``` - - \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisted-function-declaration.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisted-function-declaration.expect.md new file mode 100644 index 0000000000000..aa3246614ab41 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisted-function-declaration.expect.md @@ -0,0 +1,63 @@ + +## Input + +```javascript +function component(a) { + let t = {a}; + x(t); // hoisted call + function x(p) { + p.a.foo(); + } + return t; +} + +export const FIXTURE_ENTRYPOINT = { + fn: component, + params: [ + { + foo: () => { + console.log(42); + }, + }, + ], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +function component(a) { + const $ = _c(2); + let t; + if ($[0] !== a) { + t = { a }; + x(t); + function x(p) { + p.a.foo(); + } + $[0] = a; + $[1] = t; + } else { + t = $[1]; + } + return t; +} + +export const FIXTURE_ENTRYPOINT = { + fn: component, + params: [ + { + foo: () => { + console.log(42); + }, + }, + ], +}; + +``` + +### Eval output +(kind: ok) {"a":{"foo":"[[ function params=0 ]]"}} +logs: [42] \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisted-function-declaration.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisted-function-declaration.js new file mode 100644 index 0000000000000..6fe6dc04cc97b --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisted-function-declaration.js @@ -0,0 +1,19 @@ +function component(a) { + let t = {a}; + x(t); // hoisted call + function x(p) { + p.a.foo(); + } + return t; +} + +export const FIXTURE_ENTRYPOINT = { + fn: component, + params: [ + { + foo: () => { + console.log(42); + }, + }, + ], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/recursive-function-expression.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/recursive-function-expression.expect.md new file mode 100644 index 0000000000000..bc46c1b85578b --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/recursive-function-expression.expect.md @@ -0,0 +1,53 @@ + +## Input + +```javascript +function Component() { + function callback(x) { + if (x == 0) { + return null; + } + return callback(x - 1); + } + return callback(10); +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +function Component() { + const $ = _c(1); + let t0; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + function callback(x) { + if (x == 0) { + return null; + } + return callback(x - 1); + } + + t0 = callback(10); + $[0] = t0; + } else { + t0 = $[0]; + } + return t0; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [], +}; + +``` + +### Eval output +(kind: ok) null \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-recursive-function-expression.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/recursive-function-expression.js similarity index 67% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-recursive-function-expression.js rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/recursive-function-expression.js index f1b95e5105761..5d50de75bae50 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-recursive-function-expression.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/recursive-function-expression.js @@ -7,3 +7,8 @@ function Component() { } return callback(10); } + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [], +};