From 208ef7bd625b628f3fe7f4c53156569546314fb6 Mon Sep 17 00:00:00 2001 From: Keen Yee Liau Date: Fri, 21 Feb 2020 10:20:52 -0800 Subject: [PATCH] refactor(compiler): Remove NullAstVisitor and visitAstChildren (#35619) This commit removes the `NullAstVisitor` and `visitAstChildren` exported from `packages/compiler/src/expression_parser/ast.ts` because they contain duplicate and buggy implementation, and their use cases could be sufficiently covered by `RecursiveAstVisitor` if the latter implements the `visit` method. This use case is only needed in the language service. With this change, any visitor that extends `RecursiveAstVisitor` could just define their own `visit` function and the parent class will behave correctly. A bit of historical context: In language service, we need a way to tranverse the expression AST in a selective manner based on where the user's cursor is. This means we need a "filtering" function to decide which node to visit and which node to not visit. Instead of refactoring `RecursiveAstVisitor` to support this, `visitAstChildren` was created. `visitAstChildren` duplicates the implementation of `RecursiveAstVisitor`, but introduced some bugs along the way. For example, in `visitKeyedWrite`, it visits ``` obj -> key -> obj ``` instead of ``` obj -> key -> value ``` Moreover, because of the following line ``` visitor.visit && visitor.visit(ast, context) || ast.visit(visitor, context); ``` `visitAstChildren` visits every node *twice*. PR Close #35619 --- .../compiler/src/expression_parser/ast.ts | 183 +++++------------- .../compiler/src/render3/view/t2_binder.ts | 11 ++ .../test/expression_parser/ast_spec.ts | 42 ++++ .../compiler/test/render3/util/expression.ts | 11 ++ .../test/template_parser/util/expression.ts | 5 + .../language-service/src/expression_type.ts | 20 +- packages/language-service/src/expressions.ts | 6 +- 7 files changed, 134 insertions(+), 144 deletions(-) create mode 100644 packages/compiler/test/expression_parser/ast_spec.ts diff --git a/packages/compiler/src/expression_parser/ast.ts b/packages/compiler/src/expression_parser/ast.ts index f5ea683083..b489745488 100644 --- a/packages/compiler/src/expression_parser/ast.ts +++ b/packages/compiler/src/expression_parser/ast.ts @@ -311,109 +311,85 @@ export interface AstVisitor { visitSafeMethodCall(ast: SafeMethodCall, context: any): any; visitSafePropertyRead(ast: SafePropertyRead, context: any): any; visitASTWithSource?(ast: ASTWithSource, context: any): any; + /** + * This function is optionally defined to allow classes that implement this + * interface to selectively decide if the specified `ast` should be visited. + * @param ast node to visit + * @param context context that gets passed to the node and all its children + */ visit?(ast: AST, context?: any): any; } -export class NullAstVisitor implements AstVisitor { - visitBinary(ast: Binary, context: any): any {} - visitChain(ast: Chain, context: any): any {} - visitConditional(ast: Conditional, context: any): any {} - visitFunctionCall(ast: FunctionCall, context: any): any {} - visitImplicitReceiver(ast: ImplicitReceiver, context: any): any {} - visitInterpolation(ast: Interpolation, context: any): any {} - visitKeyedRead(ast: KeyedRead, context: any): any {} - visitKeyedWrite(ast: KeyedWrite, context: any): any {} - visitLiteralArray(ast: LiteralArray, context: any): any {} - visitLiteralMap(ast: LiteralMap, context: any): any {} - visitLiteralPrimitive(ast: LiteralPrimitive, context: any): any {} - visitMethodCall(ast: MethodCall, context: any): any {} - visitPipe(ast: BindingPipe, context: any): any {} - visitPrefixNot(ast: PrefixNot, context: any): any {} - visitNonNullAssert(ast: NonNullAssert, context: any): any {} - visitPropertyRead(ast: PropertyRead, context: any): any {} - visitPropertyWrite(ast: PropertyWrite, context: any): any {} - visitQuote(ast: Quote, context: any): any {} - visitSafeMethodCall(ast: SafeMethodCall, context: any): any {} - visitSafePropertyRead(ast: SafePropertyRead, context: any): any {} -} - export class RecursiveAstVisitor implements AstVisitor { - visitBinary(ast: Binary, context: any): any { - ast.left.visit(this, context); - ast.right.visit(this, context); - return null; + visit(ast: AST, context?: any): any { + // The default implementation just visits every node. + // Classes that extend RecursiveAstVisitor should override this function + // to selectively visit the specified node. + ast.visit(this, context); } - visitChain(ast: Chain, context: any): any { return this.visitAll(ast.expressions, context); } + visitBinary(ast: Binary, context: any): any { + this.visit(ast.left, context); + this.visit(ast.right, context); + } + visitChain(ast: Chain, context: any): any { this.visitAll(ast.expressions, context); } visitConditional(ast: Conditional, context: any): any { - ast.condition.visit(this, context); - ast.trueExp.visit(this, context); - ast.falseExp.visit(this, context); - return null; + this.visit(ast.condition, context); + this.visit(ast.trueExp, context); + this.visit(ast.falseExp, context); } visitPipe(ast: BindingPipe, context: any): any { - ast.exp.visit(this, context); + this.visit(ast.exp, context); this.visitAll(ast.args, context); - return null; } visitFunctionCall(ast: FunctionCall, context: any): any { - ast.target !.visit(this, context); + if (ast.target) { + this.visit(ast.target, context); + } this.visitAll(ast.args, context); - return null; } - visitImplicitReceiver(ast: ImplicitReceiver, context: any): any { return null; } + visitImplicitReceiver(ast: ImplicitReceiver, context: any): any {} visitInterpolation(ast: Interpolation, context: any): any { - return this.visitAll(ast.expressions, context); + this.visitAll(ast.expressions, context); } visitKeyedRead(ast: KeyedRead, context: any): any { - ast.obj.visit(this, context); - ast.key.visit(this, context); - return null; + this.visit(ast.obj, context); + this.visit(ast.key, context); } visitKeyedWrite(ast: KeyedWrite, context: any): any { - ast.obj.visit(this, context); - ast.key.visit(this, context); - ast.value.visit(this, context); - return null; + this.visit(ast.obj, context); + this.visit(ast.key, context); + this.visit(ast.value, context); } visitLiteralArray(ast: LiteralArray, context: any): any { - return this.visitAll(ast.expressions, context); + this.visitAll(ast.expressions, context); } - visitLiteralMap(ast: LiteralMap, context: any): any { return this.visitAll(ast.values, context); } - visitLiteralPrimitive(ast: LiteralPrimitive, context: any): any { return null; } + visitLiteralMap(ast: LiteralMap, context: any): any { this.visitAll(ast.values, context); } + visitLiteralPrimitive(ast: LiteralPrimitive, context: any): any {} visitMethodCall(ast: MethodCall, context: any): any { - ast.receiver.visit(this, context); - return this.visitAll(ast.args, context); - } - visitPrefixNot(ast: PrefixNot, context: any): any { - ast.expression.visit(this, context); - return null; - } - visitNonNullAssert(ast: NonNullAssert, context: any): any { - ast.expression.visit(this, context); - return null; - } - visitPropertyRead(ast: PropertyRead, context: any): any { - ast.receiver.visit(this, context); - return null; + this.visit(ast.receiver, context); + this.visitAll(ast.args, context); } + visitPrefixNot(ast: PrefixNot, context: any): any { this.visit(ast.expression, context); } + visitNonNullAssert(ast: NonNullAssert, context: any): any { this.visit(ast.expression, context); } + visitPropertyRead(ast: PropertyRead, context: any): any { this.visit(ast.receiver, context); } visitPropertyWrite(ast: PropertyWrite, context: any): any { - ast.receiver.visit(this, context); - ast.value.visit(this, context); - return null; + this.visit(ast.receiver, context); + this.visit(ast.value, context); } visitSafePropertyRead(ast: SafePropertyRead, context: any): any { - ast.receiver.visit(this, context); - return null; + this.visit(ast.receiver, context); } visitSafeMethodCall(ast: SafeMethodCall, context: any): any { - ast.receiver.visit(this, context); - return this.visitAll(ast.args, context); + this.visit(ast.receiver, context); + this.visitAll(ast.args, context); } + visitQuote(ast: Quote, context: any): any {} + // This is not part of the AstVisitor interface, just a helper method visitAll(asts: AST[], context: any): any { - asts.forEach(ast => ast.visit(this, context)); - return null; + for (const ast of asts) { + this.visit(ast, context); + } } - visitQuote(ast: Quote, context: any): any { return null; } } export class AstTransformer implements AstVisitor { @@ -683,69 +659,6 @@ export class AstMemoryEfficientTransformer implements AstVisitor { visitQuote(ast: Quote, context: any): AST { return ast; } } -export function visitAstChildren(ast: AST, visitor: AstVisitor, context?: any) { - function visit(ast: AST) { - visitor.visit && visitor.visit(ast, context) || ast.visit(visitor, context); - } - - function visitAll(asts: T[]) { asts.forEach(visit); } - - ast.visit({ - visitBinary(ast) { - visit(ast.left); - visit(ast.right); - }, - visitChain(ast) { visitAll(ast.expressions); }, - visitConditional(ast) { - visit(ast.condition); - visit(ast.trueExp); - visit(ast.falseExp); - }, - visitFunctionCall(ast) { - if (ast.target) { - visit(ast.target); - } - visitAll(ast.args); - }, - visitImplicitReceiver(ast) {}, - visitInterpolation(ast) { visitAll(ast.expressions); }, - visitKeyedRead(ast) { - visit(ast.obj); - visit(ast.key); - }, - visitKeyedWrite(ast) { - visit(ast.obj); - visit(ast.key); - visit(ast.obj); - }, - visitLiteralArray(ast) { visitAll(ast.expressions); }, - visitLiteralMap(ast) {}, - visitLiteralPrimitive(ast) {}, - visitMethodCall(ast) { - visit(ast.receiver); - visitAll(ast.args); - }, - visitPipe(ast) { - visit(ast.exp); - visitAll(ast.args); - }, - visitPrefixNot(ast) { visit(ast.expression); }, - visitNonNullAssert(ast) { visit(ast.expression); }, - visitPropertyRead(ast) { visit(ast.receiver); }, - visitPropertyWrite(ast) { - visit(ast.receiver); - visit(ast.value); - }, - visitQuote(ast) {}, - visitSafeMethodCall(ast) { - visit(ast.receiver); - visitAll(ast.args); - }, - visitSafePropertyRead(ast) { visit(ast.receiver); }, - }); -} - - // Bindings export class ParsedProperty { diff --git a/packages/compiler/src/render3/view/t2_binder.ts b/packages/compiler/src/render3/view/t2_binder.ts index 3d677daa1a..e56270c972 100644 --- a/packages/compiler/src/render3/view/t2_binder.ts +++ b/packages/compiler/src/render3/view/t2_binder.ts @@ -329,6 +329,17 @@ class TemplateBinder extends RecursiveAstVisitor implements Visitor { this.visitNode = (node: Node) => node.visit(this); } + // This method is defined to reconcile the type of TemplateBinder since both + // RecursiveAstVisitor and Visitor define the visit() method in their + // interfaces. + visit(node: AST|Node, context?: any) { + if (node instanceof AST) { + node.visit(this, context); + } else { + node.visit(this); + } + } + /** * Process a template and extract metadata about expressions and symbols within. * diff --git a/packages/compiler/test/expression_parser/ast_spec.ts b/packages/compiler/test/expression_parser/ast_spec.ts new file mode 100644 index 0000000000..a8612401e5 --- /dev/null +++ b/packages/compiler/test/expression_parser/ast_spec.ts @@ -0,0 +1,42 @@ +/** + * @license + * Copyright Google Inc. All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +import {AST, Lexer, Parser, RecursiveAstVisitor} from '@angular/compiler'; +import {ImplicitReceiver, MethodCall, PropertyRead} from '@angular/compiler/src/compiler'; + +describe('RecursiveAstVisitor', () => { + it('should visit every node', () => { + const parser = new Parser(new Lexer()); + const ast = parser.parseBinding('x.y()', null /* location */, 0 /* absoluteOffset */); + const visitor = new Visitor(); + const path: AST[] = []; + visitor.visit(ast.ast, path); + // If the visitor method of RecursiveAstVisitor is implemented correctly, + // then we should have collected the full path from root to leaf. + expect(path.length).toBe(3); + const [methodCall, propertyRead, implicitReceiver] = path; + expectType(methodCall, MethodCall); + expectType(propertyRead, PropertyRead); + expectType(implicitReceiver, ImplicitReceiver); + expect(methodCall.name).toBe('y'); + expect(methodCall.args).toEqual([]); + expect(propertyRead.name).toBe('x'); + }); +}); + +class Visitor extends RecursiveAstVisitor { + visit(node: AST, path: AST[]) { + path.push(node); + node.visit(this, path); + } +} + +type Newable = new (...args: any) => any; +function expectType(val: any, t: T): asserts val is InstanceType { + expect(val instanceof t).toBe(true, `expect ${val.constructor.name} to be ${t.name}`); +} diff --git a/packages/compiler/test/render3/util/expression.ts b/packages/compiler/test/render3/util/expression.ts index 12331d8636..65de317e1f 100644 --- a/packages/compiler/test/render3/util/expression.ts +++ b/packages/compiler/test/render3/util/expression.ts @@ -17,6 +17,17 @@ class ExpressionSourceHumanizer extends e.RecursiveAstVisitor implements t.Visit private recordAst(ast: e.AST) { this.result.push([unparse(ast), ast.sourceSpan]); } + // This method is defined to reconcile the type of ExpressionSourceHumanizer + // since both RecursiveAstVisitor and Visitor define the visit() method in + // their interfaces. + visit(node: e.AST|t.Node, context?: any) { + if (node instanceof e.AST) { + node.visit(this, context); + } else { + node.visit(this); + } + } + visitASTWithSource(ast: e.ASTWithSource) { this.recordAst(ast); this.visitAll([ast.ast], null); diff --git a/packages/compiler/test/template_parser/util/expression.ts b/packages/compiler/test/template_parser/util/expression.ts index f66f640dc9..a74b60f26a 100644 --- a/packages/compiler/test/template_parser/util/expression.ts +++ b/packages/compiler/test/template_parser/util/expression.ts @@ -17,6 +17,11 @@ class ExpressionSourceHumanizer extends e.RecursiveAstVisitor implements t.Templ private recordAst(ast: e.AST) { this.result.push([unparse(ast), ast.sourceSpan]); } + // This method is defined to reconcile the type of ExpressionSourceHumanizer + // since both RecursiveAstVisitor and TemplateAstVisitor define the visit() + // method in their interfaces. + visit(node: e.AST|t.TemplateAst, context?: any) { node.visit(this, context); } + visitASTWithSource(ast: e.ASTWithSource) { this.recordAst(ast); this.visitAll([ast.ast], null); diff --git a/packages/language-service/src/expression_type.ts b/packages/language-service/src/expression_type.ts index 911ed80fad..131eb60d37 100644 --- a/packages/language-service/src/expression_type.ts +++ b/packages/language-service/src/expression_type.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {AST, AstVisitor, Binary, BindingPipe, Chain, Conditional, FunctionCall, ImplicitReceiver, Interpolation, KeyedRead, KeyedWrite, LiteralArray, LiteralMap, LiteralPrimitive, MethodCall, NonNullAssert, PrefixNot, PropertyRead, PropertyWrite, Quote, SafeMethodCall, SafePropertyRead, visitAstChildren} from '@angular/compiler'; +import {AST, AstVisitor, Binary, BindingPipe, Chain, Conditional, FunctionCall, ImplicitReceiver, Interpolation, KeyedRead, KeyedWrite, LiteralArray, LiteralMap, LiteralPrimitive, MethodCall, NonNullAssert, PrefixNot, PropertyRead, PropertyWrite, Quote, SafeMethodCall, SafePropertyRead} from '@angular/compiler'; import * as ts from 'typescript'; import {BuiltinType, Signature, Symbol, SymbolQuery, SymbolTable} from './symbols'; @@ -178,14 +178,18 @@ export class AstType implements AstVisitor { visitChain(ast: Chain) { // If we are producing diagnostics, visit the children - visitAstChildren(ast, this); + for (const expr of ast.expressions) { + expr.visit(this); + } // The type of a chain is always undefined. return this.query.getBuiltinType(BuiltinType.Undefined); } visitConditional(ast: Conditional) { // The type of a conditional is the union of the true and false conditions. - visitAstChildren(ast, this); + ast.condition.visit(this); + ast.trueExp.visit(this); + ast.falseExp.visit(this); return this.query.getTypeUnion(this.getType(ast.trueExp), this.getType(ast.falseExp)); } @@ -235,7 +239,9 @@ export class AstType implements AstVisitor { visitInterpolation(ast: Interpolation): Symbol { // If we are producing diagnostics, visit the children. - visitAstChildren(ast, this); + for (const expr of ast.expressions) { + expr.visit(this); + } return this.undefinedType; } @@ -260,7 +266,9 @@ export class AstType implements AstVisitor { visitLiteralMap(ast: LiteralMap): Symbol { // If we are producing diagnostics, visit the children - visitAstChildren(ast, this); + for (const value of ast.values) { + value.visit(this); + } // TODO: Return a composite type. return this.anyType; } @@ -312,7 +320,7 @@ export class AstType implements AstVisitor { visitPrefixNot(ast: PrefixNot) { // If we are producing diagnostics, visit the children - visitAstChildren(ast, this); + ast.expression.visit(this); // The type of a prefix ! is always boolean. return this.query.getBuiltinType(BuiltinType.Boolean); } diff --git a/packages/language-service/src/expressions.ts b/packages/language-service/src/expressions.ts index 9d49448f93..570522ac73 100644 --- a/packages/language-service/src/expressions.ts +++ b/packages/language-service/src/expressions.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {AST, ASTWithSource, AstPath as AstPathBase, NullAstVisitor, visitAstChildren} from '@angular/compiler'; +import {AST, ASTWithSource, AstPath as AstPathBase, RecursiveAstVisitor} from '@angular/compiler'; import {AstType} from './expression_type'; import {BuiltinType, Span, Symbol, SymbolQuery, SymbolTable} from './types'; @@ -16,12 +16,12 @@ type AstPath = AstPathBase; function findAstAt(ast: AST, position: number, excludeEmpty: boolean = false): AstPath { const path: AST[] = []; - const visitor = new class extends NullAstVisitor { + const visitor = new class extends RecursiveAstVisitor { visit(ast: AST) { if ((!excludeEmpty || ast.sourceSpan.start < ast.sourceSpan.end) && inSpan(position, ast.sourceSpan)) { path.push(ast); - visitAstChildren(ast, this); + ast.visit(this); } } };