From 31e128a4413e2902e5fbfda5e5f6e9fdfc1e7619 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Wed, 6 Mar 2024 11:07:08 -0800 Subject: [PATCH] Use opaque type for Identifier.name for improved correctness Uses an enum for Identifier.name to distinguish originally named identifiers vs promoted temporaries. An opaque type for the named identifier variant makes it hard to accidentally create that type. --- .../babel-plugin-react-forget/src/HIR/HIR.ts | 78 ++++++++++++++++++- .../src/HIR/HIRBuilder.ts | 7 +- .../src/HIR/PrintHIR.ts | 8 +- .../src/Inference/AnalyseFunctions.ts | 9 ++- ...neImmediatelyInvokedFunctionExpressions.ts | 8 +- .../src/Optimization/DeadCodeElimination.ts | 4 +- .../ReactiveScopes/CodegenReactiveFunction.ts | 8 +- ...tractScopeDeclarationsFromDestructuring.ts | 8 +- .../ReactiveScopes/PromoteUsedTemporaries.ts | 8 +- .../ReactiveScopes/PropagateEarlyReturns.ts | 3 +- .../src/ReactiveScopes/RenameVariables.ts | 23 +++--- .../src/SSA/LeaveSSA.ts | 30 +++---- .../src/Validation/ValidateHooksUsage.ts | 9 ++- 13 files changed, 150 insertions(+), 53 deletions(-) diff --git a/compiler/packages/babel-plugin-react-forget/src/HIR/HIR.ts b/compiler/packages/babel-plugin-react-forget/src/HIR/HIR.ts index 3d92a54572..5a156f30b1 100644 --- a/compiler/packages/babel-plugin-react-forget/src/HIR/HIR.ts +++ b/compiler/packages/babel-plugin-react-forget/src/HIR/HIR.ts @@ -959,7 +959,7 @@ export type Identifier = { */ id: IdentifierId; // null for temporaries. name is primarily used for debugging. - name: string | null; + name: IdentifierName | null; // The range for which this variable is mutable mutableRange: MutableRange; /* @@ -970,6 +970,82 @@ export type Identifier = { type: Type; }; +export type IdentifierName = + | { kind: "named"; value: ValidIdentifierName } + | { kind: "promoted"; value: string }; + +/** + * Simulated opaque type for identifier names to ensure values can only be created + * through the below helpers. + */ +const opaqueValidIdentifierName = Symbol(); +export type ValidIdentifierName = string & { + [opaqueValidIdentifierName]: "ValidIdentifierName"; +}; + +/** + * Creates a valid identifier name. This should *not* be used for synthesizing + * identifier names: only call this method for identifier names that appear in the + * original source code. + */ +export function makeIdentifierName(name: string): IdentifierName { + CompilerError.invariant(t.isValidIdentifier(name), { + reason: `Expected a valid identifier name`, + loc: GeneratedSource, + description: `'${name}' is not a valid JavaScript identifier`, + suggestions: null, + }); + return { + kind: "named", + value: name as ValidIdentifierName, + }; +} + +/** + * Given an unnamed identifier, promote it to a named identifier. + */ +export function promoteTemporaryToNamedIdentifier( + identifier: Identifier +): void { + CompilerError.invariant(identifier.name === null, { + reason: `Expected a temporary (unnamed) identifier`, + loc: GeneratedSource, + description: `Identifier already has a name, '${identifier.name}'`, + suggestions: null, + }); + identifier.name = { + kind: "promoted", + value: `#t${identifier.id}`, + }; +} + +export function isPromotedTemporary(name: string): boolean { + return name.startsWith("#t"); +} + +/** + * Given an unnamed identifier, promote it to a named identifier, distinguishing + * it as a value that needs to be capitalized since it appears in JSX element tag position + */ +export function promoteTemporaryJsxTagToNamedIdentifier( + identifier: Identifier +): void { + CompilerError.invariant(identifier.name === null, { + reason: `Expected a temporary (unnamed) identifier`, + loc: GeneratedSource, + description: `Identifier already has a name, '${identifier.name}'`, + suggestions: null, + }); + identifier.name = { + kind: "promoted", + value: `#T${identifier.id}`, + }; +} + +export function isPromotedJsxTemporary(name: string): boolean { + return name.startsWith("#T"); +} + export type AbstractValue = { kind: ValueKind; reason: ReadonlySet; diff --git a/compiler/packages/babel-plugin-react-forget/src/HIR/HIRBuilder.ts b/compiler/packages/babel-plugin-react-forget/src/HIR/HIRBuilder.ts index 390bf7989f..3a2b703cf1 100644 --- a/compiler/packages/babel-plugin-react-forget/src/HIR/HIRBuilder.ts +++ b/compiler/packages/babel-plugin-react-forget/src/HIR/HIRBuilder.ts @@ -25,6 +25,7 @@ import { Place, Terminal, makeBlockId, + makeIdentifierName, makeInstructionId, makeType, } from "./HIR"; @@ -260,8 +261,8 @@ export default class HIRBuilder { return null; } const resolvedBinding = this.resolveBinding(babelBinding.identifier); - if (resolvedBinding.name && resolvedBinding.name !== originalName) { - babelBinding.scope.rename(originalName, resolvedBinding.name); + if (resolvedBinding.name && resolvedBinding.name.value !== originalName) { + babelBinding.scope.rename(originalName, resolvedBinding.name.value); } return resolvedBinding; } @@ -285,7 +286,7 @@ export default class HIRBuilder { const id = this.nextIdentifierId; const identifier: Identifier = { id, - name, + name: makeIdentifierName(name), mutableRange: { start: makeInstructionId(0), end: makeInstructionId(0), diff --git a/compiler/packages/babel-plugin-react-forget/src/HIR/PrintHIR.ts b/compiler/packages/babel-plugin-react-forget/src/HIR/PrintHIR.ts index e0462c1be9..e86c62c242 100644 --- a/compiler/packages/babel-plugin-react-forget/src/HIR/PrintHIR.ts +++ b/compiler/packages/babel-plugin-react-forget/src/HIR/PrintHIR.ts @@ -15,6 +15,7 @@ import type { HIR, HIRFunction, Identifier, + IdentifierName, Instruction, InstructionValue, LValue, @@ -724,8 +725,11 @@ export function printIdentifier(id: Identifier): string { return `${printName(id.name)}\$${id.id}${printScope(id.scope)}`; } -function printName(name: string | null): string { - return name ?? ""; +function printName(name: IdentifierName | null): string { + if (name === null) { + return ""; + } + return name.value; } function printScope(scope: ReactiveScope | null): string { diff --git a/compiler/packages/babel-plugin-react-forget/src/Inference/AnalyseFunctions.ts b/compiler/packages/babel-plugin-react-forget/src/Inference/AnalyseFunctions.ts index e69486b5e4..04a0118f74 100644 --- a/compiler/packages/babel-plugin-react-forget/src/Inference/AnalyseFunctions.ts +++ b/compiler/packages/babel-plugin-react-forget/src/Inference/AnalyseFunctions.ts @@ -10,6 +10,7 @@ import { Effect, HIRFunction, Identifier, + IdentifierName, LoweredFunction, Place, ReactiveScopeDependency, @@ -123,13 +124,13 @@ function infer( isMutatedOrReassigned(operand.identifier) && operand.identifier.name !== null ) { - mutations.set(operand.identifier.name, operand.effect); + mutations.set(operand.identifier.name.value, operand.effect); } operand.identifier.mutableRange.end = operand.identifier.mutableRange.start; } for (const dep of loweredFunc.dependencies) { - let name: string | null = null; + let name: IdentifierName | null = null; if (state.properties.has(dep.identifier)) { const receiver = state.properties.get(dep.identifier)!; @@ -148,7 +149,7 @@ function infer( */ dep.effect = Effect.Capture; } else if (name !== null) { - const effect = mutations.get(name); + const effect = mutations.get(name.value); if (effect !== undefined) { dep.effect = effect === Effect.Unknown ? Effect.Capture : effect; } @@ -171,7 +172,7 @@ function infer( suggestions: null, }); - const effect = mutations.get(place.identifier.name); + const effect = mutations.get(place.identifier.name.value); if (effect !== undefined) { place.effect = effect === Effect.Unknown ? Effect.Capture : effect; loweredFunc.dependencies.push(place); diff --git a/compiler/packages/babel-plugin-react-forget/src/Inference/InlineImmediatelyInvokedFunctionExpressions.ts b/compiler/packages/babel-plugin-react-forget/src/Inference/InlineImmediatelyInvokedFunctionExpressions.ts index 7c654b82e1..4e5257c37a 100644 --- a/compiler/packages/babel-plugin-react-forget/src/Inference/InlineImmediatelyInvokedFunctionExpressions.ts +++ b/compiler/packages/babel-plugin-react-forget/src/Inference/InlineImmediatelyInvokedFunctionExpressions.ts @@ -14,13 +14,13 @@ import { GeneratedSource, GotoVariant, HIRFunction, - Identifier, IdentifierId, InstructionKind, LabelTerminal, Place, makeInstructionId, makeType, + promoteTemporaryToNamedIdentifier, reversePostorderBlocks, } from "../HIR"; import { markInstructionIds, markPredecessors } from "../HIR/HIRBuilder"; @@ -159,7 +159,7 @@ export function inlineImmediatelyInvokedFunctionExpressions( declareTemporary(fn.env, block, result); // Promote the temporary with a name as we require this to persist - promoteTemporary(result.identifier); + promoteTemporaryToNamedIdentifier(result.identifier); /* * Rewrite blocks from the lambda to replace any `return` with a @@ -293,7 +293,3 @@ function declareTemporary( }, }); } - -function promoteTemporary(temp: Identifier): void { - temp.name = `#t${temp.id}`; -} diff --git a/compiler/packages/babel-plugin-react-forget/src/Optimization/DeadCodeElimination.ts b/compiler/packages/babel-plugin-react-forget/src/Optimization/DeadCodeElimination.ts index e8a56a2a9d..2e450f4f3a 100644 --- a/compiler/packages/babel-plugin-react-forget/src/Optimization/DeadCodeElimination.ts +++ b/compiler/packages/babel-plugin-react-forget/src/Optimization/DeadCodeElimination.ts @@ -68,7 +68,7 @@ class State { reference(identifier: Identifier): void { this.identifiers.add(identifier.id); if (identifier.name !== null) { - this.named.add(identifier.name); + this.named.add(identifier.name.value); } } @@ -80,7 +80,7 @@ class State { isIdOrNameUsed(identifier: Identifier): boolean { return ( this.identifiers.has(identifier.id) || - (identifier.name !== null && this.named.has(identifier.name)) + (identifier.name !== null && this.named.has(identifier.name.value)) ); } 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 6452236891..49bd7a276a 100644 --- a/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/CodegenReactiveFunction.ts +++ b/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/CodegenReactiveFunction.ts @@ -541,14 +541,16 @@ function codegenReactiveScope( t.ifStatement( t.binaryExpression( "!==", - t.identifier(scope.earlyReturnValue.value.name!), + t.identifier(scope.earlyReturnValue.value.name!.value), t.callExpression( t.memberExpression(t.identifier("Symbol"), t.identifier("for")), [t.stringLiteral(EARLY_RETURN_SENTINEL)] ) ), t.blockStatement([ - t.returnStatement(t.identifier(scope.earlyReturnValue.value.name!)), + t.returnStatement( + t.identifier(scope.earlyReturnValue.value.name!.value) + ), ]) ) ); @@ -2009,7 +2011,7 @@ function codegenPlace(cx: Context, place: Place): t.Expression | t.JSXText { function convertIdentifier(identifier: Identifier): t.Identifier { if (identifier.name !== null) { - return t.identifier(`${identifier.name}`); + return t.identifier(identifier.name.value); } return t.identifier(`t${identifier.id}`); } diff --git a/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/ExtractScopeDeclarationsFromDestructuring.ts b/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/ExtractScopeDeclarationsFromDestructuring.ts index 7341cc5f22..b77bf1197c 100644 --- a/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/ExtractScopeDeclarationsFromDestructuring.ts +++ b/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/ExtractScopeDeclarationsFromDestructuring.ts @@ -15,6 +15,7 @@ import { ReactiveInstruction, ReactiveScopeBlock, ReactiveStatement, + promoteTemporaryToNamedIdentifier, } from "../HIR"; import { eachPatternOperand, mapPatternOperands } from "../HIR/visitors"; import { @@ -152,8 +153,13 @@ function transformDestructuring( const tempId = state.env.nextIdentifierId; const temporary = { ...place, - identifier: { ...place.identifier, id: tempId, name: `#t${tempId}` }, + identifier: { + ...place.identifier, + id: tempId, + name: null, // overwritten below + }, }; + promoteTemporaryToNamedIdentifier(temporary.identifier); renamed.set(place, temporary); return temporary; }); 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 319bf7f930..f83e5e8b40 100644 --- a/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/PromoteUsedTemporaries.ts +++ b/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/PromoteUsedTemporaries.ts @@ -15,11 +15,12 @@ import { ReactiveInstruction, ReactiveScopeBlock, ReactiveValue, + promoteTemporaryJsxTagToNamedIdentifier, + promoteTemporaryToNamedIdentifier, } from "../HIR/HIR"; import { ReactiveFunctionVisitor, visitReactiveFunction } from "./visitors"; type VisitorState = { - nextId: number; tags: JsxExpressionTags; }; class Visitor extends ReactiveFunctionVisitor { @@ -70,7 +71,6 @@ export function promoteUsedTemporaries(fn: ReactiveFunction): void { const tags: JsxExpressionTags = new Set(); visitReactiveFunction(fn, new CollectJsxTagsVisitor(), tags); const state: VisitorState = { - nextId: 0, tags, }; visitReactiveFunction(fn, new Visitor(), state); @@ -85,8 +85,8 @@ function promoteTemporary(identifier: Identifier, state: VisitorState): void { suggestions: null, }); if (state.tags.has(identifier.id)) { - identifier.name = `#T${state.nextId++}`; + promoteTemporaryJsxTagToNamedIdentifier(identifier); } else { - identifier.name = `#t${state.nextId++}`; + promoteTemporaryToNamedIdentifier(identifier); } } diff --git a/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/PropagateEarlyReturns.ts b/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/PropagateEarlyReturns.ts index 609df8fb57..d1d86e046a 100644 --- a/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/PropagateEarlyReturns.ts +++ b/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/PropagateEarlyReturns.ts @@ -16,6 +16,7 @@ import { ReactiveStatement, ReactiveTerminalStatement, makeInstructionId, + promoteTemporaryToNamedIdentifier, } from "../HIR"; import { createTemporaryPlace } from "../HIR/HIRBuilder"; import { EARLY_RETURN_SENTINEL } from "./CodegenReactiveFunction"; @@ -274,7 +275,7 @@ class Transform extends ReactiveFunctionTransform { earlyReturnValue = state.earlyReturnValue; } else { const identifier = createTemporaryPlace(this.env).identifier; - identifier.name = `#t${identifier.id}`; + promoteTemporaryToNamedIdentifier(identifier); earlyReturnValue = { label: this.env.nextBlockId, loc, 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 5cf68659a0..bbf3b74ef7 100644 --- a/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/RenameVariables.ts +++ b/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/RenameVariables.ts @@ -9,11 +9,15 @@ import { CompilerError } from "../CompilerError"; import { Identifier, IdentifierId, + IdentifierName, InstructionId, Place, ReactiveBlock, ReactiveFunction, ReactiveScopeBlock, + isPromotedJsxTemporary, + isPromotedTemporary, + makeIdentifierName, } from "../HIR/HIR"; import { ReactiveFunctionVisitor, visitReactiveFunction } from "./visitors"; @@ -88,7 +92,7 @@ class Visitor extends ReactiveFunctionVisitor { } class Scopes { - #seen: Map = new Map(); + #seen: Map = new Map(); #stack: Array> = [new Map()]; visit(identifier: Identifier): void { @@ -101,27 +105,28 @@ class Scopes { identifier.name = mappedName; return; } - let name = originalName; + let name: string = originalName.value; let id = 0; - if (name.startsWith("#t")) { + if (isPromotedTemporary(originalName.value)) { name = `t${id++}`; - } else if (name.startsWith("#T")) { + } else if (isPromotedJsxTemporary(originalName.value)) { name = `T${id++}`; } let previous = this.#lookup(name); while (previous !== null) { - if (originalName.startsWith("#t")) { + if (isPromotedTemporary(originalName.value)) { name = `t${id++}`; - } else if (originalName.startsWith("#T")) { + } else if (isPromotedJsxTemporary(originalName.value)) { name = `T${id++}`; } else { name = `${identifier.name}$${id++}`; } previous = this.#lookup(name); } - identifier.name = name; - this.#seen.set(identifier.id, name); - this.#stack.at(-1)!.set(name, identifier.id); + const identifierName = makeIdentifierName(name); + identifier.name = identifierName; + this.#seen.set(identifier.id, identifierName); + this.#stack.at(-1)!.set(identifierName.value, identifier.id); } #lookup(name: string): IdentifierId | null { diff --git a/compiler/packages/babel-plugin-react-forget/src/SSA/LeaveSSA.ts b/compiler/packages/babel-plugin-react-forget/src/SSA/LeaveSSA.ts index 3f0cc5afd4..a99e9b5240 100644 --- a/compiler/packages/babel-plugin-react-forget/src/SSA/LeaveSSA.ts +++ b/compiler/packages/babel-plugin-react-forget/src/SSA/LeaveSSA.ts @@ -99,7 +99,7 @@ export function leaveSSA(fn: HIRFunction): void { for (const param of fn.params) { let place: Place = param.kind === "Identifier" ? param : param.place; if (place.identifier.name !== null) { - declarations.set(place.identifier.name, { + declarations.set(place.identifier.name.value, { lvalue: { kind: InstructionKind.Let, place, @@ -145,13 +145,13 @@ export function leaveSSA(fn: HIRFunction): void { if (value.kind === "DeclareLocal") { const name = value.lvalue.place.identifier.name; if (name !== null) { - CompilerError.invariant(!declarations.has(name), { + CompilerError.invariant(!declarations.has(name.value), { reason: `Unexpected duplicate declaration`, - description: `Found duplicate declaration for '${name}'`, + description: `Found duplicate declaration for '${name.value}'`, loc: value.lvalue.place.loc, suggestions: null, }); - declarations.set(name, { + declarations.set(name.value, { lvalue: value.lvalue, place: value.lvalue.place, }); @@ -166,7 +166,9 @@ export function leaveSSA(fn: HIRFunction): void { loc: value.lvalue.loc, suggestions: null, }); - const originalLVal = declarations.get(value.lvalue.identifier.name); + const originalLVal = declarations.get( + value.lvalue.identifier.name.value + ); CompilerError.invariant(originalLVal !== undefined, { reason: `Expected update expression to be applied to a previously defined variable`, description: null, @@ -177,7 +179,7 @@ export function leaveSSA(fn: HIRFunction): void { } else if (value.kind === "StoreLocal") { if (value.lvalue.place.identifier.name != null) { const originalLVal = declarations.get( - value.lvalue.place.identifier.name + value.lvalue.place.identifier.name.value ); if ( originalLVal === undefined || @@ -194,7 +196,7 @@ export function leaveSSA(fn: HIRFunction): void { suggestions: null, } ); - declarations.set(value.lvalue.place.identifier.name, { + declarations.set(value.lvalue.place.identifier.name.value, { lvalue: value.lvalue, place: value.lvalue.place, }); @@ -227,7 +229,7 @@ export function leaveSSA(fn: HIRFunction): void { ); kind = InstructionKind.Const; } else { - const originalLVal = declarations.get(place.identifier.name); + const originalLVal = declarations.get(place.identifier.name.value); if ( originalLVal === undefined || originalLVal.lvalue === value.lvalue @@ -241,7 +243,7 @@ export function leaveSSA(fn: HIRFunction): void { suggestions: null, } ); - declarations.set(place.identifier.name, { + declarations.set(place.identifier.name.value, { lvalue: value.lvalue, place, }); @@ -388,10 +390,10 @@ export function leaveSSA(fn: HIRFunction): void { const value = initIdentifier.value; if (value.lvalue.place.identifier.name !== null) { const originalLVal = declarations.get( - value.lvalue.place.identifier.name + value.lvalue.place.identifier.name.value ); if (originalLVal === undefined) { - declarations.set(value.lvalue.place.identifier.name, { + declarations.set(value.lvalue.place.identifier.name.value, { lvalue: value.lvalue, place: value.lvalue.place, }); @@ -440,7 +442,7 @@ export function leaveSSA(fn: HIRFunction): void { loc: null, suggestions: null, }); - const declaration = declarations.get(phi.id.name); + const declaration = declarations.get(phi.id.name.value); CompilerError.invariant(declaration != null, { loc: null, reason: "Expected a declaration for all variables", @@ -480,7 +482,7 @@ export function leaveSSA(fn: HIRFunction): void { rewrites.set(phi.id, canonicalId); if (canonicalId.name !== null) { - const declaration = declarations.get(canonicalId.name); + const declaration = declarations.get(canonicalId.name.value); if (declaration !== undefined) { declaration.lvalue.kind = InstructionKind.Let; } @@ -512,7 +514,7 @@ function rewritePlace( if (nextIdentifier === prevIdentifier) return; place.identifier = nextIdentifier; } else if (prevIdentifier.name != null) { - const declaration = declarations.get(prevIdentifier.name); + const declaration = declarations.get(prevIdentifier.name.value); // Only rewrite identifiers that were declared within the function if (declaration === undefined) return; const originalIdentifier = declaration.place.identifier; diff --git a/compiler/packages/babel-plugin-react-forget/src/Validation/ValidateHooksUsage.ts b/compiler/packages/babel-plugin-react-forget/src/Validation/ValidateHooksUsage.ts index 85ebbda978..579f69ca2e 100644 --- a/compiler/packages/babel-plugin-react-forget/src/Validation/ValidateHooksUsage.ts +++ b/compiler/packages/babel-plugin-react-forget/src/Validation/ValidateHooksUsage.ts @@ -152,7 +152,10 @@ export function validateHooksUsage(fn: HIRFunction): void { const valueKinds = new Map(); function getKindForPlace(place: Place): Kind { const knownKind = valueKinds.get(place.identifier.id); - if (place.identifier.name !== null && isHookName(place.identifier.name)) { + if ( + place.identifier.name !== null && + isHookName(place.identifier.name.value) + ) { return joinKinds(knownKind ?? Kind.Local, Kind.PotentialHook); } else { return knownKind ?? Kind.Local; @@ -179,7 +182,7 @@ export function validateHooksUsage(fn: HIRFunction): void { for (const [, block] of fn.body.blocks) { for (const phi of block.phis) { let kind: Kind = - phi.id.name !== null && isHookName(phi.id.name) + phi.id.name !== null && isHookName(phi.id.name.value) ? Kind.PotentialHook : Kind.Local; for (const [, operand] of phi.operands) { @@ -333,7 +336,7 @@ export function validateHooksUsage(fn: HIRFunction): void { for (const lvalue of eachInstructionLValue(instr)) { const isHookProperty = lvalue.identifier.name !== null && - isHookName(lvalue.identifier.name); + isHookName(lvalue.identifier.name.value); let kind: Kind; switch (objectKind) { case Kind.Error: {