-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Async fn resume after completion #66321
Async fn resume after completion #66321
Conversation
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. |
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 |
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 |
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. |
Looks reasonable to me. |
☔ The latest upstream changes (presumably #66233) made this pull request unmergeable. Please resolve the merge conflicts. |
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 |
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, |
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 |
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 |
…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 |
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
…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 |
#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