-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Object.keys() types refinement, and Object.entries() types bugfix #12253
Closed
+82
−25
Closed
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
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
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
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
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.
key of T & string
is juststring
. so this does not help you much.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.
@mhegazy Edit: sorry, I misread your comment.
key of T & string
is often("key1" & string) | ("key2" & string) | ... | ("keyn" & string)
, not juststring
.The intersection is needed because sometimes
keyof T
isstring | number
, and we want to exclude number as a possible type for the entry keys. See this test case: https://github.com/Microsoft/TypeScript/pull/12253/files#diff-3ed8d911aae864ffc1d88e62bcb8dc47R15There 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.
These intersections do make the intellisense popups uglier, but they're required for correctness.
Could the solution to that ugliness be for the compiler to do some work simplifying intersections (and unions)? E.g.,
'a' & string
could just be simplified to to'a'
(or, more generally,type & subType
could just be simplified tosubType
). Any efforts in that direction could also help with the bug I brought up in #11426. Would this be that hard? I'd kind of expect simplifying logical formulations to be a somewhat solved problem with theorem provers etc, though I haven't looked into it, so maybe that's incredibly naive. Or maybe it's way too slow to apply here in the general case. Still, specific simplification rules might be a good idea.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 would rather leave it
string[]
then. adding the& string
removes any type safety the literal types give you.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 don't really understand that. Why would you lose type safety? The
& string
version still only allows certain strings, rather than all strings, which is more safe.Let me give the real code example that motivated these two PRs:
To make that code work,
string[]
isn't enough fortypeof name
. The change merged in #12207 meanwhile, works perfectly, but fails in the somewhat obscure case that motivated the& string
addition in this PR. Using& string
, the type ofname
is inferred as("info" & string) | ("warn" & string) | ("error" & string)
. That should, if I understand correctly, be equivalent to"info" | "warn" | "error"
. However, I instead get an error. That seems like a separate bug, though (opened issue #12289 for it), also tied to simplification.Going forward... if #12289 were addressed, I think the
& string
would be the right type signature forObject.keys/entries
. But, if #12289 can't be easily addressed, maybe it's better to ignore case that& string
was meant to solve, rather than giving up on having key-named literals entirely. If we did that, the signature would be:Then,
Object.keys(loggers)
would correctly be("info" | "warn" | "error")[]
, butlet y: { [x: string]: any } = {}; Object.keys(y)
would be(string | number)[]
rather thanstring[]
. Frankly, that sounds like a net win.