fix(language-service): properly evaluate types in comparable expressi…

…ons (#36529)

This commit fixes how the language service evaluates the compatibility
of types to work with arbitrary union types. As a result, compatibility
checks are now more strict and can catch similarities or differences
more clearly.

```
number|string == string|null  // OK
number|string == number       // OK
number|string == null         // not comparable
number == string              // not comparable
```

Using Ivy as a backend should provide these diagnoses for free, but we
can backfill them for now.

Closes angular/vscode-ng-language-service#723

PR Close #36529

packages/language-service/src/diagnostic_messages.ts

@@ -18,7 +18,7 @@ type DiagnosticName = 'directive_not_in_module'|'missing_template_and_templateur
     'both_template_and_templateurl'|'invalid_templateurl'|'template_context_missing_member'|
     'callable_expression_expected_method_call'|'call_target_not_callable'|
     'expression_might_be_null'|'expected_a_number_type'|'expected_a_string_or_number_type'|
-    'expected_operands_of_similar_type_or_any'|'unrecognized_operator'|'unrecognized_primitive'|
+    'expected_operands_of_comparable_types_or_any'|'unrecognized_operator'|'unrecognized_primitive'|
     'no_pipe_found'|'unable_to_resolve_compatible_call_signature'|'unable_to_resolve_signature'|
     'could_not_resolve_type'|'identifier_not_callable'|'identifier_possibly_undefined'|
     'identifier_not_defined_in_app_context'|'identifier_not_defined_on_receiver'|
@@ -77,8 +77,8 @@ export const Diagnostic: Record<DiagnosticName, DiagnosticMessage> = {
     kind: 'Error',
   },
 
-  expected_operands_of_similar_type_or_any: {
-    message: 'Expected operands to be of similar type or any',
+  expected_operands_of_comparable_types_or_any: {
+    message: 'Expected operands to be of comparable types or any',
     kind: 'Error',
   },
 

packages/language-service/src/expression_type.ts

@@ -38,16 +38,6 @@ export class AstType implements AstVisitor {
   }
 
   visitBinary(ast: Binary): Symbol {
-    // Treat undefined and null as other.
-    function normalize(kind: BuiltinType, other: BuiltinType): BuiltinType {
-      switch (kind) {
-        case BuiltinType.Undefined:
-        case BuiltinType.Null:
-          return normalize(other, BuiltinType.Other);
-      }
-      return kind;
-    }
-
     const getType = (ast: AST, operation: string): Symbol => {
       const type = this.getType(ast);
       if (type.nullable) {
@@ -64,17 +54,14 @@ export class AstType implements AstVisitor {
             this.diagnostics.push(createDiagnostic(ast.span, Diagnostic.expression_might_be_null));
             break;
         }
-        return this.query.getNonNullableType(type);
       }
       return type;
     };
 
     const leftType = getType(ast.left, ast.operation);
     const rightType = getType(ast.right, ast.operation);
-    const leftRawKind = this.query.getTypeKind(leftType);
-    const rightRawKind = this.query.getTypeKind(rightType);
-    const leftKind = normalize(leftRawKind, rightRawKind);
-    const rightKind = normalize(rightRawKind, leftRawKind);
+    const leftKind = this.query.getTypeKind(leftType);
+    const rightKind = this.query.getTypeKind(rightType);
 
     // The following swtich implements operator typing similar to the
     // type production tables in the TypeScript specification.
@@ -154,26 +141,15 @@ export class AstType implements AstVisitor {
       case '!=':
       case '===':
       case '!==':
-        switch (operKind) {
-          case BuiltinType.Any << 8 | BuiltinType.Any:
-          case BuiltinType.Any << 8 | BuiltinType.Boolean:
-          case BuiltinType.Any << 8 | BuiltinType.Number:
-          case BuiltinType.Any << 8 | BuiltinType.String:
-          case BuiltinType.Any << 8 | BuiltinType.Other:
-          case BuiltinType.Boolean << 8 | BuiltinType.Any:
-          case BuiltinType.Boolean << 8 | BuiltinType.Boolean:
-          case BuiltinType.Number << 8 | BuiltinType.Any:
-          case BuiltinType.Number << 8 | BuiltinType.Number:
-          case BuiltinType.String << 8 | BuiltinType.Any:
-          case BuiltinType.String << 8 | BuiltinType.String:
-          case BuiltinType.Other << 8 | BuiltinType.Any:
-          case BuiltinType.Other << 8 | BuiltinType.Other:
-            return this.query.getBuiltinType(BuiltinType.Boolean);
-          default:
-            this.diagnostics.push(
-                createDiagnostic(ast.span, Diagnostic.expected_operands_of_similar_type_or_any));
-            return this.anyType;
+        if (!(leftKind & rightKind) &&
+            !((leftKind | rightKind) & (BuiltinType.Null | BuiltinType.Undefined))) {
+          // Two values are comparable only if
+          //   - they have some type overlap, or
+          //   - at least one is not defined
+          this.diagnostics.push(
+              createDiagnostic(ast.span, Diagnostic.expected_operands_of_comparable_types_or_any));
         }
+        return this.query.getBuiltinType(BuiltinType.Boolean);
       case '&&':
         return rightType;
       case '||':

packages/language-service/src/symbols.ts

@@ -192,42 +192,47 @@ export enum BuiltinType {
   /**
    * The type is a type that can hold any other type.
    */
-  Any,
+  Any = -1,  // equivalent to b11..11 = String | Union | ...
+
+  /** Unknown types are functionally identical to any. */
+  Unknown = -1,
 
   /**
    * The type of a string literal.
    */
-  String,
+  String = 1 << 0,
 
   /**
    * The type of a numeric literal.
    */
-  Number,
+  Number = 1 << 1,
 
   /**
    * The type of the `true` and `false` literals.
    */
-  Boolean,
+  Boolean = 1 << 2,
 
   /**
    * The type of the `undefined` literal.
    */
-  Undefined,
+  Undefined = 1 << 3,
 
   /**
    * the type of the `null` literal.
    */
-  Null,
+  Null = 1 << 4,
 
   /**
    * the type is an unbound type parameter.
    */
-  Unbound,
+  Unbound = 1 << 5,
 
   /**
    * Not a built-in type.
    */
-  Other
+  Other = 1 << 6,
+
+  Object = 1 << 7,
 }
 
 /**

packages/language-service/src/typescript_symbols.ts

@@ -897,25 +897,20 @@ function typeKindOf(type: ts.Type|undefined): BuiltinType {
       return BuiltinType.String;
     } else if (type.flags & (ts.TypeFlags.Number | ts.TypeFlags.NumberLike)) {
       return BuiltinType.Number;
+    } else if (type.flags & ts.TypeFlags.Object) {
+      return BuiltinType.Object;
     } else if (type.flags & (ts.TypeFlags.Undefined)) {
       return BuiltinType.Undefined;
     } else if (type.flags & (ts.TypeFlags.Null)) {
       return BuiltinType.Null;
     } else if (type.flags & ts.TypeFlags.Union) {
-      // If all the constituent types of a union are the same kind, it is also that kind.
-      let candidate: BuiltinType|null = null;
       const unionType = type as ts.UnionType;
-      if (unionType.types.length > 0) {
-        candidate = typeKindOf(unionType.types[0]);
-        for (const subType of unionType.types) {
-          if (candidate != typeKindOf(subType)) {
-            return BuiltinType.Other;
-          }
-        }
-      }
-      if (candidate != null) {
-        return candidate;
+      if (unionType.types.length === 0) return BuiltinType.Other;
+      let ty: BuiltinType = 0;
+      for (const subType of unionType.types) {
+        ty |= typeKindOf(subType);
       }
+      return ty;
     } else if (type.flags & ts.TypeFlags.TypeParameter) {
       return BuiltinType.Unbound;
     }

packages/language-service/test/diagnostics_spec.ts

@@ -23,8 +23,6 @@ import {MockTypescriptHost} from './test_utils';
  * as well.
  */
 
-const EXPRESSION_CASES = '/app/expression-cases.ts';
-const NG_FOR_CASES = '/app/ng-for-cases.ts';
 const TEST_TEMPLATE = '/app/test.ng';
 const APP_COMPONENT = '/app/app.component.ts';
 
@@ -121,8 +119,41 @@ describe('diagnostics', () => {
     expect(diagnostics).toEqual([]);
   });
 
+  describe('diagnostics for expression comparisons', () => {
+    for (let [left, right, leftTy, rightTy] of [
+             ['\'abc\'', 1, 'string', 'number'],
+             ['hero', 2, 'object', 'number'],
+             ['strOrNumber', 'hero', 'string|number', 'object'],
+    ]) {
+      it(`it should report errors for mismtched types in a comparison: ${leftTy} and ${rightTy}`,
+         () => {
+           mockHost.override(TEST_TEMPLATE, `{{ ${left} != ${right} }}`);
+           const diags = ngLS.getSemanticDiagnostics(TEST_TEMPLATE);
+           expect(diags.length).toBe(1);
+           expect(diags[0].messageText).toBe(`Expected operands to be of comparable types or any`);
+         });
+    }
+
+    for (let [left, right, leftTy, rightTy] of [
+             ['\'abc\'', 'anyValue', 'string', 'any'],
+             ['\'abc\'', null, 'string', 'null'],
+             ['\'abc\'', undefined, 'string', 'undefined'],
+             [null, null, 'null', 'null'],
+             ['{a: 1}', '{b: 2}', 'object', 'object'],
+             ['strOrNumber', '1', 'string|number', 'number'],
+    ]) {
+      it(`it should not report errors for compatible types in a comparison: ${leftTy} and ${
+             rightTy}`,
+         () => {
+           mockHost.override(TEST_TEMPLATE, `{{ ${left} != ${right} }}`);
+           const diags = ngLS.getSemanticDiagnostics(TEST_TEMPLATE);
+           expect(diags.length).toBe(0);
+         });
+    }
+  });
+
   describe('diagnostics for ngFor exported values', () => {
-    it('should report errors for mistmatched exported types', () => {
+    it('should report errors for mismatched exported types', () => {
       mockHost.override(TEST_TEMPLATE, `
         <div *ngFor="let hero of heroes; let i = index; let isFirst = first">
             'i' is a number; 'isFirst' is a boolean
@@ -131,7 +162,7 @@ describe('diagnostics', () => {
       `);
       const diags = ngLS.getSemanticDiagnostics(TEST_TEMPLATE);
       expect(diags.length).toBe(1);
-      expect(diags[0].messageText).toBe(`Expected operands to be of similar type or any`);
+      expect(diags[0].messageText).toBe(`Expected operands to be of comparable types or any`);
     });
 
     it('should not report errors for matching exported type', () => {
@@ -583,26 +614,6 @@ describe('diagnostics', () => {
     expect(length).toBe(keyword.length - 2);               // exclude leading and trailing quotes
   });
 
-  // #13412
-  it('should not report an error for using undefined under non-strict mode', () => {
-    mockHost.override(APP_COMPONENT, `
-      import { Component } from '@angular/core';
-
-      @Component({
-        template: '<div *ngIf="something === undefined"></div>'
-      })
-      export class AppComponent {
-        something = 'foo';
-      }`);
-    mockHost.overrideOptions({
-      strict: false,  // TODO: This test fails in strict mode
-    });
-    const tsDiags = tsLS.getSemanticDiagnostics(APP_COMPONENT);
-    expect(tsDiags).toEqual([]);
-    const ngDiags = ngLS.getSemanticDiagnostics(APP_COMPONENT);
-    expect(ngDiags).toEqual([]);
-  });
-
   // Issue #13326
   it('should report a narrow span for invalid pipes', () => {
     const content = mockHost.override(APP_COMPONENT, `

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

@@ -133,4 +133,5 @@ export class TemplateReference {
   readonlyHeroes: ReadonlyArray<Readonly<Hero>> = this.heroes;
   constNames = [{name: 'name'}] as const;
   private myField = 'My Field';
+  strOrNumber: string|number = '';
 }