From 1eb41804a28b8e9b7643a61f3c224a06aba1f433 Mon Sep 17 00:00:00 2001 From: Jacob Kiesel Date: Tue, 13 Sep 2022 23:50:47 -0600 Subject: [PATCH 1/4] `bool_to_int_with_if` inverse case patch --- clippy_lints/src/bool_to_int_with_if.rs | 28 +++++++++++++++---- tests/ui/bool_to_int_with_if.fixed | 6 ++++ tests/ui/bool_to_int_with_if.rs | 14 ++++++++++ tests/ui/bool_to_int_with_if.stderr | 37 +++++++++++++++++++++---- 4 files changed, 73 insertions(+), 12 deletions(-) diff --git a/clippy_lints/src/bool_to_int_with_if.rs b/clippy_lints/src/bool_to_int_with_if.rs index a4b8cbb0d82a..bea74279e033 100644 --- a/clippy_lints/src/bool_to_int_with_if.rs +++ b/clippy_lints/src/bool_to_int_with_if.rs @@ -55,15 +55,28 @@ fn check_if_else<'tcx>(ctx: &LateContext<'tcx>, expr: &'tcx rustc_hir::Expr<'tcx if let ExprKind::If(check, then, Some(else_)) = expr.kind && let Some(then_lit) = int_literal(then) && let Some(else_lit) = int_literal(else_) - && check_int_literal_equals_val(then_lit, 1) - && check_int_literal_equals_val(else_lit, 0) { + let inverted = if + check_int_literal_equals_val(then_lit, 1) + && check_int_literal_equals_val(else_lit, 0) { + false + } else if + check_int_literal_equals_val(then_lit, 0) + && check_int_literal_equals_val(else_lit, 1) { + true + } else { + // Expression isn't boolean, exit + return; + }; let mut applicability = Applicability::MachineApplicable; let snippet = snippet_block_with_applicability(ctx, check.span, "..", None, &mut applicability); + + let invert = if inverted { "!" } else { "" }; + let need_parens = should_have_parentheses(check); + let snippet_with_braces = { - let need_parens = should_have_parentheses(check); let (left_paren, right_paren) = if need_parens {("(", ")")} else {("", "")}; - format!("{left_paren}{snippet}{right_paren}") + format!("{invert}{left_paren}{snippet}{right_paren}") }; let ty = ctx.typeck_results().expr_ty(then_lit); // then and else must be of same type @@ -71,11 +84,14 @@ fn check_if_else<'tcx>(ctx: &LateContext<'tcx>, expr: &'tcx rustc_hir::Expr<'tcx let suggestion = { let wrap_in_curly = is_else_clause(ctx.tcx, expr); let (left_curly, right_curly) = if wrap_in_curly {("{", "}")} else {("", "")}; + let (left_paren, right_paren) = if inverted && need_parens {("(", ")")} else {("", "")}; format!( - "{left_curly}{ty}::from({snippet}){right_curly}" + "{left_curly}{ty}::from({invert}{left_paren}{snippet}{right_paren}){right_curly}" ) }; // when used in else clause if statement should be wrapped in curly braces + let (inverted_left_paren, inverted_right_paren) = if inverted {("(", ")")} else {("", "")}; + span_lint_and_then(ctx, BOOL_TO_INT_WITH_IF, expr.span, @@ -87,7 +103,7 @@ fn check_if_else<'tcx>(ctx: &LateContext<'tcx>, expr: &'tcx rustc_hir::Expr<'tcx suggestion, applicability, ); - diag.note(format!("`{snippet_with_braces} as {ty}` or `{snippet_with_braces}.into()` can also be valid options")); + diag.note(format!("`{snippet_with_braces} as {ty}` or `{inverted_left_paren}{snippet_with_braces}{inverted_right_paren}.into()` can also be valid options")); }); }; } diff --git a/tests/ui/bool_to_int_with_if.fixed b/tests/ui/bool_to_int_with_if.fixed index 9c1098dc4c17..c48dc941b84b 100644 --- a/tests/ui/bool_to_int_with_if.fixed +++ b/tests/ui/bool_to_int_with_if.fixed @@ -14,6 +14,7 @@ fn main() { // precedence i32::from(a); i32::from(!a); + i32::from(!a); i32::from(a || b); i32::from(cond(a, b)); i32::from(x + y < 4); @@ -23,6 +24,11 @@ fn main() { 123 } else {i32::from(b)}; + // if else if inverted + if a { + 123 + } else {i32::from(!b)}; + // Shouldn't lint if a { diff --git a/tests/ui/bool_to_int_with_if.rs b/tests/ui/bool_to_int_with_if.rs index 0c967dac6e2d..5d9496f01775 100644 --- a/tests/ui/bool_to_int_with_if.rs +++ b/tests/ui/bool_to_int_with_if.rs @@ -17,6 +17,11 @@ fn main() { } else { 0 }; + if a { + 0 + } else { + 1 + }; if !a { 1 } else { @@ -47,6 +52,15 @@ fn main() { 0 }; + // if else if inverted + if a { + 123 + } else if b { + 0 + } else { + 1 + }; + // Shouldn't lint if a { diff --git a/tests/ui/bool_to_int_with_if.stderr b/tests/ui/bool_to_int_with_if.stderr index 8647a9cffbed..cc3e0395aa4f 100644 --- a/tests/ui/bool_to_int_with_if.stderr +++ b/tests/ui/bool_to_int_with_if.stderr @@ -14,6 +14,18 @@ LL | | }; error: boolean to int conversion using if --> $DIR/bool_to_int_with_if.rs:20:5 | +LL | / if a { +LL | | 0 +LL | | } else { +LL | | 1 +LL | | }; + | |_____^ help: replace with from: `i32::from(!a)` + | + = note: `!a as i32` or `(!a).into()` can also be valid options + +error: boolean to int conversion using if + --> $DIR/bool_to_int_with_if.rs:25:5 + | LL | / if !a { LL | | 1 LL | | } else { @@ -24,7 +36,7 @@ LL | | }; = note: `!a as i32` or `!a.into()` can also be valid options error: boolean to int conversion using if - --> $DIR/bool_to_int_with_if.rs:25:5 + --> $DIR/bool_to_int_with_if.rs:30:5 | LL | / if a || b { LL | | 1 @@ -36,7 +48,7 @@ LL | | }; = note: `(a || b) as i32` or `(a || b).into()` can also be valid options error: boolean to int conversion using if - --> $DIR/bool_to_int_with_if.rs:30:5 + --> $DIR/bool_to_int_with_if.rs:35:5 | LL | / if cond(a, b) { LL | | 1 @@ -48,7 +60,7 @@ LL | | }; = note: `cond(a, b) as i32` or `cond(a, b).into()` can also be valid options error: boolean to int conversion using if - --> $DIR/bool_to_int_with_if.rs:35:5 + --> $DIR/bool_to_int_with_if.rs:40:5 | LL | / if x + y < 4 { LL | | 1 @@ -60,7 +72,7 @@ LL | | }; = note: `(x + y < 4) as i32` or `(x + y < 4).into()` can also be valid options error: boolean to int conversion using if - --> $DIR/bool_to_int_with_if.rs:44:12 + --> $DIR/bool_to_int_with_if.rs:49:12 | LL | } else if b { | ____________^ @@ -73,12 +85,25 @@ LL | | }; = note: `b as i32` or `b.into()` can also be valid options error: boolean to int conversion using if - --> $DIR/bool_to_int_with_if.rs:102:5 + --> $DIR/bool_to_int_with_if.rs:58:12 + | +LL | } else if b { + | ____________^ +LL | | 0 +LL | | } else { +LL | | 1 +LL | | }; + | |_____^ help: replace with from: `{i32::from(!b)}` + | + = note: `!b as i32` or `(!b).into()` can also be valid options + +error: boolean to int conversion using if + --> $DIR/bool_to_int_with_if.rs:116:5 | LL | if a { 1 } else { 0 } | ^^^^^^^^^^^^^^^^^^^^^ help: replace with from: `u8::from(a)` | = note: `a as u8` or `a.into()` can also be valid options -error: aborting due to 7 previous errors +error: aborting due to 9 previous errors From 992560087025370e2a0397a67106b0852e238b4e Mon Sep 17 00:00:00 2001 From: Jacob Kiesel Date: Wed, 14 Sep 2022 00:27:56 -0600 Subject: [PATCH 2/4] dogfood inverse bool_to_int_with_if --- clippy_lints/src/dereference.rs | 7 ++----- clippy_lints/src/methods/unnecessary_to_owned.rs | 2 +- tests/ui/len_without_is_empty.rs | 2 +- 3 files changed, 4 insertions(+), 7 deletions(-) diff --git a/clippy_lints/src/dereference.rs b/clippy_lints/src/dereference.rs index 88e28018e5d0..d1b25a0f1b53 100644 --- a/clippy_lints/src/dereference.rs +++ b/clippy_lints/src/dereference.rs @@ -297,13 +297,10 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing { if !is_lint_allowed(cx, EXPLICIT_DEREF_METHODS, expr.hir_id) && position.lint_explicit_deref() => { + let ty_changed_count = usize::from(!deref_method_same_type(expr_ty, typeck.expr_ty(sub_expr))); self.state = Some(( State::DerefMethod { - ty_changed_count: if deref_method_same_type(expr_ty, typeck.expr_ty(sub_expr)) { - 0 - } else { - 1 - }, + ty_changed_count, is_final_ufcs: matches!(expr.kind, ExprKind::Call(..)), target_mut, }, diff --git a/clippy_lints/src/methods/unnecessary_to_owned.rs b/clippy_lints/src/methods/unnecessary_to_owned.rs index 763bfafecef1..b32e5d8d4210 100644 --- a/clippy_lints/src/methods/unnecessary_to_owned.rs +++ b/clippy_lints/src/methods/unnecessary_to_owned.rs @@ -269,7 +269,7 @@ fn check_other_call_arg<'tcx>( // We can't add an `&` when the trait is `Deref` because `Target = &T` won't match // `Target = T`. if n_refs > 0 || is_copy(cx, receiver_ty) || trait_predicate.def_id() != deref_trait_id; - let n_refs = max(n_refs, if is_copy(cx, receiver_ty) { 0 } else { 1 }); + let n_refs = max(n_refs, usize::from(!is_copy(cx, receiver_ty))); if let Some(receiver_snippet) = snippet_opt(cx, receiver.span); then { span_lint_and_sugg( diff --git a/tests/ui/len_without_is_empty.rs b/tests/ui/len_without_is_empty.rs index 1e938e72b577..78397c2af346 100644 --- a/tests/ui/len_without_is_empty.rs +++ b/tests/ui/len_without_is_empty.rs @@ -274,7 +274,7 @@ impl AsyncLen { } pub async fn len(&self) -> usize { - if self.async_task().await { 0 } else { 1 } + usize::from(!self.async_task().await) } pub async fn is_empty(&self) -> bool { From 4ffdce09b6bbbeb6462f13cd2a3da311578a679f Mon Sep 17 00:00:00 2001 From: Jacob Kiesel Date: Wed, 14 Sep 2022 13:28:54 -0600 Subject: [PATCH 3/4] refactor: use clippy_utils::Sugg instead of direct string ops --- clippy_lints/src/bool_to_int_with_if.rs | 37 +++++++++++-------------- tests/ui/bool_to_int_with_if.fixed | 4 +-- tests/ui/bool_to_int_with_if.stderr | 6 ++-- 3 files changed, 21 insertions(+), 26 deletions(-) diff --git a/clippy_lints/src/bool_to_int_with_if.rs b/clippy_lints/src/bool_to_int_with_if.rs index bea74279e033..51e98cda8451 100644 --- a/clippy_lints/src/bool_to_int_with_if.rs +++ b/clippy_lints/src/bool_to_int_with_if.rs @@ -1,9 +1,9 @@ -use rustc_ast::{ExprPrecedence, LitKind}; +use rustc_ast::LitKind; use rustc_hir::{Block, ExprKind}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::{declare_lint_pass, declare_tool_lint}; -use clippy_utils::{diagnostics::span_lint_and_then, is_else_clause, source::snippet_block_with_applicability}; +use clippy_utils::{diagnostics::span_lint_and_then, is_else_clause, sugg::Sugg}; use rustc_errors::Applicability; declare_clippy_lint! { @@ -69,28 +69,27 @@ fn check_if_else<'tcx>(ctx: &LateContext<'tcx>, expr: &'tcx rustc_hir::Expr<'tcx return; }; let mut applicability = Applicability::MachineApplicable; - let snippet = snippet_block_with_applicability(ctx, check.span, "..", None, &mut applicability); - - let invert = if inverted { "!" } else { "" }; - let need_parens = should_have_parentheses(check); - - let snippet_with_braces = { - let (left_paren, right_paren) = if need_parens {("(", ")")} else {("", "")}; - format!("{invert}{left_paren}{snippet}{right_paren}") + let snippet = { + let mut sugg = Sugg::hir_with_applicability(ctx, check, "..", &mut applicability); + if inverted { + sugg = !sugg; + } + sugg }; let ty = ctx.typeck_results().expr_ty(then_lit); // then and else must be of same type let suggestion = { let wrap_in_curly = is_else_clause(ctx.tcx, expr); - let (left_curly, right_curly) = if wrap_in_curly {("{", "}")} else {("", "")}; - let (left_paren, right_paren) = if inverted && need_parens {("(", ")")} else {("", "")}; - format!( - "{left_curly}{ty}::from({invert}{left_paren}{snippet}{right_paren}){right_curly}" - ) + let mut s = Sugg::NonParen(format!("{ty}::from({snippet})").into()); + if wrap_in_curly { + s = s.blockify(); + } + s }; // when used in else clause if statement should be wrapped in curly braces - let (inverted_left_paren, inverted_right_paren) = if inverted {("(", ")")} else {("", "")}; + let into_snippet = snippet.clone().maybe_par(); + let as_snippet = snippet.as_ty(ty); span_lint_and_then(ctx, BOOL_TO_INT_WITH_IF, @@ -103,7 +102,7 @@ fn check_if_else<'tcx>(ctx: &LateContext<'tcx>, expr: &'tcx rustc_hir::Expr<'tcx suggestion, applicability, ); - diag.note(format!("`{snippet_with_braces} as {ty}` or `{inverted_left_paren}{snippet_with_braces}{inverted_right_paren}.into()` can also be valid options")); + diag.note(format!("`{as_snippet}` or `{into_snippet}.into()` can also be valid options")); }); }; } @@ -135,7 +134,3 @@ fn check_int_literal_equals_val<'tcx>(expr: &'tcx rustc_hir::Expr<'tcx>, expecte false } } - -fn should_have_parentheses<'tcx>(check: &'tcx rustc_hir::Expr<'tcx>) -> bool { - check.precedence().order() < ExprPrecedence::Cast.order() -} diff --git a/tests/ui/bool_to_int_with_if.fixed b/tests/ui/bool_to_int_with_if.fixed index c48dc941b84b..2c8339cdd7f8 100644 --- a/tests/ui/bool_to_int_with_if.fixed +++ b/tests/ui/bool_to_int_with_if.fixed @@ -22,12 +22,12 @@ fn main() { // if else if if a { 123 - } else {i32::from(b)}; + } else { i32::from(b) }; // if else if inverted if a { 123 - } else {i32::from(!b)}; + } else { i32::from(!b) }; // Shouldn't lint diff --git a/tests/ui/bool_to_int_with_if.stderr b/tests/ui/bool_to_int_with_if.stderr index cc3e0395aa4f..e695440f6682 100644 --- a/tests/ui/bool_to_int_with_if.stderr +++ b/tests/ui/bool_to_int_with_if.stderr @@ -33,7 +33,7 @@ LL | | 0 LL | | }; | |_____^ help: replace with from: `i32::from(!a)` | - = note: `!a as i32` or `!a.into()` can also be valid options + = note: `!a as i32` or `(!a).into()` can also be valid options error: boolean to int conversion using if --> $DIR/bool_to_int_with_if.rs:30:5 @@ -80,7 +80,7 @@ LL | | 1 LL | | } else { LL | | 0 LL | | }; - | |_____^ help: replace with from: `{i32::from(b)}` + | |_____^ help: replace with from: `{ i32::from(b) }` | = note: `b as i32` or `b.into()` can also be valid options @@ -93,7 +93,7 @@ LL | | 0 LL | | } else { LL | | 1 LL | | }; - | |_____^ help: replace with from: `{i32::from(!b)}` + | |_____^ help: replace with from: `{ i32::from(!b) }` | = note: `!b as i32` or `(!b).into()` can also be valid options From dd97c1ed205f47d13dc5bc482a73c0c76383d159 Mon Sep 17 00:00:00 2001 From: Jacob Kiesel Date: Wed, 14 Sep 2022 13:29:32 -0600 Subject: [PATCH 4/4] fix: clippy_utils::Sugg should treat hir::ExprKind::DropTemps as transparent --- clippy_utils/src/sugg.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_utils/src/sugg.rs b/clippy_utils/src/sugg.rs index d28f2bd8c17a..f08275a4ac76 100644 --- a/clippy_utils/src/sugg.rs +++ b/clippy_utils/src/sugg.rs @@ -155,8 +155,8 @@ impl<'a> Sugg<'a> { | hir::ExprKind::Ret(..) | hir::ExprKind::Struct(..) | hir::ExprKind::Tup(..) - | hir::ExprKind::DropTemps(_) | hir::ExprKind::Err => Sugg::NonParen(get_snippet(expr.span)), + hir::ExprKind::DropTemps(inner) => Self::hir_from_snippet(inner, get_snippet), hir::ExprKind::Assign(lhs, rhs, _) => { Sugg::BinOp(AssocOp::Assign, get_snippet(lhs.span), get_snippet(rhs.span)) },