-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Don't lint manual_assert
in else if
#10013
Conversation
r? @Manishearth (rustbot has picked a reviewer for you, use r? to override) |
@@ -47,6 +47,7 @@ impl<'tcx> LateLintPass<'tcx> for ManualAssert { | |||
if cx.tcx.item_name(macro_call.def_id) == sym::panic; | |||
if !cx.tcx.sess.source_map().is_multiline(cond.span); | |||
if let Some(format_args) = FormatArgsExpn::find_nested(cx, then, macro_call.expn); | |||
if !is_else_clause(cx.tcx, 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.
I don't think linting here is wrong. Couldn't we just wrap the suggestion in { ... }
in the else_clause
case?
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.
That would also work, but would probably annoy people who would otherwise like the lint (me being one of them). It could be a config to enable linting a trailing else if
.
Linting here doesn't have the same benefit of removing the whole if expression the other cases do. Changing
} else if foo {
panic!("oh no!");
}
to
} else {
assert!(foo, "oh no!");
}
is an even more pedantic change than the regular case. Because the else
clause still needs to exist, all this accomplishes is shuffling the condition around, rather than remove a construct from the code.
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.
Yeah, I kind of agree with jarcho here.
However, I would like a comment here stating why we skip the else clause
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
No need for a config option. The reason given by Jarcho totally makes sense. I don't think this lint will gain much from a config value then. |
fixes #9886
changelog:
manual_assert
: Don't lint inelse if