clippy_lints/src/assign_ops.rs

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