fix(ivy): avoid duplicate errors in safe navigations and template gua…

…rds (#34417)

The template type checker generates TypeScript expressions for any
expression that occurs in a template, so that TypeScript can check it
and produce errors. Some expressions as they occur in a template may be
translated into TypeScript code multiple times, for instance a binding
to a directive input that has a template guard.

One example would be the `NgIf` directive, which has a template guard to
narrow the type in the template as appropriate. Given the following
template:

```typescript
@component({
  template: '<div *ngIf="person">{{ person.name }}</div>'
})
class AppComponent {
  person?: { name: string };
}
```

A type check block (TCB) with roughly the following structure is
created:

```typescript
function tcb(ctx: AppComponent) {
  const t1 = NgIf.ngTypeCtor({ ngIf: ctx.person });
  if (ctx.person) {
    "" + ctx.person.name;
  }
}
```

Notice how the `*ngIf="person"` binding is present twice: once in the
type constructor call and once in the `if` guard. As such, TypeScript
will check both instances and would produce duplicate errors, if any
were found.

Another instance is when the safe navigation operator is used, where an
expression such as `person?.name` is emitted into the TCB as
`person != null ? person!.name : undefined`. As can be seen, the
left-hand side expression `person` occurs twice in the TCB.

This commit adds the ability to insert markers into the TCB that
indicate that any errors within the expression should be ignored. This
is similar to `@ts-ignore`, however it can be applied more granularly.

PR Close #34417

packages/compiler-cli/src/ngtsc/typecheck/src/diagnostics.ts

@@ -46,6 +46,16 @@ export function wrapForDiagnostics(expr: ts.Expression): ts.Expression {
   return ts.createParen(expr);
 }
 
+const IGNORE_MARKER = 'ignore';
+
+/**
+ * Adds a marker to the node that signifies that any errors within the node should not be reported.
+ */
+export function ignoreDiagnostics(node: ts.Node): void {
+  ts.addSyntheticTrailingComment(
+      node, ts.SyntaxKind.MultiLineCommentTrivia, IGNORE_MARKER, /* hasTrailingNewLine */ false);
+}
+
 /**
  * Adds a synthetic comment to the expression that represents the parse span of the provided node.
  * This comment can later be retrieved as trivia of a node to recover original source locations.
@@ -214,17 +224,20 @@ interface SourceLocation {
   span: AbsoluteSourceSpan;
 }
 
+/**
+ * Traverses up the AST starting from the given node to extract the source location from comments
+ * that have been emitted into the TCB. If the node does not exist within a TCB, or if an ignore
+ * marker comment is found up the tree, this function returns null.
+ */
 function findSourceLocation(node: ts.Node, sourceFile: ts.SourceFile): SourceLocation|null {
   // Search for comments until the TCB's function declaration is encountered.
   while (node !== undefined && !ts.isFunctionDeclaration(node)) {
-    const span =
-        ts.forEachTrailingCommentRange(sourceFile.text, node.getEnd(), (pos, end, kind) => {
-          if (kind !== ts.SyntaxKind.MultiLineCommentTrivia) {
-            return null;
-          }
-          const commentText = sourceFile.text.substring(pos, end);
-          return parseSourceSpanComment(commentText);
-        }) || null;
+    if (hasIgnoreMarker(node, sourceFile)) {
+      // There's an ignore marker on this node, so the diagnostic should not be reported.
+      return null;
+    }
+
+    const span = readSpanComment(sourceFile, node);
     if (span !== null) {
       // Once the positional information has been extracted, search further up the TCB to extract
       // the unique id that is attached with the TCB's function declaration.
@@ -243,17 +256,21 @@ function findSourceLocation(node: ts.Node, sourceFile: ts.SourceFile): SourceLoc
 
 function getTemplateId(node: ts.Node, sourceFile: ts.SourceFile): TemplateId|null {
   // Walk up to the function declaration of the TCB, the file information is attached there.
-  let tcb = node;
-  while (!ts.isFunctionDeclaration(tcb)) {
-    tcb = tcb.parent;
+  while (!ts.isFunctionDeclaration(node)) {
+    if (hasIgnoreMarker(node, sourceFile)) {
+      // There's an ignore marker on this node, so the diagnostic should not be reported.
+      return null;
+    }
+    node = node.parent;
 
     // Bail once we have reached the root.
-    if (tcb === undefined) {
+    if (node === undefined) {
       return null;
     }
   }
 
-  return ts.forEachLeadingCommentRange(sourceFile.text, tcb.getFullStart(), (pos, end, kind) => {
+  const start = node.getFullStart();
+  return ts.forEachLeadingCommentRange(sourceFile.text, start, (pos, end, kind) => {
     if (kind !== ts.SyntaxKind.MultiLineCommentTrivia) {
       return null;
     }
@@ -262,13 +279,29 @@ function getTemplateId(node: ts.Node, sourceFile: ts.SourceFile): TemplateId|nul
   }) as TemplateId || null;
 }
 
-const parseSpanComment = /^\/\*(\d+),(\d+)\*\/$/;
+const parseSpanComment = /^(\d+),(\d+)$/;
 
-function parseSourceSpanComment(commentText: string): AbsoluteSourceSpan|null {
-  const match = commentText.match(parseSpanComment);
-  if (match === null) {
-    return null;
-  }
+function readSpanComment(sourceFile: ts.SourceFile, node: ts.Node): AbsoluteSourceSpan|null {
+  return ts.forEachTrailingCommentRange(sourceFile.text, node.getEnd(), (pos, end, kind) => {
+    if (kind !== ts.SyntaxKind.MultiLineCommentTrivia) {
+      return null;
+    }
+    const commentText = sourceFile.text.substring(pos + 2, end - 2);
+    const match = commentText.match(parseSpanComment);
+    if (match === null) {
+      return null;
+    }
+
+    return new AbsoluteSourceSpan(+match[1], +match[2]);
+  }) || null;
+}
 
-  return new AbsoluteSourceSpan(+match[1], +match[2]);
+function hasIgnoreMarker(node: ts.Node, sourceFile: ts.SourceFile): boolean {
+  return ts.forEachTrailingCommentRange(sourceFile.text, node.getEnd(), (pos, end, kind) => {
+    if (kind !== ts.SyntaxKind.MultiLineCommentTrivia) {
+      return null;
+    }
+    const commentText = sourceFile.text.substring(pos + 2, end - 2);
+    return commentText === IGNORE_MARKER;
+  }) === true;
 }

packages/compiler-cli/src/ngtsc/typecheck/src/expression.ts

@@ -10,7 +10,7 @@ import {AST, ASTWithSource, AstVisitor, Binary, BindingPipe, Chain, Conditional,
 import * as ts from 'typescript';
 
 import {TypeCheckingConfig} from './api';
-import {addParseSpanInfo, wrapForDiagnostics} from './diagnostics';
+import {addParseSpanInfo, ignoreDiagnostics, wrapForDiagnostics} from './diagnostics';
 
 export const NULL_AS_ANY =
     ts.createAsExpression(ts.createNull(), ts.createKeywordTypeNode(ts.SyntaxKind.AnyKeyword));
@@ -222,11 +222,13 @@ class AstTranslator implements AstVisitor {
     // See the comment in SafePropertyRead above for an explanation of the need for the non-null
     // assertion here.
     const receiver = wrapForDiagnostics(this.translate(ast.receiver));
+    const guard = ts.getMutableClone(receiver);
+    ignoreDiagnostics(guard);
     const method = ts.createPropertyAccess(ts.createNonNullExpression(receiver), ast.name);
     const args = ast.args.map(expr => this.translate(expr));
     const expr = ts.createCall(method, undefined, args);
     const whenNull = this.config.strictSafeNavigationTypes ? UNDEFINED : NULL_AS_ANY;
-    const node = safeTernary(receiver, expr, whenNull);
+    const node = safeTernary(guard, expr, whenNull);
     addParseSpanInfo(node, ast.sourceSpan);
     return node;
   }
@@ -237,9 +239,11 @@ class AstTranslator implements AstVisitor {
     // assertion is necessary because in practice 'a' may be a method call expression, which won't
     // have a narrowed type when repeated in the ternary true branch.
     const receiver = wrapForDiagnostics(this.translate(ast.receiver));
+    const guard = ts.getMutableClone(receiver);
+    ignoreDiagnostics(guard);
     const expr = ts.createPropertyAccess(ts.createNonNullExpression(receiver), ast.name);
     const whenNull = this.config.strictSafeNavigationTypes ? UNDEFINED : NULL_AS_ANY;
-    const node = safeTernary(receiver, expr, whenNull);
+    const node = safeTernary(guard, expr, whenNull);
     addParseSpanInfo(node, ast.sourceSpan);
     return node;
   }

packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts

@@ -13,7 +13,7 @@ import {Reference} from '../../imports';
 import {ClassDeclaration} from '../../reflection';
 
 import {TemplateId, TypeCheckBlockMetadata, TypeCheckableDirectiveMeta} from './api';
-import {addParseSpanInfo, addTemplateId, wrapForDiagnostics} from './diagnostics';
+import {addParseSpanInfo, addTemplateId, ignoreDiagnostics, wrapForDiagnostics} from './diagnostics';
 import {DomSchemaChecker} from './dom';
 import {Environment} from './environment';
 import {NULL_AS_ANY, astToTypescript} from './expression';
@@ -217,6 +217,10 @@ class TcbTemplateBodyOp extends TcbOp {
             // If there is such a binding, generate an expression for it.
             const expr = tcbExpression(boundInput.value, this.tcb, this.scope);
 
+            // The expression has already been checked in the type constructor invocation, so
+            // it should be ignored when used within a template guard.
+            ignoreDiagnostics(expr);
+
             if (guard.type === 'binding') {
               // Use the binding expression itself as guard.
               directiveGuards.push(expr);

packages/compiler-cli/src/ngtsc/typecheck/test/diagnostics_spec.ts

@@ -43,7 +43,7 @@ runInEachFileSystem(() => {
           `<div *ngFor="let person of persons; let idx=index">{{ render(idx) }}</div>`, `
         class TestComponent {
           persons: {}[];
-          
+
           render(input: string): string { return input; }
         }`,
           [ngForDeclaration()], [ngForDts()]);
@@ -58,7 +58,7 @@ runInEachFileSystem(() => {
           `<div *ngFor="let person of persons;">{{ render(person.namme) }}</div>`, `
         class TestComponent {
           persons: any;
-          
+
           render(input: string): string { return input; }
         }`,
           [ngForDeclaration()], [ngForDts()]);
@@ -194,6 +194,46 @@ runInEachFileSystem(() => {
       ]);
     });
 
+    it('does not repeat diagnostics for errors within LHS of safe-navigation operator', () => {
+      const messages = diagnose(`{{ personn?.name }} {{ personn?.getName() }}`, `
+         class TestComponent {
+           person: {
+             name: string;
+             getName: () => string;
+           } | null;
+         }`);
+
+      expect(messages).toEqual([
+        `synthetic.html(1, 4): Property 'personn' does not exist on type 'TestComponent'. Did you mean 'person'?`,
+        `synthetic.html(1, 24): Property 'personn' does not exist on type 'TestComponent'. Did you mean 'person'?`,
+      ]);
+    });
+
+    it('does not repeat diagnostics for errors used in template guard expressions', () => {
+      const messages = diagnose(
+          `<div *guard="personn.name"></div>`, `
+          class GuardDir {
+            static ngTemplateGuard_guard: 'binding';
+          }
+
+          class TestComponent {
+            person: {
+              name: string;
+            };
+          }`,
+          [{
+            type: 'directive',
+            name: 'GuardDir',
+            selector: '[guard]',
+            inputs: {'guard': 'guard'},
+            ngTemplateGuards: [{inputName: 'guard', type: 'binding'}]
+          }]);
+
+      expect(messages).toEqual([
+        `synthetic.html(1, 14): Property 'personn' does not exist on type 'TestComponent'. Did you mean 'person'?`,
+      ]);
+    });
+
     it('does not produce diagnostics for user code', () => {
       const messages = diagnose(`{{ person.name }}`, `
       class TestComponent {
@@ -313,7 +353,7 @@ runInEachFileSystem(() => {
 <div>
   <img [src]="srcc"
        [height]="heihgt">
-</div>    
+</div>
 `,
           `
 class TestComponent {

packages/compiler-cli/src/ngtsc/typecheck/test/span_comments_spec.ts

@@ -97,14 +97,15 @@ describe('type check blocks diagnostics', () => {
     it('should annotate safe property access', () => {
       const TEMPLATE = `{{ a?.b }}`;
       expect(tcbWithSpans(TEMPLATE))
-          .toContain('(((ctx).a /*3,4*/) != null ? ((ctx).a /*3,4*/)!.b : undefined) /*3,8*/;');
+          .toContain(
+              '(((ctx).a /*3,4*/) /*ignore*/ != null ? ((ctx).a /*3,4*/)!.b : undefined) /*3,8*/;');
     });
 
     it('should annotate safe method calls', () => {
       const TEMPLATE = `{{ a?.method(b) }}`;
       expect(tcbWithSpans(TEMPLATE))
           .toContain(
-              '(((ctx).a /*3,4*/) != null ? ((ctx).a /*3,4*/)!.method((ctx).b /*13,14*/) : undefined) /*3,16*/;');
+              '(((ctx).a /*3,4*/) /*ignore*/ != null ? ((ctx).a /*3,4*/)!.method((ctx).b /*13,14*/) : undefined) /*3,16*/;');
     });
 
     it('should annotate $any casts', () => {