diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 5563b8094f01..a468cc19004c 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -84,10 +84,6 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[ crate::collapsible_if::COLLAPSIBLE_IF_INFO, crate::collection_is_never_read::COLLECTION_IS_NEVER_READ_INFO, crate::comparison_chain::COMPARISON_CHAIN_INFO, - crate::copies::BRANCHES_SHARING_CODE_INFO, - crate::copies::IFS_SAME_COND_INFO, - crate::copies::IF_SAME_THEN_ELSE_INFO, - crate::copies::SAME_FUNCTIONS_IN_IF_CONDITION_INFO, crate::copy_iterator::COPY_ITERATOR_INFO, crate::crate_in_macro_def::CRATE_IN_MACRO_DEF_INFO, crate::create_dir::CREATE_DIR_INFO, @@ -204,6 +200,10 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[ crate::if_let_mutex::IF_LET_MUTEX_INFO, crate::if_not_else::IF_NOT_ELSE_INFO, crate::if_then_some_else_none::IF_THEN_SOME_ELSE_NONE_INFO, + crate::ifs::BRANCHES_SHARING_CODE_INFO, + crate::ifs::IFS_SAME_COND_INFO, + crate::ifs::IF_SAME_THEN_ELSE_INFO, + crate::ifs::SAME_FUNCTIONS_IN_IF_CONDITION_INFO, crate::ignored_unit_patterns::IGNORED_UNIT_PATTERNS_INFO, crate::impl_hash_with_borrow_str_and_bytes::IMPL_HASH_BORROW_WITH_STR_AND_BYTES_INFO, crate::implicit_hasher::IMPLICIT_HASHER_INFO, diff --git a/clippy_lints/src/copies.rs b/clippy_lints/src/ifs/branches_sharing_code.rs similarity index 63% rename from clippy_lints/src/copies.rs rename to clippy_lints/src/ifs/branches_sharing_code.rs index 4fdb497950f8..eb1025f71498 100644 --- a/clippy_lints/src/copies.rs +++ b/clippy_lints/src/ifs/branches_sharing_code.rs @@ -1,218 +1,23 @@ -use clippy_config::Conf; -use clippy_utils::diagnostics::{span_lint, span_lint_and_note, span_lint_and_then}; -use clippy_utils::higher::has_let_expr; +use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::source::{IntoSpan, SpanRangeExt, first_line_of_span, indent_of, reindent_multiline, snippet}; -use clippy_utils::ty::{InteriorMut, needs_ordered_drop}; +use clippy_utils::ty::needs_ordered_drop; use clippy_utils::visitors::for_each_expr_without_closures; use clippy_utils::{ - ContainsName, HirEqInterExpr, SpanlessEq, capture_local_usage, eq_expr_value, find_binding_init, - get_enclosing_block, hash_expr, hash_stmt, if_sequence, is_else_clause, is_lint_allowed, path_to_local, - search_same, + ContainsName, HirEqInterExpr, SpanlessEq, capture_local_usage, get_enclosing_block, hash_expr, hash_stmt, + path_to_local, }; use core::iter; use core::ops::ControlFlow; use rustc_errors::Applicability; use rustc_hir::{Block, Expr, ExprKind, HirId, HirIdSet, LetStmt, Node, Stmt, StmtKind, intravisit}; -use rustc_lint::{LateContext, LateLintPass}; -use rustc_middle::ty::TyCtxt; -use rustc_session::impl_lint_pass; +use rustc_lint::LateContext; use rustc_span::hygiene::walk_chain; use rustc_span::source_map::SourceMap; use rustc_span::{Span, Symbol}; -declare_clippy_lint! { - /// ### What it does - /// Checks for consecutive `if`s with the same condition. - /// - /// ### Why is this bad? - /// This is probably a copy & paste error. - /// - /// ### Example - /// ```ignore - /// if a == b { - /// … - /// } else if a == b { - /// … - /// } - /// ``` - /// - /// Note that this lint ignores all conditions with a function call as it could - /// have side effects: - /// - /// ```ignore - /// if foo() { - /// … - /// } else if foo() { // not linted - /// … - /// } - /// ``` - #[clippy::version = "pre 1.29.0"] - pub IFS_SAME_COND, - correctness, - "consecutive `if`s with the same condition" -} - -declare_clippy_lint! { - /// ### What it does - /// Checks for consecutive `if`s with the same function call. - /// - /// ### Why is this bad? - /// This is probably a copy & paste error. - /// Despite the fact that function can have side effects and `if` works as - /// intended, such an approach is implicit and can be considered a "code smell". - /// - /// ### Example - /// ```ignore - /// if foo() == bar { - /// … - /// } else if foo() == bar { - /// … - /// } - /// ``` - /// - /// This probably should be: - /// ```ignore - /// if foo() == bar { - /// … - /// } else if foo() == baz { - /// … - /// } - /// ``` - /// - /// or if the original code was not a typo and called function mutates a state, - /// consider move the mutation out of the `if` condition to avoid similarity to - /// a copy & paste error: - /// - /// ```ignore - /// let first = foo(); - /// if first == bar { - /// … - /// } else { - /// let second = foo(); - /// if second == bar { - /// … - /// } - /// } - /// ``` - #[clippy::version = "1.41.0"] - pub SAME_FUNCTIONS_IN_IF_CONDITION, - pedantic, - "consecutive `if`s with the same function call" -} - -declare_clippy_lint! { - /// ### What it does - /// Checks for `if/else` with the same body as the *then* part - /// and the *else* part. - /// - /// ### Why is this bad? - /// This is probably a copy & paste error. - /// - /// ### Example - /// ```ignore - /// let foo = if … { - /// 42 - /// } else { - /// 42 - /// }; - /// ``` - #[clippy::version = "pre 1.29.0"] - pub IF_SAME_THEN_ELSE, - style, - "`if` with the same `then` and `else` blocks" -} - -declare_clippy_lint! { - /// ### What it does - /// Checks if the `if` and `else` block contain shared code that can be - /// moved out of the blocks. - /// - /// ### Why is this bad? - /// Duplicate code is less maintainable. - /// - /// ### Example - /// ```ignore - /// let foo = if … { - /// println!("Hello World"); - /// 13 - /// } else { - /// println!("Hello World"); - /// 42 - /// }; - /// ``` - /// - /// Use instead: - /// ```ignore - /// println!("Hello World"); - /// let foo = if … { - /// 13 - /// } else { - /// 42 - /// }; - /// ``` - #[clippy::version = "1.53.0"] - pub BRANCHES_SHARING_CODE, - nursery, - "`if` statement with shared code in all blocks" -} - -pub struct CopyAndPaste<'tcx> { - interior_mut: InteriorMut<'tcx>, -} - -impl<'tcx> CopyAndPaste<'tcx> { - pub fn new(tcx: TyCtxt<'tcx>, conf: &'static Conf) -> Self { - Self { - interior_mut: InteriorMut::new(tcx, &conf.ignore_interior_mutability), - } - } -} - -impl_lint_pass!(CopyAndPaste<'_> => [ - IFS_SAME_COND, - SAME_FUNCTIONS_IN_IF_CONDITION, - IF_SAME_THEN_ELSE, - BRANCHES_SHARING_CODE -]); - -impl<'tcx> LateLintPass<'tcx> for CopyAndPaste<'tcx> { - fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { - if !expr.span.from_expansion() && matches!(expr.kind, ExprKind::If(..)) && !is_else_clause(cx.tcx, expr) { - let (conds, blocks) = if_sequence(expr); - lint_same_cond(cx, &conds, &mut self.interior_mut); - lint_same_fns_in_if_cond(cx, &conds); - let all_same = - !is_lint_allowed(cx, IF_SAME_THEN_ELSE, expr.hir_id) && lint_if_same_then_else(cx, &conds, &blocks); - if !all_same && conds.len() != blocks.len() { - lint_branches_sharing_code(cx, &conds, &blocks, expr); - } - } - } -} - -fn lint_if_same_then_else(cx: &LateContext<'_>, conds: &[&Expr<'_>], blocks: &[&Block<'_>]) -> bool { - let mut eq = SpanlessEq::new(cx); - blocks - .array_windows::<2>() - .enumerate() - .fold(true, |all_eq, (i, &[lhs, rhs])| { - if eq.eq_block(lhs, rhs) && !has_let_expr(conds[i]) && conds.get(i + 1).is_none_or(|e| !has_let_expr(e)) { - span_lint_and_note( - cx, - IF_SAME_THEN_ELSE, - lhs.span, - "this `if` has identical blocks", - Some(rhs.span), - "same as this", - ); - all_eq - } else { - false - } - }) -} +use super::BRANCHES_SHARING_CODE; -fn lint_branches_sharing_code<'tcx>( +pub(super) fn check<'tcx>( cx: &LateContext<'tcx>, conds: &[&'tcx Expr<'_>], blocks: &[&'tcx Block<'_>], @@ -356,8 +161,8 @@ fn modifies_any_local<'tcx>(cx: &LateContext<'tcx>, s: &'tcx Stmt<'_>, locals: & .is_some() } -/// Checks if the given statement should be considered equal to the statement in the same position -/// for each block. +/// Checks if the given statement should be considered equal to the statement in the same +/// position for each block. fn eq_stmts( stmt: &Stmt<'_>, blocks: &[&Block<'_>], @@ -516,9 +321,9 @@ fn scan_block_for_eq<'tcx>( } } -/// Adjusts the index for which the statements begin to differ to the closest macro callsite. This -/// avoids giving suggestions that requires splitting a macro call in half, when only a part of the -/// macro expansion is equal. +/// Adjusts the index for which the statements begin to differ to the closest macro callsite. +/// This avoids giving suggestions that requires splitting a macro call in half, when only a +/// part of the macro expansion is equal. /// /// For example, for the following macro: /// ```rust,ignore @@ -587,70 +392,6 @@ fn check_for_warn_of_moved_symbol(cx: &LateContext<'_>, symbols: &[(HirId, Symbo }) } -fn method_caller_is_mutable<'tcx>( - cx: &LateContext<'tcx>, - caller_expr: &Expr<'_>, - interior_mut: &mut InteriorMut<'tcx>, -) -> bool { - let caller_ty = cx.typeck_results().expr_ty(caller_expr); - - interior_mut.is_interior_mut_ty(cx, caller_ty) - || caller_ty.is_mutable_ptr() - // `find_binding_init` will return the binding iff its not mutable - || path_to_local(caller_expr) - .and_then(|hid| find_binding_init(cx, hid)) - .is_none() -} - -/// Implementation of `IFS_SAME_COND`. -fn lint_same_cond<'tcx>(cx: &LateContext<'tcx>, conds: &[&Expr<'_>], interior_mut: &mut InteriorMut<'tcx>) { - for group in search_same( - conds, - |e| hash_expr(cx, e), - |lhs, rhs| { - // Ignore eq_expr side effects iff one of the expression kind is a method call - // and the caller is not a mutable, including inner mutable type. - if let ExprKind::MethodCall(_, caller, _, _) = lhs.kind { - if method_caller_is_mutable(cx, caller, interior_mut) { - false - } else { - SpanlessEq::new(cx).eq_expr(lhs, rhs) - } - } else { - eq_expr_value(cx, lhs, rhs) - } - }, - ) { - let spans: Vec<_> = group.into_iter().map(|expr| expr.span).collect(); - span_lint(cx, IFS_SAME_COND, spans, "these `if` branches have the same condition"); - } -} - -/// Implementation of `SAME_FUNCTIONS_IN_IF_CONDITION`. -fn lint_same_fns_in_if_cond(cx: &LateContext<'_>, conds: &[&Expr<'_>]) { - let eq: &dyn Fn(&&Expr<'_>, &&Expr<'_>) -> bool = &|&lhs, &rhs| -> bool { - // Do not lint if any expr originates from a macro - if lhs.span.from_expansion() || rhs.span.from_expansion() { - return false; - } - // Do not spawn warning if `IFS_SAME_COND` already produced it. - if eq_expr_value(cx, lhs, rhs) { - return false; - } - SpanlessEq::new(cx).eq_expr(lhs, rhs) - }; - - for group in search_same(conds, |e| hash_expr(cx, e), eq) { - let spans: Vec<_> = group.into_iter().map(|expr| expr.span).collect(); - span_lint( - cx, - SAME_FUNCTIONS_IN_IF_CONDITION, - spans, - "these `if` branches have the same function call", - ); - } -} - fn is_expr_parent_assignment(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { let parent = cx.tcx.parent_hir_node(expr.hir_id); if let Node::LetStmt(LetStmt { init: Some(e), .. }) diff --git a/clippy_lints/src/ifs/if_same_then_else.rs b/clippy_lints/src/ifs/if_same_then_else.rs new file mode 100644 index 000000000000..69402ec89076 --- /dev/null +++ b/clippy_lints/src/ifs/if_same_then_else.rs @@ -0,0 +1,29 @@ +use clippy_utils::SpanlessEq; +use clippy_utils::diagnostics::span_lint_and_note; +use clippy_utils::higher::has_let_expr; +use rustc_hir::{Block, Expr}; +use rustc_lint::LateContext; + +use super::IF_SAME_THEN_ELSE; + +pub(super) fn check(cx: &LateContext<'_>, conds: &[&Expr<'_>], blocks: &[&Block<'_>]) -> bool { + let mut eq = SpanlessEq::new(cx); + blocks + .array_windows::<2>() + .enumerate() + .fold(true, |all_eq, (i, &[lhs, rhs])| { + if eq.eq_block(lhs, rhs) && !has_let_expr(conds[i]) && conds.get(i + 1).is_none_or(|e| !has_let_expr(e)) { + span_lint_and_note( + cx, + IF_SAME_THEN_ELSE, + lhs.span, + "this `if` has identical blocks", + Some(rhs.span), + "same as this", + ); + all_eq + } else { + false + } + }) +} diff --git a/clippy_lints/src/ifs/ifs_same_cond.rs b/clippy_lints/src/ifs/ifs_same_cond.rs new file mode 100644 index 000000000000..ca76fc2587db --- /dev/null +++ b/clippy_lints/src/ifs/ifs_same_cond.rs @@ -0,0 +1,46 @@ +use clippy_utils::diagnostics::span_lint; +use clippy_utils::ty::InteriorMut; +use clippy_utils::{SpanlessEq, eq_expr_value, find_binding_init, hash_expr, path_to_local, search_same}; +use rustc_hir::{Expr, ExprKind}; +use rustc_lint::LateContext; + +use super::IFS_SAME_COND; + +fn method_caller_is_mutable<'tcx>( + cx: &LateContext<'tcx>, + caller_expr: &Expr<'_>, + interior_mut: &mut InteriorMut<'tcx>, +) -> bool { + let caller_ty = cx.typeck_results().expr_ty(caller_expr); + + interior_mut.is_interior_mut_ty(cx, caller_ty) + || caller_ty.is_mutable_ptr() + // `find_binding_init` will return the binding iff its not mutable + || path_to_local(caller_expr) + .and_then(|hid| find_binding_init(cx, hid)) + .is_none() +} + +/// Implementation of `IFS_SAME_COND`. +pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, conds: &[&Expr<'_>], interior_mut: &mut InteriorMut<'tcx>) { + for group in search_same( + conds, + |e| hash_expr(cx, e), + |lhs, rhs| { + // Ignore eq_expr side effects iff one of the expression kind is a method call + // and the caller is not a mutable, including inner mutable type. + if let ExprKind::MethodCall(_, caller, _, _) = lhs.kind { + if method_caller_is_mutable(cx, caller, interior_mut) { + false + } else { + SpanlessEq::new(cx).eq_expr(lhs, rhs) + } + } else { + eq_expr_value(cx, lhs, rhs) + } + }, + ) { + let spans: Vec<_> = group.into_iter().map(|expr| expr.span).collect(); + span_lint(cx, IFS_SAME_COND, spans, "these `if` branches have the same condition"); + } +} diff --git a/clippy_lints/src/ifs/mod.rs b/clippy_lints/src/ifs/mod.rs new file mode 100644 index 000000000000..739f2fc91729 --- /dev/null +++ b/clippy_lints/src/ifs/mod.rs @@ -0,0 +1,182 @@ +use clippy_config::Conf; +use clippy_utils::ty::InteriorMut; +use clippy_utils::{if_sequence, is_else_clause, is_lint_allowed}; +use rustc_hir::{Expr, ExprKind}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::ty::TyCtxt; +use rustc_session::impl_lint_pass; + +mod branches_sharing_code; +mod if_same_then_else; +mod ifs_same_cond; +mod same_functions_in_if_cond; + +declare_clippy_lint! { + /// ### What it does + /// Checks for consecutive `if`s with the same condition. + /// + /// ### Why is this bad? + /// This is probably a copy & paste error. + /// + /// ### Example + /// ```ignore + /// if a == b { + /// … + /// } else if a == b { + /// … + /// } + /// ``` + /// + /// Note that this lint ignores all conditions with a function call as it could + /// have side effects: + /// + /// ```ignore + /// if foo() { + /// … + /// } else if foo() { // not linted + /// … + /// } + /// ``` + #[clippy::version = "pre 1.29.0"] + pub IFS_SAME_COND, + correctness, + "consecutive `if`s with the same condition" +} + +declare_clippy_lint! { + /// ### What it does + /// Checks for consecutive `if`s with the same function call. + /// + /// ### Why is this bad? + /// This is probably a copy & paste error. + /// Despite the fact that function can have side effects and `if` works as + /// intended, such an approach is implicit and can be considered a "code smell". + /// + /// ### Example + /// ```ignore + /// if foo() == bar { + /// … + /// } else if foo() == bar { + /// … + /// } + /// ``` + /// + /// This probably should be: + /// ```ignore + /// if foo() == bar { + /// … + /// } else if foo() == baz { + /// … + /// } + /// ``` + /// + /// or if the original code was not a typo and called function mutates a state, + /// consider move the mutation out of the `if` condition to avoid similarity to + /// a copy & paste error: + /// + /// ```ignore + /// let first = foo(); + /// if first == bar { + /// … + /// } else { + /// let second = foo(); + /// if second == bar { + /// … + /// } + /// } + /// ``` + #[clippy::version = "1.41.0"] + pub SAME_FUNCTIONS_IN_IF_CONDITION, + pedantic, + "consecutive `if`s with the same function call" +} + +declare_clippy_lint! { + /// ### What it does + /// Checks for `if/else` with the same body as the *then* part + /// and the *else* part. + /// + /// ### Why is this bad? + /// This is probably a copy & paste error. + /// + /// ### Example + /// ```ignore + /// let foo = if … { + /// 42 + /// } else { + /// 42 + /// }; + /// ``` + #[clippy::version = "pre 1.29.0"] + pub IF_SAME_THEN_ELSE, + style, + "`if` with the same `then` and `else` blocks" +} + +declare_clippy_lint! { + /// ### What it does + /// Checks if the `if` and `else` block contain shared code that can be + /// moved out of the blocks. + /// + /// ### Why is this bad? + /// Duplicate code is less maintainable. + /// + /// ### Example + /// ```ignore + /// let foo = if … { + /// println!("Hello World"); + /// 13 + /// } else { + /// println!("Hello World"); + /// 42 + /// }; + /// ``` + /// + /// Use instead: + /// ```ignore + /// println!("Hello World"); + /// let foo = if … { + /// 13 + /// } else { + /// 42 + /// }; + /// ``` + #[clippy::version = "1.53.0"] + pub BRANCHES_SHARING_CODE, + nursery, + "`if` statement with shared code in all blocks" +} + +pub struct CopyAndPaste<'tcx> { + interior_mut: InteriorMut<'tcx>, +} + +impl<'tcx> CopyAndPaste<'tcx> { + pub fn new(tcx: TyCtxt<'tcx>, conf: &'static Conf) -> Self { + Self { + interior_mut: InteriorMut::new(tcx, &conf.ignore_interior_mutability), + } + } +} + +impl_lint_pass!(CopyAndPaste<'_> => [ + IFS_SAME_COND, + SAME_FUNCTIONS_IN_IF_CONDITION, + IF_SAME_THEN_ELSE, + BRANCHES_SHARING_CODE +]); + +impl<'tcx> LateLintPass<'tcx> for CopyAndPaste<'tcx> { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { + if !expr.span.from_expansion() && matches!(expr.kind, ExprKind::If(..)) && !is_else_clause(cx.tcx, expr) { + let (conds, blocks) = if_sequence(expr); + ifs_same_cond::check(cx, &conds, &mut self.interior_mut); + same_functions_in_if_cond::check(cx, &conds); + let all_same = + !is_lint_allowed(cx, IF_SAME_THEN_ELSE, expr.hir_id) && if_same_then_else::check(cx, &conds, &blocks); + if !all_same && conds.len() != blocks.len() { + branches_sharing_code::check(cx, &conds, &blocks, expr); + } + } + } +} diff --git a/clippy_lints/src/ifs/same_functions_in_if_cond.rs b/clippy_lints/src/ifs/same_functions_in_if_cond.rs new file mode 100644 index 000000000000..1f6bf04e22e7 --- /dev/null +++ b/clippy_lints/src/ifs/same_functions_in_if_cond.rs @@ -0,0 +1,31 @@ +use clippy_utils::diagnostics::span_lint; +use clippy_utils::{SpanlessEq, eq_expr_value, hash_expr, search_same}; +use rustc_hir::Expr; +use rustc_lint::LateContext; + +use super::SAME_FUNCTIONS_IN_IF_CONDITION; + +/// Implementation of `SAME_FUNCTIONS_IN_IF_CONDITION`. +pub(super) fn check(cx: &LateContext<'_>, conds: &[&Expr<'_>]) { + let eq: &dyn Fn(&&Expr<'_>, &&Expr<'_>) -> bool = &|&lhs, &rhs| -> bool { + // Do not lint if any expr originates from a macro + if lhs.span.from_expansion() || rhs.span.from_expansion() { + return false; + } + // Do not spawn warning if `IFS_SAME_COND` already produced it. + if eq_expr_value(cx, lhs, rhs) { + return false; + } + SpanlessEq::new(cx).eq_expr(lhs, rhs) + }; + + for group in search_same(conds, |e| hash_expr(cx, e), eq) { + let spans: Vec<_> = group.into_iter().map(|expr| expr.span).collect(); + span_lint( + cx, + SAME_FUNCTIONS_IN_IF_CONDITION, + spans, + "these `if` branches have the same function call", + ); + } +} diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index c56fa257b068..1cae377f3fbd 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -100,7 +100,6 @@ mod cognitive_complexity; mod collapsible_if; mod collection_is_never_read; mod comparison_chain; -mod copies; mod copy_iterator; mod crate_in_macro_def; mod create_dir; @@ -158,6 +157,7 @@ mod future_not_send; mod if_let_mutex; mod if_not_else; mod if_then_some_else_none; +mod ifs; mod ignored_unit_patterns; mod impl_hash_with_borrow_str_and_bytes; mod implicit_hasher; @@ -549,7 +549,7 @@ pub fn register_lint_passes(store: &mut rustc_lint::LintStore, conf: &'static Co store.register_late_pass(|_| Box::new(empty_enum::EmptyEnum)); store.register_late_pass(|_| Box::new(invalid_upcast_comparisons::InvalidUpcastComparisons)); store.register_late_pass(|_| Box::::default()); - store.register_late_pass(move |tcx| Box::new(copies::CopyAndPaste::new(tcx, conf))); + store.register_late_pass(move |tcx| Box::new(ifs::CopyAndPaste::new(tcx, conf))); store.register_late_pass(|_| Box::new(copy_iterator::CopyIterator)); let format_args = format_args_storage.clone(); store.register_late_pass(move |_| Box::new(format::UselessFormat::new(format_args.clone())));