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

Fix empty object falsiness #27157

Merged
merged 6 commits into from
Sep 18, 2018
Merged

Fix empty object falsiness #27157

merged 6 commits into from
Sep 18, 2018

Conversation

ahejlsberg
Copy link
Member

@ahejlsberg ahejlsberg commented Sep 17, 2018

We currently consider {} and unconstrained type parameters to never be falsy in --strictNullChecks mode. This is wrong as {} or an unconstrained type parameter might represent a string, number, or boolean value and thus one of the falsy values "" | 0 | false. This PR corrects our assumptions.

This PR furthermore fixes an issue with type parameters of the form T extends { [K in keyof T]: string }. For such a type parameter, keyof T will have itself as its constraint and this circularity caused every type to be considered related to keyof T.

Fixes #18196.
Fixes #27181.

@@ -14169,9 +14175,11 @@ namespace ts {
(type === falseType || type === regularFalseType) ? TypeFacts.FalseFacts : TypeFacts.TrueFacts;
}
if (flags & TypeFlags.Object) {
return isFunctionObjectType(<ObjectType>type) ?
Copy link
Member

@weswigham weswigham Sep 17, 2018

Choose a reason for hiding this comment

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

The empty object type is a red herring. The issue also exists for other structural types which have falsy types structurally assignable to them, like { toString(): string; }, { toFixed(): string; }, or { readonly length: number }.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, in an ideal world we'd check whether "", 0, or false is assignable to the type in question, but I'm concerned about the performance impact.

Copy link
Member

@weswigham weswigham Sep 17, 2018

Choose a reason for hiding this comment

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

I think it shouldn't be too bad, given that we'll be doing the check only once for all string-subtypes, number-subtypes, etc (since for structural comparison we'll be comparing the apparent type) vs each input type, then caching the relation comparison result for every other time we check it. And the check itself should (in the common case of failure) bail out really early when the property keys don't sufficiently overlap.

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Sep 17, 2018

So what's the impact here? We discussed offline and felt that this might be too painful for a lot of practical uses.

@weswigham
Copy link
Member

@DanielRosenwasser you just ask @typescript-bot test this to find out

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 17, 2018

Heya @weswigham, I've started to run the extended test suite on this PR at 17080eb. You can monitor the build here. It should now contribute to this PR's status checks.

@ahejlsberg
Copy link
Member Author

@DanielRosenwasser I just ran the RWC tests locally. No breaks as a result of this.

@ahejlsberg
Copy link
Member Author

The RWC baseline change shown by the bot is actually a desired change, so no breaks.

@RyanCavanaugh RyanCavanaugh added this to the TypeScript 3.1 milestone Sep 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Switch on typeof for keyof type loses information Type narrowing bug in empty interfaces
5 participants