clippy_lints/src/assign_ops.rs

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