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