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     #[allow(clippy::too_many_lines)]
  68     fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx hir::Expr) {
  69         match &expr.node {
  70             hir::ExprKind::AssignOp(op, lhs, rhs) => {
  71                 if let hir::ExprKind::Binary(binop, l, r) = &rhs.node {
  72                     if op.node == binop.node {
  73                         let lint = |assignee: &hir::Expr, rhs_other: &hir::Expr| {
  74                             span_lint_and_then(
  75                                 cx,
  76                                 MISREFACTORED_ASSIGN_OP,
  77                                 expr.span,
  78                                 "variable appears on both sides of an assignment operation",
  79                                 |db| {
  80                                     if let (Some(snip_a), Some(snip_r)) =
  81                                         (snippet_opt(cx, assignee.span), snippet_opt(cx, rhs_other.span))
  82                                     {
  83                                         let a = &sugg::Sugg::hir(cx, assignee, "..");
  84                                         let r = &sugg::Sugg::hir(cx, rhs, "..");
  85                                         let long =
  86                                             format!("{} = {}", snip_a, sugg::make_binop(higher::binop(op.node), a, r));
  87                                         db.span_suggestion(
  88                                             expr.span,
  89                                             &format!(
  90                                                 "Did you mean {} = {} {} {} or {}? Consider replacing it with",
  91                                                 snip_a,
  92                                                 snip_a,
  93                                                 op.node.as_str(),
  94                                                 snip_r,
  95                                                 long
  96                                             ),
  97                                             format!("{} {}= {}", snip_a, op.node.as_str(), snip_r),
  98                                             Applicability::MachineApplicable,
  99                                         );
 100                                         db.span_suggestion(
 101                                             expr.span,
 102                                             "or",
 103                                             long,
 104                                             Applicability::MachineApplicable, // snippet
 105                                         );
 106                                     }
 107                                 },
 108                             );
 109                         };
 110                         // lhs op= l op r
 111                         if SpanlessEq::new(cx).ignore_fn().eq_expr(lhs, l) {
 112                             lint(lhs, r);
 113                         }
 114                         // lhs op= l commutative_op r
 115                         if is_commutative(op.node) && SpanlessEq::new(cx).ignore_fn().eq_expr(lhs, r) {
 116                             lint(lhs, l);
 117                         }
 118                     }
 119                 }
 120             },
 121             hir::ExprKind::Assign(assignee, e) => {
 122                 if let hir::ExprKind::Binary(op, l, r) = &e.node {
 123                     #[allow(clippy::cyclomatic_complexity)]
 124                     let lint = |assignee: &hir::Expr, rhs: &hir::Expr| {
 125                         let ty = cx.tables.expr_ty(assignee);
 126                         let rty = cx.tables.expr_ty(rhs);
 127                         macro_rules! ops {
 128                             ($op:expr,
 129                              $cx:expr,
 130                              $ty:expr,
 131                              $rty:expr,
 132                              $($trait_name:ident),+) => {
 133                                 match $op {
 134                                     $(hir::BinOpKind::$trait_name => {
 135                                         let [krate, module] = crate::utils::paths::OPS_MODULE;
 136                                         let path = [krate, module, concat!(stringify!($trait_name), "Assign")];
 137                                         let trait_id = if let Some(trait_id) = get_trait_def_id($cx, &path) {
 138                                             trait_id
 139                                         } else {
 140                                             return; // useless if the trait doesn't exist
 141                                         };
 142                                         // check that we are not inside an `impl AssignOp` of this exact operation
 143                                         let parent_fn = cx.tcx.hir().get_parent(e.id);
 144                                         let parent_impl = cx.tcx.hir().get_parent(parent_fn);
 145                                         // the crate node is the only one that is not in the map
 146                                         if_chain! {
 147                                             if parent_impl != ast::CRATE_NODE_ID;
 148                                             if let hir::Node::Item(item) = cx.tcx.hir().get(parent_impl);
 149                                             if let hir::ItemKind::Impl(_, _, _, _, Some(trait_ref), _, _) =
 150                                                 &item.node;
 151                                             if trait_ref.path.def.def_id() == trait_id;
 152                                             then { return; }
 153                                         }
 154                                         implements_trait($cx, $ty, trait_id, &[$rty])
 155                                     },)*
 156                                     _ => false,
 157                                 }
 158                             }
 159                         }
 160                         if ops!(
 161                             op.node,
 162                             cx,
 163                             ty,
 164                             rty.into(),
 165                             Add,
 166                             Sub,
 167                             Mul,
 168                             Div,
 169                             Rem,
 170                             And,
 171                             Or,
 172                             BitAnd,
 173                             BitOr,
 174                             BitXor,
 175                             Shr,
 176                             Shl
 177                         ) {
 178                             span_lint_and_then(
 179                                 cx,
 180                                 ASSIGN_OP_PATTERN,
 181                                 expr.span,
 182                                 "manual implementation of an assign operation",
 183                                 |db| {
 184                                     if let (Some(snip_a), Some(snip_r)) =
 185                                         (snippet_opt(cx, assignee.span), snippet_opt(cx, rhs.span))
 186                                     {
 187                                         db.span_suggestion(
 188                                             expr.span,
 189                                             "replace it with",
 190                                             format!("{} {}= {}", snip_a, op.node.as_str(), snip_r),
 191                                             Applicability::MachineApplicable,
 192                                         );
 193                                     }
 194                                 },
 195                             );
 196                         }
 197                     };
 198
 199                     let mut visitor = ExprVisitor {
 200                         assignee,
 201                         counter: 0,
 202                         cx,
 203                     };
 204
 205                     walk_expr(&mut visitor, e);
 206
 207                     if visitor.counter == 1 {
 208                         // a = a op b
 209                         if SpanlessEq::new(cx).ignore_fn().eq_expr(assignee, l) {
 210                             lint(assignee, r);
 211                         }
 212                         // a = b commutative_op a
 213                         // Limited to primitive type as these ops are know to be commutative
 214                         if SpanlessEq::new(cx).ignore_fn().eq_expr(assignee, r)
 215                             && cx.tables.expr_ty(assignee).is_primitive_ty()
 216                         {
 217                             match op.node {
 218                                 hir::BinOpKind::Add
 219                                 | hir::BinOpKind::Mul
 220                                 | hir::BinOpKind::And
 221                                 | hir::BinOpKind::Or
 222                                 | hir::BinOpKind::BitXor
 223                                 | hir::BinOpKind::BitAnd
 224                                 | hir::BinOpKind::BitOr => {
 225                                     lint(assignee, l);
 226                                 },
 227                                 _ => {},
 228                             }
 229                         }
 230                     }
 231                 }
 232             },
 233             _ => {},
 234         }
 235     }
 236 }
 237
 238 fn is_commutative(op: hir::BinOpKind) -> bool {
 239     use rustc::hir::BinOpKind::*;
 240     match op {
 241         Add | Mul | And | Or | BitXor | BitAnd | BitOr | Eq | Ne => true,
 242         Sub | Div | Rem | Shl | Shr | Lt | Le | Ge | Gt => false,
 243     }
 244 }
 245
 246 struct ExprVisitor<'a, 'tcx: 'a> {
 247     assignee: &'a hir::Expr,
 248     counter: u8,
 249     cx: &'a LateContext<'a, 'tcx>,
 250 }
 251
 252 impl<'a, 'tcx: 'a> Visitor<'tcx> for ExprVisitor<'a, 'tcx> {
 253     fn visit_expr(&mut self, expr: &'tcx hir::Expr) {
 254         if SpanlessEq::new(self.cx).ignore_fn().eq_expr(self.assignee, expr) {
 255             self.counter += 1;
 256         }
 257
 258         walk_expr(self, expr);
 259     }
 260     fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'tcx> {
 261         NestedVisitorMap::None
 262     }
 263 }