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