Skip to content

Commit

Permalink
make [ifs_same_cond] use ignore_interior_mutablility configuration
Browse files Browse the repository at this point in the history
  • Loading branch information
J-ZhengLi committed Feb 24, 2023
1 parent 063bd09 commit b180be9
Show file tree
Hide file tree
Showing 6 changed files with 91 additions and 17 deletions.
70 changes: 55 additions & 15 deletions clippy_lints/src/copies.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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<String>,
}

impl CopyAndPaste {
pub fn new(ignore_interior_mutability: Vec<String>) -> Self {
Self {
ignore_interior_mutability,
}
}
}

impl_lint_pass!(CopyAndPaste => [
IFS_SAME_COND,
SAME_FUNCTIONS_IN_IF_CONDITION,
IF_SAME_THEN_ELSE,
Expand All @@ -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);
Expand Down Expand Up @@ -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<DefId>,
) -> 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<DefId>) {
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)
Expand Down
3 changes: 2 additions & 1 deletion clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -645,7 +645,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));
Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/utils/conf.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions tests/ui-toml/ifs_same_cond/clippy.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
ignore-interior-mutability = ["std::cell::Cell"]
24 changes: 24 additions & 0 deletions tests/ui-toml/ifs_same_cond/ifs_same_cond.rs
Original file line number Diff line number Diff line change
@@ -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 {
}
}
8 changes: 8 additions & 0 deletions tests/ui/ifs_same_cond.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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() {}

0 comments on commit b180be9

Please sign in to comment.