From a360f4b0fc361c2d8cf944aef8fd12d341d22ba4 Mon Sep 17 00:00:00 2001
From: Andrew Scott <atscott@google.com>
Date: Thu, 9 Sep 2021 17:47:59 -0700
Subject: [PATCH] refactor(compiler-cli): Add test to document use-case for
 invalid pipe argument nullish coallesce (#43419)

Adds a test to the nullish coalescing diagnostic check to serve as
self-documentation on how it works with nullish coalescing on pipes that
are often misconfigured.

This also removes that non null assertion operator, which is incorrect
because there _are_ situations where a symbol cannot be retrieved.

PR Close #43419
---
 .../nullish_coalescing_not_nullable/index.ts  |  4 +-
 .../nullish_coalescing_not_nullable/spec.ts   | 82 +++++++++++++++++++
 2 files changed, 84 insertions(+), 2 deletions(-)

diff --git a/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/nullish_coalescing_not_nullable/index.ts b/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/nullish_coalescing_not_nullable/index.ts
index 0e7415ab3ad..60b0f7c8759 100644
--- a/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/nullish_coalescing_not_nullable/index.ts
+++ b/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/nullish_coalescing_not_nullable/index.ts
@@ -27,8 +27,8 @@ export class NullishCoalescingNotNullableCheck extends
       NgTemplateDiagnostic<ErrorCode.NULLISH_COALESCING_NOT_NULLABLE>[] {
     if (!(node instanceof Binary) || node.operation !== '??') return [];
 
-    const symbolLeft = ctx.templateTypeChecker.getSymbolOfNode(node.left, component)!;
-    if (symbolLeft.kind !== SymbolKind.Expression) {
+    const symbolLeft = ctx.templateTypeChecker.getSymbolOfNode(node.left, component);
+    if (symbolLeft === null || symbolLeft.kind !== SymbolKind.Expression) {
       return [];
     }
     const typeLeft = symbolLeft.tsType;
diff --git a/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/nullish_coalescing_not_nullable/spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/nullish_coalescing_not_nullable/spec.ts
index 3bed38765ad..a5a8141dfc1 100644
--- a/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/nullish_coalescing_not_nullable/spec.ts
+++ b/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/nullish_coalescing_not_nullable/spec.ts
@@ -71,5 +71,87 @@ runInEachFileSystem(() => {
       const diags = extendedTemplateChecker.getDiagnosticsForComponent(component);
       expect(diags.length).toBe(0);
     });
+
+    it('warns for pipe arguments which are likely configured incorrectly (?? operates on "format" here)',
+       () => {
+         const fileName = absoluteFrom('/main.ts');
+         const {program, templateTypeChecker} = setup([{
+           fileName,
+           templates: {
+             'TestCmp': `{{ 123 | date: 'format' ?? 'invalid date' }}`,
+           },
+           source: `
+            export class TestCmp { var1: string | undefined = "text"; }
+            export class DatePipe {
+              transform(value: string, format: string): string[] {
+              }
+            `,
+           declarations: [{
+             type: 'pipe',
+             name: 'DatePipe',
+             pipeName: 'date',
+           }],
+         }]);
+         const sf = getSourceFileOrError(program, fileName);
+         const component = getClass(sf, 'TestCmp');
+         const extendedTemplateChecker = new ExtendedTemplateCheckerImpl(
+             templateTypeChecker, program.getTypeChecker(),
+             [new NullishCoalescingNotNullableCheck()]);
+         const diags = extendedTemplateChecker.getDiagnosticsForComponent(component);
+         expect(diags.length).toBe(1);
+         expect(diags[0].category).toBe(ts.DiagnosticCategory.Warning);
+         expect(diags[0].code).toBe(ngErrorCode(ErrorCode.NULLISH_COALESCING_NOT_NULLABLE));
+         expect(getSourceCodeForDiagnostic(diags[0])).toBe(`'format' ?? 'invalid date'`);
+       });
+
+    it('does not warn for pipe arguments when parens are used', () => {
+      const fileName = absoluteFrom('/main.ts');
+      const {program, templateTypeChecker} = setup([
+        {
+          fileName,
+          templates: {
+            'TestCmp': `{{ (123 | date: 'format') ?? 'invalid date' }}`,
+          },
+          source: `
+            export class TestCmp { var1: string | undefined = "text"; }
+            export class DatePipe {
+              transform(value: string, format: string): string[] {
+              }
+          `,
+          declarations: [{
+            type: 'pipe',
+            name: 'DatePipe',
+            pipeName: 'date',
+          }],
+        },
+      ]);
+      const sf = getSourceFileOrError(program, fileName);
+      const component = getClass(sf, 'TestCmp');
+      const extendedTemplateChecker = new ExtendedTemplateCheckerImpl(
+          templateTypeChecker, program.getTypeChecker(), [new NullishCoalescingNotNullableCheck()]);
+      const diags = extendedTemplateChecker.getDiagnosticsForComponent(component);
+      expect(diags.length).toBe(0);
+    });
+
+    it('does not blow up when there is no symbol for the LHS of the operator', () => {
+      const fileName = absoluteFrom('/main.ts');
+      const {program, templateTypeChecker} = setup([
+        {
+          fileName,
+          templates: {
+            'TestCmp': `{{ (123 | doesNotExist) ?? 'invalid date' }}`,
+          },
+          source: `
+            export class TestCmp { var1: string | undefined = "text"; }
+          `,
+        },
+      ]);
+      const sf = getSourceFileOrError(program, fileName);
+      const component = getClass(sf, 'TestCmp');
+      const extendedTemplateChecker = new ExtendedTemplateCheckerImpl(
+          templateTypeChecker, program.getTypeChecker(), [new NullishCoalescingNotNullableCheck()]);
+      const diags = extendedTemplateChecker.getDiagnosticsForComponent(component);
+      expect(diags.length).toBe(0);
+    });
   });
 });