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.

Sign up for GitHub

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

Jump to bottom

Add error for missing await in conditionals #39175

Merged
merged 1 commit into from
Feb 16, 2021
Merged

Add error for missing await in conditionals #39175

merged 1 commit into from
Feb 16, 2021

Conversation

Jack-Works
Copy link
Contributor

@Jack-Works Jack-Works commented Jun 20, 2020 •
edited
Loading

image

close #25330

@DanielRosenwasser
Copy link
Member

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 22, 2020 •
edited
Loading

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 22, 2020 •
edited
Loading

Heya @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
Copy link
Collaborator

typescript-bot commented Jun 22, 2020 •
edited
Loading

Heya @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
Member

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 22, 2020 •
edited
Loading

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 22, 2020 •
edited
Loading

Hey @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 typescript-bot mentioned this pull request Jun 22, 2020
Open
@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

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

Here they are:

Comparison Report - master..39175

Metric master 39175 Delta Best Worst
Angular - node (v10.16.3, x64)
Memory used 340,391k (± 0.01%) 339,886k (± 0.02%) -504k (- 0.15%) 339,764k 340,023k
Parse Time 1.99s (± 0.62%) 1.98s (± 0.73%) -0.01s (- 0.60%) 1.96s 2.01s
Bind Time 0.80s (± 0.74%) 0.80s (± 0.73%) -0.01s (- 0.87%) 0.79s 0.81s
Check Time 4.74s (± 0.35%) 4.69s (± 0.58%) -0.05s (- 1.01%) 4.64s 4.76s
Emit Time 5.22s (± 0.56%) 5.18s (± 0.77%) -0.04s (- 0.73%) 5.08s 5.28s
Total Time 12.75s (± 0.34%) 12.64s (± 0.62%) -0.10s (- 0.82%) 12.48s 12.84s
Monaco - node (v10.16.3, x64)
Memory used 338,891k (± 0.01%) 338,944k (± 0.02%) +54k (+ 0.02%) 338,811k 339,119k
Parse Time 1.57s (± 0.77%) 1.59s (± 0.51%) +0.01s (+ 0.83%) 1.57s 1.60s
Bind Time 0.70s (± 0.71%) 0.70s (± 0.74%) +0.00s (+ 0.43%) 0.69s 0.71s
Check Time 4.86s (± 0.49%) 4.86s (± 0.33%) +0.00s (+ 0.08%) 4.82s 4.90s
Emit Time 2.75s (± 0.84%) 2.76s (± 0.81%) +0.01s (+ 0.36%) 2.71s 2.81s
Total Time 9.88s (± 0.37%) 9.91s (± 0.28%) +0.03s (+ 0.29%) 9.85s 9.97s
TFS - node (v10.16.3, x64)
Memory used 301,843k (± 0.02%) 301,912k (± 0.03%) +69k (+ 0.02%) 301,735k 302,050k
Parse Time 1.21s (± 0.58%) 1.21s (± 0.51%) 0.00s ( 0.00%) 1.20s 1.23s
Bind Time 0.64s (± 0.92%) 0.66s (± 0.76%) +0.01s (+ 1.71%) 0.64s 0.66s
Check Time 4.35s (± 0.56%) 4.37s (± 0.53%) +0.02s (+ 0.44%) 4.31s 4.42s
Emit Time 2.86s (± 1.35%) 2.89s (± 1.17%) +0.02s (+ 0.84%) 2.82s 2.98s
Total Time 9.07s (± 0.62%) 9.13s (± 0.47%) +0.06s (+ 0.62%) 9.05s 9.24s
material-ui - node (v10.16.3, x64)
Memory used 459,543k (± 0.02%) 459,200k (± 0.01%) -343k (- 0.07%) 459,133k 459,293k
Parse Time 2.05s (± 0.76%) 2.04s (± 0.54%) -0.01s (- 0.44%) 2.01s 2.06s
Bind Time 0.66s (± 1.43%) 0.66s (± 1.24%) +0.00s (+ 0.00%) 0.64s 0.67s
Check Time 12.82s (± 0.62%) 12.85s (± 0.66%) +0.03s (+ 0.22%) 12.71s 13.08s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 15.52s (± 0.57%) 15.54s (± 0.59%) +0.02s (+ 0.15%) 15.39s 15.80s
Angular - node (v12.1.0, x64)
Memory used 317,804k (± 0.03%) 317,179k (± 0.09%) -626k (- 0.20%) 316,000k 317,459k
Parse Time 1.96s (± 0.78%) 1.95s (± 0.62%) -0.01s (- 0.71%) 1.92s 1.98s
Bind Time 0.78s (± 0.60%) 0.78s (± 1.46%) +0.00s (+ 0.26%) 0.77s 0.81s
Check Time 4.61s (± 0.56%) 4.59s (± 0.74%) -0.02s (- 0.35%) 4.53s 4.70s
Emit Time 5.34s (± 0.48%) 5.38s (± 1.58%) +0.04s (+ 0.81%) 5.29s 5.68s
Total Time 12.69s (± 0.37%) 12.71s (± 0.80%) +0.02s (+ 0.13%) 12.55s 12.97s
Monaco - node (v12.1.0, x64)
Memory used 321,481k (± 0.02%) 321,460k (± 0.01%) -21k (- 0.01%) 321,349k 321,554k
Parse Time 1.54s (± 0.94%) 1.54s (± 1.22%) +0.01s (+ 0.39%) 1.52s 1.59s
Bind Time 0.68s (± 1.70%) 0.68s (± 0.98%) +0.00s (+ 0.44%) 0.67s 0.70s
Check Time 4.65s (± 0.60%) 4.66s (± 0.49%) +0.02s (+ 0.39%) 4.61s 4.72s
Emit Time 2.79s (± 0.82%) 2.81s (± 0.77%) +0.01s (+ 0.54%) 2.77s 2.88s
Total Time 9.66s (± 0.56%) 9.70s (± 0.50%) +0.04s (+ 0.43%) 9.61s 9.86s
TFS - node (v12.1.0, x64)
Memory used 286,349k (± 0.03%) 286,369k (± 0.02%) +20k (+ 0.01%) 286,223k 286,512k
Parse Time 1.23s (± 0.94%) 1.23s (± 0.61%) -0.00s (- 0.08%) 1.22s 1.25s
Bind Time 0.63s (± 1.09%) 0.63s (± 1.50%) +0.00s (+ 0.16%) 0.61s 0.65s
Check Time 4.27s (± 0.35%) 4.28s (± 0.57%) +0.01s (+ 0.21%) 4.22s 4.33s
Emit Time 2.93s (± 0.88%) 2.93s (± 0.86%) -0.00s (- 0.07%) 2.89s 3.01s
Total Time 9.06s (± 0.33%) 9.07s (± 0.39%) +0.01s (+ 0.07%) 8.97s 9.15s
material-ui - node (v12.1.0, x64)
Memory used 437,981k (± 0.02%) 437,600k (± 0.06%) -381k (- 0.09%) 436,582k 437,800k
Parse Time 2.02s (± 0.52%) 2.02s (± 0.72%) +0.01s (+ 0.30%) 1.99s 2.06s
Bind Time 0.63s (± 0.75%) 0.63s (± 1.11%) -0.00s (- 0.63%) 0.61s 0.64s
Check Time 11.46s (± 0.61%) 11.63s (± 0.93%) +0.17s (+ 1.45%) 11.41s 11.81s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 14.11s (± 0.48%) 14.28s (± 0.79%) +0.17s (+ 1.19%) 14.05s 14.45s
Angular - node (v8.9.0, x64)
Memory used 336,794k (± 0.01%) 336,488k (± 0.02%) -306k (- 0.09%) 336,364k 336,615k
Parse Time 2.50s (± 0.53%) 2.50s (± 0.47%) +0.01s (+ 0.20%) 2.47s 2.52s
Bind Time 0.83s (± 0.36%) 0.82s (± 0.45%) -0.00s (- 0.36%) 0.82s 0.83s
Check Time 5.30s (± 0.57%) 5.34s (± 0.58%) +0.03s (+ 0.62%) 5.29s 5.43s
Emit Time 5.86s (± 1.37%) 5.91s (± 0.74%) +0.05s (+ 0.77%) 5.79s 6.03s
Total Time 14.49s (± 0.74%) 14.57s (± 0.44%) +0.08s (+ 0.53%) 14.43s 14.72s
Monaco - node (v8.9.0, x64)
Memory used 340,278k (± 0.02%) 340,268k (± 0.01%) -10k (- 0.00%) 340,143k 340,322k
Parse Time 1.87s (± 0.48%) 1.86s (± 0.45%) -0.00s (- 0.05%) 1.85s 1.89s
Bind Time 0.87s (± 0.55%) 0.87s (± 0.42%) -0.00s (- 0.12%) 0.86s 0.87s
Check Time 5.36s (± 0.67%) 5.36s (± 0.67%) +0.00s (+ 0.02%) 5.30s 5.47s
Emit Time 3.22s (± 0.57%) 3.24s (± 0.82%) +0.01s (+ 0.34%) 3.18s 3.31s
Total Time 11.31s (± 0.41%) 11.32s (± 0.58%) +0.01s (+ 0.08%) 11.19s 11.52s
TFS - node (v8.9.0, x64)
Memory used 303,621k (± 0.01%) 303,595k (± 0.02%) -27k (- 0.01%) 303,461k 303,713k
Parse Time 1.53s (± 0.69%) 1.53s (± 0.50%) +0.00s (+ 0.07%) 1.52s 1.55s
Bind Time 0.65s (± 0.85%) 0.65s (± 1.02%) +0.00s (+ 0.15%) 0.64s 0.67s
Check Time 4.94s (± 1.65%) 4.94s (± 1.44%) +0.00s (+ 0.02%) 4.81s 5.13s
Emit Time 3.08s (± 3.17%) 3.12s (± 2.07%) +0.04s (+ 1.43%) 2.95s 3.19s
Total Time 10.20s (± 0.61%) 10.25s (± 0.35%) +0.05s (+ 0.46%) 10.13s 10.31s
material-ui - node (v8.9.0, x64)
Memory used 463,645k (± 0.02%) 463,368k (± 0.01%) -277k (- 0.06%) 463,244k 463,507k
Parse Time 2.37s (± 0.55%) 2.38s (± 0.25%) +0.00s (+ 0.08%) 2.36s 2.39s
Bind Time 0.78s (± 1.15%) 0.77s (± 0.76%) -0.01s (- 1.16%) 0.76s 0.78s
Check Time 16.96s (± 0.67%) 16.94s (± 0.96%) -0.01s (- 0.06%) 16.49s 17.18s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 20.10s (± 0.58%) 20.09s (± 0.80%) -0.02s (- 0.08%) 19.64s 20.31s
Angular - node (v8.9.0, x86)
Memory used 193,334k (± 0.02%) 193,147k (± 0.01%) -187k (- 0.10%) 193,075k 193,190k
Parse Time 2.43s (± 0.63%) 2.43s (± 1.08%) +0.00s (+ 0.16%) 2.37s 2.48s
Bind Time 0.98s (± 0.78%) 0.97s (± 0.57%) -0.00s (- 0.31%) 0.96s 0.98s
Check Time 4.79s (± 0.44%) 4.78s (± 0.58%) -0.01s (- 0.21%) 4.73s 4.84s
Emit Time 5.91s (± 0.88%) 5.94s (± 0.43%) +0.03s (+ 0.52%) 5.90s 6.01s
Total Time 14.11s (± 0.54%) 14.12s (± 0.39%) +0.02s (+ 0.13%) 14.02s 14.27s
Monaco - node (v8.9.0, x86)
Memory used 193,294k (± 0.02%) 193,280k (± 0.02%) -14k (- 0.01%) 193,209k 193,358k
Parse Time 1.92s (± 1.37%) 1.90s (± 0.40%) -0.02s (- 1.25%) 1.88s 1.91s
Bind Time 0.68s (± 1.21%) 0.68s (± 0.76%) -0.00s (- 0.44%) 0.67s 0.69s
Check Time 5.47s (± 0.57%) 5.47s (± 0.78%) -0.01s (- 0.16%) 5.40s 5.60s
Emit Time 2.66s (± 0.77%) 2.67s (± 0.96%) +0.01s (+ 0.23%) 2.62s 2.73s
Total Time 10.74s (± 0.59%) 10.71s (± 0.51%) -0.03s (- 0.31%) 10.63s 10.89s
TFS - node (v8.9.0, x86)
Memory used 173,586k (± 0.03%) 173,574k (± 0.01%) -12k (- 0.01%) 173,532k 173,623k
Parse Time 1.57s (± 0.70%) 1.58s (± 0.80%) +0.01s (+ 0.70%) 1.56s 1.62s
Bind Time 0.62s (± 1.00%) 0.62s (± 0.76%) +0.00s (+ 0.16%) 0.61s 0.63s
Check Time 4.62s (± 0.58%) 4.66s (± 0.70%) +0.04s (+ 0.76%) 4.61s 4.73s
Emit Time 2.79s (± 0.80%) 2.79s (± 1.10%) -0.00s (- 0.11%) 2.74s 2.88s
Total Time 9.60s (± 0.36%) 9.64s (± 0.52%) +0.04s (+ 0.41%) 9.55s 9.77s
material-ui - node (v8.9.0, x86)
Memory used 262,483k (± 0.02%) 262,272k (± 0.01%) -211k (- 0.08%) 262,186k 262,365k
Parse Time 2.44s (± 0.79%) 2.44s (± 0.71%) -0.00s (- 0.04%) 2.40s 2.49s
Bind Time 0.67s (± 1.66%) 0.66s (± 1.33%) -0.00s (- 0.60%) 0.65s 0.68s
Check Time 15.45s (± 0.51%) 15.54s (± 0.50%) +0.09s (+ 0.61%) 15.43s 15.71s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 18.55s (± 0.52%) 18.64s (± 0.46%) +0.09s (+ 0.47%) 18.51s 18.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)
Benchmark Name Iterations
Current 39175 10
Baseline master 10

@Jack-Works
Copy link
Contributor Author

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

Is this a possible bug in vscode? 🤔

@DanielRosenwasser
Copy link
Member

@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 mentioned, I think this check might've caught a potential bug in VS Code in an un-awaited promise.

@mjbvz
Copy link
Contributor

mjbvz commented Jun 23, 2020

Thanks @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
Contributor Author

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
Contributor Author

How's going 👀

@sandersn sandersn added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Sep 4, 2020
@uhyo uhyo mentioned this pull request Oct 10, 2020
Closed
@typescript-bot
Copy link
Collaborator

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
Contributor Author

rebased to master and merge conflict resolved

@Jack-Works Jack-Works mentioned this pull request Nov 12, 2020
Closed
@typescript-bot
Copy link
Collaborator

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
Contributor Author

rebased to master and merge conflict resolved

@Jack-Works
Copy link
Contributor Author

rebased to master and conflict resolved

elibarzilay
elibarzilay suggested changes Feb 12, 2021
View reviewed changes
Copy link
Contributor

@elibarzilay elibarzilay left a comment

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.)

@Jack-Works Jack-Works changed the title Error on forget to use await on Promise in if condition Add error for missing await in conditional (if or ternary expr) Feb 13, 2021
@elibarzilay elibarzilay changed the title Add error for missing await in conditional (if or ternary expr) Add error for missing await in conditionals Feb 16, 2021
@elibarzilay
Copy link
Contributor

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

@elibarzilay elibarzilay merged commit bb8b9db into microsoft:master Feb 16, 2021
@Jack-Works Jack-Works deleted the await-in-cond branch February 17, 2021 07:24
This was referenced Apr 5, 2021
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elibarzilay elibarzilay elibarzilay approved these changes

Assignees

@elibarzilay elibarzilay

Labels
For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
PR Backlog
Archived in project
Milestone
No milestone
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