Transform blocks into `async move` for more compatibility

[SergioBenitez]

This is effectively #125 with correct drop ordering. I've kept the changes minimal, hence the temporary allow(dead_code, unused_imports), which will be removed once this is approved and I sweep out the now-unused code.

This breaks #45/#97, which from a cursory reading, looks like is a result of hard-coding previous functionality into tracing, though do let me know if there's something I can do here to maintain that compatibility.

I should add: this makes quite a few more impls in Rocket compile without needing to manually expand the async-trait or other hackery. This fixes all of the cases related to #61 in Rocket. My guess, though I haven't tested, is that this fixes #126 as well.

[SergioBenitez]

cc @nightmared @hawkw re: #45/#97

[SergioBenitez]

A significant fraction of them are broken by this PR, so I will get started on triaging those.

Awesome! Let me know if I can help with triaging some of those!

[dtolnay]

A lot of things that use self inside an expression macro fail with "borrow of moved value". Something like:

use async_trait::async_trait;

#[async_trait]
trait Trait {
    async fn f(self);
}

#[async_trait]
impl Trait for String {
    async fn f(self) {  // same for &mut self
        println!("{}", self);
    }
}

error[E0382]: borrow of moved value: `self`
  --> src/main.rs:11:27
   |
10 |     async fn f(self) {
   |                ---- value moved here
11 |         println!("{}", self);
   |                        ^^^^ value borrowed here after move
   |
   = note: move occurs because `self` has type `String`, which does not implement the `Copy` trait

[SergioBenitez]

Ah, that one's easy. So the SelfRenamer visitor only looks at Idents to rename all self to __self; the self there are arbitrary tokens in the macro call's token stream, so they're missed by the renamer.

We could visit the tokens in the token stream and rename those self's too.

Alas, I'd hope there was something more clever we could do with self. It's a real bummer to rename self to __self and then have to rename every other self in the block.

[SergioBenitez]

We could visit the tokens in the token stream and rename those self's too.

Now that I take a closer look at your original implementation, I see that this is exactly what you used to do.

[dtolnay]

Here is a "type mismatch" failure that appears to be quite common:

use async_trait::async_trait;

#[async_trait]
pub trait Trait {
    async fn f() -> Box<dyn Iterator<Item = ()>> {
        Box::new(std::iter::empty())
    }
}

error[E0271]: type mismatch resolving `<impl Future as Future>::Output == Box<(dyn Iterator<Item = ()> + 'static)>`
 --> src/main.rs:5:50
  |
5 |       async fn f() -> Box<dyn Iterator<Item = ()>> {
  |  __________________________________________________^
6 | |         Box::new(std::iter::empty())
7 | |     }
  | |_____^ expected trait object `dyn Iterator`, found struct `std::iter::Empty`
  |
  = note: expected struct `Box<(dyn Iterator<Item = ()> + 'static)>`
             found struct `Box<std::iter::Empty<_>>`
  = note: required for the cast to the object type `dyn Future<Output = Box<(dyn Iterator<Item = ()> + 'static)>> + Send`

[dtolnay]

I think those are the big two. Will check again after those are fixed. There is a third category which is some logging-related proc macros doing something tracing-like on the inner code but we can get that fixed to recognize Box::pin(async move {...}). As far as I can tell all the failures involve one of these three: self in a macro argument, trait object in return type, or hacky proc macro which is fine to break.

[dtolnay]

One more thing: this regresses #105 on compilers 1.46 and older, which is this failure in the 1.40 CI job:

error[E0425]: cannot find value `__self` in this scope
   --> tests/test.rs:599:13
    |
599 |             #[async_trait]
    |             ^^^^^^^^^^^^^^ not found in this scope

If it's easy, it would be good to get that worked around again even if new rustc fixed the bug. It was only a 1 line workaround in #105.

[nightmared]

I'll take a look at this. I guess the proper course of action would be to support both the old and the new behavior ? if so, looks like we need to do some working on tracing-attributes :p

[dtolnay]

I tried the latest commit -- it's regressing some code like this:

use async_trait::async_trait;

#[async_trait]
pub trait Trait: Sized {
    async fn f(self) {
        struct Struct;
        impl Struct {
            fn f(self) {
                let _ = self;
            }
        }
    }
}

error[E0434]: can't capture dynamic environment in a fn item
 --> src/main.rs:9:25
  |
9 |                 let _ = self;
  |                         ^^^^
  |
  = help: use the `|| { ... }` closure form instead

[dtolnay]

Also this:

#![deny(warnings)]

use async_trait::async_trait;

#[async_trait]
pub trait Trait {
    async fn f() {
        unimplemented!()
    }
}

error: unreachable expression
 --> src/main.rs:7:18
  |
7 |       async fn f() {
  |  __________________^
8 | |         unimplemented!()
  | |         ---------------- any code following this expression is unreachable
9 | |     }
  | |_____^ unreachable expression

[SergioBenitez]

Alright, that last commit should handle all of the regressions thus far. As a side effect, some error messages have improved. Feel free to hit me with more regressions.

I've also marked the tracing test should_panic for now.

[SergioBenitez]

Oh, perhaps I missed the point of the unreachable example you showed. I see the deny warnings now. Makes sense. Will take a look a bit later.

Edit: handled.

[SergioBenitez]

This looks good to me.

I'll give a little time before merging to give the tracing folks a chance to get an update out.

Awesome. I'll remove all of the unused code then.

[nightmared]

On some minor note, it is not so easy to handle arguments that aren't used inside the function: https://github.com/tokio-rs/tracing/pull/1228#issuecomment-774549503. From what I could gather, async-trait redefines the variable v with a let binding, but in the process of doing so, it introduces an unused variable. If I'm not mistaken, the logic is the following: a) async_trait rewrites the body of async fn call(&mut self, v: usize) {} in this fashion:

Box::pin(async move {
  let __ret: () = {
      let mut __self = self;
      let v = v;
  };
  #[allow(unreachable_code)]
  __ret
})

So far, it have the same behavior as an equivalent sync function would have, so the warning is expected, and thus not an issue. b) instrument then rewrites the body as:

Box::pin(async move {
  XXX // something that uses the `v` variable
  tracing::Instrument::instrument(
      async move {
          {
              let __ret: () = {
                  let mut __self = self;
                  let v = v;
              };
              #[allow(unreachable_code)]
          __ret
        }
      },
      __tracing_attr_span,
    )
    .await
})

In that case, the v variable is now used by code originated from tracing, but there is still the let v = v; binding, which is still an unused variable. Because they have the same span, the compiler reports the error as originating from the function signature:

warning: unused variable: `v`
   --> tracing-attributes/tests/async_fn.rs:176:34
    |
176 |         async fn call(&mut self, v: usize) {}
    |                                  ^ help: if this is intentional, prefix it with an underscore: `_v`
    |
    = note: `#[warn(unused_variables)]` on by default

warning: 1 warning emitted

I'm not really sure about the way to go here. Optimally, we would inject instrument code after async-trait, but I don't think we can force ordering (or this whole story of async-trait/tracing integration would never have been a real deal) of procedural macros. Any idea?

[dtolnay]

@nightmared, would it help if we insert some unique distinguishing marker like let __async_trait: (); in between the let statements and the user's original function body?

Box::pin(async move {
    let __ret: #RETURN = {
        let v = v;
        let __async_trait: ();
        #BODY
    };
    #[allow(unreachable_code)]
    __ret
})

You could crawl the macro input and replace that marker with:

Box::pin(async move {
    let __ret: #RETURN = {
        let v = v;
        if false {
            let _ = &(v + 5);  // expr from #[instrument(fields(test=%v+5))]
        }
        #BODY
    };
    #[allow(unreachable_code)]
    __ret
})

[nightmared]

Hmm yes, i guess this would work! However that also means we would have to follow variable reassignments like self being renamed to __self. Given I'm not really the one that will bear the burden of maintaining that compatibility in the long run (not being a maintainer, nor even an active contributor :p), I guess the best is to defer that decision to e.g. @hawkw and/or @davidbarsky. That being said, it would probably be the cleanest way to properly place the code generated by #[instrument], and solve this issue altogether. It's probably fairly easy too: we would just have to wrap all the statements from the position of __async_trait to the end of the block.

[nightmared]

I have started to take a look at @dtolnay's solution, but it isn't quite as trivial as I first thought because we need to keep track of variable reassignments (e.g. let __self = self). I will keep you posted on my progress.

[SergioBenitez]

To propose a different solution: what if nothing is done about the warning? The warning feels correct to me: the variable is declared and never used in the function body by the written code. Purposefully silencing this warning could result in incorrect code that otherwise would have been caught. It's also easy enough to add an _ should they really intend not to use the variable. Of course, this would imply emitting code in the non-async_trait case that retains the warning, but that perhaps entails an easier and less brittle solution, perhaps one that mirrors the redefinitions in this PR.

[nightmared]

That is a possibility yes. However with some very minimal changes in async-trait (https://github.com/nightmared/async-trait/commit/32b1573a9d237ad3b277f20645e85711c0838cd7) and a bit more work on tracing side, I was able to get this minor nitpick working ;) We'll see what the maintainers of tracing think of this hack.

[SergioBenitez]

@dtolnay I'm continuing to work to improve async-trait in my local branch. When you're ready to merge this, let me know if you'd prefer squashing into fewer commits and/or multiple PRs.

[Kiiyya]

I am getting rather odd build errors with a minimal project with commit 0cda89b somehow:

#[async_trait::async_trait]
pub trait Derp {
    async fn hi();
}

fails to build with internal errors, both latest stable and nightly. Note that I literally just have those 4 lines + a dummy main(): https://gist.github.com/Kiiyya/250dc864682cc1fc256f6ac6135db45e And I have no clue what is going on, but I thought I'd just drop this here in hopes it helps, somehow?

For some reason, using that exact same commit in a way bigger project of mine compiles, I am properly confused about that elusive build error.

[SergioBenitez]

@Kiiyya Your larger project is probably enabling features of syn that result in Debug being implemented on those types while async-trait doesn't enable such features. This is was an omission on my part ... to omit the Debug derive. I've pushed a commit that should resolve the issue! Thanks for testing.

[Kiiyya]

Yep works now, that was quick, thank you!

[dtolnay]

I ended up landing this without the workaround for tracing-attributes because https://github.com/dtolnay/async-trait/pull/143#issuecomment-777964508 / https://github.com/dtolnay/async-trait/pull/143#discussion_r583208853 are compelling to me.