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

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<T extends AST>(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 {

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.
    *

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<T extends Newable>(val: any, t: T): asserts val is InstanceType<T> {
+  expect(val instanceof t).toBe(true, `expect ${val.constructor.name} to be ${t.name}`);
+}

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);

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);

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);
   }

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<AST>;
 
 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);
       }
     }
   };