Async fn resume after completion #66321
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
| Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @zackmdavis (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 added the S-waiting-on-review | The job Click to expand the log.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 |
ninjasource force-pushed the async-fn-resume-after-completion branch from b585d99 to 238e604 Compare | The job Click to expand the log.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 |
ninjasource changed the title 
| Hi, this is my first rust compiler PR so I may need a little guidance. Been following the excellent documentation up to this point. Just a few questions:
There are more questions in the FIXME comments in the code itself. I will remove them once I have figured out what to do about them. Thanks, David. |
gilescope reviewed Nov 13, 2019
| Looks reasonable to me. |
Centril reviewed Nov 14, 2019
Centril added S-waiting-on-author 
| ☔ The latest upstream changes (presumably #66233) made this pull request unmergeable. Please resolve the merge conflicts. |
oli-obk requested changes Nov 14, 2019
| r? @oli-obk |
| Ping from triage: |
| The job Click to expand the log.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 |
ninjasource force-pushed the async-fn-resume-after-completion branch from 99565e1 to 0b1e532 Compare | The job Click to expand the log.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 |
Hi John, |
ninjasource changed the title | The job Click to expand the log.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 |
oli-obk reviewed Nov 26, 2019
| Hmm... I don't actually know how to run wasm tests. I don't even know who to ping. You can ask on zulip in the #compiler stream or on discord in the #compiler channel |
| Thanks @oli-obk, I got a response from @eddyb on Zulip and managed to run the ui tests against the wasm32-unknown-unknown target. The tests ran to completion but a third of all the tests failed so maybe you can't run them like this. Despite this I think that its worth giving the auto merge another go. I took a look at the other async tests and they all used edition:2018 instead of the compiler flags I used so I recon that that will fix the problem. I updated my tests to use edition:2018. |
| @bors r=oli-obk |
| 📌 Commit 6531ba8 has been approved by |
bors added S-waiting-on-bors bors added a commit that referenced this pull request Nov 29, 2019
…r=oli-obk Async fn resume after completion #65419 -- Attempting to run an async fn after completion mentions generators Not yet ready for review - work in progress Just need to run the tests on a proper build server
| The job Click to expand the log.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 |
| 💔 Test failed - checks-azure |
bors added S-waiting-on-review oli-obk reviewed Nov 29, 2019
| // be talking about `async fn`s instead. | ||
| | ||
| // run-fail | ||
| // error-pattern: thread 'main' panicked at '`async fn` resumed after completion' |
There was a problem hiding this comment.
So... the error is RuntimeError: unreachable. Check whether any tests check for that error pattern, or if there are any wasm panic tests and how to check them. I think it would also be OK to just add // ignore-wasm to these tests if there's no way to specify different error patterns for different platforms
There was a problem hiding this comment.
(Assuming that the ignore-wasm comment is how this is done.) Search for it in the test suite, if it exists, that's it, if not, find out how to disable tests on wasm by searching for "wasm"
There was a problem hiding this comment.
The reason it's OK to disable the tests on wasm is that here we only want to see a specific panic message. We have tests to see that panicking in general works, so assuming panicking works and the tests work on most platforms, there's basically no way they'd be broken on wasm. Wasn't just generally doesn't show the panic message, but that's not something you need to solve in this PR
| cc @rust-lang/wg-wasm See #66321 (comment) - is it possible to get panic messages from wasm, during testing? I assume |
| Thanks @oli-obk and @eddyb. I have added If my latest checkin fails then can I suggest that the tests be moved back to |
| @eddyb For raw Wasm I'm not aware of anything, but for Wasm running in JS environments there is So maybe you could use |
| 📌 Commit 851492c has been approved by |
bors added S-waiting-on-bors 
bors added a commit that referenced this pull request Nov 29, 2019
…r=oli-obk Async fn resume after completion #65419 -- Attempting to run an async fn after completion mentions generators Not yet ready for review - work in progress Just need to run the tests on a proper build server
| ☀️ Test successful - checks-azure |
This was referenced Nov 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
12 participants
Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later.
#65419 -- Attempting to run an async fn after completion mentions generators
Not yet ready for review - work in progress
Just need to run the tests on a proper build server