Skip to content

Detect and reject method impl without body. #40

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.

Sign up for GitHub

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

Open
gmadrid wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Detect and reject method impl without body. #40

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
28 changes: 22 additions & 6 deletions fehler-macros/src/throws.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

use proc_macro::*;
use syn::fold::Fold;
use syn::{ImplItemMethod, Stmt};

use crate::Args;

Expand All @@ -23,13 +24,28 @@ impl Throws {
pub fn fold(&mut self, input: TokenStream) -> TokenStream {
if let Ok(item_fn) = syn::parse(input.clone()) {
let item_fn = self.fold_item_fn(item_fn);
quote::quote!(#item_fn).into()
} else if let Ok(method) = syn::parse(input.clone()) {
let method = self.fold_impl_item_method(method);
quote::quote!(#method).into()
} else if let Ok(method) = syn::parse(input.clone()) {
return quote::quote!(#item_fn).into();
} else if let Ok(method) = syn::parse::<ImplItemMethod>(input.clone()) {
// syn will accept a fn definition with no body as ImplItemMethod, even if the
// definition is in a trait. Attempt to detect this case and reject it.
// NOTE: this is dependent on the internals of syn and how syn chooses to interpret
// an empty method body.
let mut has_body = true;
if method.block.stmts.len() == 1 {
Copy link
Owner

@withoutboats withoutboats Apr 14, 2020

Choose a reason for hiding this comment

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

I'm surprised by this, I would have guessed that method.block.stmts would equal zero in this case (because there is no block and no stmts)? What am I missing?

Copy link
Author

@gmadrid gmadrid Apr 15, 2020

Choose a reason for hiding this comment

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

I had to look at the syn source to figure this out, which is why I have the NOTE in the comment. The syn implementor made a conscious choice to both allow the parse, and to return an Item with a single token in it, a ';'. We know it was a choice, because there is a pull request to change it from, IIRC, a Stmt. So, it's arbitrary, and if they change the implementation, this code will break. But, I didn't see a good way to detect this. I find it a somewhat questionable choice for syn to accept a parse of illegal Rust just because of an implementation detail in the Rust compiler.

if let Stmt::Item(_) = method.block.stmts[0] {
has_body = false;
}
}

if has_body {
let method = self.fold_impl_item_method(method);
return quote::quote!(#method).into();
}
}

if let Ok(method) = syn::parse(input.clone()) {
let method = self.fold_trait_item_method(method);
quote::quote!(#method).into()
return quote::quote!(#method).into();
} else {
panic!("#[throws] attribute can only be applied to functions and methods")
}
Expand Down