-
Notifications
You must be signed in to change notification settings - Fork 26
annotate #[allow(unreachable_code)] to tail expr #43
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
Conversation
fehler-macros/src/throws.rs
Outdated
| #[allow(unreachable_code)] | ||
| <_ as ::fehler::__internal::_Succeed>::from_ok(#expr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not great that this ends up suppressing legitimate unreachable_code lints inside the user's tail expr (which might be a big complicated match or something). Is there a different way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding here is certainly wrong (I was confused with the case of unit return).
It seems to be desirable to add the attribute only on an implicit unit, and inherit the original attributes when an explicit tail expr is provided.
|
This does seem like strictly an improvement.. But I wonder if we could come up with a better way that also solves the problem in non-unit returning functions? I can't think of any, though :\ |
|
Hi. Thanks to withoutboats for this nice crate and to ubnt-intrepid and dtolnay for trying to improve this wart. I found that for a function returning non-unit, the following syntax does not produce the warning: My full test program: |
|
@ijackson IMO it is the expected behavior, since there are no statements after |
|
I think the best way to do this actually would be to change the desugaring to this: let _x = { #body };
#[allow(unreachable_code)]
_Succeed::from_ok(_x)I think this would suppress the lint properly in every case. |
|
Close this PR since I've rewritten the patch as #46. |
fixes #42.