From 05672ab13644b1cb03c3683fdfa4c2d186438938 Mon Sep 17 00:00:00 2001 From: Pete Bacon Darwin Date: Thu, 1 Oct 2020 13:43:26 +0100 Subject: [PATCH] refactor(compiler-cli): `attachComments()` now expects a defined `leadingComments` array (#39076) Previously the value passed to `AstFactory.attachComments()` could be `undefined` which is counterintuitive, since why attach something that doesn't exist? Now it expects there to be a defined array. Further it no longer returns a statement. Both these aspects of the interface were designed to make the usage simpler but has the result of complicating the implemenation. The `ExpressionTranslatorVisitor` now has a helper function (`attachComments()`) to handle `leadingComments` being undefined and also returning the statement. This keeps the usage in the translator simple, while ensuring that the `AstFactory` API is not influenced by how it is used. PR Close #39076 --- .../linker/src/ast/babel/babel_ast_factory.ts | 9 ++++----- .../ngtsc/translator/src/api/ast_factory.ts | 5 ++--- .../src/ngtsc/translator/src/translator.ts | 20 +++++++++++++------ .../translator/src/typescript_ast_factory.ts | 8 +------- .../src/transformers/node_emitter.ts | 2 +- 5 files changed, 22 insertions(+), 22 deletions(-) diff --git a/packages/compiler-cli/linker/src/ast/babel/babel_ast_factory.ts b/packages/compiler-cli/linker/src/ast/babel/babel_ast_factory.ts index 60b3459fac..afefce4c1f 100644 --- a/packages/compiler-cli/linker/src/ast/babel/babel_ast_factory.ts +++ b/packages/compiler-cli/linker/src/ast/babel/babel_ast_factory.ts @@ -10,17 +10,16 @@ import * as t from '@babel/types'; import {AstFactory, BinaryOperator, LeadingComment, ObjectLiteralProperty, SourceMapRange, TemplateLiteral, VariableDeclarationType} from '../../../../src/ngtsc/translator'; import {assert} from '../utils'; +/** + * A Babel flavored implementation of the AstFactory. + */ export class BabelAstFactory implements AstFactory { - attachComments(statement: t.Statement, leadingComments: LeadingComment[]|undefined): t.Statement { - if (leadingComments === undefined) { - return statement; - } + attachComments(statement: t.Statement, leadingComments: LeadingComment[]): void { // We must process the comments in reverse because `t.addComment()` will add new ones in front. for (let i = leadingComments.length - 1; i >= 0; i--) { const comment = leadingComments[i]; t.addComment(statement, 'leading', comment.toString(), !comment.multiline); } - return statement; } createArrayLiteral = t.arrayExpression; diff --git a/packages/compiler-cli/src/ngtsc/translator/src/api/ast_factory.ts b/packages/compiler-cli/src/ngtsc/translator/src/api/ast_factory.ts index fb4325dd6d..4a5890756d 100644 --- a/packages/compiler-cli/src/ngtsc/translator/src/api/ast_factory.ts +++ b/packages/compiler-cli/src/ngtsc/translator/src/api/ast_factory.ts @@ -17,11 +17,10 @@ export interface AstFactory { /** * Attach the `leadingComments` to the given `statement` node. * - * @param statement the statement where the comment is to be attached. + * @param statement the statement where the comments are to be attached. * @param leadingComments the comments to attach. - * @returns the node passed in as `statement` with the comments attached. */ - attachComments(statement: TStatement, leadingComments: LeadingComment[]|undefined): TStatement; + attachComments(statement: TStatement, leadingComments: LeadingComment[]): void; /** * Create a literal array expresion (e.g. `[expr1, expr2]`). diff --git a/packages/compiler-cli/src/ngtsc/translator/src/translator.ts b/packages/compiler-cli/src/ngtsc/translator/src/translator.ts index d644ec4cfc..2a2da50088 100644 --- a/packages/compiler-cli/src/ngtsc/translator/src/translator.ts +++ b/packages/compiler-cli/src/ngtsc/translator/src/translator.ts @@ -61,14 +61,14 @@ export class ExpressionTranslatorVisitor implements o.E const varType = this.downlevelVariableDeclarations ? 'var' : stmt.hasModifier(o.StmtModifier.Final) ? 'const' : 'let'; - return this.factory.attachComments( + return this.attachComments( this.factory.createVariableDeclaration( stmt.name, stmt.value?.visitExpression(this, context.withExpressionMode), varType), stmt.leadingComments); } visitDeclareFunctionStmt(stmt: o.DeclareFunctionStmt, context: Context): TStatement { - return this.factory.attachComments( + return this.attachComments( this.factory.createFunctionDeclaration( stmt.name, stmt.params.map(param => param.name), this.factory.createBlock( @@ -77,14 +77,14 @@ export class ExpressionTranslatorVisitor implements o.E } visitExpressionStmt(stmt: o.ExpressionStatement, context: Context): TStatement { - return this.factory.attachComments( + return this.attachComments( this.factory.createExpressionStatement( stmt.expr.visitExpression(this, context.withStatementMode)), stmt.leadingComments); } visitReturnStmt(stmt: o.ReturnStatement, context: Context): TStatement { - return this.factory.attachComments( + return this.attachComments( this.factory.createReturnStatement( stmt.value.visitExpression(this, context.withExpressionMode)), stmt.leadingComments); @@ -95,7 +95,7 @@ export class ExpressionTranslatorVisitor implements o.E } visitIfStmt(stmt: o.IfStmt, context: Context): TStatement { - return this.factory.attachComments( + return this.attachComments( this.factory.createIfStatement( stmt.condition.visitExpression(this, context), this.factory.createBlock( @@ -111,7 +111,7 @@ export class ExpressionTranslatorVisitor implements o.E } visitThrowStmt(stmt: o.ThrowStmt, context: Context): TStatement { - return this.factory.attachComments( + return this.attachComments( this.factory.createThrowStatement( stmt.error.visitExpression(this, context.withExpressionMode)), stmt.leadingComments); @@ -387,6 +387,14 @@ export class ExpressionTranslatorVisitor implements o.E T { return this.factory.setSourceMapRange(ast, createRange(span)); } + + private attachComments(statement: TStatement, leadingComments: o.LeadingComment[]|undefined): + TStatement { + if (leadingComments !== undefined) { + this.factory.attachComments(statement, leadingComments); + } + return statement; + } } /** diff --git a/packages/compiler-cli/src/ngtsc/translator/src/typescript_ast_factory.ts b/packages/compiler-cli/src/ngtsc/translator/src/typescript_ast_factory.ts index aa0204dadd..a75c03c324 100644 --- a/packages/compiler-cli/src/ngtsc/translator/src/typescript_ast_factory.ts +++ b/packages/compiler-cli/src/ngtsc/translator/src/typescript_ast_factory.ts @@ -234,12 +234,7 @@ export function createTemplateTail(cooked: string, raw: string): ts.TemplateTail * @param statement The statement that will have comments attached. * @param leadingComments The comments to attach to the statement. */ -export function attachComments( - statement: T, leadingComments: LeadingComment[]|undefined): T { - if (leadingComments === undefined) { - return statement; - } - +export function attachComments(statement: ts.Statement, leadingComments: LeadingComment[]): void { for (const comment of leadingComments) { const commentKind = comment.multiline ? ts.SyntaxKind.MultiLineCommentTrivia : ts.SyntaxKind.SingleLineCommentTrivia; @@ -252,5 +247,4 @@ export function attachComments( } } } - return statement; } diff --git a/packages/compiler-cli/src/transformers/node_emitter.ts b/packages/compiler-cli/src/transformers/node_emitter.ts index 333c324d50..6a27f1798b 100644 --- a/packages/compiler-cli/src/transformers/node_emitter.ts +++ b/packages/compiler-cli/src/transformers/node_emitter.ts @@ -287,7 +287,7 @@ export class NodeEmitterVisitor implements StatementVisitor, ExpressionVisitor { if (tsNode && !this._nodeMap.has(tsNode)) { this._nodeMap.set(tsNode, ngNode); } - if (tsNode !== null && ngNode instanceof Statement) { + if (tsNode !== null && ngNode instanceof Statement && ngNode.leadingComments !== undefined) { attachComments(tsNode as unknown as ts.Statement, ngNode.leadingComments); } return tsNode as RecordedNode;