Add manual_checked_div lint #16149
Add manual_checked_div lint #16149
Conversation
rustbot
added
needs-fcp
S-waiting-on-review
|
rustbot has assigned @samueltardieu. Use |
|
Lintcheck changes for bf07325
This comment will be updated if you push new changes |
| NonZeroBranch::Else => match r#else.map(|expr| &expr.kind) { | ||
| Some(ExprKind::Block(block, _)) => Some(block), | ||
| _ => None, | ||
| }, |
There was a problem hiding this comment.
| NonZeroBranch::Else => match r#else.map(|expr| &expr.kind) { | |
| Some(ExprKind::Block(block, _)) => Some(block), | |
| _ => None, | |
| }, | |
| NonZeroBranch::Else => match r#else?.kind { | |
| ExprKind::Block(block, _) => Some(block), | |
| _ => None, | |
| }, |
| let Some(block) = branch_block(then, r#else, branch) else { | ||
| return; | ||
| }; |
There was a problem hiding this comment.
this could be moved into the let-chain as well
| if expr.span.from_expansion() { | ||
| return; | ||
| } |
There was a problem hiding this comment.
this could be moved into the let-chain as well
| let lhs_snip = Sugg::hir_with_applicability(cx, lhs, "..", &mut applicability); | ||
| let rhs_snip = Sugg::hir_with_applicability(cx, rhs, "..", &mut applicability); |
There was a problem hiding this comment.
For single expressions, the preferred placeholder is "_"
| let lhs_snip = Sugg::hir_with_applicability(cx, lhs, "..", &mut applicability); | |
| let rhs_snip = Sugg::hir_with_applicability(cx, rhs, "..", &mut applicability); | |
| let lhs_snip = Sugg::hir_with_applicability(cx, lhs, "_", &mut applicability); | |
| let rhs_snip = Sugg::hir_with_applicability(cx, rhs, "_", &mut applicability); |
| let Some(block) = branch_block(then, r#else, branch) else { | ||
| return; | ||
| }; | ||
| let mut eq = SpanlessEq::new(cx); |
| span_lint_and_sugg( | ||
| cx, | ||
| MANUAL_CHECKED_DIV, | ||
| e.span, |
There was a problem hiding this comment.
It could be nice to also highlight the span where the variable (let's call it b) is checked for being non-zero, which you could do by passing MultiSpan::new(vec![cond.span, e.span]).
But with that, if there were multiple instances of a / b (however unlikely that is), each firing of the lint would show the said checking place, which would be suboptimal. Because of this, a better solution could be to first collect all the instances of a / b (where each one might have a different a!), then lint on all of them at once (if there are any, of course). Now that I've written all this, it does sound like a bit too much work -- if you want to give it a try, feel free, but otherwise just highlighting both the condition and the division would be a good start.
| // Should NOT trigger (signed integers) | ||
| let c = -5i32; | ||
| if c != 0 { | ||
| let _result = 10 / c; | ||
| } |
There was a problem hiding this comment.
Sorry, why exactly do we not lint here?... I see that the original issue only talks about unsigned integers, but I think signed ones should be fine as well?
|
☔ The latest upstream changes (possibly a10cafe) made this pull request unmergeable. Please resolve the merge conflicts. |
4 participants
changelog: [
manual_checked_div]: new lint suggestingchecked_divinstead of manual zero checks before unsigned divisionImplements the manual checked div from #12894. I'm relatively new to Rust and a complete newbie at Clippy, so my apologies if I forgot anything. I looked through the linked issue and could not find anything talking about the above lint.