fix(language-service): Suggest ? and ! operator on nullable receiver (#35200)

Under strict mode, the language service fails to typecheck nullable
symbols that have already been verified to be non-null.

This generates incorrect (false positive) and confusing diagnostics
for users.

To work around this issue in the short term, this commit changes the
diagnostic message from an error to a suggestion, and prompts users to
use the safe navigation operator (?) or non-null assertion operator (!).

For example, instead of

```typescript
{{ optional && optional.toString() }}
```

the following is cleaner:

```typescript
{{ optional?.toString() }}
{{ optional!.toString() }}
```

Note that with this change, users who legitimately make a typo in their
code will no longer see an error. I think this is acceptable, since
false positive is worse than false negative. However, if users follow
the suggestion, add ? or ! to their code, then the error will be surfaced.
This seems a reasonable trade-off.

References:

1. Safe navigation operator (?)
   https://angular.io/guide/template-syntax#the-safe-navigation-operator----and-null-property-paths
2. Non-null assertion operator (!)
   https://angular.io/guide/template-syntax#the-non-null-assertion-operator---

PR closes https://github.com/angular/angular/pull/35070
PR closes https://github.com/angular/vscode-ng-language-service/issues/589

PR Close #35200

packages/language-service/src/expression_type.ts

@@ -366,22 +366,19 @@ export class AstType implements AstVisitor {
     if (this.isAny(receiverType)) {
       return this.anyType;
     }
-
+    const methodType = this.resolvePropertyRead(receiverType, ast);
-    // The type of a method is the selected methods result type.
+    if (!methodType) {
-    const method = receiverType.members().get(ast.name);
-    if (!method) {
-      this.reportDiagnostic(`Unknown method '${ast.name}'`, ast);
-      return this.anyType;
-    }
-    if (!method.type) {
       this.reportDiagnostic(`Could not find a type for '${ast.name}'`, ast);
       return this.anyType;
     }
-    if (!method.type.callable) {
+    if (this.isAny(methodType)) {
+      return this.anyType;
+    }
+    if (!methodType.callable) {
       this.reportDiagnostic(`Member '${ast.name}' is not callable`, ast);
       return this.anyType;
     }
-    const signature = method.type.selectSignature(ast.args.map(arg => this.getType(arg)));
+    const signature = methodType.selectSignature(ast.args.map(arg => this.getType(arg)));
     if (!signature) {
       this.reportDiagnostic(`Unable to resolve signature for call of method ${ast.name}`, ast);
       return this.anyType;
@@ -393,34 +390,33 @@ export class AstType implements AstVisitor {
     if (this.isAny(receiverType)) {
       return this.anyType;
     }
-
     // The type of a property read is the seelcted member's type.
     const member = receiverType.members().get(ast.name);
     if (!member) {
-      let receiverInfo = receiverType.name;
+      if (receiverType.name === '$implicit') {
-      if (receiverInfo == '$implicit') {
-        receiverInfo =
-            'The component declaration, template variable declarations, and element references do';
-      } else if (receiverType.nullable) {
-        return this.reportDiagnostic(`The expression might be null`, ast.receiver);
-      } else {
-        receiverInfo = `'${receiverInfo}' does`;
-      }
         this.reportDiagnostic(
-          `Identifier '${ast.name}' is not defined. ${receiverInfo} not contain such a member`,
+            `Identifier '${ast.name}' is not defined. ` +
+                `The component declaration, template variable declarations, and element references do not contain such a member`,
             ast);
+      } else if (receiverType.nullable && ast.receiver instanceof PropertyRead) {
+        const receiver = ast.receiver.name;
+        this.reportDiagnostic(
+            `'${receiver}' is possibly undefined. Consider using the safe navigation operator (${receiver}?.${ast.name}) ` +
+                `or non-null assertion operator (${receiver}!.${ast.name}).`,
+            ast, ts.DiagnosticCategory.Suggestion);
+      } else {
+        this.reportDiagnostic(
+            `Identifier '${ast.name}' is not defined. '${receiverType.name}' does not contain such a member`,
+            ast);
+      }
       return this.anyType;
     }
     if (!member.public) {
-      let receiverInfo = receiverType.name;
-      if (receiverInfo == '$implicit') {
-        receiverInfo = 'the component';
-      } else {
-        receiverInfo = `'${receiverInfo}'`;
-      }
       this.reportDiagnostic(
-          `Identifier '${ast.name}' refers to a private member of ${receiverInfo}`, ast,
+          `Identifier '${ast.name}' refers to a private member of ${receiverType.name === '$implicit' ? 'the component' : `
-          ts.DiagnosticCategory.Warning);
+      '${receiverType.name}'
+          `}`,
+          ast, ts.DiagnosticCategory.Warning);
     }
     return member.type;
   }
@@ -430,7 +426,7 @@ export class AstType implements AstVisitor {
   }
 
   private isAny(symbol: Symbol): boolean {
-    return !symbol || this.query.getTypeKind(symbol) == BuiltinType.Any ||
+    return !symbol || this.query.getTypeKind(symbol) === BuiltinType.Any ||
         (!!symbol.type && this.isAny(symbol.type));
   }
 }

packages/language-service/test/diagnostics_spec.ts

@@ -339,7 +339,9 @@ describe('diagnostics', () => {
       const diagnostics = ngLS.getSemanticDiagnostics(TEST_TEMPLATE) !;
       expect(diagnostics.length).toBe(1);
       const {messageText, start, length} = diagnostics[0];
-      expect(messageText).toBe(`Unknown method 'notSubstring'`);
+      expect(messageText)
+          .toBe(
+              `Identifier 'notSubstring' is not defined. 'string' does not contain such a member`);
       expect(start).toBe(content.indexOf('$event'));
       expect(length).toBe('$event.notSubstring()'.length);
     });
@@ -885,4 +887,67 @@ describe('diagnostics', () => {
     const ngDiags = ngLS.getSemanticDiagnostics(fileName);
     expect(ngDiags).toEqual([]);
   });
+
+  it('should suggest ? or ! operator if method receiver is nullable', () => {
+    const content = mockHost.override(TEST_TEMPLATE, `{{optional && optional.toLowerCase()}}`);
+    const ngDiags = ngLS.getSemanticDiagnostics(TEST_TEMPLATE);
+    expect(ngDiags.length).toBe(1);
+    const {start, length, messageText, category} = ngDiags[0];
+    expect(messageText)
+        .toBe(
+            `'optional' is possibly undefined. ` +
+            `Consider using the safe navigation operator (optional?.toLowerCase) ` +
+            `or non-null assertion operator (optional!.toLowerCase).`);
+    expect(category).toBe(ts.DiagnosticCategory.Suggestion);
+    expect(content.substring(start !, start ! + length !)).toBe('optional.toLowerCase()');
+
+  });
+
+  it('should suggest ? or ! operator if property receiver is nullable', () => {
+    const content = mockHost.override(TEST_TEMPLATE, `{{optional && optional.length}}`);
+    const ngDiags = ngLS.getSemanticDiagnostics(TEST_TEMPLATE);
+    expect(ngDiags.length).toBe(1);
+    const {start, length, messageText, category} = ngDiags[0];
+    expect(messageText)
+        .toBe(
+            `'optional' is possibly undefined. ` +
+            `Consider using the safe navigation operator (optional?.length) ` +
+            `or non-null assertion operator (optional!.length).`);
+    expect(category).toBe(ts.DiagnosticCategory.Suggestion);
+    expect(content.substring(start !, start ! + length !)).toBe('optional.length');
+  });
+
+  it('should report error if method is not found on non-nullable receiver', () => {
+    const expressions = [
+      'optional?.someMethod()',
+      'optional!.someMethod()',
+    ];
+    for (const expression of expressions) {
+      const content = mockHost.override(TEST_TEMPLATE, `{{${expression}}}`);
+      const ngDiags = ngLS.getSemanticDiagnostics(TEST_TEMPLATE);
+      expect(ngDiags.length).toBe(1);
+      const {start, length, messageText, category} = ngDiags[0];
+      expect(messageText)
+          .toBe(`Identifier 'someMethod' is not defined. 'string' does not contain such a member`);
+      expect(category).toBe(ts.DiagnosticCategory.Error);
+      expect(content.substring(start !, start ! + length !)).toBe(expression);
+    }
+  });
+
+  it('should report error if property is not found on non-nullable receiver', () => {
+    const expressions = [
+      'optional?.someProp',
+      'optional!.someProp',
+    ];
+    for (const expression of expressions) {
+      const content = mockHost.override(TEST_TEMPLATE, `{{${expression}}}`);
+      const ngDiags = ngLS.getSemanticDiagnostics(TEST_TEMPLATE);
+      expect(ngDiags.length).toBe(1);
+      const {start, length, messageText, category} = ngDiags[0];
+      expect(messageText)
+          .toBe(`Identifier 'someProp' is not defined. 'string' does not contain such a member`);
+      expect(category).toBe(ts.DiagnosticCategory.Error);
+      expect(content.substring(start !, start ! + length !)).toBe(expression);
+    }
+  });
 });

packages/language-service/test/expression_diagnostics_spec.ts

@@ -144,7 +144,9 @@ describe('expression diagnostics', () => {
     `,
                                                           'Identifier \'nume\' is not defined'));
   it('should reject access to potentially undefined field',
-     () => reject(`<div>{{maybe_person.name.first}}`, 'The expression might be null'));
+     () => reject(
+         `<div>{{maybe_person.name.first}}`,
+         `'maybe_person' is possibly undefined. Consider using the safe navigation operator (maybe_person?.name) or non-null assertion operator (maybe_person!.name).`));
   it('should accept a safe accss to an undefined field',
      () => accept(`<div>{{maybe_person?.name.first}}</div>`));
   it('should accept a type assert to an undefined field',
@@ -183,7 +185,8 @@ describe('expression diagnostics', () => {
      () => accept('<div (click)="click($event)">{{person.name.first}}</div>'));
   it('should reject a misspelled event handler',
      () => reject(
-         '<div (click)="clack($event)">{{person.name.first}}</div>', 'Unknown method \'clack\''));
+         '<div (click)="clack($event)">{{person.name.first}}</div>',
+         `Identifier 'clack' is not defined. The component declaration, template variable declarations, and element references do not contain such a member`));
   it('should reject an uncalled event handler',
      () => reject(
          '<div (click)="click">{{person.name.first}}</div>', 'Unexpected callable expression'));

packages/language-service/test/project/app/parsing-cases.ts

@@ -154,6 +154,7 @@ export class TemplateReference {
   heroesByName: {[name: string]: Hero} = {};
   primitiveIndexType: {[name: string]: string} = {};
   anyValue: any;
+  optional?: string;
   myClick(event: any) {}
 }