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

Make T[never] = never instead of erroring or being any #22787

Merged
merged 4 commits into from
Mar 27, 2018

Conversation

kpdonn
Copy link
Contributor

@kpdonn kpdonn commented Mar 22, 2018

This change makes it so that when indexing a type with never(e.g. T[never]), the result is never. Previously it was an error to explicitly index a type with never and it would non-intuitively become any if it was inferred somewhere inside generics.

There is more discussion about the reasoning for the change and the implications of it from @sandersn and @yortus in #22042.

Fixes #22042

@yortus
Copy link
Contributor

yortus commented Mar 25, 2018

@kpdonn maybe clarify that this change only affects types with no index signature. So for example:

  • {[x: string]: boolean}[never] is boolean (unchanged behaviour)
  • {foo: boolean}[never] is never (new behaviour, previously was an error)

With that qualification, this is a non-breaking change because it only affects cases that are currently errors.

@mhegazy
Copy link
Contributor

mhegazy commented Mar 26, 2018

@ahejlsberg and @sandersn any comments?

@sandersn
Copy link
Member

After discussing it in person with @ahejlsberg, we didn't like the fact that ({ [n: number]: boolean})[never] is inconsistent: it's not never like everything else, just to make sure that lodash doesn't break. To fix this inconsistency, we are thinking about (finally) introducing the numeric-string type, which would what keyof { [n: number]: boolean } would be:

type Ni = { [n: number]: boolean }
type Kni = keyof Ni // numeric-string
type Nikni = Ni[Kni] // boolean

lodash would no longer break because T[keyof T] would never end up indexing a numeric-index-only type with never, but with numeric-string. Then this PR could change so that never-indexing applies to all types, not just those with properties.

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

We should wait on this until we have decided what to do about the numeric-string type.

@ahejlsberg
Copy link
Member

I'm actually okay with taking this PR since it moves us closer to where we eventually want to be, i.e. that T[never] is always never regardless of T. Longer term, if and when we fix keyof { [x: number]: T } to produce numeric-string (or whatever we call it) instead of never, we can then also change { [x: number]: T }[never] to be never.

That said, we need a small change to the PR. I will review below.

@@ -8131,6 +8131,9 @@ namespace ts {
return anyType;
}
}
if (indexType.flags & TypeFlags.Never) {
return neverType;
}
Copy link
Member

Choose a reason for hiding this comment

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

Move these three lines up and insert then before line 8122. That way an obj[key] expression where key is an expression of type never will produce never (instead of the any it produces now).

@kpdonn
Copy link
Contributor Author

kpdonn commented Mar 27, 2018

@ahejlsberg Updated with the requested change and also a test for the obj[key] case where key is never.

Not sure why the CI builds failed though, I ran gulp runtests locally before committing and did not have failures.

Edit: Nevermind, looks like rebasing off of latest master resolved the CI build failures.

@sandersn sandersn merged commit 4fa9605 into microsoft:master Mar 27, 2018
@sandersn
Copy link
Member

All right, it’s in! Thanks @kpdonn!

@kpdonn kpdonn deleted the indexingNever branch March 27, 2018 15:05
@microsoft microsoft locked and limited conversation to collaborators Jul 25, 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.

5 participants