clippy_lints/src/assign_ops.rs

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