Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use type parameter constraints for computed property types #17404

Merged
merged 10 commits into from
Aug 8, 2017
Merged

Use type parameter constraints for computed property types #17404

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
43981ea
Use type param constraints for computed prop types
sandersn Jul 25, 2017
1fb6d34
Test:use type param constraints for computed prop types
sandersn Jul 25, 2017
838fbdd
Make isTypeOfKind delegate to isTypeAssignableTo
sandersn Jul 26, 2017
d07eca7
Improve name:isTypeAssignableToKind+cleanup TODOs
sandersn Aug 2, 2017
011f712
isTypeAssignableToKind:Chain isTypeAssignableTo calls
sandersn Aug 2, 2017
aed386c
Add regression test cases and rename test
sandersn Aug 2, 2017
a0333b5
Merge branch 'master' into use-type-param-constraints-for-computed-pr…
sandersn Aug 2, 2017
e494d73
Merge branch 'master' into use-type-param-constraints-for-computed-pr…
sandersn Aug 8, 2017
e47df36
Use isTypeAny instead of checking flags directly
sandersn Aug 8, 2017
fac93a3
Add parentheses:clarify evaluation order of &&/||
sandersn Aug 8, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 47 additions & 39 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7526,11 +7526,11 @@ namespace ts {
return getTypeOfSymbol(prop);
}
}
if (isTypeAnyOrAllConstituentTypesHaveKind(indexType, TypeFlags.StringLike | TypeFlags.NumberLike | TypeFlags.ESSymbol)) {
if (!(indexType.flags & TypeFlags.Nullable) && isTypeOfKind(indexType, TypeFlags.StringLike | TypeFlags.NumberLike | TypeFlags.ESSymbol)) {
if (isTypeAny(objectType)) {
return anyType;
}
const indexInfo = isTypeAnyOrAllConstituentTypesHaveKind(indexType, TypeFlags.NumberLike) && getIndexInfoOfType(objectType, IndexKind.Number) ||
const indexInfo = isTypeOfKind(indexType, TypeFlags.NumberLike) && getIndexInfoOfType(objectType, IndexKind.Number) ||
getIndexInfoOfType(objectType, IndexKind.String) ||
undefined;
if (indexInfo) {
Expand Down Expand Up @@ -11286,7 +11286,7 @@ namespace ts {
(<BinaryExpression>parent.parent).operatorToken.kind === SyntaxKind.EqualsToken &&
(<BinaryExpression>parent.parent).left === parent &&
!isAssignmentTarget(parent.parent) &&
isTypeAnyOrAllConstituentTypesHaveKind(getTypeOfExpression((<ElementAccessExpression>parent).argumentExpression), TypeFlags.NumberLike | TypeFlags.Undefined);
isTypeOfKind(getTypeOfExpression((<ElementAccessExpression>parent).argumentExpression), TypeFlags.NumberLike);
return isLengthPushOrUnshift || isElementAssignment;
}

Expand Down Expand Up @@ -11458,7 +11458,7 @@ namespace ts {
}
else {
const indexType = getTypeOfExpression((<ElementAccessExpression>(<BinaryExpression>node).left).argumentExpression);
if (isTypeAnyOrAllConstituentTypesHaveKind(indexType, TypeFlags.NumberLike | TypeFlags.Undefined)) {
if (isTypeOfKind(indexType, TypeFlags.NumberLike)) {
evolvedType = addEvolvingArrayElementType(evolvedType, (<BinaryExpression>node).right);
}
}
Expand Down Expand Up @@ -13290,11 +13290,7 @@ namespace ts {
function isNumericComputedName(name: ComputedPropertyName): boolean {
// It seems odd to consider an expression of type Any to result in a numeric name,
// but this behavior is consistent with checkIndexedAccess
return isTypeAnyOrAllConstituentTypesHaveKind(checkComputedPropertyName(name), TypeFlags.NumberLike);
}

function isTypeAnyOrAllConstituentTypesHaveKind(type: Type, kind: TypeFlags): boolean {
return isTypeAny(type) || isTypeOfKind(type, kind);
return isTypeOfKind(checkComputedPropertyName(name), TypeFlags.NumberLike);
}

function isInfinityOrNaNString(name: string | __String): boolean {
Expand Down Expand Up @@ -13330,10 +13326,9 @@ namespace ts {
const links = getNodeLinks(node.expression);
if (!links.resolvedType) {
links.resolvedType = checkExpression(node.expression);

// This will allow types number, string, symbol or any. It will also allow enums, the unknown
// type, and any union of these types (like string | number).
if (!isTypeAnyOrAllConstituentTypesHaveKind(links.resolvedType, TypeFlags.NumberLike | TypeFlags.StringLike | TypeFlags.ESSymbol)) {
if (links.resolvedType.flags & TypeFlags.Nullable || !isTypeOfKind(links.resolvedType, TypeFlags.StringLike | TypeFlags.NumberLike | TypeFlags.ESSymbol)) {
error(node, Diagnostics.A_computed_property_name_must_be_of_type_string_number_symbol_or_any);
}
else {
Expand Down Expand Up @@ -16818,7 +16813,7 @@ namespace ts {
}

function checkArithmeticOperandType(operand: Node, type: Type, diagnostic: DiagnosticMessage): boolean {
if (!isTypeAnyOrAllConstituentTypesHaveKind(type, TypeFlags.NumberLike)) {
if (!isTypeOfKind(type, TypeFlags.NumberLike)) {
error(operand, diagnostic);
return false;
}
Expand Down Expand Up @@ -16991,31 +16986,43 @@ namespace ts {
return false;
}

// Return true if type is of the given kind. A union type is of a given kind if all constituent types
// are of the given kind. An intersection type is of a given kind if at least one constituent type is
// of the given kind.
function isTypeOfKind(type: Type, kind: TypeFlags): boolean {
if (type.flags & kind) {
function isTypeOfKind(source: Type, kind: TypeFlags, excludeAny?: boolean) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I get why we need this excludeAny parameter?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checkBinaryLikeExpression has a complicated way of checking legal types for +:

  1. It disallows null + number and number + undefined, even with strictNullChecks off. I like this check and want to keep it.
  2. It gives a different types for string + any ==> string vs T + any ==> any and T + unknown ==> unknown.

It does this with the following (simplified) structure (using the abbreviation tak for isTypeAssignableToKind):

if (tak(left, NumberLike) && tak(right, NumberLike)) {
  return numberType;
}
else if (tak(left, StringLike) || tak(right, StringLike)) {
  return stringType;
}
else if (isTypeAny(left) || isTypeAny(right)) {
  return anyType;
}
else {
  returned undefined; // signal an error
}

Without excludeAny (now called strict), isTypeAssignableTo(number, undefined) is true, and number + undefined/null is now legal. Also, isTypeAssignableTo(number, any) is true, and number + any ==> number , and any + any ==> number.

I tried a few things to pull the any checks before the calls to isTypeAssignableToKind, but was not successful. If you see a way, let me know.

if (source.flags & kind) {
return true;
}
if (type.flags & TypeFlags.Union) {
const types = (<UnionOrIntersectionType>type).types;
for (const t of types) {
if (!isTypeOfKind(t, kind)) {
return false;
}
}
return true;
if (excludeAny && source.flags & (TypeFlags.Any | TypeFlags.Void | TypeFlags.Undefined | TypeFlags.Null)) {
// TODO: The callers who want this should really handle these cases FIRST
return false;
}
if (type.flags & TypeFlags.Intersection) {
const types = (<UnionOrIntersectionType>type).types;
for (const t of types) {
if (isTypeOfKind(t, kind)) {
return true;
}
}
const targets = [];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you building up a union type here? It causes allocations and ends up being broken down to discrete checks for each contained type anyway. I think it would be better to just have a sequence of conditional isTypeAssignableTo calls here, e.g.

return kind & TypeFlags.NumberLike && isTypeAssignableTo(source, numberType) ||
    kind & TypeFlags.StringLike && isTypeAssignableTo(source, stringType) ||
    kind & TypeFlags.BooleanLike && isTypeAssignableTo(source, BooleanType) ||
    ...

Copy link
Member Author

@sandersn sandersn Aug 2, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

var p1: number | string
var o = {
  [p1]() { }
//~~~~ A computed property name must be of type 'string', 'number', ...
}

But I think I can fix that by adding a fallback to isTypeAssignableTo the union type in just the case of computed properties.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. The computed property name type check now has a fallback call to isTypeAssignableTo(links.resolvedType, getUnionType([numberType, stringType, esSymbolType]). Everything else is fine with a sequence of isTypeAssignableTo calls.

if (kind & TypeFlags.NumberLike) {
targets.push(numberType);
}
return false;
if (kind & TypeFlags.StringLike) {
targets.push(stringType);
}
if (kind & TypeFlags.BooleanLike) {
targets.push(booleanType);
}
if (kind & TypeFlags.Void) {
targets.push(voidType);
}
if (kind & TypeFlags.Never) {
targets.push(neverType);
}
if (kind & TypeFlags.Null) {
targets.push(nullType);
}
if (kind & TypeFlags.Undefined) {
targets.push(undefinedType);
}
if (kind & TypeFlags.ESSymbol) {
targets.push(esSymbolType);
}
if (kind & TypeFlags.NonPrimitive) {
targets.push(nonPrimitiveType);
}
return isTypeAssignableTo(source, getUnionType(targets));
}

function isConstEnumObjectType(type: Type): boolean {
Expand All @@ -17035,7 +17042,7 @@ namespace ts {
// and the right operand to be of type Any, a subtype of the 'Function' interface type, or have a call or construct signature.
// The result is always of the Boolean primitive type.
// NOTE: do not raise error if leftType is unknown as related error was already reported
if (isTypeOfKind(leftType, TypeFlags.Primitive)) {
if (isTypeOfKind(leftType, TypeFlags.Primitive, /*excludeAny*/ true)) {
error(left, Diagnostics.The_left_hand_side_of_an_instanceof_expression_must_be_of_type_any_an_object_type_or_a_type_parameter);
}
// NOTE: do not raise error if right is unknown as related error was already reported
Expand All @@ -17061,7 +17068,7 @@ namespace ts {
if (!(isTypeComparableTo(leftType, stringType) || isTypeOfKind(leftType, TypeFlags.NumberLike | TypeFlags.ESSymbol))) {
error(left, Diagnostics.The_left_hand_side_of_an_in_expression_must_be_of_type_any_string_number_or_symbol);
}
if (!isTypeAnyOrAllConstituentTypesHaveKind(rightType, TypeFlags.Object | TypeFlags.TypeVariable | TypeFlags.NonPrimitive)) {
if (!isTypeOfKind(rightType, TypeFlags.NonPrimitive | TypeFlags.TypeVariable)) {
error(right, Diagnostics.The_right_hand_side_of_an_in_expression_must_be_of_type_any_an_object_type_or_a_type_parameter);
}
return booleanType;
Expand Down Expand Up @@ -17370,25 +17377,26 @@ namespace ts {
return silentNeverType;
}

if (!isTypeOfKind(leftType, TypeFlags.Any | TypeFlags.StringLike) && !isTypeOfKind(rightType, TypeFlags.Any | TypeFlags.StringLike)) {
if (!isTypeOfKind(leftType, TypeFlags.StringLike) && !isTypeOfKind(rightType, TypeFlags.StringLike)) {
leftType = checkNonNullType(leftType, left);
rightType = checkNonNullType(rightType, right);
}

let resultType: Type;
if (isTypeOfKind(leftType, TypeFlags.NumberLike) && isTypeOfKind(rightType, TypeFlags.NumberLike)) {
if (isTypeOfKind(leftType, TypeFlags.NumberLike, /*excludeAny*/ true) && isTypeOfKind(rightType, TypeFlags.NumberLike, /*excludeAny*/ true)) {
// Operands of an enum type are treated as having the primitive type Number.
// If both operands are of the Number primitive type, the result is of the Number primitive type.
resultType = numberType;
}
else {
if (isTypeOfKind(leftType, TypeFlags.StringLike) || isTypeOfKind(rightType, TypeFlags.StringLike)) {
if (isTypeOfKind(leftType, TypeFlags.StringLike, /*excludeAny*/ true) || isTypeOfKind(rightType, TypeFlags.StringLike, /*excludeAny*/ true)) {
// If one or both operands are of the String primitive type, the result is of the String primitive type.
resultType = stringType;
}
else if (isTypeAny(leftType) || isTypeAny(rightType)) {
// Otherwise, the result is of type Any.
// NOTE: unknown type here denotes error type. Old compiler treated this case as any type so do we.
// TODO: Reorder this to check for any (plus void/undefined/null) first
resultType = leftType === unknownType || rightType === unknownType ? unknownType : anyType;
}

Expand Down Expand Up @@ -20314,7 +20322,7 @@ namespace ts {

// unknownType is returned i.e. if node.expression is identifier whose name cannot be resolved
// in this case error about missing name is already reported - do not report extra one
if (!isTypeAnyOrAllConstituentTypesHaveKind(rightType, TypeFlags.Object | TypeFlags.TypeVariable | TypeFlags.NonPrimitive)) {
if (!isTypeOfKind(rightType, TypeFlags.NonPrimitive | TypeFlags.TypeVariable)) {
error(node.expression, Diagnostics.The_right_hand_side_of_a_for_in_statement_must_be_of_type_any_an_object_type_or_a_type_parameter);
}

Expand Down
66 changes: 33 additions & 33 deletions tests/baselines/reference/additionOperatorWithNullValueAndInvalidOperator.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
tests/cases/conformance/expressions/binaryOperators/additionOperator/additionOperatorWithNullValueAndInvalidOperator.ts(11,10): error TS2531: Object is possibly 'null'.
tests/cases/conformance/expressions/binaryOperators/additionOperator/additionOperatorWithNullValueAndInvalidOperator.ts(12,10): error TS2531: Object is possibly 'null'.
tests/cases/conformance/expressions/binaryOperators/additionOperator/additionOperatorWithNullValueAndInvalidOperator.ts(13,10): error TS2531: Object is possibly 'null'.
tests/cases/conformance/expressions/binaryOperators/additionOperator/additionOperatorWithNullValueAndInvalidOperator.ts(14,14): error TS2531: Object is possibly 'null'.
tests/cases/conformance/expressions/binaryOperators/additionOperator/additionOperatorWithNullValueAndInvalidOperator.ts(15,14): error TS2531: Object is possibly 'null'.
tests/cases/conformance/expressions/binaryOperators/additionOperator/additionOperatorWithNullValueAndInvalidOperator.ts(16,10): error TS2531: Object is possibly 'null'.
tests/cases/conformance/expressions/binaryOperators/additionOperator/additionOperatorWithNullValueAndInvalidOperator.ts(19,10): error TS2531: Object is possibly 'null'.
tests/cases/conformance/expressions/binaryOperators/additionOperator/additionOperatorWithNullValueAndInvalidOperator.ts(20,10): error TS2531: Object is possibly 'null'.
tests/cases/conformance/expressions/binaryOperators/additionOperator/additionOperatorWithNullValueAndInvalidOperator.ts(21,10): error TS2531: Object is possibly 'null'.
tests/cases/conformance/expressions/binaryOperators/additionOperator/additionOperatorWithNullValueAndInvalidOperator.ts(22,11): error TS2531: Object is possibly 'null'.
tests/cases/conformance/expressions/binaryOperators/additionOperator/additionOperatorWithNullValueAndInvalidOperator.ts(23,11): error TS2531: Object is possibly 'null'.
tests/cases/conformance/expressions/binaryOperators/additionOperator/additionOperatorWithNullValueAndInvalidOperator.ts(11,10): error TS2365: Operator '+' cannot be applied to types 'null' and 'boolean'.
tests/cases/conformance/expressions/binaryOperators/additionOperator/additionOperatorWithNullValueAndInvalidOperator.ts(12,10): error TS2365: Operator '+' cannot be applied to types 'null' and 'Object'.
tests/cases/conformance/expressions/binaryOperators/additionOperator/additionOperatorWithNullValueAndInvalidOperator.ts(13,10): error TS2365: Operator '+' cannot be applied to types 'null' and 'void'.
tests/cases/conformance/expressions/binaryOperators/additionOperator/additionOperatorWithNullValueAndInvalidOperator.ts(14,10): error TS2365: Operator '+' cannot be applied to types 'boolean' and 'null'.
tests/cases/conformance/expressions/binaryOperators/additionOperator/additionOperatorWithNullValueAndInvalidOperator.ts(15,10): error TS2365: Operator '+' cannot be applied to types 'Object' and 'null'.
tests/cases/conformance/expressions/binaryOperators/additionOperator/additionOperatorWithNullValueAndInvalidOperator.ts(16,10): error TS2365: Operator '+' cannot be applied to types 'null' and 'void'.
tests/cases/conformance/expressions/binaryOperators/additionOperator/additionOperatorWithNullValueAndInvalidOperator.ts(19,10): error TS2365: Operator '+' cannot be applied to types 'null' and 'Number'.
tests/cases/conformance/expressions/binaryOperators/additionOperator/additionOperatorWithNullValueAndInvalidOperator.ts(20,10): error TS2365: Operator '+' cannot be applied to types 'null' and 'true'.
tests/cases/conformance/expressions/binaryOperators/additionOperator/additionOperatorWithNullValueAndInvalidOperator.ts(21,10): error TS2365: Operator '+' cannot be applied to types 'null' and '{ a: string; }'.
tests/cases/conformance/expressions/binaryOperators/additionOperator/additionOperatorWithNullValueAndInvalidOperator.ts(22,11): error TS2365: Operator '+' cannot be applied to types 'null' and 'void'.
tests/cases/conformance/expressions/binaryOperators/additionOperator/additionOperatorWithNullValueAndInvalidOperator.ts(23,11): error TS2365: Operator '+' cannot be applied to types 'null' and '() => void'.


==== tests/cases/conformance/expressions/binaryOperators/additionOperator/additionOperatorWithNullValueAndInvalidOperator.ts (11 errors) ====
Expand All @@ -23,37 +23,37 @@ tests/cases/conformance/expressions/binaryOperators/additionOperator/additionOpe

// null + boolean/Object
var r1 = null + a;
~~~~
!!! error TS2531: Object is possibly 'null'.
~~~~~~~~
!!! error TS2365: Operator '+' cannot be applied to types 'null' and 'boolean'.
var r2 = null + b;
~~~~
!!! error TS2531: Object is possibly 'null'.
~~~~~~~~
!!! error TS2365: Operator '+' cannot be applied to types 'null' and 'Object'.
var r3 = null + c;
~~~~
!!! error TS2531: Object is possibly 'null'.
~~~~~~~~
!!! error TS2365: Operator '+' cannot be applied to types 'null' and 'void'.
var r4 = a + null;
~~~~
!!! error TS2531: Object is possibly 'null'.
~~~~~~~~
!!! error TS2365: Operator '+' cannot be applied to types 'boolean' and 'null'.
var r5 = b + null;
~~~~
!!! error TS2531: Object is possibly 'null'.
~~~~~~~~
!!! error TS2365: Operator '+' cannot be applied to types 'Object' and 'null'.
var r6 = null + c;
~~~~
!!! error TS2531: Object is possibly 'null'.
~~~~~~~~
!!! error TS2365: Operator '+' cannot be applied to types 'null' and 'void'.

// other cases
var r7 = null + d;
~~~~
!!! error TS2531: Object is possibly 'null'.
~~~~~~~~
!!! error TS2365: Operator '+' cannot be applied to types 'null' and 'Number'.
var r8 = null + true;
~~~~
!!! error TS2531: Object is possibly 'null'.
~~~~~~~~~~~
!!! error TS2365: Operator '+' cannot be applied to types 'null' and 'true'.
var r9 = null + { a: '' };
~~~~
!!! error TS2531: Object is possibly 'null'.
~~~~~~~~~~~~~~~~
!!! error TS2365: Operator '+' cannot be applied to types 'null' and '{ a: string; }'.
var r10 = null + foo();
~~~~
!!! error TS2531: Object is possibly 'null'.
~~~~~~~~~~~~
!!! error TS2365: Operator '+' cannot be applied to types 'null' and 'void'.
var r11 = null + (() => { });
~~~~
!!! error TS2531: Object is possibly 'null'.
~~~~~~~~~~~~~~~~~~
!!! error TS2365: Operator '+' cannot be applied to types 'null' and '() => void'.
Loading