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

Handle return type properly #6438

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion tokio-macros/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ proc-macro = true
[dependencies]
proc-macro2 = "1.0.60"
quote = "1"
syn = { version = "2.0", features = ["full"] }
syn = { version = "2.0", features = ["full", "extra-traits", "visit"] }
Copy link
Member

Choose a reason for hiding this comment

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

Both extra-traits and visit are not very good from a view of compile-time...

Copy link
Author

Choose a reason for hiding this comment

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

Haha, yup.

Copy link
Author

Choose a reason for hiding this comment

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

Do you think this is doable without these though? I'm asking more about visit, there are workarounds for the PartialEq implementation that I'm using from extra-traits.


[dev-dependencies]
tokio = { version = "1.0.0", path = "../tokio", features = ["full"] }
Expand Down
68 changes: 59 additions & 9 deletions tokio-macros/src/entry.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use proc_macro2::{Span, TokenStream, TokenTree};
use quote::{quote, quote_spanned, ToTokens};
use syn::parse::{Parse, ParseStream, Parser};
use syn::{braced, Attribute, Ident, Path, Signature, Visibility};
use syn::{braced, parse_quote, Attribute, Ident, Path, Signature, Stmt, Visibility};

// syn::AttributeArgs does not implement syn::Parse
type AttributeArgs = syn::punctuated::Punctuated<syn::Meta, syn::Token![,]>;
Expand Down Expand Up @@ -380,7 +380,64 @@ fn parse_knobs(mut input: ItemFn, is_test: bool, config: FinalConfig) -> TokenSt
}
};

let body = input.body();
let unit_type = Box::new(parse_quote! { () });
let never_type = Box::new(parse_quote! { ! });

let output_type = match &input.sig.output {
// For functions with no return value syn doesn't print anything,
// but that doesn't work as `Output` for our boxed `Future`, so
// default to `()` (the same type as the function output).
syn::ReturnType::Default => &unit_type,
syn::ReturnType::Type(_, ret_type) if ret_type == &never_type => &unit_type,
Copy link
Author

Choose a reason for hiding this comment

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

I think I can actually remove this branch

Copy link
Member

Choose a reason for hiding this comment

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

I think you can use matches!(&**ret_type, syn::Type::Never(..)) instead of ==.

syn::ReturnType::Type(_, ret_type) => ret_type,
};

enum ContainsLoopVisitor {
Copy link
Author

@richardpringle richardpringle Mar 28, 2024

Choose a reason for hiding this comment

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

Will have to rename this

Copy link
Author

@richardpringle richardpringle Mar 28, 2024

Choose a reason for hiding this comment

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

I'm using this enum to find a possible never type, but it might be tough to make this exhaustive in a forward-compatible way. There's panic!, std::process::exit, loop, and core::panicking::panic_explicit() that I know of, but there may be others.

That doesn't include any user-defined functions that return !.

I was getting compiler warnings if I output something like:

let main_body: Result<(), Error> = loop {
    some_fn().await?;
 };

(because the type is obviously wrong)

So I'm not sure that this approach really makes sense...

Found,
NotFound,
}

impl syn::visit::Visit<'_> for ContainsLoopVisitor {
Copy link
Member

Choose a reason for hiding this comment

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

Why does this need to search the input.stmts? Wouldn't it work by just checking the return value?

Copy link
Author

Choose a reason for hiding this comment

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

When you say the "return value," do you mean the last statement? I guess any statement that contained a loop that doesn't exit would have to be the last statement otherwise any statements that follow would be dead code.

So that would be an improvement, but that doesn't change the fact that using visit could potentially be expensive (with very little gained).

fn visit_expr_loop(&mut self, _: &syn::ExprLoop) {
*self = ContainsLoopVisitor::Found;
}
Copy link
Author

Choose a reason for hiding this comment

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

This is definitely super wasteful to parse the entire syntax tree until hitting a loop.
Visit has 182 methods and I could add skip! to all of them if *self == Found but that's a lof of nothing code just to fix an error message


fn visit_macro(&mut self, mac: &syn::Macro) {
// if the last segment of the macro is panic
if let Some(ident) = mac.path.get_ident() {
if ident == "panic" {
*self = ContainsLoopVisitor::Found;
}
}
}
}

let mut visitor = ContainsLoopVisitor::NotFound;

for stmt in &input.stmts {
let Ok(node): Result<Stmt, _> = syn::parse2(stmt.clone()) else {
continue;
};

syn::visit::visit_stmt(&mut visitor, &node);

if let ContainsLoopVisitor::Found = visitor {
break;
}
}

let contains_loop = matches!(visitor, ContainsLoopVisitor::Found);

let body = if output_type == &unit_type || output_type == &never_type || contains_loop {
input.body()
} else {
let body = input.body();
input.stmts = vec![quote! {
let main_body: #output_type = #body;
main_body
}];
input.body()
};

// For test functions pin the body to the stack and use `Pin<&mut dyn
// Future>` to reduce the amount of `Runtime::block_on` (and related
Expand All @@ -392,13 +449,6 @@ fn parse_knobs(mut input: ItemFn, is_test: bool, config: FinalConfig) -> TokenSt
// We don't do this for the main function as it should only be used once so
// there will be no benefit.
let body = if is_test {
let output_type = match &input.sig.output {
// For functions with no return value syn doesn't print anything,
// but that doesn't work as `Output` for our boxed `Future`, so
// default to `()` (the same type as the function output).
syn::ReturnType::Default => quote! { () },
syn::ReturnType::Type(_, ret_type) => quote! { #ret_type },
};
quote! {
let body = async #body;
#crate_path::pin!(body);
Expand Down
Loading