Introduce the "comparable" relation #5517
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -211,6 +211,7 @@ namespace ts { | |
|
|
||
| let subtypeRelation: Map<RelationComparisonResult> = {}; | ||
| let assignableRelation: Map<RelationComparisonResult> = {}; | ||
| let comparableRelation: Map<RelationComparisonResult> = {}; | ||
| let identityRelation: Map<RelationComparisonResult> = {}; | ||
|
|
||
| // This is for caching the result of getSymbolDisplayBuilder. Do not access directly. | ||
|
|
@@ -4738,6 +4739,14 @@ namespace ts { | |
| return checkTypeAssignableTo(source, target, /*errorNode*/ undefined); | ||
| } | ||
|
|
||
| /** | ||
| * This is *not* a bi-directional relationship. | ||
| * If one needs to check both directions for comparability, use a second call to this function or 'checkTypeComparableTo'. | ||
|
There was a problem hiding this comment. All uses do need to check both directions, so probably you should have a version that does that. You might not even need the uni-directional ones except for consistency with the other relations. There was a problem hiding this comment. See this comment |
||
| */ | ||
| function isTypeComparableTo(source: Type, target: Type): boolean { | ||
| return checkTypeComparableTo(source, target, /*errorNode*/ undefined); | ||
| } | ||
|
|
||
| function checkTypeSubtypeOf(source: Type, target: Type, errorNode: Node, headMessage?: DiagnosticMessage, containingMessageChain?: DiagnosticMessageChain): boolean { | ||
| return checkTypeRelatedTo(source, target, subtypeRelation, errorNode, headMessage, containingMessageChain); | ||
| } | ||
|
|
@@ -4746,6 +4755,14 @@ namespace ts { | |
| return checkTypeRelatedTo(source, target, assignableRelation, errorNode, headMessage, containingMessageChain); | ||
| } | ||
|
|
||
| /** | ||
| * This is *not* a bi-directional relationship. | ||
| * If one needs to check both directions for comparability, use a second call to this function or 'isTypeComparableTo'. | ||
| */ | ||
| function checkTypeComparableTo(source: Type, target: Type, errorNode: Node, headMessage?: DiagnosticMessage, containingMessageChain?: DiagnosticMessageChain): boolean { | ||
|
There was a problem hiding this comment. nit: This function is hard to read on a portrait (1200 px) monitor with github's generous margins cutting it down to about 700 px. Might not be worth fixing. |
||
| return checkTypeRelatedTo(source, target, comparableRelation, errorNode, headMessage, containingMessageChain); | ||
| } | ||
|
|
||
| function isSignatureAssignableTo(source: Signature, target: Signature): boolean { | ||
| let sourceType = getOrCreateTypeFromSignature(source); | ||
| let targetType = getOrCreateTypeFromSignature(target); | ||
|
|
@@ -4756,7 +4773,7 @@ namespace ts { | |
| * Checks if 'source' is related to 'target' (e.g.: is a assignable to). | ||
| * @param source The left-hand-side of the relation. | ||
| * @param target The right-hand-side of the relation. | ||
| * @param relation The relation considered. One of 'identityRelation', 'assignableRelation', 'subTypeRelation', or 'comparableRelation'. | ||
| * Used as both to determine which checks are performed and as a cache of previously computed results. | ||
| * @param errorNode The suggested node upon which all errors will be reported, if defined. This may or may not be the actual node used. | ||
| * @param headMessage If the error chain should be prepended by a head message, then headMessage will be used. | ||
|
|
@@ -4781,6 +4798,7 @@ namespace ts { | |
|
|
||
| Debug.assert(relation !== identityRelation || !errorNode, "no error reporting in identity checking"); | ||
|
|
||
| const isAssignableOrComparableRelation = relation === assignableRelation || relation === comparableRelation; | ||
| let result = isRelatedTo(source, target, errorNode !== undefined, headMessage); | ||
| if (overflow) { | ||
| error(errorNode, Diagnostics.Excessive_stack_depth_comparing_types_0_and_1, typeToString(source), typeToString(target)); | ||
|
|
@@ -4814,7 +4832,14 @@ namespace ts { | |
| sourceType = typeToString(source, /*enclosingDeclaration*/ undefined, TypeFormatFlags.UseFullyQualifiedType); | ||
| targetType = typeToString(target, /*enclosingDeclaration*/ undefined, TypeFormatFlags.UseFullyQualifiedType); | ||
| } | ||
|
|
||
| if (!message) { | ||
| message = relation === comparableRelation ? | ||
| Diagnostics.Type_0_is_not_comparable_to_type_1 : | ||
| Diagnostics.Type_0_is_not_assignable_to_type_1; | ||
| } | ||
|
|
||
| reportError(message, sourceType, targetType); | ||
| } | ||
|
|
||
| // Compare two types and return | ||
|
|
@@ -4834,7 +4859,7 @@ namespace ts { | |
| if (source === nullType && target !== undefinedType) return Ternary.True; | ||
| if (source.flags & TypeFlags.Enum && target === numberType) return Ternary.True; | ||
| if (source.flags & TypeFlags.StringLiteral && target === stringType) return Ternary.True; | ||
| if (isAssignableOrComparableRelation) { | ||
| if (isTypeAny(source)) return Ternary.True; | ||
| if (source === numberType && target.flags & TypeFlags.Enum) return Ternary.True; | ||
| } | ||
|
|
@@ -4857,29 +4882,39 @@ namespace ts { | |
|
|
||
| let saveErrorInfo = errorInfo; | ||
|
|
||
| // Note that these checks are specifically ordered to produce correct results. | ||
| if (source.flags & TypeFlags.Union) { | ||
| if (relation === comparableRelation) { | ||
| result = someTypeRelatedToType(source as UnionType, target, reportErrors); | ||
|
There was a problem hiding this comment. Please choose one cast syntax and make at least the changed lines use it, preferably something like all the casts in the function. |
||
| } | ||
| else { | ||
| result = eachTypeRelatedToType(source as UnionType, target, reportErrors); | ||
| } | ||
|
|
||
| if (result) { | ||
| return result; | ||
| } | ||
| } | ||
| else if (target.flags & TypeFlags.Intersection) { | ||
| result = typeRelatedToEachType(source, target as IntersectionType, reportErrors); | ||
|
|
||
| if (result) { | ||
| return result; | ||
| } | ||
| } | ||
| else { | ||
| // It is necessary to try these "some" checks on both sides because there may be nested "each" checks | ||
| // on either side that need to be prioritized. For example, A | B = (A | B) & (C | D) or | ||
| // A & B = (A & B) | (C & D). | ||
| if (source.flags & TypeFlags.Intersection) { | ||
| // If target is a union type then the check following this one will report errors, | ||
|
There was a problem hiding this comment. I don't think this improves on the old wording. There was a problem hiding this comment. It does if you have more context. "The following check" implies "the one directly below" |
||
| // so we'll suppress any errors we could run into here. | ||
| if (result = someTypeRelatedToType(source as IntersectionType, target, reportErrors && !(target.flags & TypeFlags.Union))) { | ||
| return result; | ||
| } | ||
| } | ||
| if (target.flags & TypeFlags.Union) { | ||
| if (result = typeRelatedToSomeType(source, target as UnionType, reportErrors)) { | ||
| return result; | ||
| } | ||
| } | ||
|
|
@@ -4942,8 +4977,8 @@ namespace ts { | |
| } | ||
| if (source.flags & TypeFlags.Union && target.flags & TypeFlags.Union || | ||
| source.flags & TypeFlags.Intersection && target.flags & TypeFlags.Intersection) { | ||
| if (result = eachTypeRelatedToSomeType(<UnionOrIntersectionType>source, <UnionOrIntersectionType>target, /*reportErrors*/ false)) { | ||
| if (result &= eachTypeRelatedToSomeType(<UnionOrIntersectionType>target, <UnionOrIntersectionType>source, /*reportErrors*/ false)) { | ||
| return result; | ||
| } | ||
| } | ||
|
|
@@ -4958,7 +4993,7 @@ namespace ts { | |
| function isKnownProperty(type: Type, name: string): boolean { | ||
| if (type.flags & TypeFlags.ObjectType) { | ||
| const resolved = resolveStructuredTypeMembers(type); | ||
| if (isAssignableOrComparableRelation && (type === globalObjectType || resolved.properties.length === 0) || | ||
| resolved.stringIndexType || resolved.numberIndexType || getPropertyOfType(type, name)) { | ||
| return true; | ||
| } | ||
|
|
@@ -4992,11 +5027,11 @@ namespace ts { | |
| return false; | ||
| } | ||
|
|
||
| function eachTypeRelatedToSomeType(source: UnionOrIntersectionType, target: UnionOrIntersectionType, reportErrors: boolean): Ternary { | ||
| let result = Ternary.True; | ||
| let sourceTypes = source.types; | ||
| for (let sourceType of sourceTypes) { | ||
| let related = typeRelatedToSomeType(sourceType, target, reportErrors); | ||
| if (!related) { | ||
| return Ternary.False; | ||
| } | ||
|
|
@@ -9381,8 +9416,9 @@ namespace ts { | |
| let targetType = getTypeFromTypeNode(node.type); | ||
| if (produceDiagnostics && targetType !== unknownType) { | ||
| let widenedType = getWidenedType(exprType); | ||
|
|
||
| if (!isTypeComparableTo(targetType, widenedType)) { | ||
| checkTypeComparableTo(exprType, targetType, node); | ||
| } | ||
| } | ||
| return targetType; | ||
|
|
@@ -10225,7 +10261,7 @@ namespace ts { | |
| case SyntaxKind.ExclamationEqualsToken: | ||
| case SyntaxKind.EqualsEqualsEqualsToken: | ||
| case SyntaxKind.ExclamationEqualsEqualsToken: | ||
| if (!isTypeComparableTo(leftType, rightType) && !isTypeComparableTo(rightType, leftType)) { | ||
| reportOperatorError(); | ||
| } | ||
| return booleanType; | ||
|
|
@@ -12689,12 +12725,13 @@ namespace ts { | |
|
|
||
| if (produceDiagnostics && clause.kind === SyntaxKind.CaseClause) { | ||
| let caseClause = <CaseClause>clause; | ||
| // TypeScript 1.0 spec (April 2014): 5.9 | ||
| // In a 'switch' statement, each 'case' expression must be of a type that is assignable to or from the type of the 'switch' expression. | ||
|
There was a problem hiding this comment. update 'assignable' to 'comparable' |
||
| // TODO (drosen): this needs to be amended to reflect the "comparable" relationship. | ||
| let caseType = checkExpression(caseClause.expression); | ||
|
|
||
| if (!isTypeComparableTo(expressionType, caseType)) { | ||
| checkTypeComparableTo(caseType, expressionType, caseClause.expression, /*headMessage*/ undefined); | ||
| } | ||
| } | ||
| forEach(clause.statements, checkSourceElement); | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,8 @@ | ||
| tests/cases/conformance/expressions/asOperator/asOperator2.ts(1,9): error TS2323: Type 'number' is not comparable to type 'string'. | ||
|
|
||
|
|
||
| ==== tests/cases/conformance/expressions/asOperator/asOperator2.ts (1 errors) ==== | ||
| var x = 23 as string; | ||
| ~~~~~~~~~~~~ | ||
| !!! error TS2323: Type 'number' is not comparable to type 'string'. | ||
|
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,10 +1,10 @@ | ||
| tests/cases/conformance/expressions/asOperator/asOperatorContextualType.ts(2,9): error TS2323: Type '(v: number) => number' is not comparable to type '(x: number) => string'. | ||
| Type 'number' is not comparable to type 'string'. | ||
|
|
||
|
|
||
| ==== tests/cases/conformance/expressions/asOperator/asOperatorContextualType.ts (1 errors) ==== | ||
| // should error | ||
| var x = (v => v) as (x: number) => string; | ||
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
| !!! error TS2323: Type '(v: number) => number' is not comparable to type '(x: number) => string'. | ||
| !!! error TS2323: Type 'number' is not comparable to type 'string'. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,11 @@ | ||
| tests/cases/conformance/expressions/asOperator/asOperatorNames.ts(2,9): error TS2323: Type 'number' is not comparable to type 'string'. | ||
|
|
||
|
|
||
| ==== tests/cases/conformance/expressions/asOperator/asOperatorNames.ts (1 errors) ==== | ||
| var a = 20; | ||
| var b = a as string; | ||
| ~~~~~~~~~~~ | ||
| !!! error TS2323: Type 'number' is not comparable to type 'string'. | ||
| var as = "hello"; | ||
| var as1 = as as string; | ||
|
|
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this true of
isTypeAssignableTotoo? Its former usage in this diff indicates 'yes'.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is true of
isTypeAssignableTobutisTypeAssignableTohas a clear direction; on the other hand,isTypeComparableTois vague enough that it warrants a comment. One would think thatAbeing comparable toBimplies thatBis comarable toA, but that's not quite the case.I could have made
isTypeComparableToa bidirectional relation, but I decided not to based on the way in which we use the relation in type assertions. Specifically, with type assertions, we check whether the expression type is comparable to the asserted type and that the asserted type is comparable to the widened type of the expression.Actually, I'm not sure if there is any reason we don't check against the widened type for both. I'll experiment with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed offline, I think that's a bug. I will change that in a later PR