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

Detect and reject method impl without body. #40

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

Conversation

gmadrid
Copy link

@gmadrid gmadrid commented Apr 14, 2020

Fixes #39

This is some ugly Rust. I have always found the interaction of if and if let to be a bit awkward. I'm very interested in seeing how you clean this up.

Love fehler. Totally agree with your arguments in your article.

@withoutboats
Copy link
Owner

Instead of rejecting it, could we make it generate a modified method signature with no body?

// 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

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

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.

@withoutboats
Copy link
Owner

Also, it's not ugly. :) Manipulating Rust's very complex AST just requires code like this to handle all of the special cases.

@gmadrid
Copy link
Author

gmadrid commented Apr 15, 2020

I frequently want to write code like 'if let Some(foo) = bar && foo.blah() != 3' but I can't, so I have to nest another if inside it which complicates the data flow in a way that I find less functional and therefore a little less clear. I was kind of hoping you'd "clean it up" and I'd see how you handle this. You have a lot more Rust experience than I do.

@withoutboats
Copy link
Owner

There's not really a better way around it. Well, for the example you gave, this could be more elegant:

match bar {
    Some(foo) if foo.blah() != 3 => { ... }
    _ => {}
}

But in cases like this one, where the conditional is not the binding from the match, that would be confusing.

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.

throws inserts method body for trait methods
2 participants