-
-
Notifications
You must be signed in to change notification settings - Fork 661
ArraySlice / StringSlice: Fix behaviour with unions
#1291
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
Conversation
a7df7d4 to
376e418
Compare
376e418 to
8602b27
Compare
8602b27 to
223759d
Compare
| ? IsNever<Start> extends true | ||
| ? IsNever<End> extends true | ||
| ? _ArraySlice<Array_, Start, End> | ||
| : End extends unknown // To distribute `End` | ||
| ? _ArraySlice<Array_, Start, End> | ||
| : never // Never happens | ||
| : IsNever<End> extends true | ||
| ? Start extends unknown // To distribute `Start` | ||
| ? _ArraySlice<Array_, Start, End> | ||
| : never // Never happens | ||
| : Start extends unknown // To distribute `Start` | ||
| ? End extends unknown // To distribute `End` | ||
| ? _ArraySlice<Array_, Start, End> | ||
| : never // Never happens | ||
| : never // Never happens |
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 complicated/verbose because the default value of Start/End is never, so we have to explicitly check for never before distributing, which introduces several branching cases.
I guess the reason for having the default value as never was that if End were simply Array_["length"], then union instantiations of End would give incorrect results.
For example,
ArraySlice<[0, 1, 2] | [0, 1], 0> would become
ArraySlice<[0, 1, 2] | [0, 1], 0, 3 | 2>,
which would then distribute to
ArraySlice<[0, 1, 2], 3 | 2> | ArraySlice<[0, 1], 3 | 2>,
and finally to
ArraySlice<[0, 1, 2], 3> | ArraySlice<[0, 1, 2], 2> | ArraySlice<[0, 1], 3> | ArraySlice<[0, 1], 2>,
which would be incorrect.
However, if we set the default for End to UnionMax<Array_["length"]>, that would fix this issue. The only thing is that explicitly passing never would no longer behave the same as omitting the argument. But that’s probably acceptable because never shouldn’t really be a valid value for Start/End anyway. In such cases, we can just return something like readonly unknown[] or [].
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.
However, if we set the default for
EndtoUnionMax<Array_["length"]>, that would fix this issue. The only thing is that explicitly passingneverwould no longer behave the same as omitting the argument. But that’s probably acceptable becausenevershouldn’t really be a valid value forStart/Endanyway. In such cases, we can just return something likereadonly unknown[]or[].
@sindresorhus If this is fine, then I'll open a separate PR for this.
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.
Yes, seems fine.
Fixes behaviour of
ArraySlicewith unions.This also fixes behaviour of
StringSlice(Fixes #1289) with unions as it relies onArraySlice, just needed the default type parameters to be fixed.