clippy_lints/src/assign_ops.rs

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