fix(compiler-cli): handle nullable expressions correctly in the nullish coalescing extended template diagnostic (angular#43572)

dgp1130 · dylhunn · commit 263feba5c2d5 · 2021-10-04T17:32:57.000-07:00
Refs angular#42966. Previously, checking a template with the syntax: ```html <div>{{ foo() ?? 'test' }}</div> ``` Where `foo()` returns a nullable value: ```typescript @component(/* ... */) class TestCmp { foo: (): string | null => null; } ``` Would always log a nullish coalescing not nullable warning. This is because [`getSymbolOfNode(node.left)`](https://github.com/angular/angular/blob/fe691935091aaf7090864c8111a15f7cc7e53b6c/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/nullish_coalescing_not_nullable/index.ts#L30) would return the [symbol of the function (`foo`)](https://github.com/angular/angular/blob/fe691935091aaf7090864c8111a15f7cc7e53b6c/packages/compiler-cli/src/ngtsc/typecheck/src/template_symbol_builder.ts#L536-L538) rather than the symbol of its returned value (`foo()`). Fixed this by getting the symbol for the whole expression's span, rather than just the function receiver. Also made some minor refactorings to `template_symbol_builder` to make a similar change to safe method calls. This behavior was originally for the language service in order to handle quick info, as the user highlighting a function name would actually apply to the entire expression. This is no longer true as the language service will correctly request the type from the function rather than the `Call` expression, so these hacks are not necessary anymore. This broke two existing test cases of exactly this behavior which were easily updated. Also added a test to the language service to confirm that it is not broken by this change. PR Close angular#43572
diff --git a/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/nullish_coalescing_not_nullable/nullish_coalescing_not_nullable_spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/nullish_coalescing_not_nullable/nullish_coalescing_not_nullable_spec.ts
@@ -133,25 +133,29 @@ runInEachFileSystem(() => {
       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);
-    });
+    it('should not produce nullish coalescing warning when the left side is a nullable expression',
+       () => {
+         const fileName = absoluteFrom('/main.ts');
+         const {program, templateTypeChecker} = setup([
+           {
+             fileName,
+             templates: {
+               'TestCmp': `{{ func() ?? 'foo' }}`,
+             },
+             source: `
+               export class TestCmp {
+                 func: (): string | null => null;
+               }
+             `,
+           },
+         ]);
+         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);
+       });
   });
 });
diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/template_symbol_builder.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/template_symbol_builder.ts
@@ -499,8 +499,6 @@ export class SymbolBuilder {
     // AST so there is no way to retrieve a `Symbol` for just the `name` via a specific node.
     if (expression instanceof PropertyWrite) {
       withSpan = expression.nameSpan;
-    } else if (expression instanceof Call && expression.receiver instanceof PropertyRead) {
-      withSpan = expression.receiver.nameSpan;
     }
 
     let node: ts.Node|null = null;
@@ -530,12 +528,8 @@ export class SymbolBuilder {
     // - If our expression is a pipe binding ("a | test:b:c"), we want the Symbol for the
     // `transform` on the pipe.
     // - Otherwise, we retrieve the symbol for the node itself with no special considerations
-    if ((expression instanceof SafePropertyRead ||
-         (expression instanceof Call && expression.receiver instanceof SafePropertyRead)) &&
-        ts.isConditionalExpression(node)) {
-      const whenTrueSymbol = (expression instanceof Call && ts.isCallExpression(node.whenTrue)) ?
-          this.getSymbolOfTsNode(node.whenTrue.expression) :
-          this.getSymbolOfTsNode(node.whenTrue);
+    if (expression instanceof SafePropertyRead && ts.isConditionalExpression(node)) {
+      const whenTrueSymbol = this.getSymbolOfTsNode(node.whenTrue);
       if (whenTrueSymbol === null) {
         return null;
       }
diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/type_checker__get_symbol_of_template_node_spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/test/type_checker__get_symbol_of_template_node_spec.ts
@@ -467,13 +467,10 @@ runInEachFileSystem(() => {
           const safeMethodCall = nodes[2].inputs[0].value as ASTWithSource;
           const methodCallSymbol = templateTypeChecker.getSymbolOfNode(safeMethodCall, cmp)!;
           assertExpressionSymbol(methodCallSymbol);
-          expect(program.getTypeChecker().symbolToString(methodCallSymbol.tsSymbol!))
-              .toEqual('speak');
-          expect((methodCallSymbol.tsSymbol!.declarations![0] as ts.PropertyDeclaration)
-                     .parent.name!.getText())
-              .toEqual('Person');
+          // Note that the symbol returned is for the return value of the safe method call.
+          expect(methodCallSymbol.tsSymbol).toBeNull();
           expect(program.getTypeChecker().typeToString(methodCallSymbol.tsType))
-              .toEqual('string | undefined');
+              .toBe('string | undefined');
         });
 
         it('safe keyed reads', () => {
@@ -869,14 +866,9 @@ runInEachFileSystem(() => {
         const node = getAstElements(templateTypeChecker, cmp)[0];
         const callSymbol = templateTypeChecker.getSymbolOfNode(node.inputs[0].value, cmp)!;
         assertExpressionSymbol(callSymbol);
-        // Note that the symbol returned is for the method name of the Call. The AST
-        // does not support specific designation for the name so we assume that's what
-        // is wanted in this case. We don't support retrieving a symbol for the whole
-        // call expression and if you want to get a symbol for the args, you can
-        // use the AST of the args in the `Call`.
-        expect(program.getTypeChecker().symbolToString(callSymbol.tsSymbol!)).toEqual('toString');
-        expect(program.getTypeChecker().typeToString(callSymbol.tsType))
-            .toEqual('(v: any) => string');
+        // Note that the symbol returned is for the return value of the Call.
+        expect(callSymbol.tsSymbol).toBeNull();
+        expect(program.getTypeChecker().typeToString(callSymbol.tsType)).toBe('string');
       });
     });
 
diff --git a/packages/language-service/ivy/test/quick_info_spec.ts b/packages/language-service/ivy/test/quick_info_spec.ts
@@ -9,7 +9,7 @@
 import {initMockFileSystem} from '@angular/compiler-cli/src/ngtsc/file_system/testing';
 
 import * as ts from 'typescript/lib/tsserverlibrary';
-import {LanguageServiceTestEnv, Project} from '../testing';
+import {createModuleAndProjectWithDeclarations, LanguageServiceTestEnv, Project} from '../testing';
 
 function quickInfoSkeleton(): {[fileName: string]: string} {
   return {
@@ -443,6 +443,25 @@ describe('quick info', () => {
         });
       });
 
+      it('should work for safe method calls', () => {
+        const files = {
+          'app.ts': `import {Component} from '@angular/core';
+            @Component({template: '<div (click)="something?.myFunc()"></div>'})
+            export class AppCmp {
+              something!: {
+                /** Documentation for myFunc. */
+                myFunc(): void
+              };
+            }`,
+        };
+        const project = createModuleAndProjectWithDeclarations(env, 'test_project', files);
+        const appFile = project.openFile('app.ts');
+        appFile.moveCursorToText('something?.myF&brvbar;unc()');
+        const info = appFile.getQuickInfoAtPosition()!;
+        expect(toText(info.displayParts)).toEqual('(method) myFunc(): void');
+        expect(toText(info.documentation)).toEqual('Documentation for myFunc.');
+      });
+
       it('should work for accessed properties in writes', () => {
         expectQuickInfo({
           templateOverride: `<div (click)="hero.i&brvbar;d = 2"></div>`,
