Skip to content

Improve logic that chooses co- vs. contra-variant return type inferences - #59709

Closed
Mateusz Burzyński (Andarist) wants to merge 1 commit into
microsoft:mainfrom
Andarist:fix/co-contra-return-type-inference
Closed

Improve logic that chooses co- vs. contra-variant return type inferences#59709
Mateusz Burzyński (Andarist) wants to merge 1 commit into
microsoft:mainfrom
Andarist:fix/co-contra-return-type-inference

Conversation

@Andarist

Copy link
Copy Markdown
Contributor

fixes#59656
cc Wesley Wigham (@weswigham) as the reviewer of #57909 by which this got affected

@typescript-botTypeScript Bot (typescript-bot) added the For Backlog Bug PRs that fix a backlog bug label Aug 21, 2024
// and has inferences that would conflict. Otherwise, we prefer the contra-variant inference.
// Similarly ignore co-variant `any` inference when both are available as almost everything is assignable to it
// and it would spoil the overall inference.
// ideally all inferences would be tried out in a ranked order but that's too costly

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I rewritten this comment based on the excellent comment by Wesley Wigham (@weswigham)here. I tried to be concise though as this is already pretty long and Wes' comment was even longer 😉 Feel free to suggest any improvement to this to make it better. I write better in TS than in English.

Comment on lines +27004 to +27009
// however, for return types inferences subtyping is used. In those situations, the contravariant inference often comes from the argument of mapping function
// and the outer covariant requirement can still be satisfied by the return position of the return type when it's a subtype of that required return type.
// this helps to provide better contextual parameter types in scenarios like this:
//
// declare const obs: Observable<{ a?: string; b?: number }>;
// const test = (): Observable<{ a?: string }> => obs.pipe(tap((arg) => {}));

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

This part of the comment tries to explain the idea behind the fix.

An alternative idea I had was to experiment with a new inference priority to differentiate between covariant inferences made from return positions. However, that's a bigger experiment and what I propose here is much safer at this stage. It's essentially reverting part of #57909 to 5.5 behavior. So it shouldn't behave worse in any case.

But also given that return positions could themselves be within nested functions, I don't have a good intuition on how it could play out. At the moment, there is no concept of "depth" in the inference and the core of this idea conceptually is that a return position might not be more important than a contravariant inference made from the (any?) containing function.

This would have some potential to solve Wes' concern raised here related to inferring from (x: number) => 0 to (x: T) => T.

@jakebailey

Copy link
Copy Markdown
Member

TypeScript Bot (@typescript-bot) test it

@typescript-bot

TypeScript Bot (typescript-bot) commented Aug 21, 2024

Copy link
Copy Markdown
Contributor

Starting jobs; this comment will be updated as builds start and complete.

CommandStatusResults
test top400✅ Started✅ Results
user test this✅ Started✅ Results
run dt✅ Started✅ Results
perf test this faster✅ Started👀 Results

@typescript-bot

Copy link
Copy Markdown
Contributor

Hey Jake Bailey (@jakebailey), the results of running the DT tests are ready.

Everything looks the same!

You can check the log here.

@typescript-bot

Copy link
Copy Markdown
Contributor

Jake Bailey (@jakebailey) Here are the results of running the user tests with tsc comparing main and refs/pull/59709/merge:

Everything looks good!

@typescript-bot

Copy link
Copy Markdown
Contributor

Jake Bailey (@jakebailey)
The results of the perf run you requested are in!

Here they are:

tsc

Comparison Report - baseline..pr
MetricbaselineprDeltaBestWorstp-value
Compiler-Unions - node (v18.15.0, x64)
Errors3030~~~p=1.000 n=6
Symbols62,15362,153~~~p=1.000 n=6
Types50,24250,242~~~p=1.000 n=6
Memory used193,608k (± 0.92%)192,994k (± 0.77%)~192,340k196,046kp=0.936 n=6
Parse Time1.30s (± 0.58%)1.31s (± 0.39%)+0.01s (+ 0.90%)1.31s1.32sp=0.020 n=6
Bind Time0.71s0.71s~~~p=1.000 n=6
Check Time9.60s (± 0.91%)9.57s (± 0.41%)~9.52s9.61sp=0.467 n=6
Emit Time2.72s (± 0.63%)2.72s (± 1.03%)~2.66s2.74sp=0.560 n=6
Total Time14.33s (± 0.62%)14.31s (± 0.29%)~14.26s14.36sp=0.688 n=6
angular-1 - node (v18.15.0, x64)
Errors77~~~p=1.000 n=6
Symbols945,753945,753~~~p=1.000 n=6
Types410,067410,067~~~p=1.000 n=6
Memory used1,222,694k (± 0.00%)1,222,696k (± 0.00%)~1,222,670k1,222,728kp=0.936 n=6
Parse Time6.66s (± 0.36%)6.65s (± 0.44%)~6.63s6.71sp=0.684 n=6
Bind Time1.86s (± 0.40%)1.86s (± 0.53%)~1.85s1.88sp=0.858 n=6
Check Time31.18s (± 0.44%)31.14s (± 0.35%)~30.99s31.31sp=0.748 n=6
Emit Time14.91s (± 0.56%)14.98s (± 0.44%)~14.89s15.06sp=0.225 n=6
Total Time54.60s (± 0.29%)54.64s (± 0.26%)~54.46s54.79sp=1.000 n=6
mui-docs - node (v18.15.0, x64)
Errors00~~~p=1.000 n=6
Symbols2,517,7792,517,779~~~p=1.000 n=6
Types994,242994,242~~~p=1.000 n=6
Memory used2,442,563k (± 0.00%)2,442,540k (± 0.00%)~2,442,512k2,442,577kp=0.230 n=6
Parse Time9.30s (± 0.35%)9.29s (± 0.38%)~9.22s9.32sp=0.366 n=6
Bind Time2.20s (± 0.62%)2.19s (± 0.45%)~2.18s2.20sp=0.673 n=6
Check Time74.83s (± 0.29%)74.77s (± 0.51%)~74.16s75.18sp=1.000 n=6
Emit Time0.28s (± 3.19%)0.28s (± 3.53%)~0.27s0.29sp=0.798 n=6
Total Time86.60s (± 0.29%)86.53s (± 0.44%)~85.95s86.95sp=0.936 n=6
self-build-src - node (v18.15.0, x64)
Errors00~~~p=1.000 n=6
Symbols1,231,9981,231,999+1 (+ 0.00%)~~p=0.001 n=6
Types264,593264,593~~~p=1.000 n=6
Memory used2,472,512k (± 7.44%)2,472,419k (± 7.44%)~2,353,350k2,710,065kp=0.810 n=6
Parse Time6.04s (± 1.01%)5.99s (± 0.62%)~5.94s6.04sp=0.128 n=6
Bind Time2.26s (± 0.91%)2.25s (± 1.20%)~2.21s2.29sp=0.418 n=6
Check Time40.79s (± 0.79%)40.76s (± 0.76%)~40.23s41.03sp=0.689 n=6
Emit Time4.03s (± 0.87%)4.01s (± 0.74%)~3.96s4.04sp=0.335 n=6
Total Time53.14s (± 0.61%)53.03s (± 0.56%)~52.54s53.37sp=0.575 n=6
self-build-src-public-api - node (v18.15.0, x64)
Errors00~~~p=1.000 n=6
Symbols1,231,9981,231,999+1 (+ 0.00%)~~p=0.001 n=6
Types264,593264,593~~~p=1.000 n=6
Memory used2,428,294k (± 0.02%)2,428,201k (± 0.02%)~2,427,639k2,428,911kp=0.689 n=6
Parse Time6.24s (± 0.45%)6.26s (± 0.95%)~6.19s6.34sp=0.689 n=6
Bind Time2.04s (± 0.67%)2.04s (± 1.41%)~1.99s2.07sp=0.808 n=6
Check Time41.65s (± 0.63%)41.85s (± 0.59%)~41.46s42.08sp=0.230 n=6
Emit Time4.06s (± 0.63%)4.06s (± 0.76%)~4.03s4.11sp=0.809 n=6
Total Time53.99s (± 0.49%)54.24s (± 0.54%)~53.83s54.55sp=0.173 n=6
self-compiler - node (v18.15.0, x64)
Errors00~~~p=1.000 n=6
Symbols256,992256,993+1 (+ 0.00%)~~p=0.001 n=6
Types105,667105,667~~~p=1.000 n=6
Memory used429,722k (± 0.05%)429,626k (± 0.03%)~429,402k429,751kp=0.810 n=6
Parse Time2.79s (± 0.71%)2.78s (± 0.60%)~2.77s2.81sp=0.222 n=6
Bind Time1.08s (± 0.91%)1.08s (± 1.12%)~1.07s1.10sp=0.388 n=6
Check Time15.39s (± 0.35%)15.36s (± 0.41%)~15.28s15.43sp=0.521 n=6
Emit Time1.42s (± 1.07%)1.40s (± 0.84%)~1.38s1.41sp=0.071 n=6
Total Time20.68s (± 0.25%)20.62s (± 0.39%)~20.53s20.74sp=0.172 n=6
ts-pre-modules - node (v18.15.0, x64)
Errors6868~~~p=1.000 n=6
Symbols225,018225,018~~~p=1.000 n=6
Types94,24994,249~~~p=1.000 n=6
Memory used370,188k (± 0.02%)370,215k (± 0.02%)~370,090k370,318kp=0.689 n=6
Parse Time2.77s (± 0.54%)2.76s (± 1.16%)~2.73s2.81sp=0.625 n=6
Bind Time1.58s (± 1.81%)1.58s (± 0.74%)~1.57s1.60sp=0.560 n=6
Check Time15.75s (± 0.24%)15.78s (± 0.42%)~15.68s15.86sp=0.422 n=6
Emit Time0.00s0.00s~~~p=1.000 n=6
Total Time20.10s (± 0.24%)20.12s (± 0.26%)~20.04s20.17sp=0.466 n=6
vscode - node (v18.15.0, x64)
Errors6666~~~p=1.000 n=6
Symbols3,020,8703,020,870~~~p=1.000 n=6
Types1,039,2401,039,240~~~p=1.000 n=6
Memory used3,140,085k (± 0.00%)3,140,216k (± 0.00%)+131k (+ 0.00%)3,140,127k3,140,294kp=0.020 n=6
Parse Time13.98s (± 0.46%)14.02s (± 0.39%)~13.93s14.09sp=0.261 n=6
Bind Time4.35s (± 2.05%)4.30s (± 0.23%)~4.29s4.31sp=0.066 n=6
Check Time80.07s (± 0.31%)80.27s (± 0.62%)~79.66s80.86sp=0.378 n=6
Emit Time20.50s (± 0.62%)20.52s (± 0.80%)~20.34s20.70sp=0.936 n=6
Total Time118.90s (± 0.23%)119.11s (± 0.45%)~118.43s119.85sp=0.471 n=6
webpack - node (v18.15.0, x64)
Errors00~~~p=1.000 n=6
Symbols275,311275,311~~~p=1.000 n=6
Types112,431112,431~~~p=1.000 n=6
Memory used424,171k (± 0.02%)424,189k (± 0.02%)~424,072k424,324kp=0.575 n=6
Parse Time3.97s (± 0.47%)3.98s (± 0.61%)~3.94s4.00sp=0.224 n=6
Bind Time1.72s (± 0.68%)1.72s (± 0.88%)~1.70s1.74sp=0.410 n=6
Check Time17.51s (± 0.40%)17.47s (± 0.73%)~17.30s17.59sp=0.810 n=6
Emit Time0.00s0.00s~~~p=1.000 n=6
Total Time23.19s (± 0.25%)23.17s (± 0.62%)~22.96s23.32sp=1.000 n=6
xstate-main - node (v18.15.0, x64)
Errors00~~~p=1.000 n=6
Symbols534,554534,554~~~p=1.000 n=6
Types176,379176,379~~~p=1.000 n=6
Memory used479,444k (± 0.01%)479,474k (± 0.01%)+30k (+ 0.01%)479,408k479,505kp=0.045 n=6
Parse Time4.26s (± 1.04%)4.28s (± 0.59%)~4.25s4.32sp=0.625 n=6
Bind Time1.54s (± 0.95%)1.54s (± 0.76%)~1.53s1.56sp=1.000 n=6
Check Time22.48s (± 0.24%)22.49s (± 0.37%)~22.37s22.60sp=0.810 n=6
Emit Time0.00s0.00s~~~p=1.000 n=6
Total Time28.28s (± 0.29%)28.31s (± 0.33%)~28.18s28.46sp=0.521 n=6
System info unknown
Hosts
  • node (v18.15.0, x64)
Scenarios
  • Compiler-Unions - node (v18.15.0, x64)
  • angular-1 - node (v18.15.0, x64)
  • mui-docs - node (v18.15.0, x64)
  • self-build-src - node (v18.15.0, x64)
  • self-build-src-public-api - node (v18.15.0, x64)
  • self-compiler - node (v18.15.0, x64)
  • ts-pre-modules - node (v18.15.0, x64)
  • vscode - node (v18.15.0, x64)
  • webpack - node (v18.15.0, x64)
  • xstate-main - node (v18.15.0, x64)
BenchmarkNameIterations
Currentpr6
Baselinebaseline6

Developer Information:

Download Benchmarks

@typescript-bot

Copy link
Copy Markdown
Contributor

Jake Bailey (@jakebailey) Here are the results of running the top 400 repos with tsc comparing main and refs/pull/59709/merge:

Everything looks good!

@jakebailey

Copy link
Copy Markdown
Member

TypeScript Bot (@typescript-bot) pack this

@typescript-bot

TypeScript Bot (typescript-bot) commented Aug 22, 2024

Copy link
Copy Markdown
Contributor

Starting jobs; this comment will be updated as builds start and complete.

CommandStatusResults
pack this✅ Started✅ Results

@typescript-bot

TypeScript Bot (typescript-bot) commented Aug 22, 2024

Copy link
Copy Markdown
Contributor

Hey Jake Bailey (@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/163412/artifacts?artifactName=tgz&fileId=FC442BACFFC913E8502821A0DD902D0764AF8ADC5017DFEDA553B3332B29087102&fileName=/typescript-5.7.0-insiders.20240822.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/pr-build@5.7.0-pr-59709-8".;

@DanielRosenwasser

Copy link
Copy Markdown
Member

TypeScript Bot (@typescript-bot) test top999

@typescript-bot

TypeScript Bot (typescript-bot) commented Aug 22, 2024

Copy link
Copy Markdown
Contributor

Starting jobs; this comment will be updated as builds start and complete.

CommandStatusResults
test top999✅ Started✅ Results

@typescript-bot

Copy link
Copy Markdown
Contributor

Daniel Rosenwasser (@DanielRosenwasser) Here are the results of running the top 999 repos with tsc comparing main and refs/pull/59709/merge:

Everything looks good!

@typescript-bot

Copy link
Copy Markdown
Contributor

With 6.0 out as the final release vehicle for this codebase, we're closing all PRs that don't fit the merge criteria for post-6.0 patches. If you think this was a mistake and this PR fits the post-6.0 patch criteria, please post to the 6.0 iteration issue with details (specifically, which PR and which patch criteria it satisfies).

Next steps for PRs:

  • For crash bugfixes or language service improvements, PRs are currently accepted at the typescript-go repo
  • Changes to type system behavior should wait until after 7.0, at which point mainline TypeScript development will resume in this repository with the Go codebase
  • Library file updates (lib.d.ts etc) continue to live in this repo or the DOM Generator repo as appropriate

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

For Backlog BugPRs that fix a backlog bug

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5.6 regression: Incorrect param type inference for type with all optional props

5 participants

@Andarist@jakebailey@typescript-bot@DanielRosenwasser@RyanCavanaugh