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
This commit is contained in:
Ayaz Hafiz 2020-04-08 13:48:35 -07:00 committed by Matias Niemelä
parent 78840e59a7
commit 8be0972836
6 changed files with 69 additions and 81 deletions

View File

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

View File

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

View File

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

View File

@ -897,25 +897,20 @@ function typeKindOf(type: ts.Type|undefined): BuiltinType {
return BuiltinType.String; return BuiltinType.String;
} else if (type.flags & (ts.TypeFlags.Number | ts.TypeFlags.NumberLike)) { } else if (type.flags & (ts.TypeFlags.Number | ts.TypeFlags.NumberLike)) {
return BuiltinType.Number; return BuiltinType.Number;
} else if (type.flags & ts.TypeFlags.Object) {
return BuiltinType.Object;
} else if (type.flags & (ts.TypeFlags.Undefined)) { } else if (type.flags & (ts.TypeFlags.Undefined)) {
return BuiltinType.Undefined; return BuiltinType.Undefined;
} else if (type.flags & (ts.TypeFlags.Null)) { } else if (type.flags & (ts.TypeFlags.Null)) {
return BuiltinType.Null; return BuiltinType.Null;
} else if (type.flags & ts.TypeFlags.Union) { } 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; const unionType = type as ts.UnionType;
if (unionType.types.length > 0) { if (unionType.types.length === 0) return BuiltinType.Other;
candidate = typeKindOf(unionType.types[0]); let ty: BuiltinType = 0;
for (const subType of unionType.types) { for (const subType of unionType.types) {
if (candidate != typeKindOf(subType)) { ty |= typeKindOf(subType);
return BuiltinType.Other;
}
}
}
if (candidate != null) {
return candidate;
} }
return ty;
} else if (type.flags & ts.TypeFlags.TypeParameter) { } else if (type.flags & ts.TypeFlags.TypeParameter) {
return BuiltinType.Unbound; return BuiltinType.Unbound;
} }

View File

@ -23,8 +23,6 @@ import {MockTypescriptHost} from './test_utils';
* as well. * 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 TEST_TEMPLATE = '/app/test.ng';
const APP_COMPONENT = '/app/app.component.ts'; const APP_COMPONENT = '/app/app.component.ts';
@ -121,8 +119,41 @@ describe('diagnostics', () => {
expect(diagnostics).toEqual([]); 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', () => { 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, ` mockHost.override(TEST_TEMPLATE, `
<div *ngFor="let hero of heroes; let i = index; let isFirst = first"> <div *ngFor="let hero of heroes; let i = index; let isFirst = first">
'i' is a number; 'isFirst' is a boolean 'i' is a number; 'isFirst' is a boolean
@ -131,7 +162,7 @@ describe('diagnostics', () => {
`); `);
const diags = ngLS.getSemanticDiagnostics(TEST_TEMPLATE); const diags = ngLS.getSemanticDiagnostics(TEST_TEMPLATE);
expect(diags.length).toBe(1); 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', () => { 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 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 // Issue #13326
it('should report a narrow span for invalid pipes', () => { it('should report a narrow span for invalid pipes', () => {
const content = mockHost.override(APP_COMPONENT, ` const content = mockHost.override(APP_COMPONENT, `

View File

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