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

Improve error messages for potentially missing 'await' #32239

Merged

Conversation

andrewbranch
Copy link
Member

@andrewbranch andrewbranch commented Jul 3, 2019

This is a partial fix for #30646. The rest of the fix will be enabling some quick fixes triggered by these errors where appropriate (some cases are already covered in #32101).

The conformance test file I added shows one or more instances of every error I added. To summarize them here (operators shown in examples aren’t exhaustive, but the particular types of errors enumerated are):

  • x++ when x is a Promise of bigint or number
  • a | b when a or b is a Promise of bigint or number
  • a > b when await a > await b would check successfully
  • a === b when await a === await b would check successfully
  • f(x) when the parameter type in f is not a Promise and f(await x) would check successfully
  • for (const _ of x) when x is a Promise
  • for await (const _ of x) when x is a Promise
  • x() when x has no call signatures but await x does
  • new C() when C has no construct signatures but await C does

Already existed:

  • x.foo when x is a Promise that resolves to something with a foo property

Note: I followed suit with the existing property access error and did not predicate these error messages on being in an async context. I think this makes sense, because often times I begin writing a function as non-async function f(), and only when I get to the first time I need to await do I go back and add async to my function declaration. (I even think enabling a quick fix in a non-async function is ok, as another quick fix to add 'async' will appear directly after. I think having those as two separate steps, but readily available, is a pretty good UX—it's a middle ground where we make extra sure you’re really ok with changing your function to async, but we still fix up your code with very few keystrokes. But, we can dig into that in a future PR.)

/cc @bterlson for review as well, if you like.

@andrewbranch
Copy link
Member Author

@typescript-bot perf test this please and thank you

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 3, 2019

Heya @andrewbranch, I've started to run the perf test suite on this PR at 63dd5bd. You can monitor the build here. It should now contribute to this PR's status checks.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

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

Here they are:

Comparison Report - master..32239

Metric master 32239 Delta Best Worst
Angular - node (v12.1.0, x64)
Memory used 316,000k (± 0.02%) 316,085k (± 0.02%) +85k (+ 0.03%) 315,923k 316,255k
Parse Time 1.39s (± 0.76%) 1.39s (± 0.64%) -0.01s (- 0.43%) 1.37s 1.40s
Bind Time 0.73s (± 0.67%) 0.73s (± 0.46%) -0.00s (- 0.14%) 0.73s 0.74s
Check Time 4.15s (± 0.58%) 4.16s (± 0.51%) +0.01s (+ 0.24%) 4.12s 4.20s
Emit Time 5.27s (± 0.71%) 5.21s (± 0.64%) -0.06s (- 1.16%) 5.14s 5.27s
Total Time 11.55s (± 0.53%) 11.49s (± 0.43%) -0.06s (- 0.51%) 11.40s 11.62s
Monaco - node (v12.1.0, x64)
Memory used 345,613k (± 0.02%) 345,680k (± 0.02%) +67k (+ 0.02%) 345,459k 345,820k
Parse Time 1.19s (± 1.12%) 1.19s (± 0.76%) -0.00s (- 0.25%) 1.17s 1.21s
Bind Time 0.68s (± 1.01%) 0.68s (± 0.73%) +0.00s (+ 0.15%) 0.67s 0.69s
Check Time 4.25s (± 0.46%) 4.22s (± 0.59%) -0.02s (- 0.47%) 4.16s 4.28s
Emit Time 2.86s (± 0.70%) 2.86s (± 1.04%) +0.01s (+ 0.21%) 2.83s 2.97s
Total Time 8.97s (± 0.43%) 8.95s (± 0.37%) -0.02s (- 0.18%) 8.86s 9.01s
TFS - node (v12.1.0, x64)
Memory used 301,233k (± 0.02%) 301,225k (± 0.02%) -8k (- 0.00%) 301,155k 301,432k
Parse Time 0.91s (± 0.54%) 0.91s (± 0.57%) -0.00s (- 0.33%) 0.90s 0.92s
Bind Time 0.62s (± 1.07%) 0.62s (± 0.89%) -0.00s (- 0.16%) 0.61s 0.64s
Check Time 3.76s (± 0.50%) 3.78s (± 0.57%) +0.02s (+ 0.48%) 3.74s 3.82s
Emit Time 2.95s (± 0.74%) 2.94s (± 0.96%) -0.01s (- 0.44%) 2.88s 3.00s
Total Time 8.24s (± 0.53%) 8.25s (± 0.48%) +0.01s (+ 0.07%) 8.15s 8.35s
Angular - node (v8.9.0, x64)
Memory used 334,394k (± 0.01%) 334,376k (± 0.02%) -17k (- 0.01%) 334,294k 334,561k
Parse Time 1.78s (± 0.39%) 1.78s (± 0.61%) +0.00s (+ 0.06%) 1.76s 1.80s
Bind Time 0.80s (± 0.60%) 0.80s (± 0.75%) -0.00s (- 0.13%) 0.79s 0.81s
Check Time 4.93s (± 1.41%) 4.91s (± 1.92%) -0.02s (- 0.34%) 4.74s 5.08s
Emit Time 5.99s (± 2.66%) 5.99s (± 2.80%) +0.00s (+ 0.03%) 5.69s 6.34s
Total Time 13.50s (± 1.16%) 13.48s (± 1.01%) -0.01s (- 0.09%) 13.21s 13.77s
Monaco - node (v8.9.0, x64)
Memory used 362,792k (± 0.03%) 362,816k (± 0.02%) +24k (+ 0.01%) 362,733k 363,027k
Parse Time 1.43s (± 0.43%) 1.43s (± 0.36%) +0.00s (+ 0.14%) 1.42s 1.44s
Bind Time 0.91s (± 1.63%) 0.90s (± 2.35%) -0.01s (- 0.88%) 0.87s 0.94s
Check Time 5.04s (± 1.41%) 5.08s (± 1.54%) +0.04s (+ 0.85%) 4.96s 5.22s
Emit Time 3.30s (± 5.49%) 3.19s (± 6.68%) -0.11s (- 3.24%) 2.89s 3.54s
Total Time 10.68s (± 1.21%) 10.61s (± 1.50%) -0.08s (- 0.72%) 10.35s 10.88s
TFS - node (v8.9.0, x64)
Memory used 316,763k (± 0.02%) 316,858k (± 0.01%) +95k (+ 0.03%) 316,780k 316,953k
Parse Time 1.13s (± 0.66%) 1.13s (± 0.43%) +0.00s (+ 0.09%) 1.12s 1.14s
Bind Time 0.67s (± 1.24%) 0.66s (± 0.84%) -0.00s (- 0.60%) 0.65s 0.67s
Check Time 4.39s (± 0.88%) 4.37s (± 0.57%) -0.01s (- 0.32%) 4.31s 4.43s
Emit Time 3.18s (± 1.62%) 3.20s (± 0.73%) +0.02s (+ 0.53%) 3.13s 3.25s
Total Time 9.37s (± 0.77%) 9.37s (± 0.44%) -0.00s (- 0.01%) 9.24s 9.43s
Angular - node (v8.9.0, x86)
Memory used 189,490k (± 0.03%) 189,544k (± 0.03%) +54k (+ 0.03%) 189,440k 189,659k
Parse Time 1.74s (± 0.53%) 1.73s (± 0.83%) -0.01s (- 0.52%) 1.70s 1.76s
Bind Time 0.94s (± 1.03%) 0.93s (± 0.91%) -0.01s (- 0.85%) 0.91s 0.95s
Check Time 4.50s (± 0.79%) 4.49s (± 0.65%) -0.01s (- 0.27%) 4.44s 4.56s
Emit Time 5.77s (± 0.95%) 5.79s (± 1.11%) +0.01s (+ 0.24%) 5.58s 5.88s
Total Time 12.95s (± 0.64%) 12.93s (± 0.63%) -0.02s (- 0.15%) 12.67s 13.05s
Monaco - node (v8.9.0, x86)
Memory used 202,455k (± 0.02%) 202,510k (± 0.02%) +55k (+ 0.03%) 202,422k 202,594k
Parse Time 1.49s (± 0.45%) 1.49s (± 0.55%) -0.01s (- 0.40%) 1.47s 1.50s
Bind Time 0.72s (± 1.89%) 0.72s (± 1.22%) -0.00s (- 0.28%) 0.71s 0.75s
Check Time 4.79s (± 0.60%) 4.79s (± 0.45%) -0.00s (- 0.10%) 4.74s 4.84s
Emit Time 3.18s (± 0.55%) 3.18s (± 0.73%) +0.00s (+ 0.09%) 3.13s 3.25s
Total Time 10.18s (± 0.47%) 10.18s (± 0.38%) -0.01s (- 0.06%) 10.10s 10.26s
TFS - node (v8.9.0, x86)
Memory used 177,838k (± 0.02%) 177,898k (± 0.02%) +60k (+ 0.03%) 177,806k 177,981k
Parse Time 1.19s (± 0.57%) 1.19s (± 0.80%) 0.00s ( 0.00%) 1.17s 1.20s
Bind Time 0.64s (± 1.50%) 0.63s (± 0.98%) -0.01s (- 0.94%) 0.62s 0.64s
Check Time 4.19s (± 0.69%) 4.18s (± 0.52%) -0.01s (- 0.14%) 4.14s 4.22s
Emit Time 2.84s (± 0.97%) 2.85s (± 0.64%) +0.01s (+ 0.49%) 2.81s 2.88s
Total Time 8.84s (± 0.58%) 8.85s (± 0.30%) +0.00s (+ 0.05%) 8.78s 8.89s
Angular - node (v9.0.0, x64)
Memory used 334,007k (± 0.02%) 334,012k (± 0.01%) +5k (+ 0.00%) 333,913k 334,094k
Parse Time 1.64s (± 0.71%) 1.63s (± 0.47%) -0.01s (- 0.67%) 1.61s 1.64s
Bind Time 0.74s (± 1.00%) 0.74s (± 0.95%) -0.00s (- 0.27%) 0.73s 0.76s
Check Time 4.52s (± 1.38%) 4.48s (± 0.68%) -0.04s (- 0.86%) 4.44s 4.57s
Emit Time 5.79s (± 1.90%) 5.83s (± 1.56%) +0.05s (+ 0.81%) 5.71s 6.04s
Total Time 12.68s (± 0.82%) 12.67s (± 0.90%) -0.01s (- 0.07%) 12.50s 12.97s
Monaco - node (v9.0.0, x64)
Memory used 362,584k (± 0.03%) 362,644k (± 0.03%) +60k (+ 0.02%) 362,391k 362,832k
Parse Time 1.29s (± 0.58%) 1.28s (± 0.62%) -0.01s (- 0.85%) 1.27s 1.30s
Bind Time 0.86s (± 0.65%) 0.85s (± 0.55%) -0.01s (- 0.93%) 0.84s 0.86s
Check Time 4.85s (± 0.69%) 4.82s (± 0.53%) -0.03s (- 0.56%) 4.77s 4.89s
Emit Time 3.37s (± 0.57%) 3.36s (± 0.34%) -0.01s (- 0.21%) 3.34s 3.38s
Total Time 10.36s (± 0.46%) 10.31s (± 0.30%) -0.05s (- 0.49%) 10.23s 10.37s
TFS - node (v9.0.0, x64)
Memory used 316,613k (± 0.01%) 316,668k (± 0.01%) +55k (+ 0.02%) 316,616k 316,736k
Parse Time 1.02s (± 0.98%) 1.01s (± 0.66%) -0.01s (- 0.69%) 1.00s 1.03s
Bind Time 0.61s (± 0.97%) 0.61s (± 0.80%) -0.00s (- 0.00%) 0.60s 0.62s
Check Time 4.29s (± 0.54%) 4.31s (± 1.33%) +0.02s (+ 0.49%) 4.23s 4.50s
Emit Time 3.19s (± 1.32%) 3.18s (± 1.97%) -0.01s (- 0.28%) 2.94s 3.25s
Total Time 9.12s (± 0.39%) 9.12s (± 0.52%) +0.01s (+ 0.07%) 9.06s 9.24s
Angular - node (v9.0.0, x86)
Memory used 189,607k (± 0.04%) 189,617k (± 0.03%) +11k (+ 0.01%) 189,465k 189,776k
Parse Time 1.54s (± 0.58%) 1.54s (± 0.34%) -0.00s (- 0.19%) 1.53s 1.55s
Bind Time 0.88s (± 1.02%) 0.88s (± 0.86%) +0.00s (+ 0.11%) 0.86s 0.90s
Check Time 4.16s (± 0.73%) 4.19s (± 0.50%) +0.02s (+ 0.58%) 4.14s 4.24s
Emit Time 5.44s (± 0.88%) 5.43s (± 0.73%) -0.01s (- 0.13%) 5.35s 5.50s
Total Time 12.02s (± 0.59%) 12.03s (± 0.47%) +0.02s (+ 0.13%) 11.89s 12.15s
Monaco - node (v9.0.0, x86)
Memory used 202,512k (± 0.03%) 202,579k (± 0.02%) +67k (+ 0.03%) 202,505k 202,737k
Parse Time 1.31s (± 0.63%) 1.31s (± 0.46%) -0.00s (- 0.15%) 1.29s 1.32s
Bind Time 0.65s (± 1.47%) 0.64s (± 0.81%) -0.01s (- 0.77%) 0.63s 0.65s
Check Time 4.62s (± 0.37%) 4.64s (± 0.50%) +0.02s (+ 0.37%) 4.58s 4.69s
Emit Time 3.11s (± 0.46%) 3.10s (± 0.82%) -0.01s (- 0.39%) 3.05s 3.19s
Total Time 9.69s (± 0.28%) 9.69s (± 0.26%) -0.00s (- 0.04%) 9.64s 9.76s
TFS - node (v9.0.0, x86)
Memory used 177,847k (± 0.02%) 177,943k (± 0.01%) +96k (+ 0.05%) 177,890k 178,013k
Parse Time 1.03s (± 0.93%) 1.03s (± 1.01%) +0.00s (+ 0.19%) 1.01s 1.05s
Bind Time 0.58s (± 1.03%) 0.57s (± 0.59%) -0.00s (- 0.52%) 0.57s 0.58s
Check Time 4.05s (± 0.92%) 4.06s (± 0.43%) +0.00s (+ 0.12%) 4.02s 4.10s
Emit Time 2.79s (± 0.90%) 2.77s (± 0.54%) -0.01s (- 0.47%) 2.75s 2.82s
Total Time 8.44s (± 0.42%) 8.44s (± 0.25%) -0.00s (- 0.06%) 8.40s 8.49s
System
Machine Namets-ci-ubuntu
Platformlinux 4.4.0-142-generic
Architecturex64
Available Memory16 GB
Available Memory1 GB
CPUs4 × Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz
Hosts
  • node (v12.1.0, x64)
  • node (v8.9.0, x64)
  • node (v8.9.0, x86)
  • node (v9.0.0, x64)
  • node (v9.0.0, x86)
Scenarios
  • Angular - node (v12.1.0, x64)
  • Angular - node (v8.9.0, x64)
  • Angular - node (v8.9.0, x86)
  • Angular - node (v9.0.0, x64)
  • Angular - node (v9.0.0, x86)
  • Monaco - node (v12.1.0, x64)
  • Monaco - node (v8.9.0, x64)
  • Monaco - node (v8.9.0, x86)
  • Monaco - node (v9.0.0, x64)
  • Monaco - node (v9.0.0, x86)
  • TFS - node (v12.1.0, x64)
  • TFS - node (v8.9.0, x64)
  • TFS - node (v8.9.0, x86)
  • TFS - node (v9.0.0, x64)
  • TFS - node (v9.0.0, x86)
Benchmark Name Iterations
Current 32239 10
Baseline master 10

@andrewbranch andrewbranch marked this pull request as ready for review July 3, 2019 23:53
@andrewbranch andrewbranch force-pushed the enhancement/missing-await-errors branch from 1722bd2 to 6450199 Compare July 8, 2019 21:52
@andrewbranch
Copy link
Member Author

@typescript-bot test this
@typescript-bot run dt
@typescript-bot can you do two things in one comment?

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 8, 2019

Heya @andrewbranch, I've started to run the parallelized Definitely Typed test suite on this PR at 6450199. You can monitor the build here. It should now contribute to this PR's status checks.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 8, 2019

Heya @andrewbranch, I've started to run the extended test suite on this PR at 6450199. You can monitor the build here. It should now contribute to this PR's status checks.

@andrewbranch
Copy link
Member Author

andrewbranch commented Jul 8, 2019

  • f(x) when the parameter type in f is not a Promise but x is

Hm, from RWC, maybe we should go a little farther on this check:

promises.push(this.setSetting(newStore, key, settings[key]));
              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
!!! error TS2345: Argument of type 'JQueryPromise' is not assignable to parameter of type 'Promise'.
!!! related TS2773 TypeScript/Mocks/MockServicesSettings.ts:49:31: Did you forget to use 'await'?

That looks a little dopey ☝️

Actually, the logic I had was

  • f(x) when f(await x) would check

and I’m updating it to

  • f(x) when the parameter type in f is not a Promise and f(await x) would check

I think there were a bunch of cases in RWC where await x was just any so the "Did you forget 'await'?" message was too eager there.

@andrewbranch
Copy link
Member Author

@typescript-bot test this again please

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 8, 2019

Heya @andrewbranch, I've started to run the extended test suite on this PR at bdd8a3e. You can monitor the build here. It should now contribute to this PR's status checks.

@andrewbranch
Copy link
Member Author

RWC had an error I don't totally understand; the build says "Error: TF401179: An active pull request for the source and target branch already exists" among other things, but the baseline diff went back to no changes.

@typescript-bot test this?

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 9, 2019

Heya @andrewbranch, I've started to run the extended test suite on this PR at bdd8a3e. You can monitor the build here. It should now contribute to this PR's status checks.

@andrewbranch
Copy link
Member Author

@weswigham halp, what does this mean? I thought the existing baseline diff PR should be updated 🤔

@weswigham
Copy link
Member

thought the existing baseline diff PR should be updated

Yeah, just means the diff with master is nil, but master is currently failing.

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

Successfully merging this pull request may close these issues.

4 participants