mikros/rust
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Auto merge of #7474 - camsteffen:binop, r=Manishearth

Browse Source 
Use lang items for BinOp lints

changelog: none
This commit is contained in:
bors 2021-07-18 15:52:49 +00:00
commit 46363df926
7 changed files with 98 additions and 216 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

93
clippy_lints/src/assign_ops.rs
View File

@ -1,8 +1,8 @@
use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::diagnostics::span_lint_and_then;
use clippy_utils::source::snippet_opt; use clippy_utils::source::snippet_opt;
use clippy_utils::ty::implements_trait; use clippy_utils::ty::implements_trait;
use clippy_utils::{eq_expr_value, get_trait_def_id, trait_ref_of_method}; use clippy_utils::{binop_traits, sugg};
use clippy_utils::{higher, paths, sugg}; use clippy_utils::{eq_expr_value, trait_ref_of_method};
use if_chain::if_chain; use if_chain::if_chain;
use rustc_errors::Applicability; use rustc_errors::Applicability;
use rustc_hir as hir; use rustc_hir as hir;
@ -85,71 +85,34 @@ fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>) {
let lint = |assignee: &hir::Expr<'_>, rhs: &hir::Expr<'_>| { let lint = |assignee: &hir::Expr<'_>, rhs: &hir::Expr<'_>| {
let ty = cx.typeck_results().expr_ty(assignee); let ty = cx.typeck_results().expr_ty(assignee);
let rty = cx.typeck_results().expr_ty(rhs); let rty = cx.typeck_results().expr_ty(rhs);
macro_rules! ops { if_chain! {
($op:expr, if let Some((_, lang_item)) = binop_traits(op.node);
$cx:expr, if let Ok(trait_id) = cx.tcx.lang_items().require(lang_item);
$ty:expr, let parent_fn = cx.tcx.hir().get_parent_item(e.hir_id);
$rty:expr, if trait_ref_of_method(cx, parent_fn)
$($trait_name:ident),+) => { .map_or(true, |t| t.path.res.def_id() != trait_id);
match $op { if implements_trait(cx, ty, trait_id, &[rty.into()]);
$(hir::BinOpKind::$trait_name => { then {
let [krate, module] = paths::OPS_MODULE; span_lint_and_then(
let path: [&str; 3] = [krate, module, concat!(stringify!($trait_name), "Assign")]; cx,
let trait_id = if let Some(trait_id) = get_trait_def_id($cx, &path) { ASSIGN_OP_PATTERN,
trait_id expr.span,
} else { "manual implementation of an assign operation",
return; // useless if the trait doesn't exist |diag| {
}; if let (Some(snip_a), Some(snip_r)) =
// check that we are not inside an `impl AssignOp` of this exact operation (snippet_opt(cx, assignee.span), snippet_opt(cx, rhs.span))
let parent_fn = cx.tcx.hir().get_parent_item(e.hir_id); {
if_chain! { diag.span_suggestion(
if let Some(trait_ref) = trait_ref_of_method(cx, parent_fn); expr.span,
if trait_ref.path.res.def_id() == trait_id; "replace it with",
then { return; } 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 { let mut visitor = ExprVisitor {
@ -206,7 +169,7 @@ fn lint_misrefactored_assign_op(
if let (Some(snip_a), Some(snip_r)) = (snippet_opt(cx, assignee.span), snippet_opt(cx, rhs_other.span)) { if let (Some(snip_a), Some(snip_r)) = (snippet_opt(cx, assignee.span), snippet_opt(cx, rhs_other.span)) {
let a = &sugg::Sugg::hir(cx, assignee, ".."); let a = &sugg::Sugg::hir(cx, assignee, "..");
let r = &sugg::Sugg::hir(cx, rhs, ".."); let r = &sugg::Sugg::hir(cx, rhs, "..");
let long = format!("{} = {}", snip_a, sugg::make_binop(higher::binop(op.node), a, r)); let long = format!("{} = {}", snip_a, sugg::make_binop(op.node.into(), a, r));
diag.span_suggestion( diag.span_suggestion(
expr.span, expr.span,
&format!( &format!(

2
clippy_lints/src/eq_op.rs
View File

@ -102,7 +102,7 @@ fn check_expr(&mut self, cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) {
if macro_with_not_op(&left.kind) || macro_with_not_op(&right.kind) { if macro_with_not_op(&left.kind) || macro_with_not_op(&right.kind) {
return; return;
} }
if is_useless_with_eq_exprs(higher::binop(op.node)) && eq_expr_value(cx, left, right) { if is_useless_with_eq_exprs(op.node.into()) && eq_expr_value(cx, left, right) {
span_lint( span_lint(
cx, cx,
EQ_OP, EQ_OP,

143
clippy_lints/src/suspicious_trait_impl.rs
View File

@ -1,5 +1,5 @@
use clippy_utils::diagnostics::span_lint; 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 if_chain::if_chain;
use rustc_hir as hir; use rustc_hir as hir;
use rustc_hir::intravisit::{walk_expr, NestedVisitorMap, Visitor}; use rustc_hir::intravisit::{walk_expr, NestedVisitorMap, Visitor};
@ -55,135 +55,48 @@
impl<'tcx> LateLintPass<'tcx> for SuspiciousImpl { impl<'tcx> LateLintPass<'tcx> for SuspiciousImpl {
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>) { fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>) {
if let hir::ExprKind::Binary(binop, _, _) | hir::ExprKind::AssignOp(binop, ..) = expr.kind { if_chain! {
match binop.node { if let hir::ExprKind::Binary(binop, _, _) | hir::ExprKind::AssignOp(binop, ..) = expr.kind;
hir::BinOpKind::Eq if let Some((binop_trait_lang, op_assign_trait_lang)) = binop_traits(binop.node);
| hir::BinOpKind::Lt if let Ok(binop_trait_id) = cx.tcx.lang_items().require(binop_trait_lang);
| hir::BinOpKind::Le if let Ok(op_assign_trait_id) = cx.tcx.lang_items().require(op_assign_trait_lang);
| hir::BinOpKind::Ne
| hir::BinOpKind::Ge
| hir::BinOpKind::Gt => return,
_ => {},
}
// Check for more than one binary operation in the implemented function // Check for more than one binary operation in the implemented function
// Linting when multiple operations are involved can result in false positives // Linting when multiple operations are involved can result in false positives
let parent_fn = cx.tcx.hir().get_parent_item(expr.hir_id); 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::Node::ImplItem(impl_item) = cx.tcx.hir().get(parent_fn); if let hir::ImplItemKind::Fn(_, body_id) = impl_item.kind;
if let hir::ImplItemKind::Fn(_, body_id) = impl_item.kind; let body = cx.tcx.hir().body(body_id);
then { let parent_fn = cx.tcx.hir().get_parent_item(expr.hir_id);
let body = cx.tcx.hir().body(body_id); if let Some(trait_ref) = trait_ref_of_method(cx, parent_fn);
let mut visitor = BinaryExprVisitor { nb_binops: 0 }; let trait_id = trait_ref.path.res.def_id();
walk_expr(&mut visitor, &body.value); if ![binop_trait_id, op_assign_trait_id].contains(&trait_id);
if visitor.nb_binops > 1 { if let Some(&(_, lint)) = [
return; (&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 let Some(impl_trait) = check_binop( if count_binops(&body.value) == 1;
cx, then {
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( span_lint(
cx, cx,
SUSPICIOUS_ARITHMETIC_IMPL, lint,
binop.span, 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)),
);
}
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,
],
) {
span_lint(
cx,
SUSPICIOUS_OP_ASSIGN_IMPL,
binop.span,
&format!("suspicious use of binary operator in `{}` impl", impl_trait),
); );
} }
} }
} }
} }
fn check_binop( fn count_binops(expr: &hir::Expr<'_>) -> u32 {
cx: &LateContext<'_>, let mut visitor = BinaryExprVisitor::default();
expr: &hir::Expr<'_>, visitor.visit_expr(expr);
binop: hir::BinOpKind, visitor.nb_binops
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
} }
#[derive(Default)]
struct BinaryExprVisitor { struct BinaryExprVisitor {
nb_binops: u32, nb_binops: u32,
} }

25
clippy_utils/src/higher.rs
View File

@ -11,31 +11,6 @@
use rustc_lint::LateContext; use rustc_lint::LateContext;
use rustc_span::{sym, ExpnKind, Span, Symbol}; use rustc_span::{sym, ExpnKind, Span, Symbol};
/// Converts a hir binary operator to the corresponding `ast` type.
#[must_use]
pub fn binop(op: hir::BinOpKind) -> ast::BinOpKind {
match op {
hir::BinOpKind::Eq => ast::BinOpKind::Eq,
hir::BinOpKind::Ge => ast::BinOpKind::Ge,
hir::BinOpKind::Gt => ast::BinOpKind::Gt,
hir::BinOpKind::Le => ast::BinOpKind::Le,
hir::BinOpKind::Lt => ast::BinOpKind::Lt,
hir::BinOpKind::Ne => ast::BinOpKind::Ne,
hir::BinOpKind::Or => ast::BinOpKind::Or,
hir::BinOpKind::Add => ast::BinOpKind::Add,
hir::BinOpKind::And => ast::BinOpKind::And,
hir::BinOpKind::BitAnd => ast::BinOpKind::BitAnd,
hir::BinOpKind::BitOr => ast::BinOpKind::BitOr,
hir::BinOpKind::BitXor => ast::BinOpKind::BitXor,
hir::BinOpKind::Div => ast::BinOpKind::Div,
hir::BinOpKind::Mul => ast::BinOpKind::Mul,
hir::BinOpKind::Rem => ast::BinOpKind::Rem,
hir::BinOpKind::Shl => ast::BinOpKind::Shl,
hir::BinOpKind::Shr => ast::BinOpKind::Shr,
hir::BinOpKind::Sub => ast::BinOpKind::Sub,
}
}
/// Represent a range akin to `ast::ExprKind::Range`. /// Represent a range akin to `ast::ExprKind::Range`.
#[derive(Debug, Copy, Clone)] #[derive(Debug, Copy, Clone)]
pub struct Range<'a> { pub struct Range<'a> {

31
clippy_utils/src/lib.rs
View File

@ -1706,3 +1706,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") 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
}

2
clippy_utils/src/sugg.rs
View File

@ -154,7 +154,7 @@ fn hir_from_snippet(expr: &hir::Expr<'_>, snippet: Cow<'a, str>) -> Self {
| hir::ExprKind::Err => Sugg::NonParen(snippet), | hir::ExprKind::Err => Sugg::NonParen(snippet),
hir::ExprKind::Assign(..) => Sugg::BinOp(AssocOp::Assign, snippet), hir::ExprKind::Assign(..) => Sugg::BinOp(AssocOp::Assign, snippet),
hir::ExprKind::AssignOp(op, ..) => Sugg::BinOp(hirbinop2assignop(op), snippet), hir::ExprKind::AssignOp(op, ..) => Sugg::BinOp(hirbinop2assignop(op), snippet),
hir::ExprKind::Binary(op, ..) => Sugg::BinOp(AssocOp::from_ast_binop(higher::binop(op.node)), snippet), hir::ExprKind::Binary(op, ..) => Sugg::BinOp(AssocOp::from_ast_binop(op.node.into()), snippet),
hir::ExprKind::Cast(..) => Sugg::BinOp(AssocOp::As, snippet), hir::ExprKind::Cast(..) => Sugg::BinOp(AssocOp::As, snippet),
hir::ExprKind::Type(..) => Sugg::BinOp(AssocOp::Colon, snippet), hir::ExprKind::Type(..) => Sugg::BinOp(AssocOp::Colon, snippet),
} }

18
tests/ui/suspicious_arithmetic_impl.stderr
View File

@ -1,4 +1,4 @@
error: suspicious use of binary operator in `Add` impl error: suspicious use of `-` in `Add` impl
--> $DIR/suspicious_arithmetic_impl.rs:13:20 --> $DIR/suspicious_arithmetic_impl.rs:13:20
| |
LL | Foo(self.0 - other.0) LL | Foo(self.0 - other.0)
@ -6,7 +6,7 @@ LL | Foo(self.0 - other.0)
| |
= note: `-D clippy::suspicious-arithmetic-impl` implied by `-D warnings` = 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 --> $DIR/suspicious_arithmetic_impl.rs:19:23
| |
LL | *self = *self - other; LL | *self = *self - other;
@ -14,43 +14,43 @@ LL | *self = *self - other;
| |
= note: `-D clippy::suspicious-op-assign-impl` implied by `-D warnings` = 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 --> $DIR/suspicious_arithmetic_impl.rs:32:16
| |
LL | self.0 /= other.0; 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 --> $DIR/suspicious_arithmetic_impl.rs:70:20
| |
LL | Foo(self.0 / other.0) 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 --> $DIR/suspicious_arithmetic_impl.rs:78:20
| |
LL | Foo(self.0 | other.0) 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 --> $DIR/suspicious_arithmetic_impl.rs:86:20
| |
LL | Foo(self.0 ^ other.0) 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 --> $DIR/suspicious_arithmetic_impl.rs:94:20
| |
LL | Foo(self.0 & other.0) 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 --> $DIR/suspicious_arithmetic_impl.rs:102:20
| |
LL | Foo(self.0 >> other.0) 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 --> $DIR/suspicious_arithmetic_impl.rs:110:20
| |
LL | Foo(self.0 << other.0) LL | Foo(self.0 << other.0)