From e081cb344652dc3003d9194cca618292a889ff2a Mon Sep 17 00:00:00 2001 From: Joseph Savona <6425824+josephsavona@users.noreply.github.com> Date: Wed, 18 Jun 2025 13:02:43 -0700 Subject: [PATCH] [compiler] FunctionExpression context locations point to first reference (#33512) This has always been awkward: `FunctionExpression.context` places have locations set to the declaration of the identifier, whereas other references have locations pointing to the reference itself. Here, we update context operands to have their location point to the first reference of that variable within the function. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33512). * #33571 * #33558 * #33547 * #33543 * #33533 * #33532 * #33530 * #33526 * #33522 * #33518 * #33514 * #33513 * __->__ #33512 * #33504 * #33500 * #33497 * #33496 --- .../src/HIR/BuildHIR.ts | 38 +++++++++++++------ .../src/HIR/HIRBuilder.ts | 8 ++-- ...todo-valid-functiondecl-hoisting.expect.md | 10 ++--- 3 files changed, 35 insertions(+), 21 deletions(-) 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 dbdbb1dcba..998ed43b04 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/BuildHIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/BuildHIR.ts @@ -72,7 +72,7 @@ export function lower( env: Environment, // Bindings captured from the outer function, in case lower() is called recursively (for lambdas) bindings: Bindings | null = null, - capturedRefs: Array = [], + capturedRefs: Map = new Map(), ): Result { const builder = new HIRBuilder(env, { bindings, @@ -80,13 +80,13 @@ export function lower( }); const context: HIRFunction['context'] = []; - for (const ref of capturedRefs ?? []) { + for (const [ref, loc] of capturedRefs ?? []) { context.push({ kind: 'Identifier', identifier: builder.resolveBinding(ref), effect: Effect.Unknown, reactive: false, - loc: ref.loc ?? GeneratedSource, + loc, }); } @@ -3439,10 +3439,12 @@ function lowerFunction( * This isn't a problem in practice because use Babel's scope analysis to * identify the correct references. */ - const lowering = lower(expr, builder.environment, builder.bindings, [ - ...builder.context, - ...capturedContext, - ]); + const lowering = lower( + expr, + builder.environment, + builder.bindings, + new Map([...builder.context, ...capturedContext]), + ); let loweredFunc: HIRFunction; if (lowering.isErr()) { lowering @@ -4160,6 +4162,11 @@ function captureScopes({from, to}: {from: Scope; to: Scope}): Set { return scopes; } +/** + * Returns a mapping of "context" identifiers — references to free variables that + * will become part of the function expression's `context` array — along with the + * source location of their first reference within the function. + */ function gatherCapturedContext( fn: NodePath< | t.FunctionExpression @@ -4168,8 +4175,8 @@ function gatherCapturedContext( | t.ObjectMethod >, componentScope: Scope, -): Array { - const capturedIds = new Set(); +): Map { + const capturedIds = new Map(); /* * Capture all the scopes from the parent of this function up to and including @@ -4212,8 +4219,15 @@ function gatherCapturedContext( // Add the base identifier binding as a dependency. const binding = baseIdentifier.scope.getBinding(baseIdentifier.node.name); - if (binding !== undefined && pureScopes.has(binding.scope)) { - capturedIds.add(binding.identifier); + if ( + binding !== undefined && + pureScopes.has(binding.scope) && + !capturedIds.has(binding.identifier) + ) { + capturedIds.set( + binding.identifier, + path.node.loc ?? binding.identifier.loc ?? GeneratedSource, + ); } } @@ -4250,7 +4264,7 @@ function gatherCapturedContext( }, }); - return [...capturedIds.keys()]; + return capturedIds; } function notNull(value: T | null): value is T { diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/HIRBuilder.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/HIRBuilder.ts index 19ccd9a6e8..c3a6c18d3a 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/HIRBuilder.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/HIRBuilder.ts @@ -106,7 +106,7 @@ export default class HIRBuilder { #current: WipBlock; #entry: BlockId; #scopes: Array = []; - #context: Array; + #context: Map; #bindings: Bindings; #env: Environment; #exceptionHandlerStack: Array = []; @@ -121,7 +121,7 @@ export default class HIRBuilder { return this.#env.nextIdentifierId; } - get context(): Array { + get context(): Map { return this.#context; } @@ -137,13 +137,13 @@ export default class HIRBuilder { env: Environment, options?: { bindings?: Bindings | null; - context?: Array; + context?: Map; entryBlockKind?: BlockKind; }, ) { this.#env = env; this.#bindings = options?.bindings ?? new Map(); - this.#context = options?.context ?? []; + this.#context = options?.context ?? new Map(); this.#entry = makeBlockId(env.nextBlockId); this.#current = newBlock(this.#entry, options?.entryBlockKind ?? 'block'); } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-valid-functiondecl-hoisting.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-valid-functiondecl-hoisting.expect.md index fb8988c8f8..e057305d05 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-valid-functiondecl-hoisting.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-valid-functiondecl-hoisting.expect.md @@ -34,13 +34,13 @@ export const FIXTURE_ENTRYPOINT = { ## Error ``` - 13 | return bar(); + 11 | + 12 | function foo() { +> 13 | return bar(); + | ^^^ Todo: [PruneHoistedContexts] Rewrite hoisted function references (13:13) 14 | } -> 15 | function bar() { - | ^^^ Todo: [PruneHoistedContexts] Rewrite hoisted function references (15:15) + 15 | function bar() { 16 | return 42; - 17 | } - 18 | ``` \ No newline at end of file