Skip to content

Commit

Permalink
Factor BinOp utils
Browse files Browse the repository at this point in the history
  • Loading branch information
@camsteffen
camsteffen committed Jul 18, 2021
1 parent efbf7ca commit 98c500c
Show file tree
Hide file tree
Showing 4 changed files with 95 additions and 188 deletions.
91 changes: 27 additions & 64 deletions clippy_lints/src/assign_ops.rs
View file
@@ -1,8 +1,8 @@
use clippy_utils::diagnostics::span_lint_and_then;
use clippy_utils::source::snippet_opt;
use clippy_utils::ty::implements_trait;
use clippy_utils::{eq_expr_value, get_trait_def_id, trait_ref_of_method};
use clippy_utils::{paths, sugg};
use clippy_utils::{binop_traits, sugg};
use clippy_utils::{eq_expr_value, trait_ref_of_method};
use if_chain::if_chain;
use rustc_errors::Applicability;
use rustc_hir as hir;
Expand Down Expand Up @@ -85,71 +85,34 @@ impl<'tcx> LateLintPass<'tcx> for AssignOps {
let lint = |assignee: &hir::Expr<'_>, rhs: &hir::Expr<'_>| {
let ty = cx.typeck_results().expr_ty(assignee);
let rty = cx.typeck_results().expr_ty(rhs);
macro_rules! ops {
($op:expr,
$cx:expr,
$ty:expr,
$rty:expr,
$($trait_name:ident),+) => {
match $op {
$(hir::BinOpKind::$trait_name => {
let [krate, module] = paths::OPS_MODULE;
let path: [&str; 3] = [krate, module, concat!(stringify!($trait_name), "Assign")];
let trait_id = if let Some(trait_id) = get_trait_def_id($cx, &path) {
trait_id
} else {
return; // useless if the trait doesn't exist
};
// check that we are not inside an `impl AssignOp` of this exact operation
let parent_fn = cx.tcx.hir().get_parent_item(e.hir_id);
if_chain! {
if let Some(trait_ref) = trait_ref_of_method(cx, parent_fn);
if trait_ref.path.res.def_id() == trait_id;
then { return; }
if_chain! {
if let Some((_, lang_item)) = binop_traits(op.node);
if let Ok(trait_id) = cx.tcx.lang_items().require(lang_item);
let parent_fn = cx.tcx.hir().get_parent_item(e.hir_id);
if trait_ref_of_method(cx, parent_fn)
.map_or(true, |t| t.path.res.def_id() != trait_id);
if implements_trait(cx, ty, trait_id, &[rty.into()]);
then {
span_lint_and_then(
cx,
ASSIGN_OP_PATTERN,
expr.span,
"manual implementation of an assign operation",
|diag| {
if let (Some(snip_a), Some(snip_r)) =
(snippet_opt(cx, assignee.span), snippet_opt(cx, rhs.span))
{
diag.span_suggestion(
expr.span,
"replace it with",
format!("{} {}= {}", snip_a, op.node.as_str(), snip_r),
Applicability::MachineApplicable,
);
}
implements_trait($cx, $ty, trait_id, &[$rty])
},)*
_ => false,
}
},
);
}
}
if ops!(
op.node,
cx,
ty,
rty.into(),
Add,
Sub,
Mul,
Div,
Rem,
And,
Or,
BitAnd,
BitOr,
BitXor,
Shr,
Shl
) {
span_lint_and_then(
cx,
ASSIGN_OP_PATTERN,
expr.span,
"manual implementation of an assign operation",
|diag| {
if let (Some(snip_a), Some(snip_r)) =
(snippet_opt(cx, assignee.span), snippet_opt(cx, rhs.span))
{
diag.span_suggestion(
expr.span,
"replace it with",
format!("{} {}= {}", snip_a, op.node.as_str(), snip_r),
Applicability::MachineApplicable,
);
}
},
);
}
};

let mut visitor = ExprVisitor {
Expand Down
143 changes: 28 additions & 115 deletions clippy_lints/src/suspicious_trait_impl.rs
View file
@@ -1,5 +1,5 @@
use clippy_utils::diagnostics::span_lint;
use clippy_utils::{get_trait_def_id, paths, trait_ref_of_method};
use clippy_utils::{binop_traits, trait_ref_of_method, BINOP_TRAITS, OP_ASSIGN_TRAITS};
use if_chain::if_chain;
use rustc_hir as hir;
use rustc_hir::intravisit::{walk_expr, NestedVisitorMap, Visitor};
Expand Down Expand Up @@ -55,135 +55,48 @@ declare_lint_pass!(SuspiciousImpl => [SUSPICIOUS_ARITHMETIC_IMPL, SUSPICIOUS_OP_

impl<'tcx> LateLintPass<'tcx> for SuspiciousImpl {
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>) {
if let hir::ExprKind::Binary(binop, _, _) | hir::ExprKind::AssignOp(binop, ..) = expr.kind {
match binop.node {
hir::BinOpKind::Eq
| hir::BinOpKind::Lt
| hir::BinOpKind::Le
| hir::BinOpKind::Ne
| hir::BinOpKind::Ge
| hir::BinOpKind::Gt => return,
_ => {},
}
if_chain! {
if let hir::ExprKind::Binary(binop, _, _) | hir::ExprKind::AssignOp(binop, ..) = expr.kind;
if let Some((binop_trait_lang, op_assign_trait_lang)) = binop_traits(binop.node);
if let Ok(binop_trait_id) = cx.tcx.lang_items().require(binop_trait_lang);
if let Ok(op_assign_trait_id) = cx.tcx.lang_items().require(op_assign_trait_lang);

// Check for more than one binary operation in the implemented function
// Linting when multiple operations are involved can result in false positives
let parent_fn = cx.tcx.hir().get_parent_item(expr.hir_id);
if_chain! {
if let hir::Node::ImplItem(impl_item) = cx.tcx.hir().get(parent_fn);
if let hir::ImplItemKind::Fn(_, body_id) = impl_item.kind;
then {
let body = cx.tcx.hir().body(body_id);
let mut visitor = BinaryExprVisitor { nb_binops: 0 };
walk_expr(&mut visitor, &body.value);
if visitor.nb_binops > 1 {
return;
}
}
}

if let Some(impl_trait) = check_binop(
cx,
expr,
binop.node,
&[
"Add", "Sub", "Mul", "Div", "Rem", "BitAnd", "BitOr", "BitXor", "Shl", "Shr",
],
&[
hir::BinOpKind::Add,
hir::BinOpKind::Sub,
hir::BinOpKind::Mul,
hir::BinOpKind::Div,
hir::BinOpKind::Rem,
hir::BinOpKind::BitAnd,
hir::BinOpKind::BitOr,
hir::BinOpKind::BitXor,
hir::BinOpKind::Shl,
hir::BinOpKind::Shr,
],
) {
span_lint(
cx,
SUSPICIOUS_ARITHMETIC_IMPL,
binop.span,
&format!("suspicious use of binary operator in `{}` impl", impl_trait),
);
}

if let Some(impl_trait) = check_binop(
cx,
expr,
binop.node,
&[
"AddAssign",
"SubAssign",
"MulAssign",
"DivAssign",
"BitAndAssign",
"BitOrAssign",
"BitXorAssign",
"RemAssign",
"ShlAssign",
"ShrAssign",
],
&[
hir::BinOpKind::Add,
hir::BinOpKind::Sub,
hir::BinOpKind::Mul,
hir::BinOpKind::Div,
hir::BinOpKind::BitAnd,
hir::BinOpKind::BitOr,
hir::BinOpKind::BitXor,
hir::BinOpKind::Rem,
hir::BinOpKind::Shl,
hir::BinOpKind::Shr,
],
) {
if let hir::Node::ImplItem(impl_item) = cx.tcx.hir().get(parent_fn);
if let hir::ImplItemKind::Fn(_, body_id) = impl_item.kind;
let body = cx.tcx.hir().body(body_id);
let parent_fn = cx.tcx.hir().get_parent_item(expr.hir_id);
if let Some(trait_ref) = trait_ref_of_method(cx, parent_fn);
let trait_id = trait_ref.path.res.def_id();
if ![binop_trait_id, op_assign_trait_id].contains(&trait_id);
if let Some(&(_, lint)) = [
(&BINOP_TRAITS, SUSPICIOUS_ARITHMETIC_IMPL),
(&OP_ASSIGN_TRAITS, SUSPICIOUS_OP_ASSIGN_IMPL),
]
.iter()
.find(|&(ts, _)| ts.iter().any(|&t| Ok(trait_id) == cx.tcx.lang_items().require(t)));
if count_binops(&body.value) == 1;
then {
span_lint(
cx,
SUSPICIOUS_OP_ASSIGN_IMPL,
lint,
binop.span,
&format!("suspicious use of binary operator in `{}` impl", impl_trait),
&format!("suspicious use of `{}` in `{}` impl", binop.node.as_str(), cx.tcx.item_name(trait_id)),
);
}
}
}
}

fn check_binop(
cx: &LateContext<'_>,
expr: &hir::Expr<'_>,
binop: hir::BinOpKind,
traits: &[&'static str],
expected_ops: &[hir::BinOpKind],
) -> Option<&'static str> {
let mut trait_ids = vec![];
let [krate, module] = paths::OPS_MODULE;

for &t in traits {
let path = [krate, module, t];
if let Some(trait_id) = get_trait_def_id(cx, &path) {
trait_ids.push(trait_id);
} else {
return None;
}
}

// Get the actually implemented trait
let parent_fn = cx.tcx.hir().get_parent_item(expr.hir_id);

if_chain! {
if let Some(trait_ref) = trait_ref_of_method(cx, parent_fn);
if let Some(idx) = trait_ids.iter().position(|&tid| tid == trait_ref.path.res.def_id());
if binop != expected_ops[idx];
then{
return Some(traits[idx])
}
}

None
fn count_binops(expr: &hir::Expr<'_>) -> u32 {
let mut visitor = BinaryExprVisitor::default();
visitor.visit_expr(expr);
visitor.nb_binops
}

#[derive(Default)]
struct BinaryExprVisitor {
nb_binops: u32,
}
Expand Down
31 changes: 31 additions & 0 deletions clippy_utils/src/lib.rs
View file
Expand Up @@ -1710,3 +1710,34 @@ pub fn is_test_module_or_function(tcx: TyCtxt<'_>, item: &Item<'_>) -> bool {

matches!(item.kind, ItemKind::Mod(..)) && item.ident.name.as_str().contains("test")
}

macro_rules! op_utils {
($($name:ident $assign:ident)*) => {
/// Binary operation traits like `LangItem::Add`
pub static BINOP_TRAITS: &[LangItem] = &[$(LangItem::$name,)*];

/// Operator-Assign traits like `LangItem::AddAssign`
pub static OP_ASSIGN_TRAITS: &[LangItem] = &[$(LangItem::$assign,)*];

/// Converts `BinOpKind::Add` to `(LangItem::Add, LangItem::AddAssign)`, for example
pub fn binop_traits(kind: hir::BinOpKind) -> Option<(LangItem, LangItem)> {
match kind {
$(hir::BinOpKind::$name => Some((LangItem::$name, LangItem::$assign)),)*
_ => None,
}
}
};
}

op_utils! {
Add AddAssign
Sub SubAssign
Mul MulAssign
Div DivAssign
Rem RemAssign
BitXor BitXorAssign
BitAnd BitAndAssign
BitOr BitOrAssign
Shl ShlAssign
Shr ShrAssign
}
18 changes: 9 additions & 9 deletions tests/ui/suspicious_arithmetic_impl.stderr
View file
@@ -1,56 +1,56 @@
error: suspicious use of binary operator in `Add` impl
error: suspicious use of `-` in `Add` impl
--> $DIR/suspicious_arithmetic_impl.rs:13:20
|
LL | Foo(self.0 - other.0)
| ^
|
= note: `-D clippy::suspicious-arithmetic-impl` implied by `-D warnings`

error: suspicious use of binary operator in `AddAssign` impl
error: suspicious use of `-` in `AddAssign` impl
--> $DIR/suspicious_arithmetic_impl.rs:19:23
|
LL | *self = *self - other;
| ^
|
= note: `-D clippy::suspicious-op-assign-impl` implied by `-D warnings`

error: suspicious use of binary operator in `MulAssign` impl
error: suspicious use of `/` in `MulAssign` impl
--> $DIR/suspicious_arithmetic_impl.rs:32:16
|
LL | self.0 /= other.0;
| ^^

error: suspicious use of binary operator in `Rem` impl
error: suspicious use of `/` in `Rem` impl
--> $DIR/suspicious_arithmetic_impl.rs:70:20
|
LL | Foo(self.0 / other.0)
| ^

error: suspicious use of binary operator in `BitAnd` impl
error: suspicious use of `|` in `BitAnd` impl
--> $DIR/suspicious_arithmetic_impl.rs:78:20
|
LL | Foo(self.0 | other.0)
| ^

error: suspicious use of binary operator in `BitOr` impl
error: suspicious use of `^` in `BitOr` impl
--> $DIR/suspicious_arithmetic_impl.rs:86:20
|
LL | Foo(self.0 ^ other.0)
| ^

error: suspicious use of binary operator in `BitXor` impl
error: suspicious use of `&` in `BitXor` impl
--> $DIR/suspicious_arithmetic_impl.rs:94:20
|
LL | Foo(self.0 & other.0)
| ^

error: suspicious use of binary operator in `Shl` impl
error: suspicious use of `>>` in `Shl` impl
--> $DIR/suspicious_arithmetic_impl.rs:102:20
|
LL | Foo(self.0 >> other.0)
| ^^

error: suspicious use of binary operator in `Shr` impl
error: suspicious use of `<<` in `Shr` impl
--> $DIR/suspicious_arithmetic_impl.rs:110:20
|
LL | Foo(self.0 << other.0)
Expand Down

0 comments on commit 98c500c

Please sign in to comment.