From 5622c0ee91b3f011c735e4addc4693ff92ee8370 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Thu, 6 Apr 2023 15:25:30 -0700 Subject: [PATCH] Use single name resolver for nested functions This is a prerequisite to inlining `useMemo()` lambdas so that we can better optimize them. Nested functions are evaluated with a fresh HIRBuilder, which means that they currently have their own `bindings` object for mapping identifier instances to IdentifierIds. This means that identifier ids in a closure are _always_ different that those outside the closure, even when they refer to the same identifier: ``` function Component(props) { props; // becomes e.g. props$1 const onClick = () => { props // becomes e.g. props$2 }; } ``` For useMemo inlining this is problematic because we've lost the association that these identifiers actually refer to the same thing. This PR changes that, sharing the name resolution data structure between the top-level function and any nested function expressions. --- compiler/forget/src/CompilerPipeline.ts | 3 +- compiler/forget/src/HIR/BuildHIR.ts | 6 ++- compiler/forget/src/HIR/HIRBuilder.ts | 18 +++++-- ...c-alias-receiver-computed-mutate.expect.md | 4 +- ...uring-func-alias-receiver-mutate.expect.md | 4 +- ...turing-function-member-expr-call.expect.md | 4 +- ...pturing-function-shadow-captured.expect.md | 4 +- ...rtent-mutability-readonly-lambda.expect.md | 2 +- ...ed-function-shadowed-identifiers.expect.md | 52 +++++++++++++++++++ .../nested-function-shadowed-identifiers.js | 10 ++++ 10 files changed, 91 insertions(+), 16 deletions(-) create mode 100644 compiler/forget/src/__tests__/fixtures/compiler/nested-function-shadowed-identifiers.expect.md create mode 100644 compiler/forget/src/__tests__/fixtures/compiler/nested-function-shadowed-identifiers.js diff --git a/compiler/forget/src/CompilerPipeline.ts b/compiler/forget/src/CompilerPipeline.ts index b148da1b99..4e0a8bb4f8 100644 --- a/compiler/forget/src/CompilerPipeline.ts +++ b/compiler/forget/src/CompilerPipeline.ts @@ -56,7 +56,8 @@ export function* run( func: NodePath, config?: EnvironmentConfig | null ): Generator { - const hir = lower(func, new Environment(config ?? null)).unwrap(); + const env = new Environment(config ?? null); + const hir = lower(func, env).unwrap(); yield log({ kind: "hir", name: "HIR", value: hir }); mergeConsecutiveBlocks(hir); diff --git a/compiler/forget/src/HIR/BuildHIR.ts b/compiler/forget/src/HIR/BuildHIR.ts index 36f338f37f..9e6ffa7fd9 100644 --- a/compiler/forget/src/HIR/BuildHIR.ts +++ b/compiler/forget/src/HIR/BuildHIR.ts @@ -36,7 +36,7 @@ import { SpreadPattern, ThrowTerminal, } from "./HIR"; -import HIRBuilder from "./HIRBuilder"; +import HIRBuilder, { Bindings } from "./HIRBuilder"; // ******************************************************************************************* // ******************************************************************************************* @@ -58,11 +58,12 @@ import HIRBuilder from "./HIRBuilder"; export function lower( func: NodePath, env: Environment, + bindings: Bindings | null = null, capturedRefs: t.Identifier[] = [], // the outermost function being compiled, in case lower() is called recursively (for lambdas) parent: NodePath | null = null ): Result { - const builder = new HIRBuilder(env, parent ?? func, capturedRefs); + const builder = new HIRBuilder(env, parent ?? func, bindings, capturedRefs); const context: Place[] = []; for (const ref of capturedRefs ?? []) { @@ -2134,6 +2135,7 @@ function lowerFunctionExpression( const lowering = lower( expr, builder.environment, + builder.bindings, [...builder.context, ...captured.identifiers], builder.parentFunction ); diff --git a/compiler/forget/src/HIR/HIRBuilder.ts b/compiler/forget/src/HIR/HIRBuilder.ts index b4b2c147f3..de09951927 100644 --- a/compiler/forget/src/HIR/HIRBuilder.ts +++ b/compiler/forget/src/HIR/HIRBuilder.ts @@ -71,6 +71,11 @@ function newBlock(id: BlockId, kind: BlockKind): WipBlock { return { id, kind, instructions: [] }; } +export type Bindings = Map< + string, + { node: t.Identifier; identifier: Identifier } +>; + /** * Helper class for constructing a CFG */ @@ -80,8 +85,7 @@ export default class HIRBuilder { #entry: BlockId; #scopes: Array = []; #context: t.Identifier[]; - #bindings: Map = - new Map(); + #bindings: Bindings; #env: Environment; parentFunction: NodePath; errors: CompilerError = new CompilerError(); @@ -94,6 +98,10 @@ export default class HIRBuilder { return this.#context; } + get bindings(): Bindings { + return this.#bindings; + } + get environment(): Environment { return this.#env; } @@ -101,11 +109,13 @@ export default class HIRBuilder { constructor( env: Environment, parentFunction: NodePath, // the outermost function being compiled - context: t.Identifier[] + bindings: Bindings | null = null, + context: t.Identifier[] | null = null ) { this.#env = env; + this.#bindings = bindings ?? new Map(); this.parentFunction = parentFunction; - this.#context = context; + this.#context = context ?? []; this.#entry = makeBlockId(env.nextBlockId); this.#current = newBlock(this.#entry, "block"); } diff --git a/compiler/forget/src/__tests__/fixtures/compiler/capturing-func-alias-receiver-computed-mutate.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/capturing-func-alias-receiver-computed-mutate.expect.md index fbfc6a5241..3bd06433c5 100644 --- a/compiler/forget/src/__tests__/fixtures/compiler/capturing-func-alias-receiver-computed-mutate.expect.md +++ b/compiler/forget/src/__tests__/fixtures/compiler/capturing-func-alias-receiver-computed-mutate.expect.md @@ -26,8 +26,8 @@ function component(a) { const x = { a }; y = {}; (function () { - let a = y; - a["x"] = x; + let a_0 = y; + a_0["x"] = x; })(); mutate(y); $[0] = a; diff --git a/compiler/forget/src/__tests__/fixtures/compiler/capturing-func-alias-receiver-mutate.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/capturing-func-alias-receiver-mutate.expect.md index f51e9d2f5c..51d77c8f9b 100644 --- a/compiler/forget/src/__tests__/fixtures/compiler/capturing-func-alias-receiver-mutate.expect.md +++ b/compiler/forget/src/__tests__/fixtures/compiler/capturing-func-alias-receiver-mutate.expect.md @@ -26,8 +26,8 @@ function component(a) { const x = { a }; y = {}; (function () { - let a = y; - a.x = x; + let a_0 = y; + a_0.x = x; })(); mutate(y); $[0] = a; diff --git a/compiler/forget/src/__tests__/fixtures/compiler/capturing-function-member-expr-call.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/capturing-function-member-expr-call.expect.md index 235dae8c08..45a6c8314c 100644 --- a/compiler/forget/src/__tests__/fixtures/compiler/capturing-function-member-expr-call.expect.md +++ b/compiler/forget/src/__tests__/fixtures/compiler/capturing-function-member-expr-call.expect.md @@ -19,9 +19,9 @@ function component({ mutator }) { ## Code ```javascript -function component(t26) { +function component(t24) { const $ = React.unstable_useMemoCache(7); - const { mutator } = t26; + const { mutator } = t24; const c_0 = $[0] !== mutator; let t0; if (c_0) { diff --git a/compiler/forget/src/__tests__/fixtures/compiler/capturing-function-shadow-captured.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/capturing-function-shadow-captured.expect.md index d2bdb712f7..56dda6c008 100644 --- a/compiler/forget/src/__tests__/fixtures/compiler/capturing-function-shadow-captured.expect.md +++ b/compiler/forget/src/__tests__/fixtures/compiler/capturing-function-shadow-captured.expect.md @@ -21,8 +21,8 @@ function component(a) { let t0; if ($[0] === Symbol.for("react.memo_cache_sentinel")) { t0 = function () { - let z; - mutate(z); + let z_0; + mutate(z_0); }; $[0] = t0; } else { diff --git a/compiler/forget/src/__tests__/fixtures/compiler/inadvertent-mutability-readonly-lambda.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/inadvertent-mutability-readonly-lambda.expect.md index 242c736227..64be9e6ec9 100644 --- a/compiler/forget/src/__tests__/fixtures/compiler/inadvertent-mutability-readonly-lambda.expect.md +++ b/compiler/forget/src/__tests__/fixtures/compiler/inadvertent-mutability-readonly-lambda.expect.md @@ -27,7 +27,7 @@ function Component(props) { const c_0 = $[0] !== setValue; let t0; if (c_0) { - t0 = (e) => setValue((value) => value + e.target.value); + t0 = (e) => setValue((value_0) => value_0 + e.target.value); $[0] = setValue; $[1] = t0; } else { diff --git a/compiler/forget/src/__tests__/fixtures/compiler/nested-function-shadowed-identifiers.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/nested-function-shadowed-identifiers.expect.md new file mode 100644 index 0000000000..d817bb902c --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/compiler/nested-function-shadowed-identifiers.expect.md @@ -0,0 +1,52 @@ + +## Input + +```javascript +function Component(props) { + const [x, setX] = useState(null); + + const onChange = (e) => { + let x = null; // intentionally shadow the original x + setX((currentX) => currentX + x); // intentionally refer to shadowed x + }; + + return ; +} + +``` + +## Code + +```javascript +function Component(props) { + const $ = React.unstable_useMemoCache(5); + const [x, setX] = useState(null); + const c_0 = $[0] !== setX; + let t0; + if (c_0) { + t0 = (e) => { + let x_0 = null; // intentionally shadow the original x + setX((currentX) => currentX + x_0); // intentionally refer to shadowed x + }; + $[0] = setX; + $[1] = t0; + } else { + t0 = $[1]; + } + const onChange = t0; + const c_2 = $[2] !== x; + const c_3 = $[3] !== onChange; + let t1; + if (c_2 || c_3) { + t1 = ; + $[2] = x; + $[3] = onChange; + $[4] = t1; + } else { + t1 = $[4]; + } + return t1; +} + +``` + \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/compiler/nested-function-shadowed-identifiers.js b/compiler/forget/src/__tests__/fixtures/compiler/nested-function-shadowed-identifiers.js new file mode 100644 index 0000000000..30bbc1ba72 --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/compiler/nested-function-shadowed-identifiers.js @@ -0,0 +1,10 @@ +function Component(props) { + const [x, setX] = useState(null); + + const onChange = (e) => { + let x = null; // intentionally shadow the original x + setX((currentX) => currentX + x); // intentionally refer to shadowed x + }; + + return ; +}