From 8a9492aa03792f17c7b63b15d10ae9813daa4c4a Mon Sep 17 00:00:00 2001 From: J-ZhengLi Date: Thu, 16 Feb 2023 17:47:28 +0800 Subject: [PATCH 1/4] enhance [`ifs_same_cond`] to lint same immutable method calls as well --- clippy_lints/src/copies.rs | 26 +++++++++++++++++++++++--- tests/ui/ifs_same_cond.rs | 10 ++++++++++ tests/ui/ifs_same_cond.stderr | 14 +++++++++++++- 3 files changed, 46 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/copies.rs b/clippy_lints/src/copies.rs index f10c35cde52a..d729a5430c7a 100644 --- a/clippy_lints/src/copies.rs +++ b/clippy_lints/src/copies.rs @@ -3,8 +3,8 @@ use clippy_utils::source::{first_line_of_span, indent_of, reindent_multiline, sn use clippy_utils::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, 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; @@ -549,7 +549,27 @@ fn check_for_warn_of_moved_symbol(cx: &LateContext<'_>, symbols: &[(HirId, Symbo /// 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)) { + for (i, j) in search_same( + conds, + |e| hash_expr(cx, e), + |lhs, rhs| { + // If any side (ex. lhs) is a method call, and the caller is not mutable, + // then we can ignore side effects? + if let ExprKind::MethodCall(_, caller, _, _) = lhs.kind { + if path_to_local(caller) + .and_then(|hir_id| find_binding_init(cx, hir_id)) + .is_some() + { + // caller is not declared as mutable + SpanlessEq::new(cx).eq_expr(lhs, rhs) + } else { + false + } + } else { + eq_expr_value(cx, lhs, rhs) + } + }, + ) { span_lint_and_note( cx, IFS_SAME_COND, diff --git a/tests/ui/ifs_same_cond.rs b/tests/ui/ifs_same_cond.rs index 9850fc0919e1..e68ec6a85731 100644 --- a/tests/ui/ifs_same_cond.rs +++ b/tests/ui/ifs_same_cond.rs @@ -43,4 +43,14 @@ 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" { + } +} + fn main() {} diff --git a/tests/ui/ifs_same_cond.stderr b/tests/ui/ifs_same_cond.stderr index 4113087327a2..9519f6904cb1 100644 --- a/tests/ui/ifs_same_cond.stderr +++ b/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 From f0ae2b71ca4053db3b86c20e7d612ecc11d50ee2 Mon Sep 17 00:00:00 2001 From: J-ZhengLi Date: Fri, 24 Feb 2023 10:46:07 +0800 Subject: [PATCH 2/4] make [`ifs_same_cond`] use `ignore_interior_mutablility` configuration --- clippy_lints/src/copies.rs | 70 +++++++++++++++----- clippy_lints/src/lib.rs | 3 +- clippy_lints/src/utils/conf.rs | 2 +- tests/ui-toml/ifs_same_cond/clippy.toml | 1 + tests/ui-toml/ifs_same_cond/ifs_same_cond.rs | 24 +++++++ tests/ui/ifs_same_cond.rs | 8 +++ 6 files changed, 91 insertions(+), 17 deletions(-) create mode 100644 tests/ui-toml/ifs_same_cond/clippy.toml create mode 100644 tests/ui-toml/ifs_same_cond/ifs_same_cond.rs diff --git a/clippy_lints/src/copies.rs b/clippy_lints/src/copies.rs index d729a5430c7a..39f8f7220f1f 100644 --- a/clippy_lints/src/copies.rs +++ b/clippy_lints/src/copies.rs @@ -3,16 +3,19 @@ use clippy_utils::source::{first_line_of_span, indent_of, reindent_multiline, sn use clippy_utils::ty::needs_ordered_drop; use clippy_utils::visitors::for_each_expr; use clippy_utils::{ - 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, 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_data_structures::fx::FxHashSet; use rustc_errors::Applicability; +use rustc_hir::def_id::DefId; 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 +162,19 @@ declare_clippy_lint! { "`if` statement with shared code in all blocks" } -declare_lint_pass!(CopyAndPaste => [ +pub struct CopyAndPaste { + ignore_interior_mutability: Vec, +} + +impl CopyAndPaste { + pub fn new(ignore_interior_mutability: Vec) -> Self { + Self { + ignore_interior_mutability, + } + } +} + +impl_lint_pass!(CopyAndPaste => [ IFS_SAME_COND, SAME_FUNCTIONS_IN_IF_CONDITION, IF_SAME_THEN_ELSE, @@ -170,7 +185,14 @@ impl<'tcx> LateLintPass<'tcx> for CopyAndPaste { 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); + let mut ignored_ty_ids = FxHashSet::default(); + 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()) { + ignored_ty_ids.insert(id); + } + } + lint_same_cond(cx, &conds, &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,23 +569,41 @@ fn check_for_warn_of_moved_symbol(cx: &LateContext<'_>, symbols: &[(HirId, Symbo }) } +fn method_caller_is_ignored_or_mutable( + cx: &LateContext<'_>, + caller_expr: &Expr<'_>, + ignored_ty_ids: &FxHashSet, +) -> bool { + let caller_ty = cx.typeck_results().expr_ty(caller_expr); + let is_ignored_ty = if let Some(adt_id) = caller_ty.ty_adt_id() && ignored_ty_ids.contains(&adt_id) { + true + } else { + false + }; + + if is_ignored_ty + || caller_ty.is_mutable_ptr() + || path_to_local(caller_expr) + .and_then(|hid| find_binding_init(cx, hid)) + .is_none() + { + return true; + } + + false +} + /// Implementation of `IFS_SAME_COND`. -fn lint_same_cond(cx: &LateContext<'_>, conds: &[&Expr<'_>]) { +fn lint_same_cond(cx: &LateContext<'_>, conds: &[&Expr<'_>], ignored_ty_ids: &FxHashSet) { for (i, j) in search_same( conds, |e| hash_expr(cx, e), |lhs, rhs| { - // If any side (ex. lhs) is a method call, and the caller is not mutable, - // then we can ignore side effects? if let ExprKind::MethodCall(_, caller, _, _) = lhs.kind { - if path_to_local(caller) - .and_then(|hir_id| find_binding_init(cx, hir_id)) - .is_some() - { - // caller is not declared as mutable - SpanlessEq::new(cx).eq_expr(lhs, rhs) - } else { + if method_caller_is_ignored_or_mutable(cx, caller, ignored_ty_ids) { false + } else { + SpanlessEq::new(cx).eq_expr(lhs, rhs) } } else { eq_expr_value(cx, lhs, rhs) diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 491732be2087..bde84686cc1b 100644 --- a/clippy_lints/src/lib.rs +++ b/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)); diff --git a/clippy_lints/src/utils/conf.rs b/clippy_lints/src/utils/conf.rs index 1c7f3e96db89..8ba252425a3d 100644 --- a/clippy_lints/src/utils/conf.rs +++ b/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 diff --git a/tests/ui-toml/ifs_same_cond/clippy.toml b/tests/ui-toml/ifs_same_cond/clippy.toml new file mode 100644 index 000000000000..1615d970c688 --- /dev/null +++ b/tests/ui-toml/ifs_same_cond/clippy.toml @@ -0,0 +1 @@ +ignore-interior-mutability = ["std::cell::Cell"] \ No newline at end of file diff --git a/tests/ui-toml/ifs_same_cond/ifs_same_cond.rs b/tests/ui-toml/ifs_same_cond/ifs_same_cond.rs new file mode 100644 index 000000000000..92438e7d1f2a --- /dev/null +++ b/tests/ui-toml/ifs_same_cond/ifs_same_cond.rs @@ -0,0 +1,24 @@ +#![warn(clippy::ifs_same_cond)] +#![allow(clippy::if_same_then_else, clippy::comparison_chain)] + +fn main() {} + +fn issue10272() { + use std::cell::Cell; + + let x = Cell::new(true); + if x.get() { + } else if !x.take() { + } else if x.get() { + // ok, x is interior mutable type + } else { + } + + let a = [Cell::new(true)]; + if a[0].get() { + } else if a[0].take() { + } else if a[0].get() { + // ok, a contains interior mutable type + } else { + } +} diff --git a/tests/ui/ifs_same_cond.rs b/tests/ui/ifs_same_cond.rs index e68ec6a85731..ae91611c472e 100644 --- a/tests/ui/ifs_same_cond.rs +++ b/tests/ui/ifs_same_cond.rs @@ -51,6 +51,14 @@ fn issue10272() { } 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 { + } } fn main() {} From f4ccb06d699b91b8de8b797a584ce185d6856ec2 Mon Sep 17 00:00:00 2001 From: J-ZhengLi Date: Mon, 13 Mar 2023 20:17:30 +0800 Subject: [PATCH 3/4] extract `is_interior_mutable_type` from [`mut_key`] to `clippy_utils::ty`; fix configuration of [`ifs_same_cond`]; add some style improvement for [`ifs_same_cond`]; --- clippy_lints/src/copies.rs | 52 +++++++--------- clippy_lints/src/mut_key.rs | 60 ++++--------------- clippy_utils/src/ty.rs | 44 ++++++++++++++ tests/ui-toml/ifs_same_cond/clippy.toml | 2 +- tests/ui-toml/ifs_same_cond/ifs_same_cond.rs | 12 +--- .../ifs_same_cond/ifs_same_cond.stderr | 15 +++++ tests/ui/ifs_same_cond.rs | 8 +++ 7 files changed, 107 insertions(+), 86 deletions(-) create mode 100644 tests/ui-toml/ifs_same_cond/ifs_same_cond.stderr diff --git a/clippy_lints/src/copies.rs b/clippy_lints/src/copies.rs index 39f8f7220f1f..970f50049935 100644 --- a/clippy_lints/src/copies.rs +++ b/clippy_lints/src/copies.rs @@ -1,6 +1,6 @@ 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, def_path_def_ids, eq_expr_value, find_binding_init, get_enclosing_block, hash_expr, hash_stmt, @@ -8,9 +8,8 @@ use clippy_utils::{ }; use core::iter; use core::ops::ControlFlow; -use rustc_data_structures::fx::FxHashSet; use rustc_errors::Applicability; -use rustc_hir::def_id::DefId; +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}; @@ -164,12 +163,14 @@ declare_clippy_lint! { pub struct CopyAndPaste { ignore_interior_mutability: Vec, + ignored_ty_ids: DefIdSet, } impl CopyAndPaste { pub fn new(ignore_interior_mutability: Vec) -> Self { Self { ignore_interior_mutability, + ignored_ty_ids: DefIdSet::new(), } } } @@ -182,17 +183,18 @@ impl_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); - let mut ignored_ty_ids = FxHashSet::default(); - 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()) { - ignored_ty_ids.insert(id); - } - } - lint_same_cond(cx, &conds, &ignored_ty_ids); + 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); @@ -569,38 +571,30 @@ fn check_for_warn_of_moved_symbol(cx: &LateContext<'_>, symbols: &[(HirId, Symbo }) } -fn method_caller_is_ignored_or_mutable( - cx: &LateContext<'_>, - caller_expr: &Expr<'_>, - ignored_ty_ids: &FxHashSet, -) -> bool { +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); - let is_ignored_ty = if let Some(adt_id) = caller_ty.ty_adt_id() && ignored_ty_ids.contains(&adt_id) { - true - } else { - false - }; + // 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)); - if is_ignored_ty + 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() - { - return true; - } - - false } /// Implementation of `IFS_SAME_COND`. -fn lint_same_cond(cx: &LateContext<'_>, conds: &[&Expr<'_>], ignored_ty_ids: &FxHashSet) { +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_ignored_or_mutable(cx, caller, ignored_ty_ids) { + if method_caller_is_mutable(cx, caller, ignored_ty_ids) { false } else { SpanlessEq::new(cx).eq_expr(lhs, rhs) diff --git a/clippy_lints/src/mut_key.rs b/clippy_lints/src/mut_key.rs index 8aa814b74053..309f67521a3b 100644 --- a/clippy_lints/src/mut_key.rs +++ b/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)) { - span_lint(cx, MUTABLE_KEY_TYPE, span, "mutable key type"); + if !is_keyed_type { + return; } - } - } - /// 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, + 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"); + } } } } diff --git a/clippy_utils/src/ty.rs b/clippy_utils/src/ty.rs index e0ea3952785b..f1c6f1dddd8a 100644 --- a/clippy_utils/src/ty.rs +++ b/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, + } +} diff --git a/tests/ui-toml/ifs_same_cond/clippy.toml b/tests/ui-toml/ifs_same_cond/clippy.toml index 1615d970c688..90a36ecd9202 100644 --- a/tests/ui-toml/ifs_same_cond/clippy.toml +++ b/tests/ui-toml/ifs_same_cond/clippy.toml @@ -1 +1 @@ -ignore-interior-mutability = ["std::cell::Cell"] \ No newline at end of file +ignore-interior-mutability = ["std::cell::Cell"] diff --git a/tests/ui-toml/ifs_same_cond/ifs_same_cond.rs b/tests/ui-toml/ifs_same_cond/ifs_same_cond.rs index 92438e7d1f2a..d623ac7e0200 100644 --- a/tests/ui-toml/ifs_same_cond/ifs_same_cond.rs +++ b/tests/ui-toml/ifs_same_cond/ifs_same_cond.rs @@ -6,19 +6,13 @@ 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() { - // ok, x is interior mutable type - } else { - } - - let a = [Cell::new(true)]; - if a[0].get() { - } else if a[0].take() { - } else if a[0].get() { - // ok, a contains interior mutable type } else { } } diff --git a/tests/ui-toml/ifs_same_cond/ifs_same_cond.stderr b/tests/ui-toml/ifs_same_cond/ifs_same_cond.stderr new file mode 100644 index 000000000000..2841f62bc94a --- /dev/null +++ b/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 + diff --git a/tests/ui/ifs_same_cond.rs b/tests/ui/ifs_same_cond.rs index ae91611c472e..9ce9a87626a7 100644 --- a/tests/ui/ifs_same_cond.rs +++ b/tests/ui/ifs_same_cond.rs @@ -59,6 +59,14 @@ fn issue10272() { // 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() {} From 011bb463370aceee33d7f00a39d87ae8fc856a1c Mon Sep 17 00:00:00 2001 From: J-ZhengLi Date: Tue, 14 Mar 2023 10:24:28 +0800 Subject: [PATCH 4/4] update lint configuration doc for [`ifs_same_cond`] --- book/src/lint_configuration.md | 1 + 1 file changed, 1 insertion(+) diff --git a/book/src/lint_configuration.md b/book/src/lint_configuration.md index 995dd2f04b1e..9ed6627b7413 100644 --- a/book/src/lint_configuration.md +++ b/book/src/lint_configuration.md @@ -519,6 +519,7 @@ for the generic parameters for determining interior mutability **Default Value:** `["bytes::Bytes"]` (`Vec`) * [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