attributes: Apply fake return edge only to async fns #2437
Conversation
The fake return edge was added in tokio-rs#2270 to improve type errors in instrumented async functions. However, it meant that the user block was being modified outside of `gen_block`. This created a negative interaction with tokio-rs#1614, which suppressed a clippy lint internally while explicitly enabling it on the user block. The installed fake return edge generated this same lint, but the user had no way to suppress it: lint directives above the instrumentation were ignored because of the internal suppression, and lints inside the user block could not influence the fake return edge's scope. We now avoid modifying the user block outside of `gen_block`, and restrict the fake return edge to async functions. We also apply the same clippy lint suppression technique to the installed fake return edge. Closes tokio-rs#2410.
There was a problem hiding this comment.
Thanks for the fix, this looks good to me! It might be nice to add a comment to AsyncContext explaining when the fake_return_edge field is Some and when it's None, since I was slightly confused by that at first. But that's not a hard blocker, and besides that, this change looks correct!
| #[derive(Debug, Default)] | ||
| struct AsyncContext { | ||
| fake_return_edge: Option<proc_macro2::TokenStream>, | ||
| } |
There was a problem hiding this comment.
I'm not totally sure if I understand why fake_return_edge is an Option that's set to Some after the struct is created using Default --- AFAICT, we always populate this field. It seems like the code could be a bit simpler if we changed this to just be
| #[derive(Debug, Default)] | |
| struct AsyncContext { | |
| fake_return_edge: Option<proc_macro2::TokenStream>, | |
| } | |
| #[derive(Debug)] | |
| struct AsyncContext { | |
| fake_return_edge: proc_macro2::TokenStream, | |
| } |
EDIT: wait, nevermind, I see where the Option is used --- we don't generate the fake return if the body is already an async move block rather than an async fn. It might be nice if there was a comment here explaining that, so it's clearer why this is optional?
Motivation
The fake return edge was added in #2270 to improve type errors in instrumented async functions. However, it meant that the user block was being modified outside of
gen_block. This created a negative interaction with #1614, which suppressed a clippy lint internally while explicitly enabling it on the user block. The installed fake return edge generated this same lint, but the user had no way to suppress it: lint directives above the instrumentation were ignored because of the internal suppression, and lints inside the user block could not influence the fake return edge's scope.Solution
We now avoid modifying the user block outside of
gen_block, and restrict the fake return edge to async functions. We also apply the same clippy lint suppression technique to the installed fake return edge.Closes #2410.