From f4ccb06d699b91b8de8b797a584ce185d6856ec2 Mon Sep 17 00:00:00 2001 From: J-ZhengLi Date: Mon, 13 Mar 2023 20:17:30 +0800 Subject: [PATCH] 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 39f8f7220f1..970f5004993 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 8aa814b7405..309f67521a3 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)) { + 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, - } - } } diff --git a/clippy_utils/src/ty.rs b/clippy_utils/src/ty.rs index e0ea3952785..f1c6f1dddd8 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 1615d970c68..90a36ecd920 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 92438e7d1f2..d623ac7e020 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 00000000000..2841f62bc94 --- /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 ae91611c472..9ce9a87626a 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() {}