Auto merge of #10350 - J-ZhengLi:issue_10272, r=xFrednet

enhance [`ifs_same_cond`] to warn same immutable method calls as well

fixes: #10272

---

changelog: Enhancement: [`ifs_same_cond`]: Now also detects immutable method calls.
[#10350](https://github.com/rust-lang/rust-clippy/pull/10350)
<!-- changelog_checked -->

book/src/lint_configuration.md

@@ -519,6 +519,7 @@ for the generic parameters for determining interior mutability
 **Default Value:** `["bytes::Bytes"]` (`Vec<String>`)
 
 * [mutable_key_type](https://rust-lang.github.io/rust-clippy/master/index.html#mutable_key_type)
+* [ifs_same_cond](https://rust-lang.github.io/rust-clippy/master/index.html#ifs_same_cond)
 
 
 ### allow-mixed-uninlined-format-args

clippy_lints/src/copies.rs

@@ -1,18 +1,20 @@
 use clippy_utils::diagnostics::{span_lint_and_note, span_lint_and_then};
 use clippy_utils::source::{first_line_of_span, indent_of, reindent_multiline, snippet, snippet_opt};
-use clippy_utils::ty::needs_ordered_drop;
+use clippy_utils::ty::{is_interior_mut_ty, needs_ordered_drop};
 use clippy_utils::visitors::for_each_expr;
 use clippy_utils::{
-    capture_local_usage, eq_expr_value, 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, def_path_def_ids, 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,
 };
 use core::iter;
 use core::ops::ControlFlow;
 use rustc_errors::Applicability;
+use rustc_hir::def_id::DefIdSet;
 use rustc_hir::intravisit;
 use rustc_hir::{BinOpKind, Block, Expr, ExprKind, HirId, HirIdSet, Stmt, StmtKind};
 use rustc_lint::{LateContext, LateLintPass};
-use rustc_session::{declare_lint_pass, declare_tool_lint};
+use rustc_middle::query::Key;
+use rustc_session::{declare_tool_lint, impl_lint_pass};
 use rustc_span::hygiene::walk_chain;
 use rustc_span::source_map::SourceMap;
 use rustc_span::{BytePos, Span, Symbol};
@@ -159,7 +161,21 @@ declare_clippy_lint! {
     "`if` statement with shared code in all blocks"
 }
 
-declare_lint_pass!(CopyAndPaste => [
+pub struct CopyAndPaste {
+    ignore_interior_mutability: Vec<String>,
+    ignored_ty_ids: DefIdSet,
+}
+
+impl CopyAndPaste {
+    pub fn new(ignore_interior_mutability: Vec<String>) -> Self {
+        Self {
+            ignore_interior_mutability,
+            ignored_ty_ids: DefIdSet::new(),
+        }
+    }
+}
+
+impl_lint_pass!(CopyAndPaste => [
     IFS_SAME_COND,
     SAME_FUNCTIONS_IN_IF_CONDITION,
     IF_SAME_THEN_ELSE,
@@ -167,10 +183,18 @@ declare_lint_pass!(CopyAndPaste => [
 ]);
 
 impl<'tcx> LateLintPass<'tcx> for CopyAndPaste {
+    fn check_crate(&mut self, cx: &LateContext<'tcx>) {
+        for ignored_ty in &self.ignore_interior_mutability {
+            let path: Vec<&str> = ignored_ty.split("::").collect();
+            for id in def_path_def_ids(cx, path.as_slice()) {
+                self.ignored_ty_ids.insert(id);
+            }
+        }
+    }
     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);
+            lint_same_cond(cx, &conds, &self.ignored_ty_ids);
             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);
@@ -547,9 +571,39 @@ fn check_for_warn_of_moved_symbol(cx: &LateContext<'_>, symbols: &[(HirId, Symbo
     })
 }
 
+fn method_caller_is_mutable(cx: &LateContext<'_>, caller_expr: &Expr<'_>, ignored_ty_ids: &DefIdSet) -> bool {
+    let caller_ty = cx.typeck_results().expr_ty(caller_expr);
+    // Check if given type has inner mutability and was not set to ignored by the configuration
+    let is_inner_mut_ty = is_interior_mut_ty(cx, caller_ty)
+        && !matches!(caller_ty.ty_adt_id(), Some(adt_id) if ignored_ty_ids.contains(&adt_id));
+
+    is_inner_mut_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(cx: &LateContext<'_>, conds: &[&Expr<'_>]) {
-    for (i, j) in search_same(conds, |e| hash_expr(cx, e), |lhs, rhs| eq_expr_value(cx, lhs, rhs)) {
+fn lint_same_cond(cx: &LateContext<'_>, conds: &[&Expr<'_>], ignored_ty_ids: &DefIdSet) {
+    for (i, j) in search_same(
+        conds,
+        |e| hash_expr(cx, e),
+        |lhs, rhs| {
+            // Ignore eq_expr side effects iff one of the expressin 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, ignored_ty_ids) {
+                    false
+                } else {
+                    SpanlessEq::new(cx).eq_expr(lhs, rhs)
+                }
+            } else {
+                eq_expr_value(cx, lhs, rhs)
+            }
+        },
+    ) {
         span_lint_and_note(
             cx,
             IFS_SAME_COND,

clippy_lints/src/lib.rs

@@ -656,7 +656,8 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(|_| Box::new(empty_enum::EmptyEnum));
     store.register_late_pass(|_| Box::new(invalid_upcast_comparisons::InvalidUpcastComparisons));
     store.register_late_pass(|_| Box::new(regex::Regex));
-    store.register_late_pass(|_| Box::new(copies::CopyAndPaste));
+    let ignore_interior_mutability = conf.ignore_interior_mutability.clone();
+    store.register_late_pass(move |_| Box::new(copies::CopyAndPaste::new(ignore_interior_mutability.clone())));
     store.register_late_pass(|_| Box::new(copy_iterator::CopyIterator));
     store.register_late_pass(|_| Box::new(format::UselessFormat));
     store.register_late_pass(|_| Box::new(swap::Swap));

clippy_lints/src/mut_key.rs

@@ -1,10 +1,11 @@
 use clippy_utils::diagnostics::span_lint;
+use clippy_utils::ty::is_interior_mut_ty;
 use clippy_utils::{def_path_def_ids, trait_ref_of_method};
 use rustc_data_structures::fx::FxHashSet;
 use rustc_hir as hir;
 use rustc_lint::{LateContext, LateLintPass};
-use rustc_middle::ty::TypeVisitableExt;
-use rustc_middle::ty::{Adt, Array, Ref, Slice, Tuple, Ty};
+use rustc_middle::query::Key;
+use rustc_middle::ty::{Adt, Ty};
 use rustc_session::{declare_tool_lint, impl_lint_pass};
 use rustc_span::def_id::LocalDefId;
 use rustc_span::source_map::Span;
@@ -153,53 +154,18 @@ impl MutableKeyType {
             let is_keyed_type = [sym::HashMap, sym::BTreeMap, sym::HashSet, sym::BTreeSet]
                 .iter()
                 .any(|diag_item| cx.tcx.is_diagnostic_item(*diag_item, def.did()));
-            if is_keyed_type && self.is_interior_mutable_type(cx, substs.type_at(0)) {
+            if !is_keyed_type {
+                return;
+            }
+
+            let subst_ty = substs.type_at(0);
+            // Determines if a type contains interior mutability which would affect its implementation of
+            // [`Hash`] or [`Ord`].
+            if is_interior_mut_ty(cx, subst_ty)
+                && !matches!(subst_ty.ty_adt_id(), Some(adt_id) if self.ignore_mut_def_ids.contains(&adt_id))
+            {
                 span_lint(cx, MUTABLE_KEY_TYPE, span, "mutable key type");
             }
         }
     }
-
-    /// Determines if a type contains interior mutability which would affect its implementation of
-    /// [`Hash`] or [`Ord`].
-    fn is_interior_mutable_type<'tcx>(&self, cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
-        match *ty.kind() {
-            Ref(_, inner_ty, mutbl) => mutbl == hir::Mutability::Mut || self.is_interior_mutable_type(cx, inner_ty),
-            Slice(inner_ty) => self.is_interior_mutable_type(cx, inner_ty),
-            Array(inner_ty, size) => {
-                size.try_eval_target_usize(cx.tcx, cx.param_env)
-                    .map_or(true, |u| u != 0)
-                    && self.is_interior_mutable_type(cx, inner_ty)
-            },
-            Tuple(fields) => fields.iter().any(|ty| self.is_interior_mutable_type(cx, ty)),
-            Adt(def, substs) => {
-                // Special case for collections in `std` who's impl of `Hash` or `Ord` delegates to
-                // that of their type parameters.  Note: we don't include `HashSet` and `HashMap`
-                // because they have no impl for `Hash` or `Ord`.
-                let def_id = def.did();
-                let is_std_collection = [
-                    sym::Option,
-                    sym::Result,
-                    sym::LinkedList,
-                    sym::Vec,
-                    sym::VecDeque,
-                    sym::BTreeMap,
-                    sym::BTreeSet,
-                    sym::Rc,
-                    sym::Arc,
-                ]
-                .iter()
-                .any(|diag_item| cx.tcx.is_diagnostic_item(*diag_item, def_id));
-                let is_box = Some(def_id) == cx.tcx.lang_items().owned_box();
-                if is_std_collection || is_box || self.ignore_mut_def_ids.contains(&def_id) {
-                    // The type is mutable if any of its type parameters are
-                    substs.types().any(|ty| self.is_interior_mutable_type(cx, ty))
-                } else {
-                    !ty.has_escaping_bound_vars()
-                        && cx.tcx.layout_of(cx.param_env.and(ty)).is_ok()
-                        && !ty.is_freeze(cx.tcx, cx.param_env)
-                }
-            },
-            _ => false,
-        }
-    }
 }

clippy_lints/src/utils/conf.rs

@@ -437,7 +437,7 @@ define_Conf! {
     ///
     /// The maximum size of the `Err`-variant in a `Result` returned from a function
     (large_error_threshold: u64 = 128),
-    /// Lint: MUTABLE_KEY_TYPE.
+    /// Lint: MUTABLE_KEY_TYPE, IFS_SAME_COND.
     ///
     /// A list of paths to types that should be treated like `Arc`, i.e. ignored but
     /// for the generic parameters for determining interior mutability

clippy_utils/src/ty.rs

@@ -1121,3 +1121,47 @@ pub fn make_normalized_projection<'tcx>(
     }
     helper(tcx, param_env, make_projection(tcx, container_id, assoc_ty, substs)?)
 }
+
+/// Check if given type has inner mutability such as [`std::cell::Cell`] or [`std::cell::RefCell`]
+/// etc.
+pub fn is_interior_mut_ty<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
+    match *ty.kind() {
+        ty::Ref(_, inner_ty, mutbl) => mutbl == Mutability::Mut || is_interior_mut_ty(cx, inner_ty),
+        ty::Slice(inner_ty) => is_interior_mut_ty(cx, inner_ty),
+        ty::Array(inner_ty, size) => {
+            size.try_eval_target_usize(cx.tcx, cx.param_env)
+                .map_or(true, |u| u != 0)
+                && is_interior_mut_ty(cx, inner_ty)
+        },
+        ty::Tuple(fields) => fields.iter().any(|ty| is_interior_mut_ty(cx, ty)),
+        ty::Adt(def, substs) => {
+            // Special case for collections in `std` who's impl of `Hash` or `Ord` delegates to
+            // that of their type parameters.  Note: we don't include `HashSet` and `HashMap`
+            // because they have no impl for `Hash` or `Ord`.
+            let def_id = def.did();
+            let is_std_collection = [
+                sym::Option,
+                sym::Result,
+                sym::LinkedList,
+                sym::Vec,
+                sym::VecDeque,
+                sym::BTreeMap,
+                sym::BTreeSet,
+                sym::Rc,
+                sym::Arc,
+            ]
+            .iter()
+            .any(|diag_item| cx.tcx.is_diagnostic_item(*diag_item, def_id));
+            let is_box = Some(def_id) == cx.tcx.lang_items().owned_box();
+            if is_std_collection || is_box {
+                // The type is mutable if any of its type parameters are
+                substs.types().any(|ty| is_interior_mut_ty(cx, ty))
+            } else {
+                !ty.has_escaping_bound_vars()
+                    && cx.tcx.layout_of(cx.param_env.and(ty)).is_ok()
+                    && !ty.is_freeze(cx.tcx, cx.param_env)
+            }
+        },
+        _ => false,
+    }
+}

tests/ui-toml/ifs_same_cond/clippy.toml

@@ -0,0 +1 @@
+ignore-interior-mutability = ["std::cell::Cell"]

tests/ui-toml/ifs_same_cond/ifs_same_cond.rs

@@ -0,0 +1,18 @@
+#![warn(clippy::ifs_same_cond)]
+#![allow(clippy::if_same_then_else, clippy::comparison_chain)]
+
+fn main() {}
+
+fn issue10272() {
+    use std::cell::Cell;
+
+    // Because the `ignore-interior-mutability` configuration
+    // is set to ignore for `std::cell::Cell`, the following `get()` calls
+    // should trigger warning
+    let x = Cell::new(true);
+    if x.get() {
+    } else if !x.take() {
+    } else if x.get() {
+    } else {
+    }
+}

tests/ui-toml/ifs_same_cond/ifs_same_cond.stderr

@@ -0,0 +1,15 @@
+error: this `if` has the same condition as a previous `if`
+  --> $DIR/ifs_same_cond.rs:15:15
+   |
+LL |     } else if x.get() {
+   |               ^^^^^^^
+   |
+note: same as this
+  --> $DIR/ifs_same_cond.rs:13:8
+   |
+LL |     if x.get() {
+   |        ^^^^^^^
+   = note: `-D clippy::ifs-same-cond` implied by `-D warnings`
+
+error: aborting due to previous error
+

tests/ui/ifs_same_cond.rs

@@ -43,4 +43,30 @@ fn ifs_same_cond() {
     }
 }
 
+fn issue10272() {
+    let a = String::from("ha");
+    if a.contains("ah") {
+    } else if a.contains("ah") {
+        // Trigger this lint
+    } else if a.contains("ha") {
+    } else if a == "wow" {
+    }
+
+    let p: *mut i8 = std::ptr::null_mut();
+    if p.is_null() {
+    } else if p.align_offset(0) == 0 {
+    } else if p.is_null() {
+        // ok, p is mutable pointer
+    } else {
+    }
+
+    let x = std::cell::Cell::new(true);
+    if x.get() {
+    } else if !x.take() {
+    } else if x.get() {
+        // ok, x is interior mutable type
+    } else {
+    }
+}
+
 fn main() {}

tests/ui/ifs_same_cond.stderr

@@ -35,5 +35,17 @@ note: same as this
 LL |     if 2 * a == 1 {
    |        ^^^^^^^^^^
 
-error: aborting due to 3 previous errors
+error: this `if` has the same condition as a previous `if`
+  --> $DIR/ifs_same_cond.rs:49:15
+   |
+LL |     } else if a.contains("ah") {
+   |               ^^^^^^^^^^^^^^^^
+   |
+note: same as this
+  --> $DIR/ifs_same_cond.rs:48:8
+   |
+LL |     if a.contains("ah") {
+   |        ^^^^^^^^^^^^^^^^
+
+error: aborting due to 4 previous errors