GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Limit type argument inference from binding patterns by andrewbranch · Pull Request #49086 · microsoft/TypeScript

Conversation

@andrewbranch

@andrewbranch Andrew Branch (andrewbranch) commented May 12, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Second attempt of #45719 (and #46009)

Fixes #43605
Fixes #45074
Fixes #45663

In the past, when we made inferences to type arguments in return types, we did two things with those inferences: (1) put them in the primary inference context with a low priority, and (2) put them in a secondary inference context that gets used solely for instantiating contextual types of arguments in the same call expression. The consequence of (1) is easily observable when no other inference sources are present to overwrite the low-priority return type inference made from a binding pattern:

declare function f<T>(): T;
const { foo } = f(); // T: { foo: any }

#45719 fixed this by preventing return type inferences from binding patterns altogether, i.e., it stopped both (1) and (2) above. This turned out to be a problem, because (2) produces very desirable behavior with array binding patterns:

declare function f<T>(cb: () => T): T;
const [e1, e2, e3] = f(() => [1, "hi", true]); T: [number, string, boolean]

The binding pattern provides a tuple contextual type for the array literal in the return expression, allowing e1, e2, and e3 to have separate types rather than all having string | number | boolean. To preserve this desirable behavior, I tried #46009 which only prevented inferences from object binding patterns. However, it was later discovered that this still wasn’t a complete fix, because it still allowed:

declare function f<T>(): T;
const [e1, e2, e3] = f(); // T: [any, any, any]

I ended up reverting all these attempts to reevaluate, with the conclusion

While it may be true that object binding patterns never contribute anything useful to type argument inference, it is not true that array binding patterns always contribute usefully to type argument inference. What we really want is for binding patterns to contribute contextual typing information to other inference sources, while never being allowed to stand on their own as inference sources.

So that’s what this PR does. Rather than make any distinctions between object and array binding patterns, I make a distinction between inference passes into separate contexts (1) and (2) above. If a contextual type for the return type came from a binding pattern, we make no inferences into (1), but we always make inferences into (2) so that other arguments can benefit from that contextual type (primarily for the array → tuple case—it does happen in other cases but I haven’t found any that are useful).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Beraliv reacted with thumbs up emoji filipesabella and VladSez reacted with hooray emoji SlurpTheo reacted with eyes emoji
All reactions
  • 👍 1 reaction
  • 🎉 2 reactions
  • 👀 1 reaction
Andrew Branch (andrewbranch) added 6 commits May 11, 2022 16:40
@andrewbranch
WIP
dc325fe
@andrewbranch
Don’t widen literals based on bogus contextual type instantiation
b00e6b5
@andrewbranch
Split tests
73e58ea
@andrewbranch
Skip unnecessary inference pass
65f453f
@andrewbranch
Accept test baselines
c923baf
@andrewbranch
Fix stray edit
7e8c56c
@typescript-bot TypeScript Bot (typescript-bot) assigned Andrew Branch (andrewbranch) May 12, 2022
@typescript-bot TypeScript Bot (typescript-bot) added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels May 12, 2022
andrewbranch
Andrew Branch (andrewbranch) commented May 12, 2022
View reviewed changes
Comment thread src/compiler/checker.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.
@andrewbranch
Andrew Branch (andrewbranch) requested review from Anders Hejlsberg (ahejlsberg) and Wesley Wigham (weswigham) May 12, 2022 18:53
@andrewbranch Andrew Branch (andrewbranch) added the Breaking Change Would introduce errors in existing code label May 12, 2022
@andrewbranch

Andrew Branch (andrewbranch) commented May 12, 2022

Copy link
Copy Markdown
Member Author

TypeScript Bot (@typescript-bot) test this
TypeScript Bot (@typescript-bot) user test this inline
TypeScript Bot (@typescript-bot) run dt
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@typescript-bot

TypeScript Bot (typescript-bot) commented May 12, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Heya Andrew Branch (@andrewbranch), I've started to run the extended test suite on this PR at 7e8c56c. You can monitor the build here.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@typescript-bot

TypeScript Bot (typescript-bot) commented May 12, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Heya Andrew Branch (@andrewbranch), I've started to run the diff-based community code test suite on this PR at 7e8c56c. You can monitor the build here.

Update: The results are in!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@typescript-bot

TypeScript Bot (typescript-bot) commented May 12, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Heya Andrew Branch (@andrewbranch), I've started to run the parallelized Definitely Typed test suite on this PR at 7e8c56c. You can monitor the build here.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@andrewbranch

Andrew Branch (andrewbranch) commented May 12, 2022

Copy link
Copy Markdown
Member Author

TypeScript Bot (@typescript-bot) perf test this
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@typescript-bot

TypeScript Bot (typescript-bot) commented May 12, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Heya Andrew Branch (@andrewbranch), I've started to run the perf test suite on this PR at 7e8c56c. You can monitor the build here.

Update: The results are in!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@typescript-bot

TypeScript Bot (typescript-bot) commented May 12, 2022

Copy link
Copy Markdown
Contributor

Andrew Branch (@andrewbranch)
Great news! no new errors were found between main..refs/pull/49086/merge
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@typescript-bot

TypeScript Bot (typescript-bot) commented May 12, 2022

Copy link
Copy Markdown
Contributor

Andrew Branch (@andrewbranch)
The results of the perf run you requested are in!

Here they are:

Comparison Report - main..49086

Metric main 49086 Delta Best Worst
Angular - node (v10.16.3, x64)
Memory used 357,601k (± 0.02%) 357,617k (± 0.02%) +16k (+ 0.00%) 357,536k 357,775k
Parse Time 2.07s (± 0.50%) 2.07s (± 0.65%) -0.00s (- 0.14%) 2.04s 2.10s
Bind Time 0.86s (± 0.52%) 0.86s (± 0.78%) -0.00s (- 0.12%) 0.84s 0.87s
Check Time 5.84s (± 0.36%) 5.83s (± 0.54%) -0.01s (- 0.21%) 5.79s 5.94s
Emit Time 6.03s (± 0.60%) 5.99s (± 0.39%) -0.03s (- 0.56%) 5.94s 6.03s
Total Time 14.80s (± 0.36%) 14.75s (± 0.25%) -0.05s (- 0.34%) 14.68s 14.84s
Compiler-Unions - node (v10.16.3, x64)
Memory used 206,010k (± 0.04%) 205,961k (± 0.04%) -49k (- 0.02%) 205,773k 206,118k
Parse Time 0.83s (± 0.93%) 0.83s (± 0.91%) +0.00s (+ 0.36%) 0.81s 0.85s
Bind Time 0.52s (± 1.44%) 0.51s (± 1.16%) -0.00s (- 0.39%) 0.50s 0.53s
Check Time 7.91s (± 0.39%) 7.89s (± 0.36%) -0.03s (- 0.34%) 7.82s 7.94s
Emit Time 2.49s (± 1.05%) 2.51s (± 0.73%) +0.01s (+ 0.52%) 2.46s 2.54s
Total Time 11.75s (± 0.44%) 11.74s (± 0.30%) -0.01s (- 0.11%) 11.67s 11.83s
Monaco - node (v10.16.3, x64)
Memory used 343,818k (± 0.02%) 343,808k (± 0.02%) -9k (- 0.00%) 343,592k 343,923k
Parse Time 1.59s (± 0.35%) 1.59s (± 0.39%) +0.00s (+ 0.06%) 1.58s 1.60s
Bind Time 0.75s (± 0.66%) 0.76s (± 0.45%) +0.00s (+ 0.26%) 0.75s 0.76s
Check Time 5.80s (± 0.37%) 5.79s (± 0.35%) -0.00s (- 0.07%) 5.73s 5.84s
Emit Time 3.22s (± 0.40%) 3.23s (± 0.46%) +0.01s (+ 0.44%) 3.21s 3.28s
Total Time 11.36s (± 0.22%) 11.37s (± 0.17%) +0.01s (+ 0.08%) 11.33s 11.41s
TFS - node (v10.16.3, x64)
Memory used 305,440k (± 0.02%) 305,423k (± 0.02%) -17k (- 0.01%) 305,318k 305,536k
Parse Time 1.28s (± 0.39%) 1.28s (± 0.53%) -0.00s (- 0.08%) 1.27s 1.30s
Bind Time 0.71s (± 0.81%) 0.71s (± 0.69%) +0.00s (+ 0.14%) 0.70s 0.72s
Check Time 5.27s (± 0.49%) 5.25s (± 0.74%) -0.02s (- 0.40%) 5.17s 5.34s
Emit Time 3.42s (± 0.99%) 3.44s (± 1.70%) +0.02s (+ 0.55%) 3.33s 3.60s
Total Time 10.69s (± 0.40%) 10.69s (± 0.82%) -0.00s (- 0.02%) 10.51s 10.94s
material-ui - node (v10.16.3, x64)
Memory used 468,498k (± 0.01%) 467,205k (± 0.01%) -1,293k (- 0.28%) 467,115k 467,303k
Parse Time 1.84s (± 0.49%) 1.82s (± 0.26%) -0.02s (- 0.87%) 1.81s 1.83s
Bind Time 0.67s (± 1.44%) 0.68s (± 0.74%) +0.00s (+ 0.60%) 0.67s 0.69s
Check Time 14.32s (± 0.50%) 14.29s (± 0.32%) -0.04s (- 0.26%) 14.19s 14.40s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 16.83s (± 0.39%) 16.78s (± 0.28%) -0.05s (- 0.30%) 16.70s 16.90s
xstate - node (v10.16.3, x64)
Memory used 571,872k (± 0.01%) 578,517k (± 1.73%) +6,645k (+ 1.16%) 571,607k 605,461k
Parse Time 2.59s (± 0.24%) 2.59s (± 0.29%) +0.00s (+ 0.12%) 2.58s 2.61s
Bind Time 1.00s (± 0.52%) 1.01s (± 0.47%) +0.01s (+ 0.80%) 1.00s 1.02s
Check Time 1.53s (± 0.48%) 1.52s (± 0.75%) -0.01s (- 0.39%) 1.50s 1.55s
Emit Time 0.07s (± 3.14%) 0.07s (± 3.14%) 0.00s ( 0.00%) 0.07s 0.08s
Total Time 5.19s (± 0.28%) 5.19s (± 0.16%) +0.00s (+ 0.06%) 5.17s 5.21s
Angular - node (v12.1.0, x64)
Memory used 335,359k (± 0.02%) 335,383k (± 0.01%) +23k (+ 0.01%) 335,314k 335,552k
Parse Time 2.07s (± 0.91%) 2.06s (± 0.61%) -0.00s (- 0.24%) 2.04s 2.09s
Bind Time 0.82s (± 0.83%) 0.82s (± 0.75%) -0.00s (- 0.48%) 0.81s 0.83s
Check Time 5.62s (± 0.53%) 5.63s (± 0.32%) +0.00s (+ 0.07%) 5.59s 5.66s
Emit Time 6.25s (± 0.92%) 6.23s (± 0.73%) -0.02s (- 0.29%) 6.17s 6.36s
Total Time 14.77s (± 0.41%) 14.74s (± 0.32%) -0.03s (- 0.19%) 14.65s 14.87s
Compiler-Unions - node (v12.1.0, x64)
Memory used 193,600k (± 0.09%) 193,394k (± 0.11%) -206k (- 0.11%) 192,897k 193,736k
Parse Time 0.83s (± 1.25%) 0.83s (± 1.02%) -0.00s (- 0.12%) 0.82s 0.85s
Bind Time 0.53s (± 0.56%) 0.53s (± 1.22%) +0.00s (+ 0.76%) 0.52s 0.55s
Check Time 7.40s (± 0.58%) 7.42s (± 0.38%) +0.02s (+ 0.27%) 7.36s 7.47s
Emit Time 2.52s (± 0.87%) 2.53s (± 1.29%) +0.01s (+ 0.56%) 2.47s 2.61s
Total Time 11.28s (± 0.48%) 11.31s (± 0.36%) +0.03s (+ 0.30%) 11.23s 11.43s
Monaco - node (v12.1.0, x64)
Memory used 326,896k (± 0.01%) 326,818k (± 0.02%) -78k (- 0.02%) 326,694k 326,960k
Parse Time 1.55s (± 0.54%) 1.54s (± 0.57%) -0.01s (- 0.39%) 1.53s 1.56s
Bind Time 0.74s (± 0.91%) 0.75s (± 1.02%) +0.01s (+ 1.22%) 0.73s 0.77s
Check Time 5.66s (± 0.55%) 5.64s (± 0.61%) -0.02s (- 0.27%) 5.57s 5.71s
Emit Time 3.26s (± 0.54%) 3.25s (± 0.50%) -0.01s (- 0.40%) 3.21s 3.28s
Total Time 11.21s (± 0.35%) 11.17s (± 0.41%) -0.03s (- 0.29%) 11.10s 11.29s
TFS - node (v12.1.0, x64)
Memory used 290,144k (± 0.02%) 290,136k (± 0.02%) -9k (- 0.00%) 290,018k 290,231k
Parse Time 1.30s (± 1.05%) 1.30s (± 0.78%) -0.00s (- 0.15%) 1.28s 1.32s
Bind Time 0.72s (± 0.62%) 0.72s (± 1.23%) +0.00s (+ 0.28%) 0.71s 0.75s
Check Time 5.22s (± 0.54%) 5.21s (± 0.58%) -0.01s (- 0.23%) 5.15s 5.31s
Emit Time 3.47s (± 1.05%) 3.51s (± 1.08%) +0.04s (+ 1.07%) 3.44s 3.59s
Total Time 10.71s (± 0.23%) 10.74s (± 0.57%) +0.02s (+ 0.21%) 10.61s 10.92s
material-ui - node (v12.1.0, x64)
Memory used 447,475k (± 0.01%) 446,053k (± 0.07%) -1,422k (- 0.32%) 444,889k 446,253k
Parse Time 1.83s (± 0.69%) 1.83s (± 0.42%) 0.00s ( 0.00%) 1.81s 1.84s
Bind Time 0.65s (± 0.52%) 0.65s (± 0.80%) -0.00s (- 0.31%) 0.64s 0.66s
Check Time 12.89s (± 0.38%) 12.92s (± 0.64%) +0.03s (+ 0.21%) 12.77s 13.15s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 15.37s (± 0.39%) 15.40s (± 0.54%) +0.03s (+ 0.17%) 15.24s 15.61s
xstate - node (v12.1.0, x64)
Memory used 537,492k (± 0.02%) 540,831k (± 1.37%) +3,340k (+ 0.62%) 537,294k 570,725k
Parse Time 2.54s (± 0.42%) 2.53s (± 0.39%) -0.01s (- 0.35%) 2.51s 2.55s
Bind Time 1.03s (± 1.09%) 1.02s (± 0.65%) -0.01s (- 0.78%) 1.01s 1.03s
Check Time 1.48s (± 0.82%) 1.47s (± 0.52%) -0.01s (- 0.75%) 1.45s 1.49s
Emit Time 0.07s (± 0.00%) 0.07s (± 0.00%) 0.00s ( 0.00%) 0.07s 0.07s
Total Time 5.12s (± 0.29%) 5.08s (± 0.25%) -0.03s (- 0.61%) 5.06s 5.12s
Angular - node (v14.15.1, x64)
Memory used 333,572k (± 0.01%) 333,660k (± 0.01%) +87k (+ 0.03%) 333,585k 333,734k
Parse Time 2.04s (± 0.36%) 2.03s (± 0.46%) -0.01s (- 0.34%) 2.02s 2.06s
Bind Time 0.87s (± 0.42%) 0.87s (± 0.26%) +0.00s (+ 0.58%) 0.87s 0.88s
Check Time 5.63s (± 0.41%) 5.62s (± 0.59%) -0.00s (- 0.05%) 5.57s 5.71s
Emit Time 6.27s (± 0.39%) 6.29s (± 0.80%) +0.01s (+ 0.18%) 6.19s 6.41s
Total Time 14.81s (± 0.21%) 14.81s (± 0.51%) +0.00s (+ 0.01%) 14.65s 15.03s
Compiler-Unions - node (v14.15.1, x64)
Memory used 192,259k (± 0.02%) 192,167k (± 0.07%) -91k (- 0.05%) 191,598k 192,288k
Parse Time 0.85s (± 0.56%) 0.85s (± 0.90%) -0.00s (- 0.12%) 0.83s 0.87s
Bind Time 0.56s (± 0.80%) 0.56s (± 0.53%) 0.00s ( 0.00%) 0.55s 0.56s
Check Time 7.51s (± 0.54%) 7.54s (± 0.64%) +0.03s (+ 0.37%) 7.46s 7.71s
Emit Time 2.51s (± 0.64%) 2.51s (± 1.13%) +0.01s (+ 0.32%) 2.47s 2.59s
Total Time 11.42s (± 0.42%) 11.46s (± 0.44%) +0.04s (+ 0.32%) 11.35s 11.60s
Monaco - node (v14.15.1, x64)
Memory used 325,602k (± 0.01%) 325,642k (± 0.01%) +40k (+ 0.01%) 325,587k 325,709k
Parse Time 1.57s (± 0.82%) 1.56s (± 0.36%) -0.01s (- 0.76%) 1.55s 1.57s
Bind Time 0.77s (± 0.43%) 0.77s (± 0.75%) +0.00s (+ 0.39%) 0.76s 0.79s
Check Time 5.53s (± 0.35%) 5.53s (± 0.50%) -0.00s (- 0.04%) 5.47s 5.60s
Emit Time 3.34s (± 0.82%) 3.33s (± 0.66%) -0.01s (- 0.18%) 3.30s 3.38s
Total Time 11.22s (± 0.37%) 11.20s (± 0.37%) -0.01s (- 0.12%) 11.14s 11.30s
TFS - node (v14.15.1, x64)
Memory used 289,096k (± 0.01%) 289,114k (± 0.01%) +19k (+ 0.01%) 289,061k 289,153k
Parse Time 1.36s (± 1.42%) 1.36s (± 1.49%) +0.01s (+ 0.52%) 1.34s 1.44s
Bind Time 0.72s (± 0.50%) 0.72s (± 0.77%) -0.01s (- 0.83%) 0.71s 0.73s
Check Time 5.22s (± 0.38%) 5.20s (± 0.55%) -0.01s (- 0.29%) 5.15s 5.28s
Emit Time 3.48s (± 1.72%) 3.48s (± 1.83%) +0.00s (+ 0.14%) 3.39s 3.60s
Total Time 10.78s (± 0.59%) 10.77s (± 0.58%) -0.00s (- 0.05%) 10.65s 10.92s
material-ui - node (v14.15.1, x64)
Memory used 445,746k (± 0.01%) 444,446k (± 0.00%) -1,300k (- 0.29%) 444,413k 444,466k
Parse Time 1.88s (± 0.62%) 1.86s (± 0.51%) -0.01s (- 0.53%) 1.85s 1.89s
Bind Time 0.69s (± 0.69%) 0.69s (± 0.43%) -0.00s (- 0.14%) 0.69s 0.70s
Check Time 13.07s (± 0.96%) 12.99s (± 0.52%) -0.08s (- 0.60%) 12.87s 13.16s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 15.64s (± 0.80%) 15.55s (± 0.44%) -0.09s (- 0.58%) 15.42s 15.72s
xstate - node (v14.15.1, x64)
Memory used 535,240k (± 0.00%) 535,129k (± 0.00%) -111k (- 0.02%) 535,086k 535,171k
Parse Time 2.58s (± 0.60%) 2.57s (± 0.28%) -0.01s (- 0.50%) 2.56s 2.59s
Bind Time 1.14s (± 1.01%) 1.15s (± 0.86%) +0.01s (+ 0.70%) 1.13s 1.17s
Check Time 1.52s (± 0.22%) 1.51s (± 0.33%) -0.00s (- 0.20%) 1.50s 1.52s
Emit Time 0.07s (± 0.00%) 0.07s (± 0.00%) 0.00s ( 0.00%) 0.07s 0.07s
Total Time 5.31s (± 0.28%) 5.31s (± 0.22%) -0.01s (- 0.15%) 5.28s 5.33s
System
Machine Namets-ci-ubuntu
Platformlinux 4.4.0-210-generic
Architecturex64
Available Memory16 GB
Available Memory2 GB
CPUs4 × Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz
Hosts
  • node (v10.16.3, x64)
  • node (v12.1.0, x64)
  • node (v14.15.1, x64)
Scenarios
  • Angular - node (v10.16.3, x64)
  • Angular - node (v12.1.0, x64)
  • Angular - node (v14.15.1, x64)
  • Compiler-Unions - node (v10.16.3, x64)
  • Compiler-Unions - node (v12.1.0, x64)
  • Compiler-Unions - node (v14.15.1, x64)
  • Monaco - node (v10.16.3, x64)
  • Monaco - node (v12.1.0, x64)
  • Monaco - node (v14.15.1, x64)
  • TFS - node (v10.16.3, x64)
  • TFS - node (v12.1.0, x64)
  • TFS - node (v14.15.1, x64)
  • material-ui - node (v10.16.3, x64)
  • material-ui - node (v12.1.0, x64)
  • material-ui - node (v14.15.1, x64)
  • xstate - node (v10.16.3, x64)
  • xstate - node (v12.1.0, x64)
  • xstate - node (v14.15.1, x64)
Benchmark Name Iterations
Current 49086 10
Baseline main 10

Developer Information:

Download Benchmark
andrewbranch reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@andrewbranch

Andrew Branch (andrewbranch) commented May 12, 2022

Copy link
Copy Markdown
Member Author

TypeScript Bot (@typescript-bot) pack this
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@typescript-bot

TypeScript Bot (typescript-bot) commented May 12, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Heya Andrew Branch (@andrewbranch), I've started to run the tarball bundle task on this PR at 7e8c56c. You can monitor the build here.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@typescript-bot

TypeScript Bot (typescript-bot) commented May 12, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Hey Andrew Branch (@andrewbranch), 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/126203/artifacts?artifactName=tgz&fileId=3AFD572E38CB68C0D2D6AB11F11C1A08A1088683BCD2247B5A092DD8CBA6801002&fileName=/typescript-4.8.0-insiders.20220512.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]".;
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
weswigham
Wesley Wigham (weswigham) reviewed May 13, 2022
View reviewed changes
Comment thread tests/cases/compiler/bindingPatternContextualTypeDoesNotCauseWidening.ts
@@ -0,0 +1,3 @@
declare function pick<O, T extends keyof O>(keys: T[], obj?: O): Pick<O, T>;
const _ = pick(['b'], { a: 'a', b: 'b' }); // T: "b"
const { } = pick(['b'], { a: 'a', b: 'b' }); // T: "b" | "a" ??? (before fix)

@weswigham Wesley Wigham (weswigham) May 13, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

Since this repro in theory involves completions, should it have a fourslash variant?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
weswigham
Wesley Wigham (weswigham) approved these changes May 13, 2022
View reviewed changes

@weswigham Wesley Wigham (weswigham) left a comment

Copy link
Copy Markdown
Member

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.

I'm curious if other callers of getWidenedLiteralLikeTypeForContextualType need to be passing node (checkPropertyAssignment? AFAIK that maybe handles the assignment pattern case ({x} = f())...?), and some fourslash examples for the completions-relevant examples (to verify those still work using the LS overrides for signature resolution despite these changes), but the core changes themselves look good to me.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@andrewbranch

Andrew Branch (andrewbranch) commented May 13, 2022

Copy link
Copy Markdown
Member Author

An example mentioned in the design meeting is broken by this:

declare function id<T>(x: T): T;
const { f = (x: string) => x.length } = id({ f: x => x.charAt });

When getting the type of the function expression inside the call to draw inferences from, we have the contextual type from the binding pattern available, but we don’t use it because we’re skipping context-sensitive functions and return anyFunctionType from checkFunctionExpressionOrObjectLiteralMethod 😕. Any advice there?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@weswigham

Wesley Wigham (weswigham) commented May 13, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Usually that just means we'll pick up the better type in the next inference pass - the SkipContextSensitive pass implies there'll be a context-sensitive second pass that picks up the context-sensitive type.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@andrewbranch

Andrew Branch (andrewbranch) commented May 13, 2022

Copy link
Copy Markdown
Member Author

Doh, of course. That was a red herring. Still debugging.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@andrewbranch

Andrew Branch (andrewbranch) commented May 13, 2022

Copy link
Copy Markdown
Member Author

Ah, I see what the problem is. I mistook how the combineTypeMappers I added in instantiateContextualType would handle the first mapper having no inference candidates—I intended it to delegate to the second, but instead the first mapper produces the constraint/default/unknown and we just go with that. I think adding a cloneInferredPartOfContext would fix this, but I can probably accomplish the same with less object cloning.
weswigham reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@andrewbranch
Fix type mapper combination
7dc1952
@andrewbranch

Andrew Branch (andrewbranch) commented May 16, 2022

Copy link
Copy Markdown
Member Author

Hm, ok. I’m actually still having trouble combining these type mappers in a way that works for every case, and I’m not sure if it’s possible.

The issue is that for instantiating contextual signatures, we actually do use those low-priority return type inferences as well as other inferences made so far. So when I completely separated those inferences, I attempted recombine them from the two contexts, but I don’t think that’s going to work.

An earlier iteration of this PR, rather than removing binding pattern return type inferences from the primary context, flagged them with another priority flag and then avoided returning those from getInferredType. Maybe I need to go back to something like that 😕
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@andrewbranch

Andrew Branch (andrewbranch) commented May 16, 2022

Copy link
Copy Markdown
Member Author

Anders Hejlsberg (@ahejlsberg) do you want to take a look at what I have so far before I go down a rabbit hole trying to solve ☝️? Maybe you’ll have an idea for that.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) mentioned this pull request May 16, 2022
Design Meeting Notes, 5/13/2022 #49137
Closed
Andrew Branch (andrewbranch) added 3 commits May 18, 2022 10:12
@andrewbranch
Revert src/ of 7dc1952
96f2f52
@andrewbranch
Make empty binding pattern provide no contextual type
024bf7a
@andrewbranch
Add missed baseline
4537a79
@andrewbranch

Andrew Branch (andrewbranch) commented May 18, 2022

Copy link
Copy Markdown
Member Author

Anders Hejlsberg (@ahejlsberg) the changes we discussed yesterday are up. That approach actually fixed this test case:

declare function id<T>(x: T): T;
const { f = (x: string) => x.length } = id({ f: x => x.charAt });

Seems like the best outcome we could have hoped for!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@sandersn

Nathan Shively-Sanders (sandersn) commented May 23, 2022

Copy link
Copy Markdown
Member

Is this still waiting on a review or is it ready to merge?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
weswigham
Wesley Wigham (weswigham) approved these changes May 23, 2022
View reviewed changes
@andrewbranch

Andrew Branch (andrewbranch) commented May 23, 2022

Copy link
Copy Markdown
Member Author

I think Anders Hejlsberg (@ahejlsberg) may have wanted a final look at it.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ahejlsberg
Anders Hejlsberg (ahejlsberg) reviewed May 24, 2022
View reviewed changes
Comment thread src/compiler/checker.ts
}
return instantiateInstantiableTypes(contextualType, inferenceContext.nonFixingMapper);
}
if (inferenceContext?.returnMapper) {

@ahejlsberg Anders Hejlsberg (ahejlsberg) May 24, 2022

Copy link
Copy Markdown
Member

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 if statement now runs even when there are no inference candidates. Is that intended?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@andrewbranch Andrew Branch (andrewbranch) May 24, 2022

Copy link
Copy Markdown
Member Author

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, because the returnMapper pulls from a different inference context that may have candidates not present in inferenceContext. (Also, returnMapper is guaranteed to have candidates if it exists.)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ahejlsberg reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
ahejlsberg
Anders Hejlsberg (ahejlsberg) approved these changes May 24, 2022
View reviewed changes
@andrewbranch
Andrew Branch (andrewbranch) merged commit a2b785b into microsoft:main May 24, 2022
@andrewbranch
Andrew Branch (andrewbranch) deleted the bug/binding-pattern-inference branch May 24, 2022 16:05
@ahejlsberg Anders Hejlsberg (ahejlsberg) mentioned this pull request Sep 17, 2022
Literal strings with generics are inconsistant when strictNullChecks is false #50787
Open
@Andarist Mateusz Burzyński (Andarist) mentioned this pull request Jan 7, 2023
Improvement/Bug in contextual inference where the call-site is generic. #45035
Open
5 tasks done
@Andarist Mateusz Burzyński (Andarist) mentioned this pull request Feb 6, 2023
Do not infer to mapped types from implied index signatures created by rest bindings #52632
Merged
@microsoft Microsoft (microsoft) locked as resolved and limited conversation to collaborators Oct 22, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@weswigham Wesley Wigham (weswigham) weswigham approved these changes
@ahejlsberg Anders Hejlsberg (ahejlsberg) ahejlsberg approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@andrewbranch Andrew Branch (andrewbranch)

Labels

Author: Team Breaking Change Would introduce errors in existing code For Milestone Bug PRs that fix a bug with a specific milestone

Projects

PR Backlog
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

When using generic pick, object deconstruction does not provide valid keys to select Generic inference from LHS of assignment when destructuring can lead to unexpected anys Presence of a binding pattern changes inference for callback function parameters

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@andrewbranch @typescript-bot @weswigham @sandersn @ahejlsberg
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.