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