clippy_lints/src/assign_ops.rs

   1 use crate::utils::{
   2     get_trait_def_id, implements_trait, snippet_opt, span_lint_and_then, trait_ref_of_method, SpanlessEq,
   3 };
   4 use crate::utils::{higher, sugg};
   5 use if_chain::if_chain;
   6 use rustc::hir;
   7 use rustc::hir::intravisit::{walk_expr, NestedVisitorMap, Visitor};
   8 use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass};
   9 use rustc::{declare_tool_lint, lint_array};
  10 use rustc_errors::Applicability;
  11
  12 declare_clippy_lint! {
  13     /// **What it does:** Checks for `a = a op b` or `a = b commutative_op a`
  14     /// patterns.
  15     ///
  16     /// **Why is this bad?** These can be written as the shorter `a op= b`.
  17     ///
  18     /// **Known problems:** While forbidden by the spec, `OpAssign` traits may have
  19     /// implementations that differ from the regular `Op` impl.
  20     ///
  21     /// **Example:**
  22     /// ```ignore
  23     /// let mut a = 5;
  24     /// ...
  25     /// a = a + b;
  26     /// ```
  27     pub ASSIGN_OP_PATTERN,
  28     style,
  29     "assigning the result of an operation on a variable to that same variable"
  30 }
  31
  32 declare_clippy_lint! {
  33     /// **What it does:** Checks for `a op= a op b` or `a op= b op a` patterns.
  34     ///
  35     /// **Why is this bad?** Most likely these are bugs where one meant to write `a
  36     /// op= b`.
  37     ///
  38     /// **Known problems:** Clippy cannot know for sure if `a op= a op b` should have
  39     /// been `a = a op a op b` or `a = a op b`/`a op= b`. Therefore it suggests both.
  40     /// If `a op= a op b` is really the correct behaviour it should be
  41     /// written as `a = a op a op b` as it's less confusing.
  42     ///
  43     /// **Example:**
  44     /// ```ignore
  45     /// let mut a = 5;
  46     /// ...
  47     /// a += a + b;
  48     /// ```
  49     pub MISREFACTORED_ASSIGN_OP,
  50     complexity,
  51     "having a variable on both sides of an assign op"
  52 }
  53
  54 #[derive(Copy, Clone, Default)]
  55 pub struct AssignOps;
  56
  57 impl LintPass for AssignOps {
  58     fn get_lints(&self) -> LintArray {
  59         lint_array!(ASSIGN_OP_PATTERN, MISREFACTORED_ASSIGN_OP)
  60     }
  61
  62     fn name(&self) -> &'static str {
  63         "AssignOps"
  64     }
  65 }
  66
  67 impl<'a, 'tcx> LateLintPass<'a, 'tcx> for AssignOps {
  68     #[allow(clippy::too_many_lines)]
  69     fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx hir::Expr) {
  70         match &expr.node {
  71             hir::ExprKind::AssignOp(op, lhs, rhs) => {
  72                 if let hir::ExprKind::Binary(binop, l, r) = &rhs.node {
  73                     if op.node != binop.node {
  74                         return;
  75                     }
  76                     // lhs op= l op r
  77                     if SpanlessEq::new(cx).ignore_fn().eq_expr(lhs, l) {
  78                         lint_misrefactored_assign_op(cx, expr, *op, rhs, lhs, r);
  79                     }
  80                     // lhs op= l commutative_op r
  81                     if is_commutative(op.node) && SpanlessEq::new(cx).ignore_fn().eq_expr(lhs, r) {
  82                         lint_misrefactored_assign_op(cx, expr, *op, rhs, lhs, l);
  83                     }
  84                 }
  85             },
  86             hir::ExprKind::Assign(assignee, e) => {
  87                 if let hir::ExprKind::Binary(op, l, r) = &e.node {
  88                     #[allow(clippy::cognitive_complexity)]
  89                     let lint = |assignee: &hir::Expr, rhs: &hir::Expr| {
  90                         let ty = cx.tables.expr_ty(assignee);
  91                         let rty = cx.tables.expr_ty(rhs);
  92                         macro_rules! ops {
  93                             ($op:expr,
  94                              $cx:expr,
  95                              $ty:expr,
  96                              $rty:expr,
  97                              $($trait_name:ident),+) => {
  98                                 match $op {
  99                                     $(hir::BinOpKind::$trait_name => {
 100                                         let [krate, module] = crate::utils::paths::OPS_MODULE;
 101                                         let path = [krate, module, concat!(stringify!($trait_name), "Assign")];
 102                                         let trait_id = if let Some(trait_id) = get_trait_def_id($cx, &path) {
 103                                             trait_id
 104                                         } else {
 105                                             return; // useless if the trait doesn't exist
 106                                         };
 107                                         // check that we are not inside an `impl AssignOp` of this exact operation
 108                                         let parent_fn = cx.tcx.hir().get_parent_item(e.hir_id);
 109                                         if_chain! {
 110                                             if let Some(trait_ref) = trait_ref_of_method(cx, parent_fn);
 111                                             if trait_ref.path.def.def_id() == trait_id;
 112                                             then { return; }
 113                                         }
 114                                         implements_trait($cx, $ty, trait_id, &[$rty])
 115                                     },)*
 116                                     _ => false,
 117                                 }
 118                             }
 119                         }
 120                         if ops!(
 121                             op.node,
 122                             cx,
 123                             ty,
 124                             rty.into(),
 125                             Add,
 126                             Sub,
 127                             Mul,
 128                             Div,
 129                             Rem,
 130                             And,
 131                             Or,
 132                             BitAnd,
 133                             BitOr,
 134                             BitXor,
 135                             Shr,
 136                             Shl
 137                         ) {
 138                             span_lint_and_then(
 139                                 cx,
 140                                 ASSIGN_OP_PATTERN,
 141                                 expr.span,
 142                                 "manual implementation of an assign operation",
 143                                 |db| {
 144                                     if let (Some(snip_a), Some(snip_r)) =
 145                                         (snippet_opt(cx, assignee.span), snippet_opt(cx, rhs.span))
 146                                     {
 147                                         db.span_suggestion(
 148                                             expr.span,
 149                                             "replace it with",
 150                                             format!("{} {}= {}", snip_a, op.node.as_str(), snip_r),
 151                                             Applicability::MachineApplicable,
 152                                         );
 153                                     }
 154                                 },
 155                             );
 156                         }
 157                     };
 158
 159                     let mut visitor = ExprVisitor {
 160                         assignee,
 161                         counter: 0,
 162                         cx,
 163                     };
 164
 165                     walk_expr(&mut visitor, e);
 166
 167                     if visitor.counter == 1 {
 168                         // a = a op b
 169                         if SpanlessEq::new(cx).ignore_fn().eq_expr(assignee, l) {
 170                             lint(assignee, r);
 171                         }
 172                         // a = b commutative_op a
 173                         // Limited to primitive type as these ops are know to be commutative
 174                         if SpanlessEq::new(cx).ignore_fn().eq_expr(assignee, r)
 175                             && cx.tables.expr_ty(assignee).is_primitive_ty()
 176                         {
 177                             match op.node {
 178                                 hir::BinOpKind::Add
 179                                 | hir::BinOpKind::Mul
 180                                 | hir::BinOpKind::And
 181                                 | hir::BinOpKind::Or
 182                                 | hir::BinOpKind::BitXor
 183                                 | hir::BinOpKind::BitAnd
 184                                 | hir::BinOpKind::BitOr => {
 185                                     lint(assignee, l);
 186                                 },
 187                                 _ => {},
 188                             }
 189                         }
 190                     }
 191                 }
 192             },
 193             _ => {},
 194         }
 195     }
 196 }
 197
 198 fn lint_misrefactored_assign_op(
 199     cx: &LateContext<'_, '_>,
 200     expr: &hir::Expr,
 201     op: hir::BinOp,
 202     rhs: &hir::Expr,
 203     assignee: &hir::Expr,
 204     rhs_other: &hir::Expr,
 205 ) {
 206     span_lint_and_then(
 207         cx,
 208         MISREFACTORED_ASSIGN_OP,
 209         expr.span,
 210         "variable appears on both sides of an assignment operation",
 211         |db| {
 212             if let (Some(snip_a), Some(snip_r)) = (snippet_opt(cx, assignee.span), snippet_opt(cx, rhs_other.span)) {
 213                 let a = &sugg::Sugg::hir(cx, assignee, "..");
 214                 let r = &sugg::Sugg::hir(cx, rhs, "..");
 215                 let long = format!("{} = {}", snip_a, sugg::make_binop(higher::binop(op.node), a, r));
 216                 db.span_suggestion(
 217                     expr.span,
 218                     &format!(
 219                         "Did you mean {} = {} {} {} or {}? Consider replacing it with",
 220                         snip_a,
 221                         snip_a,
 222                         op.node.as_str(),
 223                         snip_r,
 224                         long
 225                     ),
 226                     format!("{} {}= {}", snip_a, op.node.as_str(), snip_r),
 227                     Applicability::MachineApplicable,
 228                 );
 229                 db.span_suggestion(
 230                     expr.span,
 231                     "or",
 232                     long,
 233                     Applicability::MachineApplicable, // snippet
 234                 );
 235             }
 236         },
 237     );
 238 }
 239
 240 fn is_commutative(op: hir::BinOpKind) -> bool {
 241     use rustc::hir::BinOpKind::*;
 242     match op {
 243         Add | Mul | And | Or | BitXor | BitAnd | BitOr | Eq | Ne => true,
 244         Sub | Div | Rem | Shl | Shr | Lt | Le | Ge | Gt => false,
 245     }
 246 }
 247
 248 struct ExprVisitor<'a, 'tcx: 'a> {
 249     assignee: &'a hir::Expr,
 250     counter: u8,
 251     cx: &'a LateContext<'a, 'tcx>,
 252 }
 253
 254 impl<'a, 'tcx: 'a> Visitor<'tcx> for ExprVisitor<'a, 'tcx> {
 255     fn visit_expr(&mut self, expr: &'tcx hir::Expr) {
 256         if SpanlessEq::new(self.cx).ignore_fn().eq_expr(self.assignee, expr) {
 257             self.counter += 1;
 258         }
 259
 260         walk_expr(self, expr);
 261     }
 262     fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'tcx> {
 263         NestedVisitorMap::None
 264     }
 265 }