PR-50377-redo-filterBooleanOverload #56281
Conversation
typescript-bot
added
the
For Uncommitted Bug
craigphicks
force-pushed
the
PR-50377-redo-filterBooleanOverload
branch
2 times, most recently
from
4fa02a2 to
111f0c6
Compare
craigphicks
force-pushed
the
PR-50377-redo-filterBooleanOverload
branch
from
c8a9cb1 to
f49ff0c
Compare
src/lib/es5.d.ts
Outdated
| * @param predicate Must be exactly "Boolean" | ||
| * @param thisArg An object to which the this keyword can refer in the predicate function. If thisArg is omitted, undefined is used as the this value. | ||
| */ | ||
| filter(predicate: BooleanConverter, thisArg?: any): (T extends false | 0 | "" | null | undefined | 0n ? never : T)[]; |
There was a problem hiding this comment.
Perhaps Falsey (Falsy?) could be a utility function.
| @@ -4,7 +4,7 @@ | |||
| // Repro from #10041 | |||
|
|
|||
| (''.match(/ /) || []).map(s => s.toLowerCase()); | |||
| >(''.match(/ /) || []).map(s => s.toLowerCase()) : any[] | |||
| >(''.match(/ /) || []).map(s => s.toLowerCase()) : string[] | |||
There was a problem hiding this comment.
Improvement (at least from user perspective it is an improvement).
typescript-bot
added
For Backlog Bug
craigphicks
force-pushed
the
PR-50377-redo-filterBooleanOverload
branch
from
29b4592 to
28da703
Compare
|
@typescript-bot test top200 |
|
Heya @jakebailey, I've started to run the regular perf test suite on this PR at 28da703. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at 28da703. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the tarball bundle task on this PR at 28da703. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at 28da703. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at 28da703. You can monitor the build here. Update: The results are in! |
|
Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
|
@jakebailey Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Something interesting changed - please have a look. Details
|
|
@jakebailey Here they are:
CompilerComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
StartupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Hey @jakebailey, the results of running the DT tests are ready. Branch only errors:Package: microsoft-live-connect Package: winjs |
That doesn't look so good. @jakebailey - do you agree? |
|
I doubt that's anything but noise. |
|
@jakebailey Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Details
|
|
Re: top-repos suite
Almost certainly due to the transformation of union-of-arrays to array-of-union-of-types in the carve-out in |
craigphicks
force-pushed
the
PR-50377-redo-filterBooleanOverload
branch
3 times, most recently
from
2a92bc7 to
0a13549
Compare
|
|
||
|
|
||
| ([] as (Fizz|Falsey)[] | (Buzz|Falsey)[]).filter(Boolean); // expect type (Fizz|Buzz)[] | ||
| >([] as (Fizz|Falsey)[] | (Buzz|Falsey)[]).filter(Boolean) : Fizz[] | Buzz[] |
There was a problem hiding this comment.
Now returns union of arrays Fizz[] | Buzz[] rather than (Fizz|Buzz)[]. (The comment "expect type (Fizz|Buzz)[]" is out of date).
|
|
||
| const arr2 = arr.filter(Boolean); // expect ("foo" | 1)[] | ||
| >arr2 : (1 | "foo")[] | ||
| >arr.filter(Boolean) : (1 | "foo")[] |
There was a problem hiding this comment.
This is a case that was brought up by #50377 (comment) in the original #50377.
craigphicks
force-pushed
the
PR-50377-redo-filterBooleanOverload
branch
from
64d8ace to
e904d77
Compare
src/compiler/checker.ts
Outdated
| return getUnionType(art, UnionReduction.Subtype); | ||
| }); | ||
|
|
||
| // Transform the type from `(A[] | B[])["member"]` to `(A | B)[]["member"]` (since we pretend array is covariant anyway) |
There was a problem hiding this comment.
The original carveout code description - "// Transform the type from (A[] | B[])["member"] to (A | B)[]["member"] "
In this pull that action is kept for the signature parameters, but the return type is reverted the union of return types, where appropriate.
src/lib/es5.d.ts
Outdated
| * @param predicate Must be exactly "Boolean" | ||
| * @param thisArg An object to which the this keyword can refer in the predicate function. If thisArg is omitted, undefined is used as the this value. | ||
| */ | ||
| filter(predicate: BooleanConstructor, thisArg?: any): (T extends false | 0 | "" | null | undefined | 0n ? never : T)[]; |
There was a problem hiding this comment.
New overload for ReadonlyArray
src/lib/es5.d.ts
Outdated
| * @param predicate Must be exactly "Boolean" | ||
| * @param thisArg An object to which the this keyword can refer in the predicate function. If thisArg is omitted, undefined is used as the this value. | ||
| */ | ||
| filter(predicate: BooleanConstructor, thisArg?: any): (T extends false | 0 | "" | null | undefined | 0n ? never : T)[]; |
| >['foo', 'bar'] : string[] | ||
| >'foo' : "foo" | ||
| >'bar' : "bar" | ||
| >filter : { <S extends string>(predicate: (value: string, index: number, array: string[]) => value is S, thisArg?: any): S[]; (predicate: (value: string, index: number, array: string[]) => unknown, thisArg?: any): string[]; } | ||
| >filter : { <S extends string>(predicate: (value: string, index: number, array: string[]) => value is S, thisArg?: any): S[]; (predicate: BooleanConstructor, thisArg?: any): string[]; (predicate: (value: string, index: number, array: string[]) => unknown, thisArg?: any): string[]; } | ||
| >id() : (t: string) => boolean |
There was a problem hiding this comment.
In #56013 (bug), id() was not evaluated when an external filter(Boolean) overload was added to the Array interface in usercode. Now it works (with the overload in TypeScript code).
| asn.map(pisect2); | ||
| ~~~~~~~ |
There was a problem hiding this comment.
Regression:
Array<string|number>.map(((value: string, index: number, array: string[]) => unknown) & ((value: number, index: number, array: number[]) => unknown)) is an error.
| asn.map(poload2); | ||
| ~~~~~~~ | ||
| !!! error TS2345: Argument of type 'ArrayPredOverload<unknown, unknown>' is not assignable to parameter of type '(value: string | number, index: number, array: (string | number)[]) => unknown'. |
| // | (method) Array<T>.indexOf(searchElement: never, fromIndex?: number): number | ||
| // | (method) Array<T>.filter<S>(predicate: (value: string | number, index: number, array: (string | number)[]) => value is S, thisArg?: any): S[] (+2 overloads) | ||
| // | (method) Array<T>.forEach(callbackfn: (value: string | number, index: number, array: (string | number)[]) => void, thisArg?: any): void | ||
| // | (method) Array<T>.indexOf(searchElement: string | number, fromIndex?: number): number | ||
| // | (method) Array<T>.join(separator?: string): string |
There was a problem hiding this comment.
Improvement never -> string | number (similar cases below)
| @@ -77,8 +76,6 @@ controlFlowArrayErrors.ts(63,9): error TS7005: Variable 'x' implicitly has an 'a | |||
| } | |||
| x; // boolean[] | (string | number)[] | |||
| x.push(99); // Error | |||
| ~~ | |||
| !!! error TS2345: Argument of type 'number' is not assignable to parameter of type 'never'. | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Possible regression. Notably the types have not change, so the original error was a not a type error.
| filter(callbackfn: (value: T, index: number, array: T[]) => boolean, thisArg?: any): T[]; | ||
| ~~~~~~ | ||
| !!! error TS2416: Property 'filter' in type 'MyArray<T>' is not assignable to the same property in base type 'T[]'. | ||
| !!! error TS2416: Type '(callbackfn: (value: T, index: number, array: T[]) => boolean, thisArg?: any) => T[]' is not assignable to type '{ <S extends T>(predicate: (value: T, index: number, array: T[]) => value is S, thisArg?: any): S[]; (predicate: BooleanConstructor, thisArg?: any): (T extends false | "" | 0 | 0n ? never : T)[]; (predicate: (value: T, index: number, array: T[]) => unknown, thisArg?: any): T[]; }'. |
There was a problem hiding this comment.
Benign error - informing change in definition of filter.
| >([] as (Fizz|Falsey)[] | readonly (Buzz|Falsey)[]).filter(Boolean) : (Fizz | Buzz | Falsey)[] | ||
| >([] as (Fizz|Falsey)[] | readonly (Buzz|Falsey)[]).filter : { <S extends Fizz | Falsey>(predicate: (value: Fizz | Falsey, index: number, array: (Fizz | Falsey)[]) => value is S, thisArg?: any): S[]; (predicate: (value: Fizz | Falsey, index: number, array: (Fizz | Falsey)[]) => unknown, thisArg?: any): (Fizz | Falsey)[]; } | { <S extends Buzz | Falsey>(predicate: (value: Buzz | Falsey, index: number, array: readonly (Buzz | Falsey)[]) => value is S, thisArg?: any): S[]; (predicate: (value: Buzz | Falsey, index: number, array: readonly (Buzz | Falsey)[]) => unknown, thisArg?: any): (Buzz | Falsey)[]; } | ||
| >([] as (Fizz|Falsey)[] | readonly (Buzz|Falsey)[]).filter(Boolean) : Fizz[] | Buzz[] | ||
| >([] as (Fizz|Falsey)[] | readonly (Buzz|Falsey)[]).filter : { <S extends Fizz | Falsey>(predicate: (value: Fizz | Falsey, index: number, array: (Fizz | Falsey)[]) => value is S, thisArg?: any): S[]; (predicate: BooleanConstructor, thisArg?: any): Fizz[]; (predicate: (value: Fizz | Falsey, index: number, array: (Fizz | Falsey)[]) => unknown, thisArg?: any): (Fizz | Falsey)[]; } | { <S extends Buzz | Falsey>(predicate: (value: Buzz | Falsey, index: number, array: readonly (Buzz | Falsey)[]) => value is S, thisArg?: any): S[]; (predicate: BooleanConstructor, thisArg?: any): Buzz[]; (predicate: (value: Buzz | Falsey, index: number, array: readonly (Buzz | Falsey)[]) => unknown, thisArg?: any): (Buzz | Falsey)[]; } |
There was a problem hiding this comment.
Falsey is correctly filteredy, however the readonly trait is not preserved. Not preserving readonly was also a defect in the previous version. so it is not a regression. Maybe a problem in getUnionOfTypes().
src/compiler/checker.ts
Outdated
| const reducedType = getReducedApparentType(typeIn); | ||
| function carveoutResult(): readonly Signature[] | undefined { | ||
| if (kind === SignatureKind.Call && reducedType.flags & TypeFlags.Union) { | ||
| // If the union is all different instantiations of a member of the global array type... |
There was a problem hiding this comment.
This carveout was originally introduced in #53480. Previously it was only used in the case where getSignaturesOfStructuredType() return a length zero result: !length(result).
With new filter overload, getSignaturesOfStructuredType() returns an invalid result of length 1. Therefore, in order to piggyback onto this carveout to solve the problem, the condition would have to be expanded to inlcude at least length 1, at least for the case of method filter.
In the interests of generality, the length condition is dropped altogether.
src/compiler/checker.ts
Outdated
| } | ||
|
|
||
| // calculate return types as union of return types over the associated type, rather than return type associated with the union of types | ||
| const numTypes = (reducedType as UnionType).types.length; |
There was a problem hiding this comment.
An additional change was made to #53489, such that return types are calculated as union of return types over the associated type, rather than return type associated with the union of types. E.g., filter returns "A[] | B[]" rather than "(A | B)[]".
|
Only team members can start these jobs. @typescript-bot test top200 @typescript-bot perf test this To be clear I'm just running this out of interest and haven't read the PR or anything. |
|
Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at e904d77. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the tarball bundle task on this PR at e904d77. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at e904d77. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at e904d77. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the regular perf test suite on this PR at e904d77. You can monitor the build here. Update: The results are in! |
|
Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
|
@jakebailey Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Something interesting changed - please have a look. Details
|
|
@jakebailey Here they are:
CompilerComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
StartupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Hey @jakebailey, the results of running the DT tests are ready. Branch only errors:Package: microsoft-live-connect Package: winjs |
|
@jakebailey Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Details
|
Upon analysis it appears this error is due to the arguments types to |
craigphicks
force-pushed
the
PR-50377-redo-filterBooleanOverload
branch
from
e904d77 to
8e1fb57
Compare
|
Repoening. In this revision, es5.d.ts is not modified at all. Instead, the return type is changed dynamically in chooseOverload in checker.ts. Therefore, the user won't see a completion suggesting Boolean, unlike the last revision. Obviously that is a disadvantage. The advantage is that there are less changes overall, so the pull is easier to review. |
Fixes #50377 - allowing
Booleanas a predicate to Array.filter with falsey types filtered in the result.Fixes #56013 () - Adding an overload to .filter breaks specific inference with nested functions
( with qualification: The original #56013 added the filter overload externally. The bug is only fixed for the case where the overload is added internally, as it is in this pull).
Source Files changed:
src/compiler/checker.tsAdded Test Files
Related issues
filter(Boolean)
#50377 Explore adding the Boolean to filter to narrow out null/undefined in array#filter (closed pull)
#50387 Add BooleanConstructor as an overload to .filter to allow for easy type predicate filtering (inDiscussion)
#30621 narrow array type with .filter(Boolean) (marked as duplicate of #16655, although #16655 is more broad than #30621)
#56013 Adding an overload to .filter breaks specific inference with nested functions (related)
#55777
Boolean as falsey check in general
#16655 Boolean() cannot be used to perform a null check (broader than filter(Boolean))
#29955 Allow Boolean() to be used to perform a null check (accepted pull, may have reverted)
#53489 Add fallback logic for generating signatures for unions of array members
Source code changes
checker.tsIn
chooseOverload, afterresultis selected, when the argument is of typeBooleanConstructorand the result signature isArray.filter, the result signature is cloned, and itsresolvedReturnTypeproperty is set.Regressions
None found with runtests
Discussion
In this revision, es5.d.ts is not modified at all. Instead, the return type is changed dynamically in chooseOverload in checker.ts.
Therefore, the user won't see a completion suggesting Boolean, unlike the last revision. Obviously that is a disadvantage.
The advantage is that there are less changes overall, so the pull is easier to review. (Previously there were ~60 files changes, and that's a lot to review). There are a lot of `outdated code change annotations below that can now be ignored. (Is there a way to hide these?).