-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Type guards should not affect values of type any #1433
Conversation
This needs to get merged into |
@@ -1240,7 +1240,7 @@ module ts { | |||
StringLike = String | StringLiteral, | |||
NumberLike = Number | Enum, | |||
ObjectType = Class | Interface | Reference | Tuple | Anonymous, | |||
Structured = Any | ObjectType | Union | TypeParameter |
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.
Add comment why 'any' is considered to be a 'structured' type. (for that matter, comment what we mean by a 'structured type' and thus why a TypeParameter is also considered one).
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.
This change removes Any from StructuredType, leaving just ObjectType, Union, and TypeParameter.
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 doesn't getNarrowedTypeOfSymbol call isStructuredType? Instead it checks the flag directly
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.
The TypeFlags.Structured flag is set for a union type regardless of the state of the flag for each of the constituent types. The isStructuredType performs the deep check. In the case of narrowing we only care about the shallow state (e.g. you can narrow a union of primitive types).
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.
This is somethign that would compltely break my expectations from reading the code. can you rename the isStructuredType flag to 'isStructuredTypeAtAnyDepth()' or something that can at least indicate that it's doing something extra.
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've gotten rid of the confusing TypeFlags.Structured flag and just inlined the actual flags in the few cases it was used. I think isStructuredType is a fine name for what it does and the comment on the function explains it further.
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.
Thanks, that is helpful
👍 as @DanielRosenwasser noted, this needs to go into branch release-1.4 |
Type guards should not affect values of type any
Cherry picked and applied changes to release-1.4 branch as well. |
Does it make sense to remove narrowing 'any' in all cases? I can see why things like instanceof are a bit iffy, but for examples like this: function f(x) {
if (typeof x === "string") {
return x.lengthh;
}
} It feels like we have enough information to know we should give the user an error. Is that not true here as well? |
Looks like we should really revisit this to get a sense of what we could still do with |
This PR changes type guards to not affect values of type any.
Fixes #1426.