diff --git a/compiler/forget/src/HIR/BuildHIR.ts b/compiler/forget/src/HIR/BuildHIR.ts index 131903a844..dcc1b6dec0 100644 --- a/compiler/forget/src/HIR/BuildHIR.ts +++ b/compiler/forget/src/HIR/BuildHIR.ts @@ -34,24 +34,6 @@ import HIRBuilder, { Environment } from "./HIRBuilder"; // ******************************************************************************************* // ******************************************************************************************* -const GLOBALS: Map = new Map([ - ["Map", t.identifier("Map")], - ["Set", t.identifier("Set")], - ["Math", t.identifier("Math")], -]); - -// TODO: This will work as a stopgap but it isn't really correct. We need proper handling of globals -// and module-scoped variables, which means understanding module constants and imports. -function getOrAddGlobal(identifierName: string): t.Identifier { - const ident = GLOBALS.get(identifierName); - if (ident != null) { - return ident; - } - const newIdent = t.identifier(identifierName); - GLOBALS.set(identifierName, newIdent); - return newIdent; -} - /** * Lower a function declaration into a control flow graph that models aspects of * control flow that are necessary for memoization. Notably, only control flow @@ -70,14 +52,14 @@ export function lower( const builder = new HIRBuilder(env); const id = - func.isFunctionDeclaration() && func.node.id != null - ? builder.resolveIdentifier(func.node.id) + func.isFunctionDeclaration() && func.get("id").hasNode() + ? builder.resolveIdentifier(func.get("id") as NodePath) : null; const params: Array = []; - for (const param of func.get("params")) { + func.get("params").forEach((param) => { if (param.isIdentifier()) { - const identifier = builder.resolveIdentifier(param.node); + const identifier = builder.resolveIdentifier(param); const place: Place = { kind: "Identifier", identifier, @@ -97,9 +79,8 @@ export function lower( source: func.toString(), loc: param.node.loc ?? null, }); - continue; } - } + }); const body = func.get("body"); if (body.isExpression()) { @@ -1360,14 +1341,7 @@ function lowerJsxElementName( const exprLoc = exprPath.node.loc ?? GeneratedSource; const tag: string = exprPath.node.name; if (tag.match(/^[A-Z]/)) { - const binding = - exprPath.scope.getBindingIdentifier(tag) ?? getOrAddGlobal(tag); - invariant( - binding != null, - `Expected to find a binding for variable '%s'`, - tag - ); - const identifier = builder.resolveIdentifier(binding); + const identifier = builder.resolveIdentifier(exprPath); const place: Place = { kind: "Identifier", identifier: identifier, @@ -1487,15 +1461,7 @@ function lowerIdentifier( ): Place { const exprNode = exprPath.node; const exprLoc = exprNode.loc ?? GeneratedSource; - const binding = - exprPath.scope.getBindingIdentifier(exprNode.name) ?? - getOrAddGlobal(exprNode.name); - invariant( - binding != null, - `Expected to find a binding for variable '%s'`, - exprNode.name - ); - const identifier = builder.resolveIdentifier(binding); + const identifier = builder.resolveIdentifier(exprPath); const place: Place = { kind: "Identifier", identifier: identifier, @@ -1694,21 +1660,20 @@ function gatherCapturedDeps( fn.get("body").traverse({ Expression(path) { // TODO(gsn): Handle member expressions - if (!path.isIdentifier) { + if (!path.isIdentifier()) { return; } - const id = path as NodePath; - const binding = id.scope.getBinding(id.node.name); + const binding = path.scope.getBinding(path.node.name); if (binding === undefined || !pureScopes.has(binding.scope)) { return; } captured.add({ kind: "Identifier", - identifier: builder.resolveIdentifier(binding.identifier), + identifier: builder.resolveIdentifier(path), effect: Effect.Unknown, - loc: id.node.loc!, + loc: path.node.loc ?? GeneratedSource, }); }, }); diff --git a/compiler/forget/src/HIR/Globals.ts b/compiler/forget/src/HIR/Globals.ts new file mode 100644 index 0000000000..71390ba065 --- /dev/null +++ b/compiler/forget/src/HIR/Globals.ts @@ -0,0 +1,26 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +import * as t from "@babel/types"; + +const GLOBALS: Map = new Map([ + ["Map", t.identifier("Map")], + ["Set", t.identifier("Set")], + ["Math", t.identifier("Math")], +]); + +// TODO: This will work as a stopgap but it isn't really correct. We need proper handling of globals +// and module-scoped variables, which means understanding module constants and imports. +export function getOrAddGlobal(identifierName: string): t.Identifier { + const ident = GLOBALS.get(identifierName); + if (ident != null) { + return ident; + } + const newIdent = t.identifier(identifierName); + GLOBALS.set(identifierName, newIdent); + return newIdent; +} diff --git a/compiler/forget/src/HIR/HIRBuilder.ts b/compiler/forget/src/HIR/HIRBuilder.ts index c625e90806..ad958e43e5 100644 --- a/compiler/forget/src/HIR/HIRBuilder.ts +++ b/compiler/forget/src/HIR/HIRBuilder.ts @@ -5,11 +5,13 @@ * LICENSE file in the root directory of this source tree. */ +import { NodePath } from "@babel/traverse"; import * as t from "@babel/types"; import invariant from "invariant"; import { CompilerError, CompilerErrorOptions } from "../CompilerError"; import { logHIR } from "../Utils/logger"; import { assertExhaustive } from "../Utils/utils"; +import { getOrAddGlobal } from "./Globals"; import { BasicBlock, BlockId, @@ -82,7 +84,8 @@ export default class HIRBuilder { #current: WipBlock = newBlock(makeBlockId(0)); #entry: BlockId = makeBlockId(0); #scopes: Array = []; - #bindings: Map = new Map(); + #bindings: Map = + new Map(); #env: Environment; errors: CompilerError[] = []; @@ -124,23 +127,67 @@ export default class HIRBuilder { }; } - resolveIdentifier(node: t.Identifier): Identifier { - let identifier = this.#bindings.get(node); - if (identifier == null) { - const id = this.nextIdentifierId; - identifier = { - id, - name: node.name, - mutableRange: { - start: makeInstructionId(0), - end: makeInstructionId(0), - }, - scope: null, - type: makeType(), - }; - this.#bindings.set(node, identifier); + /** + * Maps an Identifier (or JSX identifier) Babel node to an internal `Identifier` + * which represents the variable being referenced, according to the JS scoping rules. + * + * Because Forget does not preserve _all_ block scopes in the input (only those that + * happen to occur from control flow), this resolution ensures that different variables + * with the same name are mapped to a unique name. Concretely, this function maintains + * the invariant that all references to a given variable will return an `Identifier` + * with the same (unique for the function) `name` and `id`. + * + * Example: + * + * ```javascript + * function foo() { + * const x = 0; + * { + * const x = 1; + * } + * return x; + * } + * ``` + * + * The above converts as follows: + * + * ``` + * Const Identifier { name: 'x', id: 0 } = Primitive { value: 0 }; + * Const Identifier { name: 'x_0', id: 1 } = Primitive { value: 1 }; + * Return Identifier { name: 'x', id: 0}; + * ``` + */ + resolveIdentifier( + path: NodePath + ): Identifier { + const originalName = path.node.name; + const node = + path.scope.getBindingIdentifier(originalName) ?? + getOrAddGlobal(originalName); + let name = originalName; + let index = 0; + while (true) { + const mapping = this.#bindings.get(name); + if (mapping === undefined) { + const id = this.nextIdentifierId; + const identifier: Identifier = { + id, + name, + mutableRange: { + start: makeInstructionId(0), + end: makeInstructionId(0), + }, + scope: null, + type: makeType(), + }; + this.#bindings.set(name, { node, identifier }); + return identifier; + } else if (mapping.node === node) { + return mapping.identifier; + } else { + name = `${originalName}_${index++}`; + } } - return identifier; } /** diff --git a/compiler/forget/src/__tests__/fixtures/hir/ssa-shadowing.expect.md b/compiler/forget/src/__tests__/fixtures/hir/ssa-shadowing.expect.md index e56789be06..3f59ef5659 100644 --- a/compiler/forget/src/__tests__/fixtures/hir/ssa-shadowing.expect.md +++ b/compiler/forget/src/__tests__/fixtures/hir/ssa-shadowing.expect.md @@ -35,11 +35,11 @@ function Foo(cond) { str$0 = str; if (cond) { - const str$1 = "other test"; - log(str$1); + const str_0 = "other test"; + log(str_0); } else { - const str$2 = "fallthrough test"; - str$0 = str$2; + const str$1 = "fallthrough test"; + str$0 = str$1; } $[0] = cond; diff --git a/compiler/forget/src/__tests__/fixtures/hir/type-test-return-type-inference.expect.md b/compiler/forget/src/__tests__/fixtures/hir/type-test-return-type-inference.expect.md index d8cb32a74f..59ade258b2 100644 --- a/compiler/forget/src/__tests__/fixtures/hir/type-test-return-type-inference.expect.md +++ b/compiler/forget/src/__tests__/fixtures/hir/type-test-return-type-inference.expect.md @@ -40,7 +40,7 @@ function component() { const z = {}; } - const z = foo(); + const z_0 = foo(); } ```