From f0ae2b71ca4053db3b86c20e7d612ecc11d50ee2 Mon Sep 17 00:00:00 2001 From: J-ZhengLi Date: Fri, 24 Feb 2023 10:46:07 +0800 Subject: [PATCH] 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 d729a5430c7..39f8f7220f1 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 491732be208..bde84686cc1 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 1c7f3e96db8..8ba252425a3 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 00000000000..1615d970c68 --- /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 00000000000..92438e7d1f2 --- /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 e68ec6a8573..ae91611c472 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() {}