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