From aa0ac8a74c848be2e45ea02116f7c9bda7bdd3a3 Mon Sep 17 00:00:00 2001 From: Dylan Hunn Date: Tue, 26 Sep 2023 22:25:18 -0700 Subject: [PATCH] refactor(compiler): Move conditional branch logic into an expression. (#51931) This is a pure refactor: we previously crammed a lot of data into a complicated array on the conditional op. Now, we use a new conditional branch expression to store that information. PR Close #51931 --- .../src/template/pipeline/ir/src/enums.ts | 5 +++ .../template/pipeline/ir/src/expression.ts | 44 +++++++++++++++++-- .../template/pipeline/ir/src/ops/update.ts | 7 +-- .../src/template/pipeline/src/ingest.ts | 10 +++-- .../pipeline/src/phases/conditionals.ts | 14 +++--- 5 files changed, 64 insertions(+), 16 deletions(-) diff --git a/packages/compiler/src/template/pipeline/ir/src/enums.ts b/packages/compiler/src/template/pipeline/ir/src/enums.ts index 879e1503a69..6834dca2583 100644 --- a/packages/compiler/src/template/pipeline/ir/src/enums.ts +++ b/packages/compiler/src/template/pipeline/ir/src/enums.ts @@ -304,6 +304,11 @@ export enum ExpressionKind { * An expression that will cause a literal slot index to be emitted. */ SlotLiteralExpr, + + /** + * A test expression for a conditional op. + */ + ConditionalCase, } /** diff --git a/packages/compiler/src/template/pipeline/ir/src/expression.ts b/packages/compiler/src/template/pipeline/ir/src/expression.ts index 68847596179..13f0b28060f 100644 --- a/packages/compiler/src/template/pipeline/ir/src/expression.ts +++ b/packages/compiler/src/template/pipeline/ir/src/expression.ts @@ -23,7 +23,7 @@ export type Expression = LexicalReadExpr|ReferenceExpr|ContextExpr|NextContextExpr|GetCurrentViewExpr|RestoreViewExpr| ResetViewExpr|ReadVariableExpr|PureFunctionExpr|PureFunctionParameterExpr|PipeBindingExpr| PipeBindingVariadicExpr|SafePropertyReadExpr|SafeKeyedReadExpr|SafeInvokeFunctionExpr|EmptyExpr| - AssignTemporaryExpr|ReadTemporaryExpr|SanitizerExpr|SlotLiteralExpr; + AssignTemporaryExpr|ReadTemporaryExpr|SanitizerExpr|SlotLiteralExpr|ConditionalCaseExpr; /** * Transformer type which converts expressions into general `o.Expression`s (which may be an @@ -762,6 +762,44 @@ export class SlotLiteralExpr extends ExpressionBase { override transformInternalExpressions(): void {} } +export class ConditionalCaseExpr extends ExpressionBase { + override readonly kind = ExpressionKind.ConditionalCase; + + /** + * Create an expression for one branch of a conditional. + * @param expr The expression to be tested for this case. Might be null, as in an `else` case. + * @param target The Xref of the view to be displayed if this condition is true. + */ + constructor(public expr: o.Expression|null, readonly target: XrefId) { + super(); + } + + override visitExpression(visitor: o.ExpressionVisitor, context: any): any { + if (this.expr !== null) { + this.expr.visitExpression(visitor, context); + } + } + + override isEquivalent(e: Expression): boolean { + return e instanceof ConditionalCaseExpr && e.expr === this.expr; + } + + override isConstant() { + return true; + } + + override clone(): ConditionalCaseExpr { + return new ConditionalCaseExpr(this.expr, this.target); + } + + override transformInternalExpressions(transform: ExpressionTransform, flags: VisitorContextFlag): + void { + if (this.expr !== null) { + this.expr = transformExpressionsInExpression(this.expr, transform, flags); + } + } +} + /** * Visits all `Expression`s in the AST of `op` with the `visitor` function. */ @@ -831,11 +869,11 @@ export function transformExpressionsInOp( break; case OpKind.Conditional: for (const condition of op.conditions) { - if (condition[1] === null) { + if (condition.expr === null) { // This is a default case. continue; } - condition[1] = transformExpressionsInExpression(condition[1]!, transform, flags); + condition.expr = transformExpressionsInExpression(condition.expr, transform, flags); } if (op.processed !== null) { op.processed = transformExpressionsInExpression(op.processed, transform, flags); diff --git a/packages/compiler/src/template/pipeline/ir/src/ops/update.ts b/packages/compiler/src/template/pipeline/ir/src/ops/update.ts index df9e917038f..f6895d87f40 100644 --- a/packages/compiler/src/template/pipeline/ir/src/ops/update.ts +++ b/packages/compiler/src/template/pipeline/ir/src/ops/update.ts @@ -11,6 +11,7 @@ import * as i18n from '../../../../../i18n/i18n_ast'; import * as o from '../../../../../output/output_ast'; import {ParseSourceSpan} from '../../../../../parse_util'; import {BindingKind, OpKind} from '../enums'; +import type {ConditionalCaseExpr} from '../expression'; import {Op, XrefId} from '../operations'; import {ConsumesVarsTrait, DependsOnSlotContextOpTrait, TRAIT_CONSUMES_VARS, TRAIT_DEPENDS_ON_SLOT_CONTEXT, TRAIT_USES_SLOT_INDEX, UsesSlotIndexTrait} from '../traits'; @@ -496,7 +497,7 @@ export interface ConditionalOp extends Op, DependsOnSlotContextOp * Each possible embedded view that could be displayed has a condition (or is default). This * structure maps each view xref to its corresponding condition. */ - conditions: Array<[XrefId, o.Expression|null]>; + conditions: Array; /** * After processing, this will be a single collapsed Joost-expression that evaluates to the right @@ -511,13 +512,13 @@ export interface ConditionalOp extends Op, DependsOnSlotContextOp * Create a conditional op, which will display an embedded view according to a condtion. */ export function createConditionalOp( - target: XrefId, test: o.Expression|null, conditions: Array<[XrefId, o.Expression | null]>, + target: XrefId, test: o.Expression|null, conditions: Array, sourceSpan: ParseSourceSpan): ConditionalOp { return { kind: OpKind.Conditional, target, test, - conditions: conditions, + conditions, processed: null, sourceSpan, ...NEW_OP, diff --git a/packages/compiler/src/template/pipeline/src/ingest.ts b/packages/compiler/src/template/pipeline/src/ingest.ts index b07d37d6cfc..63f1130556f 100644 --- a/packages/compiler/src/template/pipeline/src/ingest.ts +++ b/packages/compiler/src/template/pipeline/src/ingest.ts @@ -269,7 +269,7 @@ function ingestBoundText(unit: ViewCompilationUnit, text: t.BoundText): void { */ function ingestIfBlock(unit: ViewCompilationUnit, ifBlock: t.IfBlock): void { let firstXref: ir.XrefId|null = null; - let conditions: Array<[ir.XrefId, o.Expression | null]> = []; + let conditions: Array = []; for (const ifCase of ifBlock.branches) { const cView = unit.job.allocateView(unit.xref); if (firstXref === null) { @@ -278,7 +278,8 @@ function ingestIfBlock(unit: ViewCompilationUnit, ifBlock: t.IfBlock): void { unit.create.push( ir.createTemplateOp(cView.xref, 'Conditional', ir.Namespace.HTML, true, ifCase.sourceSpan)); const caseExpr = ifCase.expression ? convertAst(ifCase.expression, unit.job, null) : null; - conditions.push([cView.xref, caseExpr]); + const conditionalCaseExpr = new ir.ConditionalCaseExpr(caseExpr, cView.xref); + conditions.push(conditionalCaseExpr); ingestNodes(cView, ifCase.children); } const conditional = ir.createConditionalOp(firstXref!, null, conditions, ifBlock.sourceSpan); @@ -290,7 +291,7 @@ function ingestIfBlock(unit: ViewCompilationUnit, ifBlock: t.IfBlock): void { */ function ingestSwitchBlock(unit: ViewCompilationUnit, switchBlock: t.SwitchBlock): void { let firstXref: ir.XrefId|null = null; - let conditions: Array<[ir.XrefId, o.Expression | null]> = []; + let conditions: Array = []; for (const switchCase of switchBlock.cases) { const cView = unit.job.allocateView(unit.xref); if (firstXref === null) { @@ -301,7 +302,8 @@ function ingestSwitchBlock(unit: ViewCompilationUnit, switchBlock: t.SwitchBlock const caseExpr = switchCase.expression ? convertAst(switchCase.expression, unit.job, switchBlock.startSourceSpan) : null; - conditions.push([cView.xref, caseExpr]); + const conditionalCaseExpr = new ir.ConditionalCaseExpr(caseExpr, cView.xref); + conditions.push(conditionalCaseExpr); ingestNodes(cView, switchCase.children); } const conditional = ir.createConditionalOp( diff --git a/packages/compiler/src/template/pipeline/src/phases/conditionals.ts b/packages/compiler/src/template/pipeline/src/phases/conditionals.ts index 7b2869826af..9ff2919d7a0 100644 --- a/packages/compiler/src/template/pipeline/src/phases/conditionals.ts +++ b/packages/compiler/src/template/pipeline/src/phases/conditionals.ts @@ -23,9 +23,9 @@ export function phaseConditionals(job: ComponentCompilationJob): void { let test: o.Expression; // Any case with a `null` condition is `default`. If one exists, default to it instead. - const defaultCase = op.conditions.findIndex(([xref, cond]) => cond === null); + const defaultCase = op.conditions.findIndex((cond) => cond.expr === null); if (defaultCase >= 0) { - const [xref, cond] = op.conditions.splice(defaultCase, 1)[0]; + const xref = op.conditions.splice(defaultCase, 1)[0].target; test = new ir.SlotLiteralExpr(xref); } else { // By default, a switch evaluates to `-1`, causing no template to be displayed. @@ -37,15 +37,17 @@ export function phaseConditionals(job: ComponentCompilationJob): void { // For each remaining condition, test whether the temporary satifies the check. for (let i = op.conditions.length - 1; i >= 0; i--) { - let [xref, check]: [ir.XrefId, o.Expression|null] = op.conditions[i]; - if (check === null) { + let conditionalCase = op.conditions[i]; + if (conditionalCase.expr === null) { continue; } if (tmp !== null) { const useTmp = i === 0 ? tmp : new ir.ReadTemporaryExpr(tmp.xref); - check = new o.BinaryOperatorExpr(o.BinaryOperator.Identical, useTmp, check); + conditionalCase.expr = + new o.BinaryOperatorExpr(o.BinaryOperator.Identical, useTmp, conditionalCase.expr); } - test = new o.ConditionalExpr(check, new ir.SlotLiteralExpr(xref), test); + test = new o.ConditionalExpr( + conditionalCase.expr, new ir.SlotLiteralExpr(conditionalCase.target), test); } // Save the resulting aggregate Joost-expression.