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

PR-50377-redo-filterBooleanOverload #56281

Conversation

craigphicks
Copy link

@craigphicks craigphicks commented Nov 1, 2023

Fixes #50377 - allowing Boolean as 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.ts

Added Test Files

  • tests/cases/compiler/arrayFilterBooleanOverload#56013WithoutExternalOverload.ts
  • tests/cases/compiler/arrayFilterBooleanOverload.ts

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.ts
    In chooseOverload, after result is selected, when the argument is of type BooleanConstructor and the result signature is Array.filter, the result signature is cloned, and its resolvedReturnType property 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?).

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Nov 1, 2023
@craigphicks craigphicks force-pushed the PR-50377-redo-filterBooleanOverload branch 2 times, most recently from 4fa02a2 to 111f0c6 Compare November 1, 2023 07:10
@craigphicks craigphicks force-pushed the PR-50377-redo-filterBooleanOverload branch from c8a9cb1 to f49ff0c Compare November 1, 2023 08:35
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)[];
Copy link
Author

Choose a reason for hiding this comment

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

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[]
Copy link
Author

@craigphicks craigphicks Nov 1, 2023

Choose a reason for hiding this comment

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

Improvement (at least from user perspective it is an improvement).

@typescript-bot typescript-bot added For Backlog Bug PRs that fix a backlog bug and removed For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Nov 1, 2023
@craigphicks craigphicks force-pushed the PR-50377-redo-filterBooleanOverload branch from 29b4592 to 28da703 Compare November 2, 2023 05:02
@jakebailey
Copy link
Member

@typescript-bot test top200
@typescript-bot user test this
@typescript-bot run dt
@typescript-bot perf test this
@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Nov 2, 2023

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!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Nov 2, 2023

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!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Nov 2, 2023

Heya @jakebailey, I've started to run the tarball bundle task on this PR at 28da703. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Nov 2, 2023

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!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Nov 2, 2023

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!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Nov 2, 2023

Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/158429/artifacts?artifactName=tgz&fileId=F98F028DFF7F4E7472202E4BAC9E46DD54E5F810681E2870F1829F665254343D02&fileName=/typescript-5.3.0-insiders.20231102.tgz"
    }
}

and then running npm install.


There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/[email protected]".;

@typescript-bot
Copy link
Collaborator

@jakebailey Here are the results of running the user test suite comparing main and refs/pull/56281/merge:

There were infrastructure failures potentially unrelated to your change:

  • 3 instances of "Package install failed"

Otherwise...

Something interesting changed - please have a look.

Details

webpack

tsconfig.types.json

  • [MISSING] error TS2345: Argument of type 'Comparator<Module>' is not assignable to parameter of type '(a: Module | null, b: Module | null) => number'.
  • [MISSING] error TS18047: 'module' is possibly 'null'.
  • [MISSING] error TS2345: Argument of type 'Module | null' is not assignable to parameter of type 'Module'.

@typescript-bot
Copy link
Collaborator

@jakebailey
The results of the perf run you requested are in!

Here they are:

Compiler

Comparison Report - baseline..pr
Metric baseline pr Delta Best Worst p-value
Angular - node (v18.15.0, x64)
Memory used 295,123k (± 0.01%) 295,183k (± 0.01%) +60k (+ 0.02%) 295,128k 295,231k p=0.045 n=6
Parse Time 2.62s (± 0.34%) 2.62s (± 0.67%) ~ 2.59s 2.63s p=1.000 n=6
Bind Time 0.83s (± 0.49%) 0.83s (± 0.62%) ~ 0.83s 0.84s p=0.595 n=6
Check Time 8.06s (± 0.29%) 8.08s (± 0.49%) ~ 8.03s 8.13s p=0.628 n=6
Emit Time 7.09s (± 0.17%) 7.09s (± 0.40%) ~ 7.06s 7.14s p=0.742 n=6
Total Time 18.60s (± 0.21%) 18.62s (± 0.39%) ~ 18.53s 18.73s p=0.936 n=6
Compiler-Unions - node (v18.15.0, x64)
Memory used 193,598k (± 1.64%) 192,771k (± 1.55%) ~ 190,821k 196,638k p=0.378 n=6
Parse Time 1.35s (± 0.73%) 1.36s (± 1.52%) ~ 1.33s 1.39s p=0.591 n=6
Bind Time 0.73s (± 0.00%) 0.73s (± 0.74%) ~ 0.73s 0.74s p=0.071 n=6
Check Time 9.17s (± 0.54%) 9.33s (± 0.47%) +0.16s (+ 1.75%) 9.26s 9.39s p=0.005 n=6
Emit Time 2.63s (± 0.34%) 2.65s (± 0.57%) ~ 2.62s 2.66s p=0.060 n=6
Total Time 13.88s (± 0.42%) 14.06s (± 0.41%) +0.19s (+ 1.33%) 14.00s 14.17s p=0.005 n=6
Monaco - node (v18.15.0, x64)
Memory used 347,344k (± 0.00%) 347,359k (± 0.01%) ~ 347,321k 347,382k p=0.109 n=6
Parse Time 2.46s (± 0.50%) 2.46s (± 0.48%) ~ 2.44s 2.47s p=0.677 n=6
Bind Time 0.94s (± 0.87%) 0.94s (± 0.43%) ~ 0.93s 0.94s p=1.000 n=6
Check Time 6.92s (± 0.30%) 6.96s (± 0.40%) +0.04s (+ 0.60%) 6.93s 6.99s p=0.019 n=6
Emit Time 4.04s (± 0.31%) 4.05s (± 0.49%) ~ 4.04s 4.09s p=0.137 n=6
Total Time 14.35s (± 0.22%) 14.42s (± 0.31%) +0.06s (+ 0.45%) 14.37s 14.47s p=0.024 n=6
TFS - node (v18.15.0, x64)
Memory used 302,602k (± 0.01%) 302,595k (± 0.00%) ~ 302,583k 302,610k p=1.000 n=6
Parse Time 1.99s (± 0.59%) 2.02s (± 0.75%) +0.03s (+ 1.34%) 2.00s 2.04s p=0.011 n=6
Bind Time 1.01s (± 0.40%) 1.00s (± 1.03%) ~ 0.99s 1.02s p=0.257 n=6
Check Time 6.25s (± 0.63%) 6.28s (± 0.27%) ~ 6.25s 6.30s p=0.295 n=6
Emit Time 3.59s (± 0.73%) 3.56s (± 0.48%) -0.03s (- 0.88%) 3.53s 3.58s p=0.043 n=6
Total Time 12.84s (± 0.48%) 12.86s (± 0.24%) ~ 12.82s 12.90s p=0.521 n=6
material-ui - node (v18.15.0, x64)
Memory used 470,496k (± 0.00%) 470,608k (± 0.00%) +113k (+ 0.02%) 470,592k 470,647k p=0.005 n=6
Parse Time 2.57s (± 0.75%) 2.57s (± 0.49%) ~ 2.55s 2.58s p=0.741 n=6
Bind Time 0.99s (± 0.83%) 0.99s (± 1.64%) ~ 0.97s 1.01s p=0.458 n=6
Check Time 16.63s (± 0.32%) 16.71s (± 0.40%) ~ 16.64s 16.82s p=0.064 n=6
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) ~ 0.00s 0.00s p=1.000 n=6
Total Time 20.18s (± 0.32%) 20.28s (± 0.39%) ~ 20.20s 20.41s p=0.065 n=6
xstate - node (v18.15.0, x64)
Memory used 512,663k (± 0.01%) 512,667k (± 0.01%) ~ 512,614k 512,705k p=0.936 n=6
Parse Time 3.27s (± 0.16%) 3.27s (± 0.33%) ~ 3.26s 3.29s p=0.324 n=6
Bind Time 1.55s (± 0.49%) 1.55s (± 0.49%) ~ 1.54s 1.56s p=1.000 n=6
Check Time 2.84s (± 0.65%) 2.84s (± 0.59%) ~ 2.81s 2.85s p=0.934 n=6
Emit Time 0.08s (± 0.00%) 0.07s (± 7.03%) 🟩-0.01s (- 8.33%) 0.07s 0.08s p=0.025 n=6
Total Time 7.73s (± 0.23%) 7.74s (± 0.33%) ~ 7.71s 7.77s p=0.517 n=6
System info unknown
Hosts
  • node (v18.15.0, x64)
Scenarios
  • Angular - node (v18.15.0, x64)
  • Compiler-Unions - node (v18.15.0, x64)
  • Monaco - node (v18.15.0, x64)
  • TFS - node (v18.15.0, x64)
  • material-ui - node (v18.15.0, x64)
  • xstate - node (v18.15.0, x64)
Benchmark Name Iterations
Current pr 6
Baseline baseline 6

tsserver

Comparison Report - baseline..pr
Metric baseline pr Delta Best Worst p-value
Compiler-UnionsTSServer - node (v18.15.0, x64)
Req 1 - updateOpen 2,382ms (± 1.02%) 2,384ms (± 1.14%) ~ 2,353ms 2,417ms p=1.000 n=6
Req 2 - geterr 5,382ms (± 1.53%) 5,442ms (± 1.50%) ~ 5,360ms 5,549ms p=0.378 n=6
Req 3 - references 328ms (± 1.17%) 327ms (± 0.92%) ~ 325ms 333ms p=0.746 n=6
Req 4 - navto 276ms (± 1.15%) 274ms (± 0.44%) ~ 272ms 275ms p=0.624 n=6
Req 5 - completionInfo count 1,356 (± 0.00%) 1,356 (± 0.00%) ~ 1,356 1,356 p=1.000 n=6
Req 5 - completionInfo 84ms (± 8.45%) 92ms (± 2.83%) 🔻+9ms (+10.18%) 90ms 97ms p=0.011 n=6
CompilerTSServer - node (v18.15.0, x64)
Req 1 - updateOpen 2,482ms (± 1.05%) 2,479ms (± 0.99%) ~ 2,432ms 2,502ms p=0.810 n=6
Req 2 - geterr 4,141ms (± 1.84%) 4,163ms (± 2.06%) ~ 4,075ms 4,263ms p=0.471 n=6
Req 3 - references 336ms (± 0.97%) 338ms (± 1.19%) ~ 334ms 342ms p=0.181 n=6
Req 4 - navto 282ms (± 0.30%) 288ms (± 1.59%) +7ms (+ 2.37%) 284ms 293ms p=0.004 n=6
Req 5 - completionInfo count 1,518 (± 0.00%) 1,518 (± 0.00%) ~ 1,518 1,518 p=1.000 n=6
Req 5 - completionInfo 80ms (± 5.76%) 82ms (± 7.85%) ~ 75ms 88ms p=0.744 n=6
xstateTSServer - node (v18.15.0, x64)
Req 1 - updateOpen 2,589ms (± 0.41%) 2,586ms (± 0.57%) ~ 2,562ms 2,602ms p=0.872 n=6
Req 2 - geterr 1,717ms (± 1.32%) 1,722ms (± 2.25%) ~ 1,681ms 1,780ms p=1.000 n=6
Req 3 - references 114ms (± 8.08%) 116ms (± 9.13%) ~ 106ms 126ms p=0.326 n=6
Req 4 - navto 365ms (± 0.21%) 366ms (± 0.66%) ~ 363ms 370ms p=0.867 n=6
Req 5 - completionInfo count 2,073 (± 0.00%) 2,073 (± 0.00%) ~ 2,073 2,073 p=1.000 n=6
Req 5 - completionInfo 309ms (± 1.87%) 304ms (± 2.05%) ~ 298ms 314ms p=0.149 n=6
System info unknown
Hosts
  • node (v18.15.0, x64)
Scenarios
  • CompilerTSServer - node (v18.15.0, x64)
  • Compiler-UnionsTSServer - node (v18.15.0, x64)
  • xstateTSServer - node (v18.15.0, x64)
Benchmark Name Iterations
Current pr 6
Baseline baseline 6

Startup

Comparison Report - baseline..pr
Metric baseline pr Delta Best Worst p-value
tsc-startup - node (v18.15.0, x64)
Execution time 152.06ms (± 0.18%) 152.06ms (± 0.20%) ~ 150.86ms 157.55ms p=0.749 n=600
tsserver-startup - node (v18.15.0, x64)
Execution time 227.69ms (± 0.16%) 227.46ms (± 0.17%) -0.23ms (- 0.10%) 226.21ms 236.38ms p=0.000 n=600
tsserverlibrary-startup - node (v18.15.0, x64)
Execution time 228.96ms (± 0.17%) 229.10ms (± 0.16%) +0.14ms (+ 0.06%) 227.57ms 235.27ms p=0.000 n=600
typescript-startup - node (v18.15.0, x64)
Execution time 228.57ms (± 0.16%) 228.52ms (± 0.15%) ~ 227.14ms 231.12ms p=0.270 n=600
System info unknown
Hosts
  • node (v18.15.0, x64)
Scenarios
  • tsc-startup - node (v18.15.0, x64)
  • tsserver-startup - node (v18.15.0, x64)
  • tsserverlibrary-startup - node (v18.15.0, x64)
  • typescript-startup - node (v18.15.0, x64)
Benchmark Name Iterations
Current pr 6
Baseline baseline 6

Developer Information:

Download Benchmarks

@typescript-bot
Copy link
Collaborator

Hey @jakebailey, the results of running the DT tests are ready.
There were interesting changes:

Branch only errors:

Package: microsoft-live-connect
Error:

Error: Errors in typescript@local for external dependencies:
../winjs/index.d.ts(10075,9): error TS2416: Property 'filter' in type 'QueryCollection<T>' is not assignable to the same property in base type 'T[]'.
  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 | ... 2 more ... | 0n ? never : T)[]; (predicate: (value: T, index: number, array: T[]) => unknown, thisArg?: any): T[]; }'.
    Type 'T[]' is not assignable to type '(T extends false | "" | 0 | 0n ? never : T)[]'.
      Type 'T' is not assignable to type 'T extends false | "" | 0 | 0n ? never : T'.

    at testTypesVersion (/home/vsts/work/1/DefinitelyTyped/node_modules/.pnpm/@[email protected][email protected]/node_modules/@definitelytyped/dtslint/dist/index.js:171:15)
    at async runTests (/home/vsts/work/1/DefinitelyTyped/node_modules/.pnpm/@[email protected][email protected]/node_modules/@definitelytyped/dtslint/dist/index.js:130:9)

Package: winjs
Error:

Error: /home/vsts/work/1/DefinitelyTyped/types/winjs/index.d.ts:10075:9
ERROR: 10075:9  expect  TypeScript@local compile error: 
Property 'filter' in type 'QueryCollection<T>' is not assignable to the same property in base type 'T[]'.
  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 | ... 4 more ... | undefined ? never : T)[]; (predicate: (value: T, index: number, array: T[]) => unknown, thisArg?: any): T[]; }'.
    Type 'T[]' is not assignable to type '(T extends false | "" | 0 | 0n | null | undefined ? never : T)[]'.
      Type 'T' is not assignable to type 'T extends false | "" | 0 | 0n | null | undefined ? never : T'.

    at testTypesVersion (/home/vsts/work/1/DefinitelyTyped/node_modules/.pnpm/@[email protected][email protected]/node_modules/@definitelytyped/dtslint/dist/index.js:171:15)
    at async runTests (/home/vsts/work/1/DefinitelyTyped/node_modules/.pnpm/@[email protected][email protected]/node_modules/@definitelytyped/dtslint/dist/index.js:130:9)

You can check the log here.

@craigphicks
Copy link
Author

Comparison Report - baseline..pr
Compiler-UnionsTSServer - node (v18.15.0, x64)
Req 5 - completionInfo
Delta +9ms (+10.18%)

That doesn't look so good. @jakebailey - do you agree?
"Unions" fits the description of Arrays with Union Template Types which are processed in getSignaturesOfType in the carve out branch. Currently its taking the carve out branch more than the minimum required to get to the Boolean overload to pass. I could change that in an attempt to reduce the time. How about it?

@jakebailey
Copy link
Member

I doubt that's anything but noise.

@typescript-bot
Copy link
Collaborator

@jakebailey Here are the results of running the top-repos suite comparing main and refs/pull/56281/merge:

Something interesting changed - please have a look.

Details

neoclide/coc.nvim

tsconfig.json

pubkey/rxdb

8 of 10 projects failed to build with the old tsc and were ignored

config/tsconfig.types.json

tsconfig.json

recharts/recharts

2 of 3 projects failed to build with the old tsc and were ignored

demo/tsconfig.json

@craigphicks
Copy link
Author

Re: top-repos suite

error TS2322: Type '{ word: string | VimCompleteItem; }[]' is not assignable to type 'ExtendedCompleteItem[]'.
error TS2322: Type 'string | undefined' is not assignable to type 'string'.

Almost certainly due to the transformation of union-of-arrays to array-of-union-of-types in the carve-out in getSignaturesOfType. Those errors would probably be eliminated by making the planned changes described in section "Possible improvements / changes" of the pull description at the top of this page.

@craigphicks craigphicks force-pushed the PR-50377-redo-filterBooleanOverload branch 3 times, most recently from 2a92bc7 to 0a13549 Compare November 4, 2023 00:32


([] as (Fizz|Falsey)[] | (Buzz|Falsey)[]).filter(Boolean); // expect type (Fizz|Buzz)[]
>([] as (Fizz|Falsey)[] | (Buzz|Falsey)[]).filter(Boolean) : Fizz[] | Buzz[]
Copy link
Author

@craigphicks craigphicks Nov 4, 2023

Choose a reason for hiding this comment

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

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")[]
Copy link
Author

Choose a reason for hiding this comment

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

This is a case that was brought up by #50377 (comment) in the original #50377.

@craigphicks craigphicks force-pushed the PR-50377-redo-filterBooleanOverload branch from 64d8ace to e904d77 Compare November 4, 2023 07:17
return getUnionType(art, UnionReduction.Subtype);
});

// Transform the type from `(A[] | B[])["member"]` to `(A | B)[]["member"]` (since we pretend array is covariant anyway)
Copy link
Author

Choose a reason for hiding this comment

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

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)[];
Copy link
Author

Choose a reason for hiding this comment

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

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)[];
Copy link
Author

Choose a reason for hiding this comment

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

New Overload for Array

>['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
Copy link
Author

Choose a reason for hiding this comment

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

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);
~~~~~~~
Copy link
Author

@craigphicks craigphicks Nov 4, 2023

Choose a reason for hiding this comment

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

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'.
Copy link
Author

Choose a reason for hiding this comment

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

Regression

// | (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
Copy link
Author

@craigphicks craigphicks Nov 4, 2023

Choose a reason for hiding this comment

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

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'.
}

Copy link
Author

@craigphicks craigphicks Nov 4, 2023

Choose a reason for hiding this comment

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

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[]; }'.
Copy link
Author

Choose a reason for hiding this comment

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

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)[]; }
Copy link
Author

Choose a reason for hiding this comment

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

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().

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...
Copy link
Author

@craigphicks craigphicks Nov 4, 2023

Choose a reason for hiding this comment

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

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.

}

// 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;
Copy link
Author

Choose a reason for hiding this comment

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

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)[]".

@craigphicks craigphicks marked this pull request as ready for review November 4, 2023 19:44
@jakebailey
Copy link
Member

Only team members can start these jobs.

@typescript-bot test top200
@typescript-bot user test this
@typescript-bot run dt

@typescript-bot perf test this
@typescript-bot pack this

To be clear I'm just running this out of interest and haven't read the PR or anything.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Nov 4, 2023

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!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Nov 4, 2023

Heya @jakebailey, I've started to run the tarball bundle task on this PR at e904d77. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Nov 4, 2023

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!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Nov 4, 2023

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!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Nov 4, 2023

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!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Nov 4, 2023

Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/158489/artifacts?artifactName=tgz&fileId=F021262C2ED262A388861D770D1E9E70107D79CB2A4ADE63898389FF6700272F02&fileName=/typescript-5.4.0-insiders.20231104.tgz"
    }
}

and then running npm install.


There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/[email protected]".;

@typescript-bot
Copy link
Collaborator

@jakebailey Here are the results of running the user test suite comparing main and refs/pull/56281/merge:

There were infrastructure failures potentially unrelated to your change:

  • 3 instances of "Package install failed"

Otherwise...

Something interesting changed - please have a look.

Details

webpack

tsconfig.types.json

  • [MISSING] error TS2345: Argument of type 'Comparator<Module>' is not assignable to parameter of type '(a: Module | null, b: Module | null) => number'.
  • [MISSING] error TS18047: 'module' is possibly 'null'.
  • [MISSING] error TS2345: Argument of type 'Module | null' is not assignable to parameter of type 'Module'.

@typescript-bot
Copy link
Collaborator

@jakebailey
The results of the perf run you requested are in!

Here they are:

Compiler

Comparison Report - baseline..pr
Metric baseline pr Delta Best Worst p-value
Angular - node (v18.15.0, x64)
Memory used 295,159k (± 0.01%) 295,142k (± 0.01%) ~ 295,084k 295,196k p=0.471 n=6
Parse Time 2.64s (± 0.39%) 2.62s (± 0.99%) ~ 2.57s 2.64s p=0.180 n=6
Bind Time 0.83s (± 1.00%) 0.84s (± 0.90%) ~ 0.83s 0.85s p=0.432 n=6
Check Time 8.04s (± 0.24%) 8.06s (± 0.26%) ~ 8.03s 8.08s p=0.150 n=6
Emit Time 7.10s (± 0.36%) 7.07s (± 0.09%) -0.03s (- 0.38%) 7.06s 7.08s p=0.024 n=6
Total Time 18.61s (± 0.20%) 18.59s (± 0.17%) ~ 18.55s 18.63s p=0.517 n=6
Compiler-Unions - node (v18.15.0, x64)
Memory used 195,535k (± 1.20%) 192,313k (± 1.29%) ~ 190,840k 196,944k p=0.298 n=6
Parse Time 1.35s (± 1.24%) 1.35s (± 0.00%) ~ 1.35s 1.35s p=0.599 n=6
Bind Time 0.73s (± 0.56%) 0.73s (± 0.00%) ~ 0.73s 0.73s p=0.405 n=6
Check Time 9.16s (± 0.42%) 9.30s (± 0.25%) +0.14s (+ 1.49%) 9.26s 9.33s p=0.005 n=6
Emit Time 2.63s (± 0.50%) 2.63s (± 0.82%) ~ 2.61s 2.67s p=0.933 n=6
Total Time 13.87s (± 0.39%) 14.01s (± 0.21%) +0.14s (+ 1.01%) 13.98s 14.05s p=0.008 n=6
Monaco - node (v18.15.0, x64)
Memory used 347,379k (± 0.01%) 347,354k (± 0.01%) ~ 347,332k 347,391k p=0.093 n=6
Parse Time 2.45s (± 0.48%) 2.46s (± 0.56%) ~ 2.44s 2.47s p=0.461 n=6
Bind Time 0.94s (± 0.00%) 0.94s (± 0.43%) ~ 0.93s 0.94s p=0.405 n=6
Check Time 6.92s (± 0.38%) 6.95s (± 0.28%) +0.03s (+ 0.43%) 6.93s 6.97s p=0.040 n=6
Emit Time 4.05s (± 0.53%) 4.05s (± 0.30%) ~ 4.03s 4.06s p=1.000 n=6
Total Time 14.35s (± 0.26%) 14.39s (± 0.24%) ~ 14.35s 14.45s p=0.170 n=6
TFS - node (v18.15.0, x64)
Memory used 302,587k (± 0.01%) 302,592k (± 0.01%) ~ 302,563k 302,646k p=0.873 n=6
Parse Time 1.99s (± 1.73%) 1.99s (± 1.07%) ~ 1.96s 2.02s p=0.935 n=6
Bind Time 1.01s (± 1.58%) 1.00s (± 1.09%) ~ 0.99s 1.02s p=0.187 n=6
Check Time 6.26s (± 0.58%) 6.27s (± 0.49%) ~ 6.23s 6.31s p=0.518 n=6
Emit Time 3.58s (± 0.45%) 3.58s (± 0.35%) ~ 3.56s 3.59s p=0.615 n=6
Total Time 12.85s (± 0.47%) 12.85s (± 0.41%) ~ 12.75s 12.90s p=0.936 n=6
material-ui - node (v18.15.0, x64)
Memory used 470,555k (± 0.00%) 470,643k (± 0.00%) +88k (+ 0.02%) 470,608k 470,674k p=0.005 n=6
Parse Time 2.56s (± 0.32%) 2.57s (± 0.52%) ~ 2.55s 2.59s p=0.198 n=6
Bind Time 0.99s (± 1.81%) 0.99s (± 1.11%) ~ 0.97s 1.00s p=0.868 n=6
Check Time 16.63s (± 0.27%) 16.75s (± 0.33%) +0.12s (+ 0.72%) 16.67s 16.81s p=0.010 n=6
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) ~ 0.00s 0.00s p=1.000 n=6
Total Time 20.18s (± 0.27%) 20.31s (± 0.28%) +0.13s (+ 0.64%) 20.23s 20.37s p=0.008 n=6
xstate - node (v18.15.0, x64)
Memory used 512,870k (± 0.02%) 512,848k (± 0.02%) ~ 512,772k 513,056k p=0.471 n=6
Parse Time 3.27s (± 0.19%) 3.27s (± 0.23%) ~ 3.26s 3.28s p=0.718 n=6
Bind Time 1.54s (± 0.35%) 1.54s (± 0.53%) ~ 1.53s 1.55s p=0.859 n=6
Check Time 2.85s (± 0.68%) 2.84s (± 0.57%) ~ 2.82s 2.86s p=0.373 n=6
Emit Time 0.08s (± 4.99%) 0.08s (± 6.19%) ~ 0.08s 0.09s p=0.595 n=6
Total Time 7.75s (± 0.28%) 7.74s (± 0.25%) ~ 7.72s 7.77s p=0.570 n=6
System info unknown
Hosts
  • node (v18.15.0, x64)
Scenarios
  • Angular - node (v18.15.0, x64)
  • Compiler-Unions - node (v18.15.0, x64)
  • Monaco - node (v18.15.0, x64)
  • TFS - node (v18.15.0, x64)
  • material-ui - node (v18.15.0, x64)
  • xstate - node (v18.15.0, x64)
Benchmark Name Iterations
Current pr 6
Baseline baseline 6

tsserver

Comparison Report - baseline..pr
Metric baseline pr Delta Best Worst p-value
Compiler-UnionsTSServer - node (v18.15.0, x64)
Req 1 - updateOpen 2,369ms (± 0.69%) 2,377ms (± 0.57%) ~ 2,354ms 2,390ms p=0.378 n=6
Req 2 - geterr 5,400ms (± 1.56%) 5,389ms (± 0.92%) ~ 5,348ms 5,485ms p=0.810 n=6
Req 3 - references 329ms (± 1.58%) 328ms (± 0.56%) ~ 326ms 330ms p=0.871 n=6
Req 4 - navto 277ms (± 1.90%) 275ms (± 0.58%) ~ 272ms 276ms p=0.681 n=6
Req 5 - completionInfo count 1,356 (± 0.00%) 1,356 (± 0.00%) ~ 1,356 1,356 p=1.000 n=6
Req 5 - completionInfo 84ms (± 7.92%) 86ms (± 2.15%) ~ 85ms 90ms p=0.869 n=6
CompilerTSServer - node (v18.15.0, x64)
Req 1 - updateOpen 2,476ms (± 0.93%) 2,490ms (± 0.81%) ~ 2,465ms 2,514ms p=0.298 n=6
Req 2 - geterr 4,122ms (± 2.02%) 4,159ms (± 1.87%) ~ 4,086ms 4,246ms p=0.173 n=6
Req 3 - references 339ms (± 1.57%) 332ms (± 0.63%) -7ms (- 2.11%) 329ms 334ms p=0.012 n=6
Req 4 - navto 282ms (± 0.29%) 288ms (± 1.33%) +6ms (+ 2.13%) 283ms 292ms p=0.006 n=6
Req 5 - completionInfo count 1,518 (± 0.00%) 1,518 (± 0.00%) ~ 1,518 1,518 p=1.000 n=6
Req 5 - completionInfo 83ms (± 7.91%) 74ms (± 3.52%) 🟩-9ms (-10.84%) 71ms 77ms p=0.008 n=6
xstateTSServer - node (v18.15.0, x64)
Req 1 - updateOpen 2,590ms (± 0.50%) 2,593ms (± 0.59%) ~ 2,564ms 2,607ms p=0.936 n=6
Req 2 - geterr 1,695ms (± 2.45%) 1,734ms (± 1.86%) ~ 1,686ms 1,766ms p=0.128 n=6
Req 3 - references 110ms (± 7.51%) 120ms (± 9.00%) ~ 106ms 129ms p=0.413 n=6
Req 4 - navto 366ms (± 0.23%) 368ms (± 1.54%) ~ 365ms 379ms p=0.775 n=6
Req 5 - completionInfo count 2,073 (± 0.00%) 2,073 (± 0.00%) ~ 2,073 2,073 p=1.000 n=6
Req 5 - completionInfo 301ms (± 1.90%) 310ms (± 1.24%) +9ms (+ 2.99%) 305ms 316ms p=0.028 n=6
System info unknown
Hosts
  • node (v18.15.0, x64)
Scenarios
  • CompilerTSServer - node (v18.15.0, x64)
  • Compiler-UnionsTSServer - node (v18.15.0, x64)
  • xstateTSServer - node (v18.15.0, x64)
Benchmark Name Iterations
Current pr 6
Baseline baseline 6

Startup

Comparison Report - baseline..pr
Metric baseline pr Delta Best Worst p-value
tsc-startup - node (v18.15.0, x64)
Execution time 152.76ms (± 0.18%) 152.79ms (± 0.18%) ~ 151.62ms 155.05ms p=0.453 n=600
tsserver-startup - node (v18.15.0, x64)
Execution time 228.12ms (± 0.16%) 228.06ms (± 0.17%) -0.06ms (- 0.03%) 226.72ms 231.07ms p=0.011 n=600
tsserverlibrary-startup - node (v18.15.0, x64)
Execution time 228.85ms (± 0.20%) 228.83ms (± 0.19%) ~ 227.07ms 235.57ms p=0.768 n=600
typescript-startup - node (v18.15.0, x64)
Execution time 229.22ms (± 0.17%) 229.41ms (± 0.19%) +0.19ms (+ 0.08%) 227.65ms 237.69ms p=0.000 n=600
System info unknown
Hosts
  • node (v18.15.0, x64)
Scenarios
  • tsc-startup - node (v18.15.0, x64)
  • tsserver-startup - node (v18.15.0, x64)
  • tsserverlibrary-startup - node (v18.15.0, x64)
  • typescript-startup - node (v18.15.0, x64)
Benchmark Name Iterations
Current pr 6
Baseline baseline 6

Developer Information:

Download Benchmarks

@typescript-bot
Copy link
Collaborator

Hey @jakebailey, the results of running the DT tests are ready.
There were interesting changes:

Branch only errors:

Package: microsoft-live-connect
Error:

Error: Errors in typescript@local for external dependencies:
../winjs/index.d.ts(10075,9): error TS2416: Property 'filter' in type 'QueryCollection<T>' is not assignable to the same property in base type 'T[]'.
  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 | ... 2 more ... | 0n ? never : T)[]; (predicate: (value: T, index: number, array: T[]) => unknown, thisArg?: any): T[]; }'.
    Type 'T[]' is not assignable to type '(T extends false | "" | 0 | 0n ? never : T)[]'.
      Type 'T' is not assignable to type 'T extends false | "" | 0 | 0n ? never : T'.

    at testTypesVersion (/home/vsts/work/1/DefinitelyTyped/node_modules/.pnpm/@[email protected][email protected]/node_modules/@definitelytyped/dtslint/dist/index.js:171:15)
    at async runTests (/home/vsts/work/1/DefinitelyTyped/node_modules/.pnpm/@[email protected][email protected]/node_modules/@definitelytyped/dtslint/dist/index.js:130:9)

Package: winjs
Error:

Error: /home/vsts/work/1/DefinitelyTyped/types/winjs/index.d.ts:10075:9
ERROR: 10075:9  expect  TypeScript@local compile error: 
Property 'filter' in type 'QueryCollection<T>' is not assignable to the same property in base type 'T[]'.
  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 | ... 4 more ... | undefined ? never : T)[]; (predicate: (value: T, index: number, array: T[]) => unknown, thisArg?: any): T[]; }'.
    Type 'T[]' is not assignable to type '(T extends false | "" | 0 | 0n | null | undefined ? never : T)[]'.
      Type 'T' is not assignable to type 'T extends false | "" | 0 | 0n | null | undefined ? never : T'.

    at testTypesVersion (/home/vsts/work/1/DefinitelyTyped/node_modules/.pnpm/@[email protected][email protected]/node_modules/@definitelytyped/dtslint/dist/index.js:171:15)
    at async runTests (/home/vsts/work/1/DefinitelyTyped/node_modules/.pnpm/@[email protected][email protected]/node_modules/@definitelytyped/dtslint/dist/index.js:130:9)

You can check the log here.

@typescript-bot
Copy link
Collaborator

@jakebailey Here are the results of running the top-repos suite comparing main and refs/pull/56281/merge:

Something interesting changed - please have a look.

Details

neoclide/coc.nvim

tsconfig.json

pubkey/rxdb

8 of 10 projects failed to build with the old tsc and were ignored

config/tsconfig.types.json

tsconfig.json

recharts/recharts

2 of 3 projects failed to build with the old tsc and were ignored

demo/tsconfig.json

@craigphicks
Copy link
Author

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!

Upon analysis it appears this error

> error TS2322: Type '{ word: string | VimCompleteItem; }[]' is not assignable to type 'ExtendedCompleteItem[]'.

is due to the arguments types to map being squished together, and nothing to with the result type.
I am going to take this into development while I attempt to fix it.

@craigphicks craigphicks marked this pull request as draft November 6, 2023 17:05
@craigphicks craigphicks closed this Nov 9, 2023
@craigphicks craigphicks force-pushed the PR-50377-redo-filterBooleanOverload branch from e904d77 to 8e1fb57 Compare November 9, 2023 19:42
@craigphicks
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Backlog Bug PRs that fix a backlog bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding an overload to .filter breaks specific inference with nested functions
4 participants