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

tests for async/await drop order #63294

Merged
merged 4 commits into from
Aug 7, 2019
Merged

tests for async/await drop order #63294

merged 4 commits into from
Aug 7, 2019

Conversation

alsuren
Copy link
Contributor

@alsuren alsuren commented Aug 5, 2019

This is just me helping out with #62121 where I can.

I'm also going to use this as a public place to collect my thoughts about what has already been done and what hasn't (adding comments to the dropbox paper doc was quickly getting spammy).

I've tried to keep my commit messages similar to the line items on https://paper.dropbox.com/doc/async.await-Call-for-Tests--AiKouT0L41mSnK1741s~TiiRAg-nMyZGrra7dz9KcFRMLKJy as possible.

A bunch of my tests are likely to be either redundant with other tests, or lower quality than other tests that people are writing. A reasonable approach might be to tell me which commits you want to keep and I'll throw away the rest of them.

The part from the dropbox paper doc that I'm concentrating on here is:
(items marked with ? are ones that I can't immediately think of how to test, so I will leave for other people. Items with checkboxes are things that I have done or will try to do next)

Dynamic semantics

  • async/await with unusual locals:
  • Control flow:
    • basic
    • complex
  • .await while holding variables of different sizes
  • (possibly) drop order
  • ? interaction with const eval, promoteds, and temporaries
  • drop the resulting future and check that local variables and parameters are dropped in the expected order (interaction with cancellation, in other words)

Explanation of commits:

  • 0a1bdd4 is the simplest place I could think of to explicitly test .await while holding variables of different sizes. I'm pretty sure that this will end up being redundant with something else, so I'm happy to drop it.
  • f40190a is a copy-paste from drop-order-for-async-fn-parameters.rs with NeverReady.await dumped on the end of each testcase.
    • Normally I don't like copy-paste-based tests, but drop-order-for-async-fn-parameters-by-ref-binding.rs is also copy-paste, so I thought it might be okay.
    • I'm a bit sad that this doesn't cover non-parameter locals, but I think it should be easy enough to extend in that direction, so I might have a crack at that tomorrow.
  • c4940e0 makes a bunch of local variables and moves them into either {} blocks or async move {} blocks, checking for any surprising differences.
    • I have tried to give the test functions descriptive names
    • I have not duplicated the tests for methods with/without self.
    • I think that all of these tests could be rewritten to be clearer if I could write down the expected drop order next to each test.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 5, 2019
@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-6.0 of your PR failed (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
2019-08-05T16:24:29.0644832Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-08-05T16:24:29.0831785Z ##[command]git config gc.auto 0
2019-08-05T16:24:29.0900769Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-08-05T16:24:29.0961266Z ##[command]git config --get-all http.proxy
2019-08-05T16:24:29.1099564Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/63294/merge:refs/remotes/pull/63294/merge
---
2019-08-05T16:25:04.3108729Z do so (now or later) by using -b with the checkout command again. Example:
2019-08-05T16:25:04.3108988Z 
2019-08-05T16:25:04.3109465Z   git checkout -b <new-branch-name>
2019-08-05T16:25:04.3109739Z 
2019-08-05T16:25:04.3109953Z HEAD is now at 6e1e72d49 Merge f40190a6a541479e504d783cd984670c6fa6cb39 into 4be067558962c004b638e4c6f162d50f7c0c98b6
2019-08-05T16:25:04.3256136Z ##[section]Starting: Collect CPU-usage statistics in the background
2019-08-05T16:25:04.3258445Z ==============================================================================
2019-08-05T16:25:04.3258895Z Task         : Bash
2019-08-05T16:25:04.3258946Z Description  : Run a Bash script on macOS, Linux, or Windows
---
2019-08-05T16:30:50.1786949Z    Compiling serde_json v1.0.40
2019-08-05T16:30:54.3462222Z    Compiling tidy v0.1.0 (/checkout/src/tools/tidy)
2019-08-05T16:31:02.8425266Z     Finished release [optimized] target(s) in 1m 27s
2019-08-05T16:31:03.4407251Z tidy check
2019-08-05T16:31:03.4408392Z tidy error: /checkout/src/test/ui/async-await/drop-order/drop-order-when-cancelled.rs:202: trailing whitespace
2019-08-05T16:31:04.7512593Z some tidy checks failed
2019-08-05T16:31:04.7513400Z 
2019-08-05T16:31:04.7513400Z 
2019-08-05T16:31:04.7514539Z command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor"
2019-08-05T16:31:04.7514888Z 
2019-08-05T16:31:04.7514974Z 
2019-08-05T16:31:04.7518122Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
2019-08-05T16:31:04.7518357Z Build completed unsuccessfully in 0:01:30
2019-08-05T16:31:04.7518357Z Build completed unsuccessfully in 0:01:30
2019-08-05T16:31:06.2473775Z ##[error]Bash exited with code '1'.
2019-08-05T16:31:06.2523112Z ##[section]Starting: Checkout
2019-08-05T16:31:06.2524581Z ==============================================================================
2019-08-05T16:31:06.2524635Z Task         : Get sources
2019-08-05T16:31:06.2524676Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@Centril
Copy link
Contributor

Centril commented Aug 5, 2019

r? @cramertj

@alsuren alsuren changed the title WIP: async/await tests async/await tests for drop order Aug 6, 2019
@alsuren alsuren changed the title async/await tests for drop order tests for async/await drop order Aug 6, 2019
@cramertj
Copy link
Member

cramertj commented Aug 6, 2019

This looks great, thanks for the PR!

@bors r+

@bors
Copy link
Contributor

bors commented Aug 6, 2019

📌 Commit c4940e0 has been approved by cramertj

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 6, 2019
@gorup
Copy link
Contributor

gorup commented Aug 6, 2019

I posted a PR for the conditional initialization item here: #63331

Centril added a commit to Centril/rust that referenced this pull request Aug 6, 2019
tests for async/await drop order

This is just me helping out with rust-lang#62121 where I can.

I'm also going to use this as a public place to collect my thoughts about what has already been done and what hasn't (adding comments to the dropbox paper doc was quickly getting spammy).

I've tried to keep my commit messages similar to the line items on https://paper.dropbox.com/doc/async.await-Call-for-Tests--AiKouT0L41mSnK1741s~TiiRAg-nMyZGrra7dz9KcFRMLKJy as possible.

A bunch of my tests are likely to be either redundant with other tests, or lower quality than other tests that people are writing. A reasonable approach might be to tell me which commits you want to keep and I'll throw away the rest of them.

The part from the dropbox paper doc that I'm concentrating on here is:
(items marked with `?` are ones that I can't immediately think of how to test, so I will leave for other people. Items with checkboxes are things that I have done or will try to do next)

### Dynamic semantics
- `async`/`await` with unusual locals:
    - ? partially uninhabited
    - ? conditionally initialized
    - ~drop impls~ already done in src/test/ui/async-await/drop-order/*
    - ? nested drop impls
    - ~partially moved (e.g., `let x = (vec![], vec![]); drop(x.0); foo.await; drop(x.1);`)~ see  rust-lang#63310
- Control flow:
    - basic
    - complex
- [x] `.await` while holding variables of different sizes
- (possibly) drop order
    - [x] including drop order for locals when a future is dropped part-way through execution
         - Parameters' drop order is covered in my commit f40190a
    - ~An async fn version of `dynamic-drop.rs`~
        - already done by matthewjasper in https://github.com/rust-lang/rust/pull/62193/files
- ? interaction with const eval, promoteds, and temporaries
- [x] drop the resulting future and check that local variables and parameters are dropped in the expected order (interaction with cancellation, in other words)
    - also in f40190a

Explanation of commits:

* 0a1bdd4 is the simplest place I could think of to explicitly test `.await` while holding variables of different sizes. I'm pretty sure that this will end up being redundant with something else, so I'm happy to drop it.
* f40190a is a copy-paste from `drop-order-for-async-fn-parameters.rs` with `NeverReady.await` dumped on the end of each testcase.
    * Normally I don't like copy-paste-based tests, but `drop-order-for-async-fn-parameters-by-ref-binding.rs` is also copy-paste, so I thought it might be okay.
    * [x] I'm a bit sad that this doesn't cover non-parameter locals, but I think it should be easy enough to extend in that direction, so I might have a crack at that tomorrow.
* c4940e0 makes a bunch of local variables and moves them into either `{}` blocks or `async move {}` blocks, checking for any surprising differences.
    * I have tried to give the test functions descriptive names
    * I have not duplicated the tests for methods with/without self.
    * I think that all of these tests could be rewritten to be clearer if I could write down the expected drop order next to each test.
bors added a commit that referenced this pull request Aug 7, 2019
Rollup of 9 pull requests

Successful merges:

 - #63034 (Fix generator size regressions due to optimization)
 - #63035 (Use MaybeUninit to produce more correct layouts)
 - #63163 (add a pair of whitespace after remove parentheses)
 - #63294 (tests for async/await drop order)
 - #63307 (don't ignore mir_dump folder)
 - #63308 (PlaceRef's base is already a reference)
 - #63310 (Tests around moving parts of structs and tuples across await points)
 - #63314 (doc: the content has since been moved to the guide)
 - #63333 (Remove unnecessary features from compiler error code list)

Failed merges:

r? @ghost
@bors bors merged commit c4940e0 into rust-lang:master Aug 7, 2019
Centril added a commit to Centril/rust that referenced this pull request Aug 7, 2019
Test conditional initialization validation in async fns

r? @cramertj

Per [paper doc](https://paper.dropbox.com/doc/async.await-Call-for-Tests--AiWF2Nt8tgDiA70qFI~oiLOOAg-nMyZGrra7dz9KcFRMLKJy) calling for async/.await tests, tests are desired for conditionally initialized local variables. This PR hopes to provide tests for that.

rust-lang#63294 seems to be tracking the items from the paper doc that this PR is related to
rust-lang#62121 is an open issue asking for more async/.await tests that this relates to

---
:+1: executed 2 new tests
:+1: tidy
pietroalbini added a commit to pietroalbini/rust that referenced this pull request Aug 7, 2019
Test conditional initialization validation in async fns

r? @cramertj

Per [paper doc](https://paper.dropbox.com/doc/async.await-Call-for-Tests--AiWF2Nt8tgDiA70qFI~oiLOOAg-nMyZGrra7dz9KcFRMLKJy) calling for async/.await tests, tests are desired for conditionally initialized local variables. This PR hopes to provide tests for that.

rust-lang#63294 seems to be tracking the items from the paper doc that this PR is related to
rust-lang#62121 is an open issue asking for more async/.await tests that this relates to

---
:+1: executed 2 new tests
:+1: tidy
Centril added a commit to Centril/rust that referenced this pull request Aug 8, 2019
Test conditional initialization validation in async fns

r? @cramertj

Per [paper doc](https://paper.dropbox.com/doc/async.await-Call-for-Tests--AiWF2Nt8tgDiA70qFI~oiLOOAg-nMyZGrra7dz9KcFRMLKJy) calling for async/.await tests, tests are desired for conditionally initialized local variables. This PR hopes to provide tests for that.

rust-lang#63294 seems to be tracking the items from the paper doc that this PR is related to
rust-lang#62121 is an open issue asking for more async/.await tests that this relates to

---
:+1: executed 2 new tests
:+1: tidy
Centril added a commit to Centril/rust that referenced this pull request Aug 8, 2019
Test conditional initialization validation in async fns

r? @cramertj

Per [paper doc](https://paper.dropbox.com/doc/async.await-Call-for-Tests--AiWF2Nt8tgDiA70qFI~oiLOOAg-nMyZGrra7dz9KcFRMLKJy) calling for async/.await tests, tests are desired for conditionally initialized local variables. This PR hopes to provide tests for that.

rust-lang#63294 seems to be tracking the items from the paper doc that this PR is related to
rust-lang#62121 is an open issue asking for more async/.await tests that this relates to

---
:+1: executed 2 new tests
:+1: tidy
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants