Skip to content

Conversation

@jocutajar
Copy link

@jocutajar jocutajar commented Aug 31, 2020

  • This change adds a virtual #[future_is[BOUNDS]] attribute for async fns so you can opt in for Sync or 'static future
  • Refactored to async move block instead of inner function as it removed a whole lot of complexity and enabled the 'static stuff
  • closes Future type returned by trait methods should impl Sync #77

@jocutajar jocutajar changed the title Feature sync futures #77 Optional sync futures, async block Aug 31, 2020
@jocutajar
Copy link
Author

jocutajar commented Aug 31, 2020

Hi @dtolnay, kindly review.

[SOLVED] Currently when I take my fork as a dependency it fails on syn::* types equality. Any idea? While all is green in async-trait.

[dependencies]
async-trait = { git = "https://github.com/BrightOpen/async-trait.git", branch = "feature/sync-futures-#77" }

results in a few of these:

error[E0369]: binary operation `==` cannot be applied to type `syn::Path`
   --> /home/debian/.cargo/git/checkouts/async-trait-47bf8a75611e2af0/0fe069a/src/expand.rs:113:22
    |
113 |         if attr.path == model.path
    |            --------- ^^ ---------- syn::Path
    |            |
    |            syn::Path

* generated README.md from lib.rs with `cargo readme`
* more tests
* documentation ofnew feature and improved support for elided lifetimes.
@jocutajar
Copy link
Author

binary operation == cannot be applied to type syn::Path

Solved thanks to @link2xt - added "extra-traits" feature for syn.

Meanwhile, added docs and tests, updated README.md

Copy link
Owner

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. I don't think the async block stuff is a correct change. See for example this code:

use async_trait::async_trait;

#[async_trait]
trait Trait {
    async fn f(_: Plop);
}

struct Struct;

#[async_trait]
impl Trait for Struct {
    async fn f(_: Plop) {
        println!("Struct::f");
    }
}

struct Plop;

impl Drop for Plop {
    fn drop(&mut self) {
        println!("plop");
    }
}

async fn standalone_f(_: Plop) {
    println!("standalone_f");
}

fn main() {
    futures::executor::block_on(Struct::f(Plop));
    futures::executor::block_on(standalone_f(Plop));
}

Before:

Struct::f
plop
standalone_f
plop

This PR:

plop
Struct::f
standalone_f
plop

As you can see, before this PR the async trait method had behavior that is consistent with an async non-trait method, and after this PR it does something different.

@jocutajar
Copy link
Author

Hi @dtolnay, thanks for the feedback.

From your example, I understand that an unused value is dropped before the async block is executed. A useful optimization in the discretion of the compiler without effect on functionality. Perhaps the next version of rust will optimize the async fn similarly...

What would happen if you used the variable in the printout or explicitly dropped it after?

Copy link
Owner

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

A useful optimization in the discretion of the compiler without effect on functionality.

This is not correct.

Perhaps the next version of rust will optimize the async fn similarly.

This would definitely not be allowed.

Drop reordering is not "an optimization". Drop order is extremely sensitive and governed by language semantics, not compiler optimization. Drop happening in a precisely defined place is what makes mutex guards possible, for example.

@dtolnay
Copy link
Owner

dtolnay commented Sep 22, 2020

I think I'll close for now, since this is pretty hard to review with the unrelated changes included together. Please open a new PR without drop order changes or the readme changes.

@dtolnay dtolnay closed this Sep 22, 2020
@taiki-e taiki-e mentioned this pull request Oct 1, 2020
@jocutajar
Copy link
Author

jocutajar commented Oct 29, 2020

The improvements plus better support for 'static futures are available in a fork samotop-async-trait

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Future type returned by trait methods should impl Sync

2 participants