From 66758bc450226f177fb70c74bd0714865bafa8a1 Mon Sep 17 00:00:00 2001 From: Mariana Miranda Date: Mon, 20 May 2024 16:11:53 +0100 Subject: [PATCH] Implement manual_and and manual_or lints Suggests replacing if-else expressions where one branch is a boolean literal with && / || operators. Co-authored-by: Francisco Salgueiro --- CHANGELOG.md | 2 + clippy_lints/src/declared_lints.rs | 2 + clippy_lints/src/escape.rs | 6 +- clippy_lints/src/functions/must_use.rs | 7 +- clippy_lints/src/lib.rs | 2 + clippy_lints/src/literal_representation.rs | 6 +- clippy_lints/src/manual_and_or.rs | 193 ++++++++++++++++++ .../src/methods/bind_instead_of_map.rs | 7 +- clippy_lints/src/methods/chars_last_cmp.rs | 7 +- .../src/methods/chars_last_cmp_with_unwrap.rs | 7 +- clippy_lints/src/methods/search_is_some.rs | 7 +- .../src/non_send_fields_in_send_ty.rs | 7 +- clippy_lints/src/types/vec_box.rs | 101 +++++---- clippy_lints/src/unit_types/unit_arg.rs | 8 +- clippy_utils/src/attrs.rs | 8 +- clippy_utils/src/hir_utils.rs | 9 +- clippy_utils/src/ty.rs | 9 +- tests/ui/manual_and.fixed | 42 ++++ tests/ui/manual_and.rs | 42 ++++ tests/ui/manual_and.stderr | 17 ++ tests/ui/manual_or.fixed | 26 +++ tests/ui/manual_or.rs | 26 +++ tests/ui/manual_or.stderr | 23 +++ tests/ui/needless_bool/fixable.fixed | 6 +- tests/ui/needless_bool/fixable.rs | 6 +- tests/ui/needless_bool/fixable.stderr | 42 ++-- tests/ui/needless_bool/simple.rs | 5 +- tests/ui/needless_bool/simple.stderr | 8 +- tests/ui/significant_drop_in_scrutinee.rs | 2 +- 29 files changed, 487 insertions(+), 146 deletions(-) create mode 100644 clippy_lints/src/manual_and_or.rs create mode 100644 tests/ui/manual_and.fixed create mode 100644 tests/ui/manual_and.rs create mode 100644 tests/ui/manual_and.stderr create mode 100644 tests/ui/manual_or.fixed create mode 100644 tests/ui/manual_or.rs create mode 100644 tests/ui/manual_or.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index e17d6b3cf274..3d348bc062b5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5451,6 +5451,7 @@ Released 2018-09-13 [`macro_metavars_in_unsafe`]: https://rust-lang.github.io/rust-clippy/master/index.html#macro_metavars_in_unsafe [`macro_use_imports`]: https://rust-lang.github.io/rust-clippy/master/index.html#macro_use_imports [`main_recursion`]: https://rust-lang.github.io/rust-clippy/master/index.html#main_recursion +[`manual_and`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_and [`manual_assert`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_assert [`manual_async_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_async_fn [`manual_bits`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_bits @@ -5474,6 +5475,7 @@ Released 2018-09-13 [`manual_next_back`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_next_back [`manual_non_exhaustive`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_non_exhaustive [`manual_ok_or`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_ok_or +[`manual_or`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_or [`manual_range_contains`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_range_contains [`manual_range_patterns`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_range_patterns [`manual_rem_euclid`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_rem_euclid diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 6850ec821e84..eacfef55f854 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -299,6 +299,8 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::macro_metavars_in_unsafe::MACRO_METAVARS_IN_UNSAFE_INFO, crate::macro_use::MACRO_USE_IMPORTS_INFO, crate::main_recursion::MAIN_RECURSION_INFO, + crate::manual_and_or::MANUAL_AND_INFO, + crate::manual_and_or::MANUAL_OR_INFO, crate::manual_assert::MANUAL_ASSERT_INFO, crate::manual_async_fn::MANUAL_ASYNC_FN_INFO, crate::manual_bits::MANUAL_BITS_INFO, diff --git a/clippy_lints/src/escape.rs b/clippy_lints/src/escape.rs index 8d6e27700d8d..9b4df42a56d4 100644 --- a/clippy_lints/src/escape.rs +++ b/clippy_lints/src/escape.rs @@ -183,10 +183,6 @@ impl<'a, 'tcx> Delegate<'tcx> for EscapeDelegate<'a, 'tcx> { impl<'a, 'tcx> EscapeDelegate<'a, 'tcx> { fn is_large_box(&self, ty: Ty<'tcx>) -> bool { // Large types need to be boxed to avoid stack overflows. - if ty.is_box() { - self.cx.layout_of(ty.boxed_ty()).map_or(0, |l| l.size.bytes()) > self.too_large_for_stack - } else { - false - } + ty.is_box() && self.cx.layout_of(ty.boxed_ty()).map_or(0, |l| l.size.bytes()) > self.too_large_for_stack } } diff --git a/clippy_lints/src/functions/must_use.rs b/clippy_lints/src/functions/must_use.rs index e7ec2b3151e6..51e8b285c8ed 100644 --- a/clippy_lints/src/functions/must_use.rs +++ b/clippy_lints/src/functions/must_use.rs @@ -185,11 +185,8 @@ fn is_mutable_pat(cx: &LateContext<'_>, pat: &hir::Pat<'_>, tys: &mut DefIdSet) if let hir::PatKind::Wild = pat.kind { return false; // ignore `_` patterns } - if cx.tcx.has_typeck_results(pat.hir_id.owner.def_id) { - is_mutable_ty(cx, cx.tcx.typeck(pat.hir_id.owner.def_id).pat_ty(pat), tys) - } else { - false - } + cx.tcx.has_typeck_results(pat.hir_id.owner.def_id) + && is_mutable_ty(cx, cx.tcx.typeck(pat.hir_id.owner.def_id).pat_ty(pat), tys) } static KNOWN_WRAPPER_TYS: &[Symbol] = &[sym::Rc, sym::Arc]; diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 3328d642bd85..b8f630587ad5 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -197,6 +197,7 @@ mod loops; mod macro_metavars_in_unsafe; mod macro_use; mod main_recursion; +mod manual_and_or; mod manual_assert; mod manual_async_fn; mod manual_bits; @@ -1165,6 +1166,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) { ..Default::default() }) }); + store.register_late_pass(|_| Box::new(manual_and_or::ManualAndOr)); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/clippy_lints/src/literal_representation.rs b/clippy_lints/src/literal_representation.rs index 2348dd18220f..782dec57c114 100644 --- a/clippy_lints/src/literal_representation.rs +++ b/clippy_lints/src/literal_representation.rs @@ -374,11 +374,7 @@ impl LiteralDigitGrouping { } let group_sizes: Vec = num_lit.integer.split('_').map(str::len).collect(); - if UUID_GROUP_LENS.len() == group_sizes.len() { - iter::zip(&UUID_GROUP_LENS, &group_sizes).all(|(&a, &b)| a == b) - } else { - false - } + UUID_GROUP_LENS.len() == group_sizes.len() && iter::zip(&UUID_GROUP_LENS, &group_sizes).all(|(&a, &b)| a == b) } /// Given the sizes of the digit groups of both integral and fractional diff --git a/clippy_lints/src/manual_and_or.rs b/clippy_lints/src/manual_and_or.rs new file mode 100644 index 000000000000..517e986132f7 --- /dev/null +++ b/clippy_lints/src/manual_and_or.rs @@ -0,0 +1,193 @@ +use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::visitors::for_each_expr; +use clippy_utils::{get_parent_expr, higher, peel_blocks}; +use core::ops::ControlFlow; +use rustc_ast::ast::LitKind; +use rustc_ast::BinOpKind; +use rustc_errors::Applicability; +use rustc_hir::{Expr, ExprKind}; +use rustc_lint::{LateContext, LateLintPass, LintContext}; +use rustc_session::declare_lint_pass; + +declare_clippy_lint! { + /// ### What it does + /// Detects `if`-then-`else` that can be replaced with `&&`. + /// + /// ### Why is this bad? + /// `&&` is simpler than `if`-then-`else`. + /// + /// ### Example + /// ```ignore + /// if a { + /// b + /// } else { + /// false + /// } + /// ``` + /// Use instead: + /// ```ignore + /// a && b + /// ``` + #[clippy::version = "1.80.0"] + pub MANUAL_AND, + complexity, + "this `if`-then-`else` that can be replaced with `&&`." +} + +declare_clippy_lint! { + /// ### What it does + /// Detects `if`-then-`else` that can be replaced with `||`. + /// + /// ### Why is this bad? + /// `||` is simpler than `if`-then-`else`. + /// + /// ### Example + /// ```ignore + /// if a { + /// true + /// } else { + /// b + /// } + /// ``` + /// Use instead: + /// ```ignore + /// a || b + /// ``` + #[clippy::version = "1.80.0"] + pub MANUAL_OR, + complexity, + "this `if`-then-`else` expression can be simplified with `||`" +} + +declare_lint_pass!(ManualAndOr => [MANUAL_AND, MANUAL_OR]); + +fn extract_final_expression_snippet<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>) -> Option { + if let ExprKind::Block(block, _) = expr.kind { + if let Some(final_expr) = block.expr { + return cx.sess().source_map().span_to_snippet(final_expr.span).ok(); + } + } + cx.sess().source_map().span_to_snippet(expr.span).ok() +} + +fn fetch_bool_expr(expr: &Expr<'_>) -> Option { + if let ExprKind::Lit(lit_ptr) = peel_blocks(expr).kind { + if let LitKind::Bool(value) = lit_ptr.node { + return Some(value); + } + } + None +} + +fn contains_let(cond: &Expr<'_>) -> bool { + for_each_expr(cond, |e| { + if matches!(e.kind, ExprKind::Let(_)) { + ControlFlow::Break(()) + } else { + ControlFlow::Continue(()) + } + }) + .is_some() +} + +fn contains_or(cond: &Expr<'_>) -> bool { + for_each_expr(cond, |e| { + if let ExprKind::Binary(ref n, _, _) = e.kind { + if n.node == BinOpKind::Or { + ControlFlow::Break(()) + } else { + ControlFlow::Continue(()) + } + } else { + ControlFlow::Continue(()) + } + }) + .is_some() +} + +fn check_and<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>, cond: &Expr<'tcx>, then: &Expr<'tcx>) { + if let Some(parent) = get_parent_expr(cx, expr) { + if let ExprKind::If(_, _, _) = parent.kind { + return; + } + } + if contains_or(cond) || contains_or(then) || fetch_bool_expr(then).is_some() { + return; + } + if match then.kind { + ExprKind::Block(block, _) => !block.stmts.is_empty(), + _ => false, + } { + return; + } + + let applicability = Applicability::MachineApplicable; + let cond_snippet = cx + .sess() + .source_map() + .span_to_snippet(cond.span) + .unwrap_or_else(|_| "..".to_string()); + + let then_snippet = extract_final_expression_snippet(cx, then).unwrap_or_else(|| "..".to_string()); + let suggestion = format!("{cond_snippet} && {then_snippet}"); + span_lint_and_sugg( + cx, + MANUAL_AND, + expr.span, + "this `if`-then-`else` expression can be simplified with `&&`", + "try", + suggestion, + applicability, + ); +} + +fn check_or<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>, cond: &Expr<'tcx>, else_expr: &Expr<'tcx>) { + if matches!(else_expr.kind, ExprKind::If(..)) || fetch_bool_expr(else_expr).is_some() { + return; + } + if match else_expr.kind { + ExprKind::Block(block, _) => !block.stmts.is_empty(), + _ => false, + } { + return; + } + + let applicability = Applicability::MachineApplicable; + let cond_snippet = cx + .sess() + .source_map() + .span_to_snippet(cond.span) + .unwrap_or_else(|_| "..".to_string()); + + let else_snippet = extract_final_expression_snippet(cx, else_expr).unwrap_or_else(|| "..".to_string()); + let suggestion = format!("{cond_snippet} || {else_snippet}"); + span_lint_and_sugg( + cx, + MANUAL_OR, + expr.span, + "this `if`-then-`else` expression can be simplified with `||`", + "try", + suggestion, + applicability, + ); +} + +impl<'tcx> LateLintPass<'tcx> for ManualAndOr { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &Expr<'tcx>) { + if let Some(higher::If { + cond, + then, + r#else: Some(else_expr), + }) = higher::If::hir(expr) + { + if contains_let(cond) { + return; + } + if let Some(false) = fetch_bool_expr(else_expr) { + check_and(cx, expr, cond, then); + } else if let Some(true) = fetch_bool_expr(then) { + check_or(cx, expr, cond, else_expr); + } + } + } +} diff --git a/clippy_lints/src/methods/bind_instead_of_map.rs b/clippy_lints/src/methods/bind_instead_of_map.rs index fb440ce656ed..45b56b9cd01d 100644 --- a/clippy_lints/src/methods/bind_instead_of_map.rs +++ b/clippy_lints/src/methods/bind_instead_of_map.rs @@ -144,11 +144,8 @@ pub(crate) trait BindInsteadOfMap { let closure_body = cx.tcx.hir().body(body); let closure_expr = peel_blocks(closure_body.value); - if Self::lint_closure_autofixable(cx, expr, recv, closure_expr, fn_decl_span) { - true - } else { - Self::lint_closure(cx, expr, closure_expr) - } + Self::lint_closure_autofixable(cx, expr, recv, closure_expr, fn_decl_span) + || Self::lint_closure(cx, expr, closure_expr) }, // `_.and_then(Some)` case, which is no-op. hir::ExprKind::Path(QPath::Resolved(_, path)) if Self::is_variant(cx, path.res) => { diff --git a/clippy_lints/src/methods/chars_last_cmp.rs b/clippy_lints/src/methods/chars_last_cmp.rs index 2efff4c3c549..43f9d9738ea2 100644 --- a/clippy_lints/src/methods/chars_last_cmp.rs +++ b/clippy_lints/src/methods/chars_last_cmp.rs @@ -5,9 +5,6 @@ use super::CHARS_LAST_CMP; /// Checks for the `CHARS_LAST_CMP` lint. pub(super) fn check(cx: &LateContext<'_>, info: &crate::methods::BinaryExprInfo<'_>) -> bool { - if chars_cmp::check(cx, info, &["chars", "last"], CHARS_LAST_CMP, "ends_with") { - true - } else { - chars_cmp::check(cx, info, &["chars", "next_back"], CHARS_LAST_CMP, "ends_with") - } + chars_cmp::check(cx, info, &["chars", "last"], CHARS_LAST_CMP, "ends_with") + || chars_cmp::check(cx, info, &["chars", "next_back"], CHARS_LAST_CMP, "ends_with") } diff --git a/clippy_lints/src/methods/chars_last_cmp_with_unwrap.rs b/clippy_lints/src/methods/chars_last_cmp_with_unwrap.rs index 5b8713f7d790..9ea15ecaf2d0 100644 --- a/clippy_lints/src/methods/chars_last_cmp_with_unwrap.rs +++ b/clippy_lints/src/methods/chars_last_cmp_with_unwrap.rs @@ -5,9 +5,6 @@ use super::CHARS_LAST_CMP; /// Checks for the `CHARS_LAST_CMP` lint with `unwrap()`. pub(super) fn check(cx: &LateContext<'_>, info: &crate::methods::BinaryExprInfo<'_>) -> bool { - if chars_cmp_with_unwrap::check(cx, info, &["chars", "last", "unwrap"], CHARS_LAST_CMP, "ends_with") { - true - } else { - chars_cmp_with_unwrap::check(cx, info, &["chars", "next_back", "unwrap"], CHARS_LAST_CMP, "ends_with") - } + chars_cmp_with_unwrap::check(cx, info, &["chars", "last", "unwrap"], CHARS_LAST_CMP, "ends_with") + || chars_cmp_with_unwrap::check(cx, info, &["chars", "next_back", "unwrap"], CHARS_LAST_CMP, "ends_with") } diff --git a/clippy_lints/src/methods/search_is_some.rs b/clippy_lints/src/methods/search_is_some.rs index f5f1e94bbf45..841673833baa 100644 --- a/clippy_lints/src/methods/search_is_some.rs +++ b/clippy_lints/src/methods/search_is_some.rs @@ -110,11 +110,8 @@ pub(super) fn check<'tcx>( else if search_method == "find" { let is_string_or_str_slice = |e| { let self_ty = cx.typeck_results().expr_ty(e).peel_refs(); - if is_type_lang_item(cx, self_ty, hir::LangItem::String) { - true - } else { - self_ty.is_str() - } + + is_type_lang_item(cx, self_ty, hir::LangItem::String) || self_ty.is_str() }; if is_string_or_str_slice(search_recv) && is_string_or_str_slice(search_arg) { let msg = format!("called `{option_check_method}()` after calling `find()` on a string"); diff --git a/clippy_lints/src/non_send_fields_in_send_ty.rs b/clippy_lints/src/non_send_fields_in_send_ty.rs index 74e6c57b52d4..6c9f0517c34c 100644 --- a/clippy_lints/src/non_send_fields_in_send_ty.rs +++ b/clippy_lints/src/non_send_fields_in_send_ty.rs @@ -207,16 +207,13 @@ fn ty_allowed_with_raw_pointer_heuristic<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'t .all(|ty| ty_allowed_with_raw_pointer_heuristic(cx, ty, send_trait)), ty::Array(ty, _) | ty::Slice(ty) => ty_allowed_with_raw_pointer_heuristic(cx, *ty, send_trait), ty::Adt(_, args) => { - if contains_pointer_like(cx, ty) { + contains_pointer_like(cx, ty) // descends only if ADT contains any raw pointers - args.iter().all(|generic_arg| match generic_arg.unpack() { + && args.iter().all(|generic_arg| match generic_arg.unpack() { GenericArgKind::Type(ty) => ty_allowed_with_raw_pointer_heuristic(cx, ty, send_trait), // Lifetimes and const generics are not solid part of ADT and ignored GenericArgKind::Lifetime(_) | GenericArgKind::Const(_) => true, }) - } else { - false - } }, // Raw pointers are `!Send` but allowed by the heuristic ty::RawPtr(_, _) => true, diff --git a/clippy_lints/src/types/vec_box.rs b/clippy_lints/src/types/vec_box.rs index 29996a6f783e..856c56fd107d 100644 --- a/clippy_lints/src/types/vec_box.rs +++ b/clippy_lints/src/types/vec_box.rs @@ -19,59 +19,56 @@ pub(super) fn check<'tcx>( def_id: DefId, box_size_threshold: u64, ) -> bool { - if cx.tcx.is_diagnostic_item(sym::Vec, def_id) { - if let Some(last) = last_path_segment(qpath).args - // Get the _ part of Vec<_> - && let Some(GenericArg::Type(ty)) = last.args.first() - // extract allocator from the Vec for later - && let vec_alloc_ty = last.args.get(1) - // ty is now _ at this point - && let TyKind::Path(ref ty_qpath) = ty.kind - && let res = cx.qpath_res(ty_qpath, ty.hir_id) - && let Some(def_id) = res.opt_def_id() - && Some(def_id) == cx.tcx.lang_items().owned_box() - // At this point, we know ty is Box, now get T - && let Some(last) = last_path_segment(ty_qpath).args - && let Some(GenericArg::Type(boxed_ty)) = last.args.first() - // extract allocator from the Box for later - && let boxed_alloc_ty = last.args.get(1) - && let ty_ty = lower_ty(cx.tcx, boxed_ty) - && !ty_ty.has_escaping_bound_vars() - && ty_ty.is_sized(cx.tcx, cx.param_env) - && let Ok(ty_ty_size) = cx.layout_of(ty_ty).map(|l| l.size.bytes()) - && ty_ty_size < box_size_threshold - // https://github.com/rust-lang/rust-clippy/issues/7114 - && match (vec_alloc_ty, boxed_alloc_ty) { - (None, None) => true, - // this is in the event that we have something like - // Vec<_, Global>, in which case is equivalent to - // Vec<_> - (None, Some(GenericArg::Type(inner))) | (Some(GenericArg::Type(inner)), None) => { - if let TyKind::Path(path) = inner.kind - && let Some(did) = cx.qpath_res(&path, inner.hir_id).opt_def_id() { - cx.tcx.lang_items().get(LangItem::GlobalAlloc) == Some(did) - } else { - false - } - }, - (Some(GenericArg::Type(l)), Some(GenericArg::Type(r))) => - lower_ty(cx.tcx, l) == lower_ty(cx.tcx, r), - _ => false - } - { - span_lint_and_sugg( - cx, - VEC_BOX, - hir_ty.span, - "`Vec` is already on the heap, the boxing is unnecessary", - "try", - format!("Vec<{}>", snippet(cx, boxed_ty.span, "..")), - Applicability::Unspecified, - ); - true - } else { - false + if cx.tcx.is_diagnostic_item(sym::Vec, def_id) + && let Some(last) = last_path_segment(qpath).args + // Get the _ part of Vec<_> + && let Some(GenericArg::Type(ty)) = last.args.first() + // extract allocator from the Vec for later + && let vec_alloc_ty = last.args.get(1) + // ty is now _ at this point + && let TyKind::Path(ref ty_qpath) = ty.kind + && let res = cx.qpath_res(ty_qpath, ty.hir_id) + && let Some(def_id) = res.opt_def_id() + && Some(def_id) == cx.tcx.lang_items().owned_box() + // At this point, we know ty is Box, now get T + && let Some(last) = last_path_segment(ty_qpath).args + && let Some(GenericArg::Type(boxed_ty)) = last.args.first() + // extract allocator from the Box for later + && let boxed_alloc_ty = last.args.get(1) + && let ty_ty = lower_ty(cx.tcx, boxed_ty) + && !ty_ty.has_escaping_bound_vars() + && ty_ty.is_sized(cx.tcx, cx.param_env) + && let Ok(ty_ty_size) = cx.layout_of(ty_ty).map(|l| l.size.bytes()) + && ty_ty_size < box_size_threshold + // https://github.com/rust-lang/rust-clippy/issues/7114 + && match (vec_alloc_ty, boxed_alloc_ty) { + (None, None) => true, + // this is in the event that we have something like + // Vec<_, Global>, in which case is equivalent to + // Vec<_> + (None, Some(GenericArg::Type(inner))) | (Some(GenericArg::Type(inner)), None) => { + if let TyKind::Path(path) = inner.kind + && let Some(did) = cx.qpath_res(&path, inner.hir_id).opt_def_id() { + cx.tcx.lang_items().get(LangItem::GlobalAlloc) == Some(did) + } else { + false + } + }, + (Some(GenericArg::Type(l)), Some(GenericArg::Type(r))) => + lower_ty(cx.tcx, l) == lower_ty(cx.tcx, r), + _ => false } + { + span_lint_and_sugg( + cx, + VEC_BOX, + hir_ty.span, + "`Vec` is already on the heap, the boxing is unnecessary", + "try", + format!("Vec<{}>", snippet(cx, boxed_ty.span, "..")), + Applicability::Unspecified, + ); + true } else { false } diff --git a/clippy_lints/src/unit_types/unit_arg.rs b/clippy_lints/src/unit_types/unit_arg.rs index afc53e6f32d9..5dd2089ed424 100644 --- a/clippy_lints/src/unit_types/unit_arg.rs +++ b/clippy_lints/src/unit_types/unit_arg.rs @@ -34,14 +34,12 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { let args_to_recover = args .into_iter() .filter(|arg| { - if cx.typeck_results().expr_ty(arg).is_unit() && !utils::is_unit_literal(arg) { - !matches!( + cx.typeck_results().expr_ty(arg).is_unit() + && !utils::is_unit_literal(arg) + && !matches!( &arg.kind, ExprKind::Match(.., MatchSource::TryDesugar(_)) | ExprKind::Path(..) ) - } else { - false - } }) .collect::>(); if !args_to_recover.is_empty() && !is_from_proc_macro(cx, expr) { diff --git a/clippy_utils/src/attrs.rs b/clippy_utils/src/attrs.rs index d2200bcf7103..f2fa296bae2b 100644 --- a/clippy_utils/src/attrs.rs +++ b/clippy_utils/src/attrs.rs @@ -70,8 +70,9 @@ pub fn get_attr<'a>( return false; }; let attr_segments = &attr.path.segments; - if attr_segments.len() == 2 && attr_segments[0].ident.name == sym::clippy { - BUILTIN_ATTRIBUTES + attr_segments.len() == 2 + && attr_segments[0].ident.name == sym::clippy + && BUILTIN_ATTRIBUTES .iter() .find_map(|&(builtin_name, ref deprecation_status)| { if attr_segments[1].ident.name.as_str() == builtin_name { @@ -112,9 +113,6 @@ pub fn get_attr<'a>( } }, ) - } else { - false - } }) } diff --git a/clippy_utils/src/hir_utils.rs b/clippy_utils/src/hir_utils.rs index c921168df290..d4f82e870b01 100644 --- a/clippy_utils/src/hir_utils.rs +++ b/clippy_utils/src/hir_utils.rs @@ -484,12 +484,9 @@ impl HirEqInterExpr<'_, '_, '_> { } fn eq_path_parameters(&mut self, left: &GenericArgs<'_>, right: &GenericArgs<'_>) -> bool { - if left.parenthesized == right.parenthesized { - over(left.args, right.args, |l, r| self.eq_generic_arg(l, r)) // FIXME(flip1995): may not work - && over(left.bindings, right.bindings, |l, r| self.eq_type_binding(l, r)) - } else { - false - } + left.parenthesized == right.parenthesized + && over(left.args, right.args, |l, r| self.eq_generic_arg(l, r)) // FIXME(flip1995): may not work + && over(left.bindings, right.bindings, |l, r| self.eq_type_binding(l, r)) } pub fn eq_path_segments(&mut self, left: &[PathSegment<'_>], right: &[PathSegment<'_>]) -> bool { diff --git a/clippy_utils/src/ty.rs b/clippy_utils/src/ty.rs index fadb58802d69..a997c78cc1ed 100644 --- a/clippy_utils/src/ty.rs +++ b/clippy_utils/src/ty.rs @@ -379,8 +379,8 @@ fn is_normalizable_helper<'tcx>( cache.insert(ty, false); let infcx = cx.tcx.infer_ctxt().build(); let cause = ObligationCause::dummy(); - let result = if infcx.at(&cause, param_env).query_normalize(ty).is_ok() { - match ty.kind() { + let result = infcx.at(&cause, param_env).query_normalize(ty).is_ok() + && match ty.kind() { ty::Adt(def, args) => def.variants().iter().all(|variant| { variant .fields @@ -393,10 +393,7 @@ fn is_normalizable_helper<'tcx>( }, _ => true, // if inner_ty == ty, we've already checked it }), - } - } else { - false - }; + }; cache.insert(ty, result); result } diff --git a/tests/ui/manual_and.fixed b/tests/ui/manual_and.fixed new file mode 100644 index 000000000000..93418f10cf66 --- /dev/null +++ b/tests/ui/manual_and.fixed @@ -0,0 +1,42 @@ +#![warn(clippy::manual_and)] +#[allow(irrefutable_let_patterns)] + +fn main() { + let a = true; + let b = false; + let c = true; + + let _ = a && b; + //~^ ERROR: this `if`-then-`else` expression can be simplified with `&&` + + let _ = a && c && b; + //~^ ERROR: this `if`-then-`else` expression can be simplified with `&&` + + // Should not lint + + // with or in condition + let _ = if a || c { b } else { false }; + + // with or in then-branch + let _ = if a { b || c } else { false }; + + // with if-let + let _ = if let x = a { x } else { false }; + + // when the then-branch is a block + let _ = if let x = a { + println!("foo"); + x + } else { + false + }; + + // when part of a chain of if-elses + let _ = if a { + b + } else if b { + a + } else { + false + }; +} diff --git a/tests/ui/manual_and.rs b/tests/ui/manual_and.rs new file mode 100644 index 000000000000..6031eb01bff4 --- /dev/null +++ b/tests/ui/manual_and.rs @@ -0,0 +1,42 @@ +#![warn(clippy::manual_and)] +#[allow(irrefutable_let_patterns)] + +fn main() { + let a = true; + let b = false; + let c = true; + + let _ = if a { b } else { false }; + //~^ ERROR: this `if`-then-`else` expression can be simplified with `&&` + + let _ = if a && c { b } else { false }; + //~^ ERROR: this `if`-then-`else` expression can be simplified with `&&` + + // Should not lint + + // with or in condition + let _ = if a || c { b } else { false }; + + // with or in then-branch + let _ = if a { b || c } else { false }; + + // with if-let + let _ = if let x = a { x } else { false }; + + // when the then-branch is a block + let _ = if let x = a { + println!("foo"); + x + } else { + false + }; + + // when part of a chain of if-elses + let _ = if a { + b + } else if b { + a + } else { + false + }; +} diff --git a/tests/ui/manual_and.stderr b/tests/ui/manual_and.stderr new file mode 100644 index 000000000000..ad7a554ba3d3 --- /dev/null +++ b/tests/ui/manual_and.stderr @@ -0,0 +1,17 @@ +error: this `if`-then-`else` expression can be simplified with `&&` + --> tests/ui/manual_and.rs:9:13 + | +LL | let _ = if a { b } else { false }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `a && b` + | + = note: `-D clippy::manual-and` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::manual_and)]` + +error: this `if`-then-`else` expression can be simplified with `&&` + --> tests/ui/manual_and.rs:12:13 + | +LL | let _ = if a && c { b } else { false }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `a && c && b` + +error: aborting due to 2 previous errors + diff --git a/tests/ui/manual_or.fixed b/tests/ui/manual_or.fixed new file mode 100644 index 000000000000..e47a350ae8c1 --- /dev/null +++ b/tests/ui/manual_or.fixed @@ -0,0 +1,26 @@ +#![warn(clippy::manual_or)] +#[allow(irrefutable_let_patterns)] + +fn main() { + let a = true; + let b = false; + let c = true; + + let _ = a || b; //~ ERROR: this `if`-then-`else` expression can be simplified with `||` + + let _ = if a { + true + } else if b { + false + } else { + c + }; + + let _ = a || !b; + //~^ ERROR: this `if`-then-`else` expression can be simplified with `||` + + let _ = !a || b; + //~^ ERROR: this `if`-then-`else` expression can be simplified with `||` + + let _ = if let x = a { true } else { b }; +} diff --git a/tests/ui/manual_or.rs b/tests/ui/manual_or.rs new file mode 100644 index 000000000000..c9b29e6f3c10 --- /dev/null +++ b/tests/ui/manual_or.rs @@ -0,0 +1,26 @@ +#![warn(clippy::manual_or)] +#[allow(irrefutable_let_patterns)] + +fn main() { + let a = true; + let b = false; + let c = true; + + let _ = if a { true } else { b }; //~ ERROR: this `if`-then-`else` expression can be simplified with `||` + + let _ = if a { + true + } else if b { + false + } else { + c + }; + + let _ = if a { true } else { !b }; + //~^ ERROR: this `if`-then-`else` expression can be simplified with `||` + + let _ = if !a { true } else { b }; + //~^ ERROR: this `if`-then-`else` expression can be simplified with `||` + + let _ = if let x = a { true } else { b }; +} diff --git a/tests/ui/manual_or.stderr b/tests/ui/manual_or.stderr new file mode 100644 index 000000000000..c52b294b63c5 --- /dev/null +++ b/tests/ui/manual_or.stderr @@ -0,0 +1,23 @@ +error: this `if`-then-`else` expression can be simplified with `||` + --> tests/ui/manual_or.rs:9:13 + | +LL | let _ = if a { true } else { b }; + | ^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `a || b` + | + = note: `-D clippy::manual-or` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::manual_or)]` + +error: this `if`-then-`else` expression can be simplified with `||` + --> tests/ui/manual_or.rs:19:13 + | +LL | let _ = if a { true } else { !b }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `a || !b` + +error: this `if`-then-`else` expression can be simplified with `||` + --> tests/ui/manual_or.rs:22:13 + | +LL | let _ = if !a { true } else { b }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `!a || b` + +error: aborting due to 3 previous errors + diff --git a/tests/ui/needless_bool/fixable.fixed b/tests/ui/needless_bool/fixable.fixed index 3059de8f89c4..b83f7c6c79e3 100644 --- a/tests/ui/needless_bool/fixable.fixed +++ b/tests/ui/needless_bool/fixable.fixed @@ -8,7 +8,8 @@ clippy::needless_if, clippy::needless_return, clippy::self_named_constructors, - clippy::struct_field_names + clippy::struct_field_names, + clippy::manual_and )] use std::cell::Cell; @@ -54,7 +55,7 @@ fn main() { x } else { false - }; // would also be questionable, but we don't catch this yet + }; bool_ret3(x); bool_ret4(x); bool_ret5(x, x); @@ -108,6 +109,7 @@ fn needless_bool3(x: bool) { } fn needless_bool_in_the_suggestion_wraps_the_predicate_of_if_else_statement_in_brackets() { + #![allow(clippy::manual_or)] let b = false; let returns_bool = || false; diff --git a/tests/ui/needless_bool/fixable.rs b/tests/ui/needless_bool/fixable.rs index b2cbe86e2235..99121dd175d4 100644 --- a/tests/ui/needless_bool/fixable.rs +++ b/tests/ui/needless_bool/fixable.rs @@ -8,7 +8,8 @@ clippy::needless_if, clippy::needless_return, clippy::self_named_constructors, - clippy::struct_field_names + clippy::struct_field_names, + clippy::manual_and )] use std::cell::Cell; @@ -90,7 +91,7 @@ fn main() { x } else { false - }; // would also be questionable, but we don't catch this yet + }; bool_ret3(x); bool_ret4(x); bool_ret5(x, x); @@ -160,6 +161,7 @@ fn needless_bool3(x: bool) { } fn needless_bool_in_the_suggestion_wraps_the_predicate_of_if_else_statement_in_brackets() { + #![allow(clippy::manual_or)] let b = false; let returns_bool = || false; diff --git a/tests/ui/needless_bool/fixable.stderr b/tests/ui/needless_bool/fixable.stderr index 9746e931f50f..125e042e8776 100644 --- a/tests/ui/needless_bool/fixable.stderr +++ b/tests/ui/needless_bool/fixable.stderr @@ -1,5 +1,5 @@ error: this if-then-else expression returns a bool literal - --> tests/ui/needless_bool/fixable.rs:41:5 + --> tests/ui/needless_bool/fixable.rs:42:5 | LL | / if x { LL | | true @@ -12,7 +12,7 @@ LL | | }; = help: to override `-D warnings` add `#[allow(clippy::needless_bool)]` error: this if-then-else expression returns a bool literal - --> tests/ui/needless_bool/fixable.rs:46:5 + --> tests/ui/needless_bool/fixable.rs:47:5 | LL | / if x { LL | | false @@ -22,7 +22,7 @@ LL | | }; | |_____^ help: you can reduce it to: `!x` error: this if-then-else expression returns a bool literal - --> tests/ui/needless_bool/fixable.rs:51:5 + --> tests/ui/needless_bool/fixable.rs:52:5 | LL | / if x && y { LL | | false @@ -32,7 +32,7 @@ LL | | }; | |_____^ help: you can reduce it to: `!(x && y)` error: this if-then-else expression returns a bool literal - --> tests/ui/needless_bool/fixable.rs:59:5 + --> tests/ui/needless_bool/fixable.rs:60:5 | LL | / if a == b { LL | | false @@ -42,7 +42,7 @@ LL | | }; | |_____^ help: you can reduce it to: `a != b` error: this if-then-else expression returns a bool literal - --> tests/ui/needless_bool/fixable.rs:64:5 + --> tests/ui/needless_bool/fixable.rs:65:5 | LL | / if a != b { LL | | false @@ -52,7 +52,7 @@ LL | | }; | |_____^ help: you can reduce it to: `a == b` error: this if-then-else expression returns a bool literal - --> tests/ui/needless_bool/fixable.rs:69:5 + --> tests/ui/needless_bool/fixable.rs:70:5 | LL | / if a < b { LL | | false @@ -62,7 +62,7 @@ LL | | }; | |_____^ help: you can reduce it to: `a >= b` error: this if-then-else expression returns a bool literal - --> tests/ui/needless_bool/fixable.rs:74:5 + --> tests/ui/needless_bool/fixable.rs:75:5 | LL | / if a <= b { LL | | false @@ -72,7 +72,7 @@ LL | | }; | |_____^ help: you can reduce it to: `a > b` error: this if-then-else expression returns a bool literal - --> tests/ui/needless_bool/fixable.rs:79:5 + --> tests/ui/needless_bool/fixable.rs:80:5 | LL | / if a > b { LL | | false @@ -82,7 +82,7 @@ LL | | }; | |_____^ help: you can reduce it to: `a <= b` error: this if-then-else expression returns a bool literal - --> tests/ui/needless_bool/fixable.rs:84:5 + --> tests/ui/needless_bool/fixable.rs:85:5 | LL | / if a >= b { LL | | false @@ -92,7 +92,7 @@ LL | | }; | |_____^ help: you can reduce it to: `a < b` error: this if-then-else expression returns a bool literal - --> tests/ui/needless_bool/fixable.rs:112:5 + --> tests/ui/needless_bool/fixable.rs:113:5 | LL | / if x { LL | | return true; @@ -102,7 +102,7 @@ LL | | }; | |_____^ help: you can reduce it to: `return x` error: this if-then-else expression returns a bool literal - --> tests/ui/needless_bool/fixable.rs:120:5 + --> tests/ui/needless_bool/fixable.rs:121:5 | LL | / if x { LL | | return false; @@ -112,7 +112,7 @@ LL | | }; | |_____^ help: you can reduce it to: `return !x` error: this if-then-else expression returns a bool literal - --> tests/ui/needless_bool/fixable.rs:128:5 + --> tests/ui/needless_bool/fixable.rs:129:5 | LL | / if x && y { LL | | return true; @@ -122,7 +122,7 @@ LL | | }; | |_____^ help: you can reduce it to: `return x && y` error: this if-then-else expression returns a bool literal - --> tests/ui/needless_bool/fixable.rs:136:5 + --> tests/ui/needless_bool/fixable.rs:137:5 | LL | / if x && y { LL | | return false; @@ -132,7 +132,7 @@ LL | | }; | |_____^ help: you can reduce it to: `return !(x && y)` error: equality checks against true are unnecessary - --> tests/ui/needless_bool/fixable.rs:144:8 + --> tests/ui/needless_bool/fixable.rs:145:8 | LL | if x == true {}; | ^^^^^^^^^ help: try simplifying it as shown: `x` @@ -141,25 +141,25 @@ LL | if x == true {}; = help: to override `-D warnings` add `#[allow(clippy::bool_comparison)]` error: equality checks against false can be replaced by a negation - --> tests/ui/needless_bool/fixable.rs:148:8 + --> tests/ui/needless_bool/fixable.rs:149:8 | LL | if x == false {}; | ^^^^^^^^^^ help: try simplifying it as shown: `!x` error: equality checks against true are unnecessary - --> tests/ui/needless_bool/fixable.rs:158:8 + --> tests/ui/needless_bool/fixable.rs:159:8 | LL | if x == true {}; | ^^^^^^^^^ help: try simplifying it as shown: `x` error: equality checks against false can be replaced by a negation - --> tests/ui/needless_bool/fixable.rs:159:8 + --> tests/ui/needless_bool/fixable.rs:160:8 | LL | if x == false {}; | ^^^^^^^^^^ help: try simplifying it as shown: `!x` error: this if-then-else expression returns a bool literal - --> tests/ui/needless_bool/fixable.rs:168:12 + --> tests/ui/needless_bool/fixable.rs:170:12 | LL | } else if returns_bool() { | ____________^ @@ -170,7 +170,7 @@ LL | | }; | |_____^ help: you can reduce it to: `{ !returns_bool() }` error: this if-then-else expression returns a bool literal - --> tests/ui/needless_bool/fixable.rs:181:5 + --> tests/ui/needless_bool/fixable.rs:183:5 | LL | / if unsafe { no(4) } & 1 != 0 { LL | | true @@ -180,13 +180,13 @@ LL | | }; | |_____^ help: you can reduce it to: `(unsafe { no(4) } & 1 != 0)` error: this if-then-else expression returns a bool literal - --> tests/ui/needless_bool/fixable.rs:186:30 + --> tests/ui/needless_bool/fixable.rs:188:30 | LL | let _brackets_unneeded = if unsafe { no(4) } & 1 != 0 { true } else { false }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: you can reduce it to: `unsafe { no(4) } & 1 != 0` error: this if-then-else expression returns a bool literal - --> tests/ui/needless_bool/fixable.rs:189:9 + --> tests/ui/needless_bool/fixable.rs:191:9 | LL | if unsafe { no(4) } & 1 != 0 { true } else { false } | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: you can reduce it to: `(unsafe { no(4) } & 1 != 0)` diff --git a/tests/ui/needless_bool/simple.rs b/tests/ui/needless_bool/simple.rs index 588bb88f4461..fa75e94b804e 100644 --- a/tests/ui/needless_bool/simple.rs +++ b/tests/ui/needless_bool/simple.rs @@ -5,7 +5,8 @@ clippy::no_effect, clippy::if_same_then_else, clippy::needless_return, - clippy::branches_sharing_code + clippy::branches_sharing_code, + clippy::manual_and )] fn main() { @@ -25,7 +26,7 @@ fn main() { x } else { false - }; // would also be questionable, but we don't catch this yet + }; bool_ret(x); bool_ret2(x); } diff --git a/tests/ui/needless_bool/simple.stderr b/tests/ui/needless_bool/simple.stderr index bf30a56f43e7..b5e730ddb7e1 100644 --- a/tests/ui/needless_bool/simple.stderr +++ b/tests/ui/needless_bool/simple.stderr @@ -1,5 +1,5 @@ error: this if-then-else expression will always return true - --> tests/ui/needless_bool/simple.rs:14:5 + --> tests/ui/needless_bool/simple.rs:15:5 | LL | / if x { LL | | true @@ -12,7 +12,7 @@ LL | | }; = help: to override `-D warnings` add `#[allow(clippy::needless_bool)]` error: this if-then-else expression will always return false - --> tests/ui/needless_bool/simple.rs:19:5 + --> tests/ui/needless_bool/simple.rs:20:5 | LL | / if x { LL | | false @@ -22,7 +22,7 @@ LL | | }; | |_____^ error: this if-then-else expression will always return true - --> tests/ui/needless_bool/simple.rs:34:5 + --> tests/ui/needless_bool/simple.rs:35:5 | LL | / if x { LL | | return true; @@ -32,7 +32,7 @@ LL | | }; | |_____^ error: this if-then-else expression will always return false - --> tests/ui/needless_bool/simple.rs:42:5 + --> tests/ui/needless_bool/simple.rs:43:5 | LL | / if x { LL | | return false; diff --git a/tests/ui/significant_drop_in_scrutinee.rs b/tests/ui/significant_drop_in_scrutinee.rs index 7fc89bb95380..6e869ad0605e 100644 --- a/tests/ui/significant_drop_in_scrutinee.rs +++ b/tests/ui/significant_drop_in_scrutinee.rs @@ -464,7 +464,7 @@ fn should_not_trigger_lint_for_if_in_scrutinee() { match if i > 1 { mutex.lock().unwrap().s.len() > 1 } else { - false + i < 1 } { true => { mutex.lock().unwrap().s.len();