From 2ae0f36543e3306bc4b72de01a1ad8554c9176bf Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Wed, 6 Mar 2024 11:07:10 -0800 Subject: [PATCH] Promote and rename within nested functions Another title for this PR could be "Yet another reason for HIR-everywhere" ReactiveFunctionVisitor doesn't traverse into HIRFunctions from FunctionExpression and ObjectMethod values. This means that PromoteUsedTemporaries and RenameVariables also weren't traversing into such functions, and those values weren't getting promoted and renamed correctly. This PR updates ReactiveFunctionVisitor with a method that can optionally be invoked to traverse an HIRFunction and call the appropriate visitor methods. PromoteUsedTemporaries and RenameVariables invoke this to ensure they visit all places, even in nested HIRFunctions. --- .../ReactiveScopes/CodegenReactiveFunction.ts | 9 +--- .../ReactiveScopes/PromoteUsedTemporaries.ts | 13 ++--- .../src/ReactiveScopes/RenameVariables.ts | 15 ++++++ .../src/ReactiveScopes/visitors.ts | 24 +++++++++ ...function-expressions-with-params.expect.md | 54 +++++++++++++++++++ ...nested-function-expressions-with-params.js | 16 ++++++ ...ction-with-param-as-captured-dep.expect.md | 4 +- .../packages/snap/src/SproutTodoFilter.ts | 1 + 8 files changed, 120 insertions(+), 16 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/deeply-nested-function-expressions-with-params.expect.md create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/deeply-nested-function-expressions-with-params.js 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 802e190d1d..90fbecc0e8 100644 --- a/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/CodegenReactiveFunction.ts +++ b/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/CodegenReactiveFunction.ts @@ -6,12 +6,7 @@ */ import * as t from "@babel/types"; -import { - pruneHoistedContexts, - pruneUnusedLValues, - pruneUnusedLabels, - renameVariables, -} from "."; +import { pruneHoistedContexts, pruneUnusedLValues, pruneUnusedLabels } from "."; import { CompilerError, ErrorSeverity } from "../CompilerError"; import { Environment, EnvironmentConfig, ExternalFunction } from "../HIR"; import { @@ -1342,7 +1337,6 @@ function codegenInstructionValue( const reactiveFunction = buildReactiveFunction(loweredFunc.func); pruneUnusedLabels(reactiveFunction); pruneUnusedLValues(reactiveFunction); - renameVariables(reactiveFunction); const fn = codegenReactiveFunction( new Context( cx.env, @@ -1547,7 +1541,6 @@ function codegenInstructionValue( const reactiveFunction = buildReactiveFunction(loweredFunc); pruneUnusedLabels(reactiveFunction); pruneUnusedLValues(reactiveFunction); - renameVariables(reactiveFunction); pruneHoistedContexts(reactiveFunction); const fn = codegenReactiveFunction( new Context(cx.env, reactiveFunction.id ?? "[[ anonymous ]]", cx.temp), diff --git a/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/PromoteUsedTemporaries.ts b/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/PromoteUsedTemporaries.ts index 81b6be49d6..e0ce00eec4 100644 --- a/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/PromoteUsedTemporaries.ts +++ b/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/PromoteUsedTemporaries.ts @@ -46,6 +46,12 @@ class Visitor extends ReactiveFunctionVisitor { } } + override visitParam(place: Place, state: VisitorState): void { + if (place.identifier.name === null) { + promoteTemporary(place.identifier, state); + } + } + override visitValue( id: InstructionId, value: ReactiveValue, @@ -53,12 +59,7 @@ class Visitor extends ReactiveFunctionVisitor { ): void { this.traverseValue(id, value, state); if (value.kind === "FunctionExpression" || value.kind === "ObjectMethod") { - for (const operand of value.loweredFunc.func.params) { - const place = operand.kind === "Identifier" ? operand : operand.place; - if (place.identifier.name === null) { - promoteTemporary(place.identifier, state); - } - } + this.visitHirFunction(value.loweredFunc.func, state); } } diff --git a/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/RenameVariables.ts b/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/RenameVariables.ts index bbf3b74ef7..4c216db573 100644 --- a/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/RenameVariables.ts +++ b/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/RenameVariables.ts @@ -15,6 +15,7 @@ import { ReactiveBlock, ReactiveFunction, ReactiveScopeBlock, + ReactiveValue, isPromotedJsxTemporary, isPromotedTemporary, makeIdentifierName, @@ -62,6 +63,9 @@ function renameVariablesImpl( } class Visitor extends ReactiveFunctionVisitor { + override visitParam(place: Place, state: Scopes): void { + state.visit(place.identifier); + } override visitLValue(_id: InstructionId, lvalue: Place, state: Scopes): void { state.visit(lvalue.identifier); } @@ -81,6 +85,17 @@ class Visitor extends ReactiveFunctionVisitor { this.traverseScope(scope, state); } + override visitValue( + id: InstructionId, + value: ReactiveValue, + state: Scopes + ): void { + this.traverseValue(id, value, state); + if (value.kind === "FunctionExpression" || value.kind === "ObjectMethod") { + this.visitHirFunction(value.loweredFunc.func, state); + } + } + override visitReactiveFunctionValue( _id: InstructionId, _dependencies: Place[], diff --git a/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/visitors.ts b/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/visitors.ts index a7f26056e9..a27fc17846 100644 --- a/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/visitors.ts +++ b/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/visitors.ts @@ -6,6 +6,7 @@ */ import { + HIRFunction, InstructionId, Place, ReactiveBlock, @@ -20,6 +21,7 @@ import { import { eachInstructionLValue, eachInstructionValueOperand, + eachTerminalOperand, } from "../HIR/visitors"; import { assertExhaustive } from "../Utils/utils"; @@ -33,6 +35,7 @@ export function visitReactiveFunction( export class ReactiveFunctionVisitor { visitID(_id: InstructionId, _state: TState): void {} + visitParam(_place: Place, _state: TState): void {} visitLValue(_id: InstructionId, _lvalue: Place, _state: TState): void {} visitPlace(_id: InstructionId, _place: Place, _state: TState): void {} visitReactiveFunctionValue( @@ -219,6 +222,27 @@ export class ReactiveFunctionVisitor { } } } + + visitHirFunction(fn: HIRFunction, state: TState): void { + for (const param of fn.params) { + const place = param.kind === "Identifier" ? param : param.place; + this.visitParam(place, state); + } + for (const [, block] of fn.body.blocks) { + for (const instr of block.instructions) { + this.visitInstruction(instr, state); + if ( + instr.value.kind === "FunctionExpression" || + instr.value.kind === "ObjectMethod" + ) { + this.visitHirFunction(instr.value.loweredFunc.func, state); + } + } + for (const operand of eachTerminalOperand(block.terminal)) { + this.visitPlace(block.terminal.id, operand, state); + } + } + } } export type TransformedValue = diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/deeply-nested-function-expressions-with-params.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/deeply-nested-function-expressions-with-params.expect.md new file mode 100644 index 0000000000..1906b8dc8b --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/deeply-nested-function-expressions-with-params.expect.md @@ -0,0 +1,54 @@ + +## Input + +```javascript +function Foo() { + return (function t() { + let x = {}; + let y = {}; + return function a(x = () => {}) { + return (function b(y = []) { + return [x, y]; + })(); + }; + })(); +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [], +}; + +``` + +## Code + +```javascript +import { unstable_useMemoCache as useMemoCache } from "react"; +function Foo() { + const $ = useMemoCache(1); + let t0; + let t1; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + t1 = function a(t2) { + const x_0 = t2 === undefined ? () => {} : t2; + return (function b(t3) { + const y_0 = t3 === undefined ? [] : t3; + return [x_0, y_0]; + })(); + }; + $[0] = t1; + } else { + t1 = $[0]; + } + t0 = t1; + return t0; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [], +}; + +``` + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/deeply-nested-function-expressions-with-params.js b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/deeply-nested-function-expressions-with-params.js new file mode 100644 index 0000000000..310204ae62 --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/deeply-nested-function-expressions-with-params.js @@ -0,0 +1,16 @@ +function Foo() { + return (function t() { + let x = {}; + let y = {}; + return function a(x = () => {}) { + return (function b(y = []) { + return [x, y]; + })(); + }; + })(); +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [], +}; diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/nested-function-with-param-as-captured-dep.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/nested-function-with-param-as-captured-dep.expect.md index df7b7b6852..8d3c3f936c 100644 --- a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/nested-function-with-param-as-captured-dep.expect.md +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/nested-function-with-param-as-captured-dep.expect.md @@ -28,8 +28,8 @@ function Foo() { let t0; let t1; if ($[0] === Symbol.for("react.memo_cache_sentinel")) { - t1 = function a(t0) { - const x_0 = t0 === undefined ? () => {} : t0; + t1 = function a(t2) { + const x_0 = t2 === undefined ? () => {} : t2; return x_0; }; $[0] = t1; diff --git a/compiler/packages/snap/src/SproutTodoFilter.ts b/compiler/packages/snap/src/SproutTodoFilter.ts index 8bc5073403..a33b347a76 100644 --- a/compiler/packages/snap/src/SproutTodoFilter.ts +++ b/compiler/packages/snap/src/SproutTodoFilter.ts @@ -422,6 +422,7 @@ const skipFilter = new Set([ "component-declaration-basic.flow", "hook-declaration-basic.flow", "nested-function-with-param-as-captured-dep", + "deeply-nested-function-expressions-with-params", "readonly-object-method-calls", "readonly-object-method-calls-mutable-lambda",