Skip to content

Implement #[async_send] attribute for async fns #107056

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

Closed
wants to merge 5 commits into from

Conversation

eholk
Copy link
Contributor

@eholk eholk commented Jan 19, 2023

This gives a potential solution for the spawning from generics issue with async functions in traits. The goal here is to make something that users can experiment with and provide feedback on, but this is not intended to be the final syntax or even final design.

r? @compiler-errors

cc @rust-lang/wg-async

This gives a potential solution for the spawning from generics issue
with async functions in traits. The goal here is to make something that
users can experiment with and provide feedback on, but this is not
intended to be the final syntax or even final design.
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jan 19, 2023
@rustbot
Copy link
Collaborator

rustbot commented Jan 19, 2023

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@compiler-errors
Copy link
Member

compiler-errors commented Jan 19, 2023

This needs several test, both for functionality and for the feature gate (proving that adding this attribute requires enabling a feature gate)

@eholk
Copy link
Contributor Author

eholk commented Jan 19, 2023

This needs a test, both for functionality and for the feature gate (proving that adding this attribute requires enabling a feature gate)

Oh, d'oh! I forgot to add the file with my functional test. I need to add one for the feature gate too though.

@@ -1967,6 +1968,31 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
));
debug!("lower_async_fn_ret_ty: generic_params={:#?}", generic_params);

let is_async_send = this.tcx.features().async_fn_in_trait
&& (true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The true definitely isn't! (Sorry, that was left over from by debugging attempts).

The async_fn_in_trait check is probably not needed either since you can't even type this attribute without it. I'll try and remove it.

@eholk eholk marked this pull request as draft January 19, 2023 00:32
@eholk
Copy link
Contributor Author

eholk commented Jan 19, 2023

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 19, 2023
@rust-log-analyzer

This comment has been minimized.

@eholk eholk marked this pull request as ready for review January 19, 2023 22:09
@eholk
Copy link
Contributor Author

eholk commented Jan 19, 2023

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 19, 2023
@rust-log-analyzer

This comment has been minimized.

Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation looks fine to me, though I was curious if there was some support from @rust-lang/wg-async or @rust-lang/lang for adding this. I was unsure if this was the direction that we wanted to go with adding support for Send, etc. to futures.

@eholk
Copy link
Contributor Author

eholk commented Jan 20, 2023

I'd also love to hear other opinions from @rust-lang/wg-async. I did talk with @yoshuawuyts about this idea earlier this week and he seemed supportive of it, although mostly for experimentation and not as the final version.

As for me, I'll admit I don't particularly like this solution. I'd rather land on something like async<Send> fn foo(), but I think it's not too hard to go from this PR to something like that. I like the idea of merging this sooner though so there's something in nightly that people can play around with.

I think this question of Send bounds on async methods is something we should sort out before stabilizing AFIT, so the sooner we can get experience with solutions and start iterating on them the better.

@nrc
Copy link
Member

nrc commented Jan 24, 2023

I think this is fine for experimentation. I think we'll want other syntax in the future, but for now, this is fine.

@eholk
Copy link
Contributor Author

eholk commented Jan 24, 2023

Based on some of the objections raised by @Dirbaio in the Zulip thread, I'm kind of inclined to hold off on merging this for now. There's not really any harm in merging it for experimentation, but if we aren't confident that this or something like it is something we'll want long term, there's not a lot of point in merging something that we'll have to remove later.

@compiler-errors
Copy link
Member

Gonna mark this as S-blocked for now then.

@rustbot blocked

@rustbot rustbot added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 26, 2023
@joshtriplett joshtriplett added T-lang Relevant to the language team, which will review and decide on the PR/issue. I-lang-nominated Nominated for discussion during a lang team meeting. and removed I-lang-nominated Nominated for discussion during a lang team meeting. labels Jan 29, 2023
@bors
Copy link
Collaborator

bors commented May 4, 2023

☔ The latest upstream changes (presumably #111169) made this pull request unmergeable. Please resolve the merge conflicts.

@compiler-errors
Copy link
Member

I'm gonna close this since it's gone somewhat stale, and I'm not sure if this is the approach we're looking at experimenting with anymore...

Obviously this doesn't delete the branch or anything so please feel free to reopen a new PR and request review from me if wg-async does decide that this is worth landing experimentally...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-blocked Status: Blocked on something else such as an RFC or other implementation work. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants