clippy_lints/src/assign_ops.rs

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