Skip to content

Add error for missing await in conditionals - #39175

Merged
Eli Barzilay (elibarzilay) merged 1 commit into
microsoft:masterfrom
Jack-Works:await-in-cond
Feb 16, 2021
Merged

Add error for missing await in conditionals#39175
Eli Barzilay (elibarzilay) merged 1 commit into
microsoft:masterfrom
Jack-Works:await-in-cond

Conversation

@Jack-Works

@Jack-WorksJack Works (Jack-Works) commented Jun 20, 2020

Copy link
Copy Markdown
Contributor

image

close#25330

@DanielRosenwasser

Copy link
Copy Markdown
Member

@typescript-bot

TypeScript Bot (typescript-bot) commented Jun 22, 2020

Copy link
Copy Markdown
Contributor

Heya Daniel Rosenwasser (@DanielRosenwasser), I've started to run the extended test suite on this PR at 49ee464. You can monitor the build here.

@typescript-bot

TypeScript Bot (typescript-bot) commented Jun 22, 2020

Copy link
Copy Markdown
Contributor

Heya Daniel Rosenwasser (@DanielRosenwasser), I've started to run the perf test suite on this PR at 49ee464. You can monitor the build here.

Update: The results are in!

@typescript-bot

TypeScript Bot (typescript-bot) commented Jun 22, 2020

Copy link
Copy Markdown
Contributor

Heya Daniel Rosenwasser (@DanielRosenwasser), I've started to run the parallelized community code test suite on this PR at 49ee464. You can monitor the build here.

@DanielRosenwasser

Copy link
Copy Markdown
Member

TypeScript Bot (@typescript-bot) pack this

@typescript-bot

TypeScript Bot (typescript-bot) commented Jun 22, 2020

Copy link
Copy Markdown
Contributor

Heya Daniel Rosenwasser (@DanielRosenwasser), I've started to run the tarball bundle task on this PR at 49ee464. You can monitor the build here.

@typescript-bot

TypeScript Bot (typescript-bot) commented Jun 22, 2020

Copy link
Copy Markdown
Contributor

Hey Daniel Rosenwasser (@DanielRosenwasser), 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/77391/artifacts?artifactName=tgz&fileId=064113E0A813A04752FE56147E7B13F436B18C253E6A75A293CFF3415CFC69B702&fileName=/typescript-4.0.0-insiders.20200622.tgz"
}
}

and then running npm install.


There is also a playground for this build.

@typescript-bot

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor

Daniel Rosenwasser (@DanielRosenwasser)
The results of the perf run you requested are in!

Here they are:

Comparison Report - master..39175

Metricmaster39175DeltaBestWorst
Angular - node (v10.16.3, x64)
Memory used340,391k (± 0.01%)339,886k (± 0.02%)-504k (- 0.15%)339,764k340,023k
Parse Time1.99s (± 0.62%)1.98s (± 0.73%)-0.01s (- 0.60%)1.96s2.01s
Bind Time0.80s (± 0.74%)0.80s (± 0.73%)-0.01s (- 0.87%)0.79s0.81s
Check Time4.74s (± 0.35%)4.69s (± 0.58%)-0.05s (- 1.01%)4.64s4.76s
Emit Time5.22s (± 0.56%)5.18s (± 0.77%)-0.04s (- 0.73%)5.08s5.28s
Total Time12.75s (± 0.34%)12.64s (± 0.62%)-0.10s (- 0.82%)12.48s12.84s
Monaco - node (v10.16.3, x64)
Memory used338,891k (± 0.01%)338,944k (± 0.02%)+54k (+ 0.02%)338,811k339,119k
Parse Time1.57s (± 0.77%)1.59s (± 0.51%)+0.01s (+ 0.83%)1.57s1.60s
Bind Time0.70s (± 0.71%)0.70s (± 0.74%)+0.00s (+ 0.43%)0.69s0.71s
Check Time4.86s (± 0.49%)4.86s (± 0.33%)+0.00s (+ 0.08%)4.82s4.90s
Emit Time2.75s (± 0.84%)2.76s (± 0.81%)+0.01s (+ 0.36%)2.71s2.81s
Total Time9.88s (± 0.37%)9.91s (± 0.28%)+0.03s (+ 0.29%)9.85s9.97s
TFS - node (v10.16.3, x64)
Memory used301,843k (± 0.02%)301,912k (± 0.03%)+69k (+ 0.02%)301,735k302,050k
Parse Time1.21s (± 0.58%)1.21s (± 0.51%)0.00s ( 0.00%)1.20s1.23s
Bind Time0.64s (± 0.92%)0.66s (± 0.76%)+0.01s (+ 1.71%)0.64s0.66s
Check Time4.35s (± 0.56%)4.37s (± 0.53%)+0.02s (+ 0.44%)4.31s4.42s
Emit Time2.86s (± 1.35%)2.89s (± 1.17%)+0.02s (+ 0.84%)2.82s2.98s
Total Time9.07s (± 0.62%)9.13s (± 0.47%)+0.06s (+ 0.62%)9.05s9.24s
material-ui - node (v10.16.3, x64)
Memory used459,543k (± 0.02%)459,200k (± 0.01%)-343k (- 0.07%)459,133k459,293k
Parse Time2.05s (± 0.76%)2.04s (± 0.54%)-0.01s (- 0.44%)2.01s2.06s
Bind Time0.66s (± 1.43%)0.66s (± 1.24%)+0.00s (+ 0.00%)0.64s0.67s
Check Time12.82s (± 0.62%)12.85s (± 0.66%)+0.03s (+ 0.22%)12.71s13.08s
Emit Time0.00s (± 0.00%)0.00s (± 0.00%)0.00s ( NaN%)0.00s0.00s
Total Time15.52s (± 0.57%)15.54s (± 0.59%)+0.02s (+ 0.15%)15.39s15.80s
Angular - node (v12.1.0, x64)
Memory used317,804k (± 0.03%)317,179k (± 0.09%)-626k (- 0.20%)316,000k317,459k
Parse Time1.96s (± 0.78%)1.95s (± 0.62%)-0.01s (- 0.71%)1.92s1.98s
Bind Time0.78s (± 0.60%)0.78s (± 1.46%)+0.00s (+ 0.26%)0.77s0.81s
Check Time4.61s (± 0.56%)4.59s (± 0.74%)-0.02s (- 0.35%)4.53s4.70s
Emit Time5.34s (± 0.48%)5.38s (± 1.58%)+0.04s (+ 0.81%)5.29s5.68s
Total Time12.69s (± 0.37%)12.71s (± 0.80%)+0.02s (+ 0.13%)12.55s12.97s
Monaco - node (v12.1.0, x64)
Memory used321,481k (± 0.02%)321,460k (± 0.01%)-21k (- 0.01%)321,349k321,554k
Parse Time1.54s (± 0.94%)1.54s (± 1.22%)+0.01s (+ 0.39%)1.52s1.59s
Bind Time0.68s (± 1.70%)0.68s (± 0.98%)+0.00s (+ 0.44%)0.67s0.70s
Check Time4.65s (± 0.60%)4.66s (± 0.49%)+0.02s (+ 0.39%)4.61s4.72s
Emit Time2.79s (± 0.82%)2.81s (± 0.77%)+0.01s (+ 0.54%)2.77s2.88s
Total Time9.66s (± 0.56%)9.70s (± 0.50%)+0.04s (+ 0.43%)9.61s9.86s
TFS - node (v12.1.0, x64)
Memory used286,349k (± 0.03%)286,369k (± 0.02%)+20k (+ 0.01%)286,223k286,512k
Parse Time1.23s (± 0.94%)1.23s (± 0.61%)-0.00s (- 0.08%)1.22s1.25s
Bind Time0.63s (± 1.09%)0.63s (± 1.50%)+0.00s (+ 0.16%)0.61s0.65s
Check Time4.27s (± 0.35%)4.28s (± 0.57%)+0.01s (+ 0.21%)4.22s4.33s
Emit Time2.93s (± 0.88%)2.93s (± 0.86%)-0.00s (- 0.07%)2.89s3.01s
Total Time9.06s (± 0.33%)9.07s (± 0.39%)+0.01s (+ 0.07%)8.97s9.15s
material-ui - node (v12.1.0, x64)
Memory used437,981k (± 0.02%)437,600k (± 0.06%)-381k (- 0.09%)436,582k437,800k
Parse Time2.02s (± 0.52%)2.02s (± 0.72%)+0.01s (+ 0.30%)1.99s2.06s
Bind Time0.63s (± 0.75%)0.63s (± 1.11%)-0.00s (- 0.63%)0.61s0.64s
Check Time11.46s (± 0.61%)11.63s (± 0.93%)+0.17s (+ 1.45%)11.41s11.81s
Emit Time0.00s (± 0.00%)0.00s (± 0.00%)0.00s ( NaN%)0.00s0.00s
Total Time14.11s (± 0.48%)14.28s (± 0.79%)+0.17s (+ 1.19%)14.05s14.45s
Angular - node (v8.9.0, x64)
Memory used336,794k (± 0.01%)336,488k (± 0.02%)-306k (- 0.09%)336,364k336,615k
Parse Time2.50s (± 0.53%)2.50s (± 0.47%)+0.01s (+ 0.20%)2.47s2.52s
Bind Time0.83s (± 0.36%)0.82s (± 0.45%)-0.00s (- 0.36%)0.82s0.83s
Check Time5.30s (± 0.57%)5.34s (± 0.58%)+0.03s (+ 0.62%)5.29s5.43s
Emit Time5.86s (± 1.37%)5.91s (± 0.74%)+0.05s (+ 0.77%)5.79s6.03s
Total Time14.49s (± 0.74%)14.57s (± 0.44%)+0.08s (+ 0.53%)14.43s14.72s
Monaco - node (v8.9.0, x64)
Memory used340,278k (± 0.02%)340,268k (± 0.01%)-10k (- 0.00%)340,143k340,322k
Parse Time1.87s (± 0.48%)1.86s (± 0.45%)-0.00s (- 0.05%)1.85s1.89s
Bind Time0.87s (± 0.55%)0.87s (± 0.42%)-0.00s (- 0.12%)0.86s0.87s
Check Time5.36s (± 0.67%)5.36s (± 0.67%)+0.00s (+ 0.02%)5.30s5.47s
Emit Time3.22s (± 0.57%)3.24s (± 0.82%)+0.01s (+ 0.34%)3.18s3.31s
Total Time11.31s (± 0.41%)11.32s (± 0.58%)+0.01s (+ 0.08%)11.19s11.52s
TFS - node (v8.9.0, x64)
Memory used303,621k (± 0.01%)303,595k (± 0.02%)-27k (- 0.01%)303,461k303,713k
Parse Time1.53s (± 0.69%)1.53s (± 0.50%)+0.00s (+ 0.07%)1.52s1.55s
Bind Time0.65s (± 0.85%)0.65s (± 1.02%)+0.00s (+ 0.15%)0.64s0.67s
Check Time4.94s (± 1.65%)4.94s (± 1.44%)+0.00s (+ 0.02%)4.81s5.13s
Emit Time3.08s (± 3.17%)3.12s (± 2.07%)+0.04s (+ 1.43%)2.95s3.19s
Total Time10.20s (± 0.61%)10.25s (± 0.35%)+0.05s (+ 0.46%)10.13s10.31s
material-ui - node (v8.9.0, x64)
Memory used463,645k (± 0.02%)463,368k (± 0.01%)-277k (- 0.06%)463,244k463,507k
Parse Time2.37s (± 0.55%)2.38s (± 0.25%)+0.00s (+ 0.08%)2.36s2.39s
Bind Time0.78s (± 1.15%)0.77s (± 0.76%)-0.01s (- 1.16%)0.76s0.78s
Check Time16.96s (± 0.67%)16.94s (± 0.96%)-0.01s (- 0.06%)16.49s17.18s
Emit Time0.00s (± 0.00%)0.00s (± 0.00%)0.00s ( NaN%)0.00s0.00s
Total Time20.10s (± 0.58%)20.09s (± 0.80%)-0.02s (- 0.08%)19.64s20.31s
Angular - node (v8.9.0, x86)
Memory used193,334k (± 0.02%)193,147k (± 0.01%)-187k (- 0.10%)193,075k193,190k
Parse Time2.43s (± 0.63%)2.43s (± 1.08%)+0.00s (+ 0.16%)2.37s2.48s
Bind Time0.98s (± 0.78%)0.97s (± 0.57%)-0.00s (- 0.31%)0.96s0.98s
Check Time4.79s (± 0.44%)4.78s (± 0.58%)-0.01s (- 0.21%)4.73s4.84s
Emit Time5.91s (± 0.88%)5.94s (± 0.43%)+0.03s (+ 0.52%)5.90s6.01s
Total Time14.11s (± 0.54%)14.12s (± 0.39%)+0.02s (+ 0.13%)14.02s14.27s
Monaco - node (v8.9.0, x86)
Memory used193,294k (± 0.02%)193,280k (± 0.02%)-14k (- 0.01%)193,209k193,358k
Parse Time1.92s (± 1.37%)1.90s (± 0.40%)-0.02s (- 1.25%)1.88s1.91s
Bind Time0.68s (± 1.21%)0.68s (± 0.76%)-0.00s (- 0.44%)0.67s0.69s
Check Time5.47s (± 0.57%)5.47s (± 0.78%)-0.01s (- 0.16%)5.40s5.60s
Emit Time2.66s (± 0.77%)2.67s (± 0.96%)+0.01s (+ 0.23%)2.62s2.73s
Total Time10.74s (± 0.59%)10.71s (± 0.51%)-0.03s (- 0.31%)10.63s10.89s
TFS - node (v8.9.0, x86)
Memory used173,586k (± 0.03%)173,574k (± 0.01%)-12k (- 0.01%)173,532k173,623k
Parse Time1.57s (± 0.70%)1.58s (± 0.80%)+0.01s (+ 0.70%)1.56s1.62s
Bind Time0.62s (± 1.00%)0.62s (± 0.76%)+0.00s (+ 0.16%)0.61s0.63s
Check Time4.62s (± 0.58%)4.66s (± 0.70%)+0.04s (+ 0.76%)4.61s4.73s
Emit Time2.79s (± 0.80%)2.79s (± 1.10%)-0.00s (- 0.11%)2.74s2.88s
Total Time9.60s (± 0.36%)9.64s (± 0.52%)+0.04s (+ 0.41%)9.55s9.77s
material-ui - node (v8.9.0, x86)
Memory used262,483k (± 0.02%)262,272k (± 0.01%)-211k (- 0.08%)262,186k262,365k
Parse Time2.44s (± 0.79%)2.44s (± 0.71%)-0.00s (- 0.04%)2.40s2.49s
Bind Time0.67s (± 1.66%)0.66s (± 1.33%)-0.00s (- 0.60%)0.65s0.68s
Check Time15.45s (± 0.51%)15.54s (± 0.50%)+0.09s (+ 0.61%)15.43s15.71s
Emit Time0.00s (± 0.00%)0.00s (± 0.00%)0.00s ( NaN%)0.00s0.00s
Total Time18.55s (± 0.52%)18.64s (± 0.46%)+0.09s (+ 0.47%)18.51s18.86s
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)
BenchmarkNameIterations
Current3917510
Baselinemaster10

@Jack-Works

Copy link
Copy Markdown
ContributorAuthor

https://github.com/typescript-bot/TypeScript/pull/49/files#diff-0e62d568d28b82cee9e66b2b8fafd290R7

Is this a possible bug in vscode? 🤔

@DanielRosenwasser

Copy link
Copy Markdown
Member

Matt Bierner (@mjbvz) if we can get some of the numbers down, we'd like to get this merged in for a future release. Like Jack Works (@Jack-Works) mentioned, I think this check might've caught a potential bug in VS Code in an un-awaited promise.

@mjbvz

Copy link
Copy Markdown

Thanks Jack Works (@Jack-Works)!

The error in editorActions looks like it should not cause problems. We can probably just remove the conditional

The error in server looks like bad typings since the value really can be undefined (but we don't type it that way)

@Jack-Works

Copy link
Copy Markdown
ContributorAuthor

Another question, should it behave like the check to function? If the variable is undefined | Promise, and use as a if condition. If it is not used in the if body, should it be an error?

@Jack-Works

Copy link
Copy Markdown
ContributorAuthor

How's going 👀

@typescript-bot

Copy link
Copy Markdown
Contributor

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

@Jack-Works

Copy link
Copy Markdown
ContributorAuthor

rebased to master and merge conflict resolved

@typescript-bot

Copy link
Copy Markdown
Contributor

The TypeScript team hasn't accepted the linked issue #25330. If you can get it accepted, this PR will have a better chance of being reviewed.

@Jack-Works

Copy link
Copy Markdown
ContributorAuthor

rebased to master and merge conflict resolved

@Jack-Works

Copy link
Copy Markdown
ContributorAuthor

rebased to master and conflict resolved

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

See the comments.

(Another pickiness: the title of the PR and the commit could be better, since it's not only in if statements. The title of the linked issue is probably good for this PR too.)

Comment threadsrc/compiler/checker.ts Outdated
Comment threadsrc/compiler/checker.ts
@Jack-WorksJack Works (Jack-Works) changed the title Error on forget to use await on Promise in if conditionAdd error for missing await in conditional (if or ternary expr)Feb 13, 2021
@elibarzilayEli Barzilay (elibarzilay) changed the title Add error for missing await in conditional (if or ternary expr)Add error for missing await in conditionalsFeb 16, 2021
@elibarzilay

Copy link
Copy Markdown
Contributor

Rebased, resolved conflicts, squashed; re-edited the PR subject and the commit message.

@elibarzilay
Eli Barzilay (elibarzilay) merged commit bb8b9db into microsoft:masterFeb 16, 2021
@Jack-Works
Jack Works (Jack-Works) deleted the await-in-cond branch February 17, 2021 07:24
@microsoftMicrosoft (microsoft) locked as resolved and limited conversation to collaborators Oct 21, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

For Uncommitted BugPR for untriaged, rejected, closed or missing bug

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Error for missing await in conditional

6 participants

@Jack-Works@DanielRosenwasser@typescript-bot@mjbvz@elibarzilay@sandersn