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

Fix inference of type arguments when source is 'awaited' #37540

Closed
wants to merge 4 commits into from

Conversation

rbuckton
Copy link
Member

This changes inference behavior when inferring a type argument that is awaited:

function f<T>(a: T): Promise<awaited T> {
    // will now infer `Promise<T>` instead of `Promise<awaited T>`
    return new Promise(r => r(a));
}

Fixes #37526

@ahejlsberg
Copy link
Member

ahejlsberg commented Mar 24, 2020

Hmm, I'm not sure I follow the rationale here. Just because T is used in an awaited T type constructor somewhere doesn't mean that we want to change inference behavior everywhere for T.

Can you provide a bit more context on what you're trying to accomplish and why?

@ahejlsberg
Copy link
Member

Is a correct summary that you intend for inference from awaited S to a type parameter T, where T only occurs in awaited T type constructors, to infer S instead of awaited S?

@rbuckton
Copy link
Member Author

At the end of the day, awaited's main purpose is to give you the right type for the value parameter of the onfulfilled callback of the then method of a Promise (or Promise/A+-compatible promise-like implementation), as that is the only place where a value with that type is actually produced.

Consider this minimal definition for something promise-like:

interface Promise<T> {
  then(onfulfilled: (value: awaited T) => any): any
                     ~~~~~
}

The type of value will be the same for all of the following instantiations:

promise value
Promise<T> awaited T
Promise<Promise<T>> awaited T
Promise<Promise<Promise<T>>> awaited T
Promise<awaited T>  awaited T
Promise<awaited awaited T>  awaited T
Promise<awaited Promise<T>> awaited T
Promise<awaited Promise<awaited T>> awaited T
Promise<T | awaited T> awaited T
Promise<T | Promise<T>> awaited T
Promise<awaited T | Promise<T>> awaited T

In addition, awaited shares some of same difficulties as conditional types when it comes to working with the type in expression-space. This is apparent if you look at #37526, since there's no way to resolve a promise with a T when it is instantiated with an awaited T, despite the fact that the resulting value will have the same type. Since both awaited T and T will resolve to awaited T, resolving it with a T should be permitted. However, we don't want to make T assignable to awaited T (or vice versa), so generally we want to unwrap any inference to awaited T to become just T as that's more useful.

It is possible this proposed heuristic may be too loose. In reality we may only want to perform this inference when the awaited T is only referenced as the type of value, above. If that is the case I would welcome suggestions on a better way to enforce that.

@rbuckton
Copy link
Member Author

Is a correct summary that you intend for inference from awaited S to a type parameter T, where T only occurs in awaited T type constructors, to infer S instead of awaited S?

I believe that would be the case, yes. At least, in cases where the T is produced as a value to expression-space. For example, the finally method returns Promise<T> which would be a reference to T that is not awaited. In addition, the TResult1 type parameter for then uses T as a default without necessarily wrapping it in an awaited.

@ahejlsberg
Copy link
Member

I did a little experiment where I changed inference between references to the same generic type to always proceed structurally if any type parameters have variances marked unreliable (as is the case with T in Promise<T>). This causes the original example to just work as expected. What's happening in the example is that the new Promise(r => r(a)) is contextually typed by Promise<awaited T>. As we proceed with structural inference, we end up with an awaited (awaited T) for the value parameter in the contextual type, which reduces to awaited T, and we then make the correct inference T.

@rbuckton
Copy link
Member Author

Do we want inference to proceed structurally for all "unreliable" variances or just cases where awaited is used? I'm happy to close this PR if that turns out to be a better solution.

@ahejlsberg
Copy link
Member

@rbuckton Just pushed the changes we talked about. We now perform structural type inference for any generic type with a type parameter that occurs in an awaited T type constructor.

Only change in baselines is that we flatten inferences from Promise<Promise<number>> which actually seems desirable.

@rbuckton
Copy link
Member Author

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 24, 2020

Heya @rbuckton, I've started to run the parallelized Definitely Typed test suite on this PR at 70f92ff. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 24, 2020

Heya @rbuckton, I've started to run the parallelized community code test suite on this PR at 70f92ff. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 24, 2020

Heya @rbuckton, I've started to run the perf test suite on this PR at 70f92ff. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 24, 2020

Heya @rbuckton, I've started to run the extended test suite on this PR at 70f92ff. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master.

@typescript-bot
Copy link
Collaborator

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

Here they are:

Comparison Report - master..37540

Metric master 37540 Delta Best Worst
Angular - node (v10.16.3, x64)
Memory used 328,158k (± 0.02%) 328,414k (± 0.01%) +257k (+ 0.08%) 328,291k 328,470k
Parse Time 1.62s (± 0.37%) 1.62s (± 0.76%) -0.00s (- 0.12%) 1.60s 1.66s
Bind Time 0.89s (± 0.53%) 0.89s (± 0.92%) -0.00s (- 0.34%) 0.87s 0.90s
Check Time 4.76s (± 0.38%) 4.80s (± 0.40%) +0.05s (+ 1.03%) 4.75s 4.85s
Emit Time 5.32s (± 0.45%) 5.32s (± 0.79%) +0.00s (+ 0.08%) 5.25s 5.45s
Total Time 12.59s (± 0.23%) 12.63s (± 0.53%) +0.05s (+ 0.36%) 12.54s 12.81s
Monaco - node (v10.16.3, x64)
Memory used 327,136k (± 0.02%) 327,094k (± 0.01%) -43k (- 0.01%) 327,042k 327,168k
Parse Time 1.26s (± 0.49%) 1.26s (± 0.40%) -0.00s (- 0.32%) 1.25s 1.27s
Bind Time 0.77s (± 0.80%) 0.78s (± 0.48%) +0.00s (+ 0.52%) 0.77s 0.78s
Check Time 4.75s (± 0.40%) 4.73s (± 0.25%) -0.02s (- 0.36%) 4.71s 4.76s
Emit Time 2.94s (± 0.45%) 2.91s (± 0.76%) -0.02s (- 0.85%) 2.85s 2.96s
Total Time 9.72s (± 0.16%) 9.68s (± 0.23%) -0.04s (- 0.44%) 9.62s 9.72s
TFS - node (v10.16.3, x64)
Memory used 291,989k (± 0.02%) 291,982k (± 0.02%) -7k (- 0.00%) 291,843k 292,162k
Parse Time 0.95s (± 0.49%) 0.95s (± 0.38%) -0.00s (- 0.42%) 0.94s 0.95s
Bind Time 0.74s (± 0.66%) 0.74s (± 0.75%) -0.00s (- 0.27%) 0.73s 0.75s
Check Time 4.28s (± 0.46%) 4.26s (± 0.38%) -0.02s (- 0.47%) 4.22s 4.30s
Emit Time 3.04s (± 0.60%) 3.05s (± 0.90%) +0.01s (+ 0.43%) 2.97s 3.09s
Total Time 9.01s (± 0.35%) 9.00s (± 0.29%) -0.01s (- 0.12%) 8.91s 9.04s
material-ui - node (v10.16.3, x64)
Memory used 453,294k (± 0.01%) 453,051k (± 0.02%) -242k (- 0.05%) 452,854k 453,206k
Parse Time 1.78s (± 0.52%) 1.78s (± 0.54%) -0.00s (- 0.11%) 1.76s 1.80s
Bind Time 0.69s (± 0.72%) 0.68s (± 0.70%) -0.00s (- 0.44%) 0.67s 0.69s
Check Time 13.68s (± 0.47%) 13.71s (± 0.61%) +0.03s (+ 0.24%) 13.57s 13.91s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 16.14s (± 0.38%) 16.17s (± 0.54%) +0.03s (+ 0.19%) 16.03s 16.39s
Angular - node (v12.1.0, x64)
Memory used 303,839k (± 0.02%) 304,013k (± 0.06%) +174k (+ 0.06%) 303,268k 304,289k
Parse Time 1.57s (± 0.78%) 1.57s (± 0.63%) -0.00s (- 0.13%) 1.55s 1.60s
Bind Time 0.88s (± 0.66%) 0.87s (± 1.08%) -0.01s (- 0.80%) 0.85s 0.89s
Check Time 4.64s (± 0.19%) 4.68s (± 0.83%) +0.03s (+ 0.73%) 4.63s 4.82s
Emit Time 5.45s (± 0.79%) 5.44s (± 0.58%) -0.01s (- 0.17%) 5.38s 5.51s
Total Time 12.54s (± 0.45%) 12.56s (± 0.52%) +0.02s (+ 0.16%) 12.48s 12.79s
Monaco - node (v12.1.0, x64)
Memory used 307,053k (± 0.01%) 307,098k (± 0.02%) +45k (+ 0.01%) 306,941k 307,343k
Parse Time 1.21s (± 0.69%) 1.21s (± 0.55%) +0.00s (+ 0.08%) 1.19s 1.22s
Bind Time 0.74s (± 0.90%) 0.74s (± 0.66%) +0.00s (+ 0.54%) 0.73s 0.75s
Check Time 4.55s (± 0.41%) 4.55s (± 0.48%) -0.00s (- 0.02%) 4.50s 4.60s
Emit Time 2.95s (± 0.76%) 2.96s (± 0.71%) +0.01s (+ 0.31%) 2.90s 3.00s
Total Time 9.45s (± 0.34%) 9.46s (± 0.39%) +0.01s (+ 0.10%) 9.34s 9.51s
TFS - node (v12.1.0, x64)
Memory used 274,251k (± 0.02%) 274,261k (± 0.01%) +9k (+ 0.00%) 274,210k 274,354k
Parse Time 0.93s (± 0.81%) 0.93s (± 0.97%) -0.01s (- 1.07%) 0.91s 0.94s
Bind Time 0.71s (± 0.97%) 0.71s (± 1.35%) +0.00s (+ 0.14%) 0.69s 0.73s
Check Time 4.18s (± 0.42%) 4.18s (± 0.34%) +0.00s (+ 0.10%) 4.15s 4.21s
Emit Time 3.09s (± 0.44%) 3.08s (± 0.74%) -0.01s (- 0.23%) 3.03s 3.12s
Total Time 8.91s (± 0.33%) 8.90s (± 0.26%) -0.01s (- 0.13%) 8.86s 8.96s
material-ui - node (v12.1.0, x64)
Memory used 430,726k (± 0.01%) 430,528k (± 0.01%) -198k (- 0.05%) 430,435k 430,644k
Parse Time 1.75s (± 0.34%) 1.75s (± 0.42%) -0.00s (- 0.23%) 1.73s 1.76s
Bind Time 0.63s (± 0.54%) 0.63s (± 1.05%) +0.00s (+ 0.48%) 0.62s 0.65s
Check Time 12.12s (± 0.56%) 12.15s (± 0.78%) +0.03s (+ 0.26%) 12.01s 12.43s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 14.50s (± 0.50%) 14.53s (± 0.70%) +0.03s (+ 0.21%) 14.37s 14.82s
Angular - node (v8.9.0, x64)
Memory used 323,163k (± 0.02%) 323,531k (± 0.01%) +368k (+ 0.11%) 323,426k 323,629k
Parse Time 2.12s (± 0.45%) 2.11s (± 0.16%) -0.00s (- 0.09%) 2.11s 2.12s
Bind Time 0.92s (± 0.87%) 0.91s (± 0.54%) -0.00s (- 0.44%) 0.91s 0.93s
Check Time 5.48s (± 0.47%) 5.52s (± 0.86%) +0.05s (+ 0.88%) 5.38s 5.63s
Emit Time 6.22s (± 0.62%) 6.23s (± 1.40%) +0.01s (+ 0.10%) 5.98s 6.44s
Total Time 14.73s (± 0.46%) 14.78s (± 0.45%) +0.04s (+ 0.31%) 14.59s 14.90s
Monaco - node (v8.9.0, x64)
Memory used 325,520k (± 0.02%) 325,529k (± 0.01%) +9k (+ 0.00%) 325,462k 325,648k
Parse Time 1.54s (± 0.58%) 1.54s (± 0.34%) -0.00s (- 0.19%) 1.53s 1.55s
Bind Time 0.90s (± 1.06%) 0.89s (± 0.53%) -0.01s (- 0.67%) 0.88s 0.90s
Check Time 5.38s (± 0.45%) 5.35s (± 0.52%) -0.03s (- 0.56%) 5.29s 5.41s
Emit Time 3.51s (± 0.63%) 3.53s (± 0.39%) +0.02s (+ 0.63%) 3.50s 3.57s
Total Time 11.32s (± 0.44%) 11.31s (± 0.34%) -0.02s (- 0.16%) 11.22s 11.38s
TFS - node (v8.9.0, x64)
Memory used 291,362k (± 0.01%) 291,356k (± 0.01%) -6k (- 0.00%) 291,276k 291,475k
Parse Time 1.25s (± 0.72%) 1.25s (± 0.42%) 0.00s ( 0.00%) 1.24s 1.27s
Bind Time 0.75s (± 0.53%) 0.75s (± 0.49%) -0.00s (- 0.40%) 0.74s 0.75s
Check Time 4.89s (± 1.47%) 4.88s (± 1.90%) -0.01s (- 0.18%) 4.77s 5.17s
Emit Time 3.29s (± 1.80%) 3.28s (± 3.00%) -0.00s (- 0.06%) 3.04s 3.55s
Total Time 10.18s (± 0.37%) 10.16s (± 0.74%) -0.01s (- 0.11%) 10.02s 10.36s
material-ui - node (v8.9.0, x64)
Memory used 455,829k (± 0.01%) 455,631k (± 0.02%) -198k (- 0.04%) 455,503k 455,895k
Parse Time 2.11s (± 0.83%) 2.10s (± 0.29%) -0.01s (- 0.52%) 2.09s 2.11s
Bind Time 0.80s (± 1.25%) 0.81s (± 0.95%) +0.00s (+ 0.12%) 0.79s 0.82s
Check Time 17.81s (± 0.51%) 17.71s (± 0.73%) -0.10s (- 0.56%) 17.41s 18.00s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 20.72s (± 0.45%) 20.61s (± 0.63%) -0.11s (- 0.52%) 20.32s 20.92s
Angular - node (v8.9.0, x86)
Memory used 185,946k (± 0.03%) 186,168k (± 0.02%) +222k (+ 0.12%) 186,095k 186,280k
Parse Time 2.07s (± 1.04%) 2.05s (± 0.60%) -0.02s (- 0.92%) 2.03s 2.09s
Bind Time 1.07s (± 0.71%) 1.07s (± 0.63%) -0.01s (- 0.65%) 1.06s 1.09s
Check Time 5.01s (± 0.31%) 5.06s (± 0.44%) +0.05s (+ 1.02%) 5.03s 5.13s
Emit Time 6.12s (± 1.45%) 6.03s (± 0.36%) -0.09s (- 1.50%) 5.98s 6.08s
Total Time 14.27s (± 0.74%) 14.22s (± 0.22%) -0.06s (- 0.41%) 14.16s 14.29s
Monaco - node (v8.9.0, x86)
Memory used 185,294k (± 0.02%) 185,291k (± 0.03%) -3k (- 0.00%) 185,173k 185,409k
Parse Time 1.60s (± 1.34%) 1.59s (± 0.60%) -0.01s (- 0.50%) 1.57s 1.61s
Bind Time 0.77s (± 0.89%) 0.76s (± 0.73%) -0.00s (- 0.52%) 0.75s 0.78s
Check Time 5.42s (± 0.56%) 5.41s (± 0.60%) -0.01s (- 0.13%) 5.37s 5.53s
Emit Time 2.87s (± 0.24%) 2.86s (± 1.08%) -0.02s (- 0.59%) 2.79s 2.94s
Total Time 10.66s (± 0.42%) 10.63s (± 0.56%) -0.03s (- 0.31%) 10.52s 10.80s
TFS - node (v8.9.0, x86)
Memory used 166,793k (± 0.03%) 166,771k (± 0.02%) -22k (- 0.01%) 166,713k 166,864k
Parse Time 1.29s (± 0.43%) 1.28s (± 0.64%) -0.00s (- 0.31%) 1.27s 1.31s
Bind Time 0.71s (± 0.81%) 0.71s (± 0.70%) +0.00s (+ 0.28%) 0.71s 0.73s
Check Time 4.62s (± 0.40%) 4.60s (± 0.30%) -0.02s (- 0.41%) 4.57s 4.63s
Emit Time 3.00s (± 2.06%) 2.99s (± 1.92%) -0.01s (- 0.37%) 2.90s 3.20s
Total Time 9.62s (± 0.67%) 9.59s (± 0.69%) -0.03s (- 0.29%) 9.48s 9.84s
material-ui - node (v8.9.0, x86)
Memory used 257,880k (± 0.02%) 257,733k (± 0.02%) -146k (- 0.06%) 257,655k 257,848k
Parse Time 2.18s (± 0.38%) 2.17s (± 0.35%) -0.00s (- 0.05%) 2.16s 2.19s
Bind Time 0.68s (± 1.21%) 0.69s (± 0.72%) +0.00s (+ 0.29%) 0.68s 0.70s
Check Time 16.30s (± 0.71%) 16.21s (± 0.53%) -0.09s (- 0.56%) 16.03s 16.38s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 19.16s (± 0.59%) 19.07s (± 0.45%) -0.09s (- 0.45%) 18.88s 19.26s
System
Machine Namets-ci-ubuntu
Platformlinux 4.4.0-166-generic
Architecturex64
Available Memory16 GB
Available Memory1 GB
CPUs4 × Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz
Hosts
  • node (v10.16.3, x64)
  • node (v12.1.0, x64)
  • node (v8.9.0, x64)
  • node (v8.9.0, x86)
Scenarios
  • Angular - node (v10.16.3, x64)
  • Angular - node (v12.1.0, x64)
  • Angular - node (v8.9.0, x64)
  • Angular - node (v8.9.0, x86)
  • Monaco - node (v10.16.3, x64)
  • Monaco - node (v12.1.0, x64)
  • Monaco - node (v8.9.0, x64)
  • Monaco - node (v8.9.0, x86)
  • TFS - node (v10.16.3, x64)
  • TFS - node (v12.1.0, x64)
  • TFS - node (v8.9.0, x64)
  • TFS - node (v8.9.0, x86)
  • material-ui - node (v10.16.3, x64)
  • material-ui - node (v12.1.0, x64)
  • material-ui - node (v8.9.0, x64)
  • material-ui - node (v8.9.0, x86)
Benchmark Name Iterations
Current 37540 10
Baseline master 10

@rbuckton
Copy link
Member Author

This seems to have improved things in vscode: https://github.com/typescript-bot/TypeScript/pull/9/files#diff-0e62d568d28b82cee9e66b2b8fafd290R7, however there seem to be a larger number of errors now in office-ui-fabric (https://github.com/typescript-bot/TypeScript/pull/9/files#diff-976ec9fe54f7b841cd49ec069e0303a9L135), though that may not be related to this change.

@rbuckton
Copy link
Member Author

Closing this in favor of #37610 for now while we continue to investigate the impact of awaited.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't infer T from awaited T
3 participants