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

[dtolnay]

Thanks, this is great. I am on board with this if we can get the regression rate low. Luckily I have a work codebase with thousands of uses of #[async_trait]. A significant fraction of them are broken by this PR, so I will get started on triaging those.

[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!

[SergioBenitez]

Suggested change

	// __self + x
	// __self + x + a + b

[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.

[dtolnay]

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.

[dtolnay]

Do you have any insight into what's going on with this error message? The error before this PR is what I would expect and matches what you get in the non-trait case: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=f8833ba4ae1fad764bbaa11f1f6f2701. The error after seems misplaced to me.

[SergioBenitez]

Ah, interesting. I had read this message differently and believed it to be an improved version of the previous one, but I see now what the intention of the message is and how this is now a bit more confusing.

The span for that second self, which is renamed to __self, is set to the span of the receiver to fix #105. Besides being able to change only the resolution context, I'm not sure what heuristic to apply here to change only affected selfs.

[SergioBenitez]

I'm really not sure what to do here. The previous workaround seemed to "work" because we generated a new function. Now that we don't do so, we need to replace the span of all selfs, not just of the one argument to the function as before. This will result in all error messages that correspond to self to point to the receiver, which is an unfortunate compromise.

My suggestion is that we gate this workaround to rustc versions <= 1.45, or whenever the fix landed in the compiler.

[dtolnay]

Sounds good, let's do that. Thanks!

[SergioBenitez]

Have a preference for library to use? My go-to is versioncheck.

[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: tokio-rs/tracing#1228 (comment).
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 (nightmared@32b1573) 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!

[nightmared]

In order for tokio-rs/tracing#1228 to work well, could you consider adding let __async_trait: (); between the #(#decls)* and #(#stmts)* lines? That should do the job. Thanks!

[SergioBenitez]

I think this is a poor solution to the problem at hand, and that emitting warnings is, in fact, the desired effect. Nevertheless, I've pushed a commit that implements this.

[nightmared]

I do agree that it's a hack, and a nasty one at that. On the other hand, I do not believe that users will understand that exhibit A will trigger a warning and not exhibit B. That doesn't appear very coherent (at least with what I think is the behavior of instrument in non-async-trait functions) either.

Exhibit A:

#[instrument(fields(test=%v+5))]
async fn call(&mut self, v: usize) {}

Exhibit B:

#[instrument(fields(test=%_v+5))]
async fn call(&mut self, _v: usize) {}

[SergioBenitez]

I disagree that users won't understand. Without the attribute, A triggers a warning and B does not because v is unused in the body of A. Even when the attribute is applied, v continues to be unused in A, and so it is my opinion that A should continue to result in a warning.

I don't mean to imply that instrument should inconsistently emit warnings based on whether async-trait is used, but instead, that it should consistently emit warnings as would be emitted without the attribute.

[dtolnay]

Thanks, this is great.

[dtolnay]

I ended up landing this without the workaround for tracing-attributes because #143 (comment) / #143 (comment) are compelling to me.