From 8be09728361cb994b28f52fde02ce500786d0f99 Mon Sep 17 00:00:00 2001 From: Ayaz Hafiz Date: Wed, 8 Apr 2020 13:48:35 -0700 Subject: [PATCH] fix(language-service): properly evaluate types in comparable expressions (#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 https://github.com/angular/vscode-ng-language-service/issues/723 PR Close #36529 --- .../src/diagnostic_messages.ts | 6 +- .../language-service/src/expression_type.ts | 44 ++++---------- packages/language-service/src/symbols.ts | 21 ++++--- .../src/typescript_symbols.ts | 19 +++--- .../language-service/test/diagnostics_spec.ts | 59 +++++++++++-------- .../test/project/app/parsing-cases.ts | 1 + 6 files changed, 69 insertions(+), 81 deletions(-) diff --git a/packages/language-service/src/diagnostic_messages.ts b/packages/language-service/src/diagnostic_messages.ts index 749dceb8fd..f045d7f751 100644 --- a/packages/language-service/src/diagnostic_messages.ts +++ b/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 = { 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', }, diff --git a/packages/language-service/src/expression_type.ts b/packages/language-service/src/expression_type.ts index 9364d07c52..732fe97910 100644 --- a/packages/language-service/src/expression_type.ts +++ b/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 '||': diff --git a/packages/language-service/src/symbols.ts b/packages/language-service/src/symbols.ts index f4eb514282..a3d3426db0 100644 --- a/packages/language-service/src/symbols.ts +++ b/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, } /** diff --git a/packages/language-service/src/typescript_symbols.ts b/packages/language-service/src/typescript_symbols.ts index b09cbb8bc7..dabd772459 100644 --- a/packages/language-service/src/typescript_symbols.ts +++ b/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; } diff --git a/packages/language-service/test/diagnostics_spec.ts b/packages/language-service/test/diagnostics_spec.ts index 3beb06c819..22a1e937c7 100644 --- a/packages/language-service/test/diagnostics_spec.ts +++ b/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, `
'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: '
' - }) - 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, ` diff --git a/packages/language-service/test/project/app/parsing-cases.ts b/packages/language-service/test/project/app/parsing-cases.ts index 69b6c5b8fe..36a79948a5 100644 --- a/packages/language-service/test/project/app/parsing-cases.ts +++ b/packages/language-service/test/project/app/parsing-cases.ts @@ -133,4 +133,5 @@ export class TemplateReference { readonlyHeroes: ReadonlyArray> = this.heroes; constNames = [{name: 'name'}] as const; private myField = 'My Field'; + strOrNumber: string|number = ''; }