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.

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

Merged
merged 3 commits into from
Dec 10, 2014
Merged

Conversation

ahejlsberg
Copy link
Member

This PR changes type guards to not affect values of type any.

Fixes #1426.

@DanielRosenwasser
Copy link
Member

This needs to get merged into release-1.4, or at least cherry-picked in.

@@ -1240,7 +1240,7 @@ module ts {
StringLike = String | StringLiteral,
NumberLike = Number | Enum,
ObjectType = Class | Interface | Reference | Tuple | Anonymous,
Structured = Any | ObjectType | Union | TypeParameter
Copy link
Contributor

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).

Copy link
Member Author

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.

Copy link
Contributor

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

Copy link
Member Author

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).

Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, that is helpful

@mhegazy
Copy link
Contributor

mhegazy commented Dec 10, 2014

👍

as @DanielRosenwasser noted, this needs to go into branch release-1.4

ahejlsberg added a commit that referenced this pull request Dec 10, 2014
Type guards should not affect values of type any
@ahejlsberg ahejlsberg merged commit ab4706a into master Dec 10, 2014
@ahejlsberg
Copy link
Member Author

Cherry picked and applied changes to release-1.4 branch as well.

@ahejlsberg ahejlsberg deleted the typeGuardWithAny branch December 10, 2014 22:52
@sophiajt
Copy link
Contributor

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?

@DanielRosenwasser
Copy link
Member

Looks like we should really revisit this to get a sense of what we could still do with any.

@microsoft microsoft locked and limited conversation to collaborators Jun 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Instanceof in type guards should not narrow any
6 participants