-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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.
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
Use type parameter constraints for computed property types #17404
Use type parameter constraints for computed property types #17404
Conversation
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.
@sandersn Is there a test for T extends number
as well? It's probably the only thing that'd be nice to have if not present, since I think that would produce one fewer errors now? (like the changed U extends string
test)
Aside from that remark, this looks like a relatively neat change. There might be a more general way to solve this and some of our other suggestions about things like intersection types and computed property names, but I think this is probably fine for this issue, and can be expanded upon later if need be.
Instead of this targeted change, it might make more sense to review all the uses of |
This improves a number of error messages to do with adding 'null' or 'undefined' with other types. It also allows you to add two type parameters that both extend `number`.
I replaced the body of |
Note that this most recent commit improves the error message for expressions like function f<T extends number>(t: T) {
return t + t
} Here, the return type of |
src/compiler/checker.ts
Outdated
return true; | ||
} | ||
} | ||
const targets = []; |
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.
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) ||
...
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.
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.
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.
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.
src/compiler/checker.ts
Outdated
// of the given kind. | ||
function isTypeOfKind(type: Type, kind: TypeFlags): boolean { | ||
if (type.flags & kind) { | ||
function isTypeOfKind(source: Type, kind: TypeFlags, excludeAny?: boolean) { |
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.
I'm not sure I get why we need this excludeAny
parameter?
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.
checkBinaryLikeExpression
has a complicated way of checking legal types for +
:
- It disallows
null + number
andnumber + undefined
, even withstrictNullChecks
off. I like this check and want to keep it. - It gives a different types for
string + any ==> string
vsT + any ==> any
andT + 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.
Instead of building up a list and creating a union type.
@simonbuchan Your example #16687 has the computed property name error as a side effect, but this fix doesn't provide the type that you're looking for there. |
This PR is ready to look at again. |
src/compiler/checker.ts
Outdated
} | ||
return false; | ||
return kind & TypeFlags.NumberLike && isTypeAssignableTo(source, numberType) || |
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.
Calling isTypeAssignableTo
so many times seems fairly expensive if source
is not a simple type. While the assignability check has a fast path for simple types via isSimpleTypeRelatedTo
, if the relation doesn't hold we fall back to the more complex checkTypeRelatedTo
function. I would recommend you determine if there is any additional check time overhead with this approach.
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.
Also note that calls to isTypeAssignableToKind
only ever pass 1-3 flags in, with 1 being the most common. So the expected perf hit would be just from calling isTypeAssignableTo
once compared to the old logic.
Sure, I'll test it, although @ahejlsberg was confident that his changes to |
Here are the ts-perf results (angular doesn't build on my installation of ts-perf). Total time goes down a tiny amount. Check time went up by 0.01s for TFS, and down 0.01s for Monaco. So I don't think there's any performance impact with this change.
|
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.
One minor comment, though I would probably wait to see if @ahejlsberg has any other further comments before merging.
src/compiler/checker.ts
Outdated
@@ -17073,7 +17061,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 (!(leftType.flags & TypeFlags.Any) && isTypeAssignableToKind(leftType, TypeFlags.Primitive)) { |
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.
We have an isTypeAny
that is a bit more readable than the bitmask, e.g. !isTypeAny(leftType) && isTypeAssignableToKind(leftType, TypeFlags.Primitive)
I'd like to get this into 2.5 RC so people can catch any lurking bugs. I'm on vacation starting Thursday, so let's leave this open until Friday to see if @ahejlsberg has a chance to look at this while he's gone too. I'll leave it up to you to merge or not at that point. @rbuckton. I'll also try running ts-perf on a stress-test scenario that has lots of object type + string expressions. |
I can see how This may be acceptable overhead for cases where we are likely to fail, but something we should consider. It may be worthwhile for us to compose a performance benchmark against a scenario we tailor to be filled with incorrect code so that we can watch for performance regressions in those cases. |
After reviewing this further with @sandersn, I'm not too concerned about the performance since we don't dig into |
in isTypeAssignableToKind
Fixes #14473
Fixes #15501
Fixes #17069
Note this code calls
getBaseConstraintOfType
the same way thatgetApparentType
does. CallinggetApparentType
does not work here because it convertsstring
toString
and so on.