Skip to content

Commit

Permalink
Auto merge of rust-lang#7474 - camsteffen:binop, r=Manishearth
Browse files Browse the repository at this point in the history 
Use lang items for BinOp lints

changelog: none
  • Loading branch information
@bors
bors committed Jul 18, 2021
2 parents e93df0b + 98c500c commit 46363df
Show file tree
Hide file tree
Showing 7 changed files with 98 additions and 216 deletions.
93 changes: 28 additions & 65 deletions clippy_lints/src/assign_ops.rs
View file
Original file line number Diff line number Diff line change
@@ -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::{higher, 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 Expand Up @@ -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)) {
let a = &sugg::Sugg::hir(cx, assignee, "..");
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(
expr.span,
&format!(
Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/eq_op.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ impl<'tcx> LateLintPass<'tcx> for EqOp {
if macro_with_not_op(&left.kind) || macro_with_not_op(&right.kind) {
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(
cx,
EQ_OP,
Expand Down
143 changes: 28 additions & 115 deletions clippy_lints/src/suspicious_trait_impl.rs
View file
Original file line number Diff line number Diff line change
@@ -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
25 changes: 0 additions & 25 deletions clippy_utils/src/higher.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,31 +11,6 @@ use rustc_hir::{BorrowKind, Expr, ExprKind, StmtKind, UnOp};
use rustc_lint::LateContext;
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`.
#[derive(Debug, Copy, Clone)]
pub struct Range<'a> {
Expand Down
31 changes: 31 additions & 0 deletions clippy_utils/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}

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 changes: 1 addition & 1 deletion clippy_utils/src/sugg.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ impl<'a> Sugg<'a> {
| hir::ExprKind::Err => Sugg::NonParen(snippet),
hir::ExprKind::Assign(..) => Sugg::BinOp(AssocOp::Assign, 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::Type(..) => Sugg::BinOp(AssocOp::Colon, snippet),
}
Expand Down
Loading

0 comments on commit 46363df

Please sign in to comment.