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