Skip to content
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

attributes: Apply fake return edge only to async fns #2437

Open
wants to merge 1 commit into
base: v0.1.x
Choose a base branch
from

Conversation

str4d
Copy link

@str4d str4d commented Jan 9, 2023

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.

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.
Copy link
Member

@hawkw hawkw 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 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!

Comment on lines +17 to +20
#[derive(Debug, Default)]
struct AsyncContext {
fake_return_edge: Option<proc_macro2::TokenStream>,
}
Copy link
Member

Choose a reason for hiding this comment

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

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

Suggested change
#[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?

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.

2 participants