clippy_lints/src/assign_ops.rs

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