Skip to content

Use lang items for BinOp lints #7474

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jul 18, 2021
Merged

Use lang items for BinOp lints #7474

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
efbf7ca
Use From to convert BinOpKind
camsteffen Jun 28, 2021
98c500c
Factor BinOp utils
camsteffen Jul 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
93 changes: 28 additions & 65 deletions clippy_lints/src/assign_ops.rs
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
Original file line number Diff line number Diff line change
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
}
2 changes: 1 addition & 1 deletion clippy_utils/src/sugg.rs
View file
Open in desktop
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