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

Make tokio-macros attributes more IDE friendly #4162

Merged
merged 2 commits into from Oct 11, 2021

Conversation

Veykril
Copy link
Contributor

@Veykril Veykril commented Oct 9, 2021

Motivation

Fixes #4154

Solution

As outlined in the aforementioned issue, in case the attribute fails its expansion for whatever reason, it now emits the input item alongside the compile_error! macro invocation to keep the item alive for IDE analysis.

@Noah-Kennedy Noah-Kennedy self-requested a review October 9, 2021 19:23
@taiki-e
Copy link
Member

taiki-e commented Oct 9, 2021

impl LGTM. Could you add a short comment about why this trick is necessary? (because this is not a common pattern)

@Darksonn Darksonn added the A-tokio-macros Area: The tokio-macros crate label Oct 9, 2021
@Darksonn
Copy link
Contributor

Darksonn commented Oct 9, 2021

There is an alternative to the above implementation, which seems even better: First, move this entire thing into a new function that returns a Result<FinalConfig, SomeError>:

if input.sig.asyncness.take().is_none() {
let msg = "the `async` keyword is missing from the function declaration";
return Err(syn::Error::new_spanned(input.sig.fn_token, msg));
}
let mut config = Configuration::new(is_test, rt_multi_thread);
let macro_name = config.macro_name();
for arg in args {
match arg {
syn::NestedMeta::Meta(syn::Meta::NameValue(namevalue)) => {
let ident = namevalue
.path
.get_ident()
.ok_or_else(|| {
syn::Error::new_spanned(&namevalue, "Must have specified ident")
})?
.to_string()
.to_lowercase();
match ident.as_str() {
"worker_threads" => {
config.set_worker_threads(
namevalue.lit.clone(),
syn::spanned::Spanned::span(&namevalue.lit),
)?;
}
"flavor" => {
config.set_flavor(
namevalue.lit.clone(),
syn::spanned::Spanned::span(&namevalue.lit),
)?;
}
"start_paused" => {
config.set_start_paused(
namevalue.lit.clone(),
syn::spanned::Spanned::span(&namevalue.lit),
)?;
}
"core_threads" => {
let msg = "Attribute `core_threads` is renamed to `worker_threads`";
return Err(syn::Error::new_spanned(namevalue, msg));
}
name => {
let msg = format!(
"Unknown attribute {} is specified; expected one of: `flavor`, `worker_threads`, `start_paused`",
name,
);
return Err(syn::Error::new_spanned(namevalue, msg));
}
}
}
syn::NestedMeta::Meta(syn::Meta::Path(path)) => {
let name = path
.get_ident()
.ok_or_else(|| syn::Error::new_spanned(&path, "Must have specified ident"))?
.to_string()
.to_lowercase();
let msg = match name.as_str() {
"threaded_scheduler" | "multi_thread" => {
format!(
"Set the runtime flavor with #[{}(flavor = \"multi_thread\")].",
macro_name
)
}
"basic_scheduler" | "current_thread" | "single_threaded" => {
format!(
"Set the runtime flavor with #[{}(flavor = \"current_thread\")].",
macro_name
)
}
"flavor" | "worker_threads" | "start_paused" => {
format!("The `{}` attribute requires an argument.", name)
}
name => {
format!("Unknown attribute {} is specified; expected one of: `flavor`, `worker_threads`, `start_paused`", name)
}
};
return Err(syn::Error::new_spanned(path, msg));
}
other => {
return Err(syn::Error::new_spanned(
other,
"Unknown attribute inside the macro",
));
}
}
}
let config = config.build()?;

Then, if the call to the method fails, we simply run the rest of the macro using a dummy config such as FinalConfig { flavor: current_thread, worker_threads: None, start_paused: None }, then add the compile_error! annotation at the end of that.

This way, the macro output is much closer to what it would be without the error.

@Veykril
Copy link
Contributor Author

Veykril commented Oct 10, 2021

That is definitely an even better approach 👍

@Darksonn
Copy link
Contributor

It seems like the #[tokio::test] macro isn't currently working correctly, causing CI to fail.

@Darksonn
Copy link
Contributor

Please avoid force pushing the PR. It makes it difficult for me to see what has changed since I last looked at the PR.

@Veykril
Copy link
Contributor Author

Veykril commented Oct 10, 2021

Sorry, a habit from another repo.
The sole change was that build_config mutated the signature(which was then discarded), moved the mutation to parse_knobs now.

Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

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

Looks good to me. If it also passes CI, then we can merge it.

@Darksonn
Copy link
Contributor

@Veykril Did you try whether your change actually makes rust-analyzer happy?

@Veykril
Copy link
Contributor Author

Veykril commented Oct 11, 2021

Yep, here is a gif of before the PR
8AIsxigiL4
and one of after the PR
LCsZZzMLqn
(Note everything suddenly going red is a bug in rust analyzer still, important part here are the completions)

Copy link
Contributor

@Noah-Kennedy Noah-Kennedy 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 this improvement

@Darksonn Darksonn merged commit fadd019 into tokio-rs:master Oct 11, 2021
@Veykril Veykril deleted the macros branch October 11, 2021 20:09
oliver-giersch pushed a commit to oliver-giersch/tokio that referenced this pull request Oct 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio-macros Area: The tokio-macros crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tokio's attribute macros are not IDE friendly
4 participants