clippy_lints/src/assign_ops.rs

   1 use crate::utils::{eq_expr_value, get_trait_def_id, snippet_opt, span_lint_and_then, trait_ref_of_method};
   2 use crate::utils::{higher, sugg};
   3 use clippy_utils::ty::implements_trait;
   4 use if_chain::if_chain;
   5 use rustc_errors::Applicability;
   6 use rustc_hir as hir;
   7 use rustc_hir::intravisit::{walk_expr, NestedVisitorMap, Visitor};
   8 use rustc_lint::{LateContext, LateLintPass};
   9 use rustc_middle::hir::map::Map;
  10 use rustc_session::{declare_lint_pass, declare_tool_lint};
  11
  12 declare_clippy_lint! {
  13     /// **What it does:** Checks for `a = a op b` or `a = b commutative_op a`
  14     /// patterns.
  15     ///
  16     /// **Why is this bad?** These can be written as the shorter `a op= b`.
  17     ///
  18     /// **Known problems:** While forbidden by the spec, `OpAssign` traits may have
  19     /// implementations that differ from the regular `Op` impl.
  20     ///
  21     /// **Example:**
  22     /// ```rust
  23     /// let mut a = 5;
  24     /// let b = 0;
  25     /// // ...
  26     /// // Bad
  27     /// a = a + b;
  28     ///
  29     /// // Good
  30     /// a += b;
  31     /// ```
  32     pub ASSIGN_OP_PATTERN,
  33     style,
  34     "assigning the result of an operation on a variable to that same variable"
  35 }
  36
  37 declare_clippy_lint! {
  38     /// **What it does:** Checks for `a op= a op b` or `a op= b op a` patterns.
  39     ///
  40     /// **Why is this bad?** Most likely these are bugs where one meant to write `a
  41     /// op= b`.
  42     ///
  43     /// **Known problems:** Clippy cannot know for sure if `a op= a op b` should have
  44     /// been `a = a op a op b` or `a = a op b`/`a op= b`. Therefore, it suggests both.
  45     /// If `a op= a op b` is really the correct behaviour it should be
  46     /// written as `a = a op a op b` as it's less confusing.
  47     ///
  48     /// **Example:**
  49     /// ```rust
  50     /// let mut a = 5;
  51     /// let b = 2;
  52     /// // ...
  53     /// a += a + b;
  54     /// ```
  55     pub MISREFACTORED_ASSIGN_OP,
  56     complexity,
  57     "having a variable on both sides of an assign op"
  58 }
  59
  60 declare_lint_pass!(AssignOps => [ASSIGN_OP_PATTERN, MISREFACTORED_ASSIGN_OP]);
  61
  62 impl<'tcx> LateLintPass<'tcx> for AssignOps {
  63     #[allow(clippy::too_many_lines)]
  64     fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>) {
  65         match &expr.kind {
  66             hir::ExprKind::AssignOp(op, lhs, rhs) => {
  67                 if let hir::ExprKind::Binary(binop, l, r) = &rhs.kind {
  68                     if op.node != binop.node {
  69                         return;
  70                     }
  71                     // lhs op= l op r
  72                     if eq_expr_value(cx, lhs, l) {
  73                         lint_misrefactored_assign_op(cx, expr, *op, rhs, lhs, r);
  74                     }
  75                     // lhs op= l commutative_op r
  76                     if is_commutative(op.node) && eq_expr_value(cx, lhs, r) {
  77                         lint_misrefactored_assign_op(cx, expr, *op, rhs, lhs, l);
  78                     }
  79                 }
  80             },
  81             hir::ExprKind::Assign(assignee, e, _) => {
  82                 if let hir::ExprKind::Binary(op, l, r) = &e.kind {
  83                     let lint = |assignee: &hir::Expr<'_>, rhs: &hir::Expr<'_>| {
  84                         let ty = cx.typeck_results().expr_ty(assignee);
  85                         let rty = cx.typeck_results().expr_ty(rhs);
  86                         macro_rules! ops {
  87                             ($op:expr,
  88                              $cx:expr,
  89                              $ty:expr,
  90                              $rty:expr,
  91                              $($trait_name:ident),+) => {
  92                                 match $op {
  93                                     $(hir::BinOpKind::$trait_name => {
  94                                         let [krate, module] = crate::utils::paths::OPS_MODULE;
  95                                         let path: [&str; 3] = [krate, module, concat!(stringify!($trait_name), "Assign")];
  96                                         let trait_id = if let Some(trait_id) = get_trait_def_id($cx, &path) {
  97                                             trait_id
  98                                         } else {
  99                                             return; // useless if the trait doesn't exist
 100                                         };
 101                                         // check that we are not inside an `impl AssignOp` of this exact operation
 102                                         let parent_fn = cx.tcx.hir().get_parent_item(e.hir_id);
 103                                         if_chain! {
 104                                             if let Some(trait_ref) = trait_ref_of_method(cx, parent_fn);
 105                                             if trait_ref.path.res.def_id() == trait_id;
 106                                             then { return; }
 107                                         }
 108                                         implements_trait($cx, $ty, trait_id, &[$rty])
 109                                     },)*
 110                                     _ => false,
 111                                 }
 112                             }
 113                         }
 114                         if ops!(
 115                             op.node,
 116                             cx,
 117                             ty,
 118                             rty.into(),
 119                             Add,
 120                             Sub,
 121                             Mul,
 122                             Div,
 123                             Rem,
 124                             And,
 125                             Or,
 126                             BitAnd,
 127                             BitOr,
 128                             BitXor,
 129                             Shr,
 130                             Shl
 131                         ) {
 132                             span_lint_and_then(
 133                                 cx,
 134                                 ASSIGN_OP_PATTERN,
 135                                 expr.span,
 136                                 "manual implementation of an assign operation",
 137                                 |diag| {
 138                                     if let (Some(snip_a), Some(snip_r)) =
 139                                         (snippet_opt(cx, assignee.span), snippet_opt(cx, rhs.span))
 140                                     {
 141                                         diag.span_suggestion(
 142                                             expr.span,
 143                                             "replace it with",
 144                                             format!("{} {}= {}", snip_a, op.node.as_str(), snip_r),
 145                                             Applicability::MachineApplicable,
 146                                         );
 147                                     }
 148                                 },
 149                             );
 150                         }
 151                     };
 152
 153                     let mut visitor = ExprVisitor {
 154                         assignee,
 155                         counter: 0,
 156                         cx,
 157                     };
 158
 159                     walk_expr(&mut visitor, e);
 160
 161                     if visitor.counter == 1 {
 162                         // a = a op b
 163                         if eq_expr_value(cx, assignee, l) {
 164                             lint(assignee, r);
 165                         }
 166                         // a = b commutative_op a
 167                         // Limited to primitive type as these ops are know to be commutative
 168                         if eq_expr_value(cx, assignee, r) && cx.typeck_results().expr_ty(assignee).is_primitive_ty() {
 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         |diag| {
 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                 diag.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                 diag.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<'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 eq_expr_value(self.cx, 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 }