refactor(compiler-cli): use TemplateCheckWithVisitor in InvalidBananaInBoxCheck (#43232)

Modify the existing template check to use the new abstract class. This
makes template checks easier to read and understand.

Refs #42966

PR Close #43232

packages/compiler-cli/src/ngtsc/typecheck/extended/checks/invalid_banana_in_box/index.ts

@@ -6,62 +6,36 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {TmplAstBoundEvent, TmplAstNode, TmplAstRecursiveVisitor} from '@angular/compiler';
+import {AST, TmplAstBoundEvent, TmplAstNode} from '@angular/compiler';
 import * as ts from 'typescript';
 
 import {ErrorCode} from '../../../../diagnostics';
 import {NgTemplateDiagnostic} from '../../../api';
-import {TemplateCheck, TemplateContext} from '../../api';
+import {TemplateCheckWithVisitor, TemplateContext} from '../../api';
 
 /**
  * Ensures the two-way binding syntax is correct.
  * Parentheses should be inside the brackets "[()]".
  * Will return diagnostic information when "([])" is found.
  */
-export class InvalidBananaInBoxCheck implements TemplateCheck<ErrorCode.INVALID_BANANA_IN_BOX> {
-  code: ErrorCode.INVALID_BANANA_IN_BOX = 8101;
+export class InvalidBananaInBoxCheck extends
+    TemplateCheckWithVisitor<ErrorCode.INVALID_BANANA_IN_BOX> {
+  override code = ErrorCode.INVALID_BANANA_IN_BOX as const;
 
-  run(ctx: TemplateContext, component: ts.ClassDeclaration,
-      template: TmplAstNode[]): NgTemplateDiagnostic<ErrorCode.INVALID_BANANA_IN_BOX>[] {
-    const visitor = new BananaVisitor(ctx, component);
+  override visitNode(ctx: TemplateContext, component: ts.ClassDeclaration, node: TmplAstNode|AST):
+      NgTemplateDiagnostic<ErrorCode.INVALID_BANANA_IN_BOX>[] {
+    if (!(node instanceof TmplAstBoundEvent)) return [];
 
-    return visitor.getDiagnostics(template);
-  }
-}
-
-class BananaVisitor extends TmplAstRecursiveVisitor {
-  private diagnostics: NgTemplateDiagnostic<ErrorCode.INVALID_BANANA_IN_BOX>[] = [];
-
-  constructor(
-      public readonly ctx: TemplateContext, private readonly component: ts.ClassDeclaration) {
-    super();
-  }
-
-  /**
-   * Check for outputs with names surrounded in brackets "[]".
-   * The syntax '([foo])="bar"' would be interpreted as an @Output()
-   * with name '[foo]'. Just like '(foo)="bar"' would have the name 'foo'.
-   * Generate diagnostic information for the cases found.
-   */
-  override visitBoundEvent(boundEvent: TmplAstBoundEvent) {
-    const name = boundEvent.name;
-    if (name.startsWith('[') && name.endsWith(']')) {
-      const boundSyntax = boundEvent.sourceSpan.toString();
-      const expectedBoundSyntax = boundSyntax.replace(`(${name})`, `[(${name.slice(1, -1)})]`);
-      this.diagnostics.push(this.ctx.templateTypeChecker.makeTemplateDiagnostic(
-          this.component, boundEvent.sourceSpan, ts.DiagnosticCategory.Warning,
-          ErrorCode.INVALID_BANANA_IN_BOX,
-          `In the two-way binding syntax the parentheses should be inside the brackets, ex. '${
-              expectedBoundSyntax}'. 
-                Find more at https://angular.io/guide/two-way-binding`));
-    }
-  }
-
-  getDiagnostics(template: TmplAstNode[]): NgTemplateDiagnostic<ErrorCode.INVALID_BANANA_IN_BOX>[] {
-    this.diagnostics = [];
-    for (const node of template) {
-      node.visit(this);
-    }
-    return this.diagnostics;
+    const name = node.name;
+    if (!name.startsWith('[') || !name.endsWith(']')) return [];
+
+    const boundSyntax = node.sourceSpan.toString();
+    const expectedBoundSyntax = boundSyntax.replace(`(${name})`, `[(${name.slice(1, -1)})]`);
+    const diagnostic = ctx.templateTypeChecker.makeTemplateDiagnostic(
+        component, node.sourceSpan, ts.DiagnosticCategory.Warning, ErrorCode.INVALID_BANANA_IN_BOX,
+        `In the two-way binding syntax the parentheses should be inside the brackets, ex. '${
+            expectedBoundSyntax}'. 
+        Find more at https://angular.io/guide/two-way-binding`);
+    return [diagnostic];
   }
 }

packages/compiler-cli/src/ngtsc/typecheck/extended/src/extended_template_checker.ts

@@ -33,61 +33,9 @@ export class ExtendedTemplateCheckerImpl implements ExtendedTemplateChecker {
     const diagnostics: TemplateDiagnostic[] = [];
 
     for (const check of this.templateChecks) {
-      diagnostics.push(...deduplicateDiagnostics(check.run(this.ctx, component, template)));
+      diagnostics.push(...check.run(this.ctx, component, template));
     }
 
     return diagnostics;
   }
 }
-
-
-// Filter out duplicated diagnostics, this is possible due to the way the compiler
-// handles desugaring and produces `AST`s. Ex.
-//
-// ```
-// <div *ngIf="true" (foo)="bar">test</div>
-// ```
-//
-// Would result in the following AST:
-//
-// ```
-// Template {
-//   outputs: [
-//    BoundEvent {
-//      name: 'foo',
-//      /.../
-//    }
-//   ],
-//   children: [
-//     Element {
-//       outputs: [
-//         BoundEvent {
-//           name: 'foo',
-//           /.../
-//         }
-//       ]
-//     }
-//   ],
-//   /.../
-// }
-// ```
-//
-// In this case a duplicated diagnostic could be generated for the output `foo`.
-// TODO(danieltrevino): handle duplicated diagnostics when they are being generated
-// to avoid extra work (could be directly in the visitor).
-// https://github.com/angular/angular/pull/42984#discussion_r684823926
-function deduplicateDiagnostics<T extends ts.Diagnostic>(diagnostics: T[]): T[] {
-  const result: T[] = [];
-  for (const newDiag of diagnostics) {
-    const isDuplicateDiag = result.some(existingDiag => areDiagnosticsEqual(newDiag, existingDiag));
-    if (!isDuplicateDiag) {
-      result.push(newDiag);
-    }
-  }
-  return result;
-}
-
-function areDiagnosticsEqual(first: ts.Diagnostic, second: ts.Diagnostic): boolean {
-  return first.file?.fileName === second.file?.fileName && first.start === second.start &&
-      first.length === second.length && first.code === second.code;
-}