-
Notifications
You must be signed in to change notification settings - Fork 12.3k
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 anyArray.filter(Boolean) return any[], not unknown[] #31515
Conversation
Allows anys.filter(Boolean) to once again return any[], not unknown[].
I'm also going to keep looking at the original failure to see whether there's a bug in assignability that causes. |
@typescript-bot run dt |
@typescript-bot run rwc |
@typescript-bot test this |
declare var Bullean: BulleanConstructor; | ||
declare let anys: Ari<any>; | ||
var xs: Ari<any>; | ||
var xs = anys.filter(Bullean) |
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 should fail since I didn't fix the small repro example.
~~ | ||
!!! error TS2403: Subsequent variable declarations must have the same type. Variable 'xs' must be of type 'Ari<any>', but here has type 'Ari<unknown>'. | ||
|
||
declare let realanys: any[]; |
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 should pass, and does, after the workaround.
RWC, DT and user tests are all clean. |
Hold the phone. I changed ReadonlyArray.filter by mistake, not Array.filter. Both need to be changed. I'll re-run tests and re-report how much breaks. |
I want to test how well this works.
Well, that didn't work. I'm changing the Boolean factory function for now. The user tests show no changes except fixes for the errors introduced by the original PR. |
@typescript-bot test this |
@typescript-bot run dt |
@typescript-bot user test this |
src/lib/es5.d.ts
Outdated
@@ -513,7 +513,7 @@ interface Boolean { | |||
|
|||
interface BooleanConstructor { | |||
new(value?: any): Boolean; | |||
<T>(value?: T): value is Exclude<T, false | null | undefined | '' | 0>; | |||
<T extends any>(value?: T): value is Exclude<T, false | null | undefined | '' | 0>; |
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.
#29571 will 100% break this (and we only held off merging it because we got spooked with the number of breaks already in 3.5 IIRC), since this is a terrible hack that makes T
"look like any" even though it's a type parameter and extends any
should be identical to extends unknown
or simply no constraint.
I'm proposing we just revert #29955. The cure here seems worse than the disease we were trying to address |
@RyanCavanaugh I agree. I didn't even notice that the Boolean factory was not a type guard until 3 weeks ago. I switched it back to boolean and wrote up our options in the description. |
@typescript-bot run dt User tests look good on my local machine; the |
@@ -513,7 +513,7 @@ interface Boolean { | |||
|
|||
interface BooleanConstructor { | |||
new(value?: any): Boolean; | |||
<T extends any>(value?: T): value is Exclude<T, false | null | undefined | '' | 0>; | |||
<T>(value?: T): boolean; |
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.
but what about
(someArray as SomeType[]).map(some => {
if (!some.test) return; // after this line the result will be (SomeType || undefined)[]
return some;
}).filter(Boolean); // here undefined was filtered with Exclude in definitions
it breaks #29955 :{
Allows anys.filter(Boolean) to once again return any[], not unknown[].
Fixes #31189
Doesn't break any of our test suite, but I'm running it on user tests and I need to request a DT and RWC run.
Edit: I've found three solutions:
T
toT extends any
. This will break if Reinterpret a type parameter constrained to any as an upper bound constraint #29571 goes in [1].string | undefined
union by callingBoolean
(but you can define your own type guard of course).filter
toI like (2) the best since you can always write your own type guard and I have never seen
if (Boolean(x))
in JS or TS before, and we haven't shipped it so nobody relies on it yet. (3) is a bad example in a 🦑 👽 🦀 "why not overloads + conditional types" way.I'll switch to (2) and make sure the test results look good.
[1] I'm not sure that #29571 is a good change, but it is a safer change. My intent with
T extends any
is basically "disable type checking for this type parameter", but I think it's also common to use it to meanT extends unknown
.