clippy_lints/src/assign_ops.rs

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