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

Remove the old await! macro #60675

Merged
merged 1 commit into from May 10, 2019

Conversation

Projects
None yet
6 participants
@cramertj
Copy link
Member

commented May 9, 2019

This doesn't work anymore, and its continued presence is cause for confusion. yield can no longer be used to return Pending from an async body.

cc #60660
cc @taiki-e
cc tokio-rs/tokio#1080

Remove the old await! macro
This doesn't work anymore, and its continued
presence is cause for confusion.
@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented May 9, 2019

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

@taiki-e

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

Hmm... I thought that we could fix this by changing std::r#await! macro as follows and the compiler ignores await! macro (and maybe adjust the feature gate).

macro_rules! r#await {
    ($e:expr) => { 
        $e.await
    }
}

However, if it is difficult, I think this PR is fine.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

I was also assuming that we would change await! to just be foo.await -- at least for now. I guess there is a good question of when to remove support "for good".

@cramertj

This comment has been minimized.

Copy link
Member Author

commented May 9, 2019

@taiki-e Unfortunately that doesn't work because we ban yield in async bodies, so the yield inside the await! macro won't work.

@cramertj

This comment has been minimized.

Copy link
Member Author

commented May 9, 2019

@nikomatsakis I think that'd still be confusing since await!(...) doesn't call the std macro anymore.

@taiki-e

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

@cramertj

@taiki-e Unfortunately that doesn't work because we ban yield in async bodies, so the yield inside the await! macro won't work.

The current await! macro doesn't work because it uses yield internally. My suggestion is to use the await syntax instead of them. I think it works because it is almost the same as a macro such as futures::pending.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

@cramertj oh, I didn't realize that. Never mind, then.

@cramertj

This comment has been minimized.

Copy link
Member Author

commented May 9, 2019

@taiki-e yeah, we could do that, but people could also do that in an external library, and as I mentioned to @nikomatsakis, I think it runs the risk of being more confusing than helpful.

@taiki-e

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

@taiki-e yeah, we could do that, but people could also do that in an external library, and as I mentioned to @nikomatsakis, I think it runs the risk of being more confusing than helpful.

Oh, that makes sense.

@taiki-e

taiki-e approved these changes May 9, 2019

@Centril

This comment has been minimized.

Copy link
Member

commented May 9, 2019

@bors r=nikomatsakis,Centril

@bors

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

📌 Commit df41e4f has been approved by nikomatsakis,Centril

Centril added a commit to Centril/rust that referenced this pull request May 9, 2019

Rollup merge of rust-lang#60675 - cramertj:no-await-macro, r=nikomats…
…akis,Centril

Remove the old await! macro

This doesn't work anymore, and its continued presence is cause for confusion. `yield` can no longer be used to return `Pending` from an `async` body.

cc rust-lang#60660
cc @taiki-e
cc tokio-rs/tokio#1080

@Centril Centril referenced this pull request May 9, 2019

Merged

Rollup of 8 pull requests #60683

bors added a commit that referenced this pull request May 10, 2019

Auto merge of #60683 - Centril:rollup-p05qh5d, r=Centril
Rollup of 8 pull requests

Successful merges:

 - #59348 (Clean up and add tests for slice drop shims)
 - #60188 (Identify when a stmt could have been parsed as an expr)
 - #60234 (std: Derive `Default` for `io::Cursor`)
 - #60618 (Comment ext::tt::transcribe)
 - #60648 (Skip codegen for one UI test with long file path)
 - #60671 (remove unneeded `extern crate`s from build tools)
 - #60675 (Remove the old await! macro)
 - #60676 (Fix async desugaring providing wrong input to procedural macros.)

Failed merges:

r? @ghost

@bors bors merged commit df41e4f into rust-lang:master May 10, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@cramertj cramertj deleted the cramertj:no-await-macro branch May 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.