src/tools/clippy/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     /// // 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 SpanlessEq::new(cx).ignore_fn().eq_expr(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) && SpanlessEq::new(cx).ignore_fn().eq_expr(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 SpanlessEq::new(cx).ignore_fn().eq_expr(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 SpanlessEq::new(cx).ignore_fn().eq_expr(assignee, r)
 170                             && cx.typeck_results().expr_ty(assignee).is_primitive_ty()
 171                         {
 172                             match op.node {
 173                                 hir::BinOpKind::Add
 174                                 | hir::BinOpKind::Mul
 175                                 | hir::BinOpKind::And
 176                                 | hir::BinOpKind::Or
 177                                 | hir::BinOpKind::BitXor
 178                                 | hir::BinOpKind::BitAnd
 179                                 | hir::BinOpKind::BitOr => {
 180                                     lint(assignee, l);
 181                                 },
 182                                 _ => {},
 183                             }
 184                         }
 185                     }
 186                 }
 187             },
 188             _ => {},
 189         }
 190     }
 191 }
 192
 193 fn lint_misrefactored_assign_op(
 194     cx: &LateContext<'_>,
 195     expr: &hir::Expr<'_>,
 196     op: hir::BinOp,
 197     rhs: &hir::Expr<'_>,
 198     assignee: &hir::Expr<'_>,
 199     rhs_other: &hir::Expr<'_>,
 200 ) {
 201     span_lint_and_then(
 202         cx,
 203         MISREFACTORED_ASSIGN_OP,
 204         expr.span,
 205         "variable appears on both sides of an assignment operation",
 206         |diag| {
 207             if let (Some(snip_a), Some(snip_r)) = (snippet_opt(cx, assignee.span), snippet_opt(cx, rhs_other.span)) {
 208                 let a = &sugg::Sugg::hir(cx, assignee, "..");
 209                 let r = &sugg::Sugg::hir(cx, rhs, "..");
 210                 let long = format!("{} = {}", snip_a, sugg::make_binop(higher::binop(op.node), a, r));
 211                 diag.span_suggestion(
 212                     expr.span,
 213                     &format!(
 214                         "Did you mean `{} = {} {} {}` or `{}`? Consider replacing it with",
 215                         snip_a,
 216                         snip_a,
 217                         op.node.as_str(),
 218                         snip_r,
 219                         long
 220                     ),
 221                     format!("{} {}= {}", snip_a, op.node.as_str(), snip_r),
 222                     Applicability::MaybeIncorrect,
 223                 );
 224                 diag.span_suggestion(
 225                     expr.span,
 226                     "or",
 227                     long,
 228                     Applicability::MaybeIncorrect, // snippet
 229                 );
 230             }
 231         },
 232     );
 233 }
 234
 235 #[must_use]
 236 fn is_commutative(op: hir::BinOpKind) -> bool {
 237     use rustc_hir::BinOpKind::{
 238         Add, And, BitAnd, BitOr, BitXor, Div, Eq, Ge, Gt, Le, Lt, Mul, Ne, Or, Rem, Shl, Shr, Sub,
 239     };
 240     match op {
 241         Add | Mul | And | Or | BitXor | BitAnd | BitOr | Eq | Ne => true,
 242         Sub | Div | Rem | Shl | Shr | Lt | Le | Ge | Gt => false,
 243     }
 244 }
 245
 246 struct ExprVisitor<'a, 'tcx> {
 247     assignee: &'a hir::Expr<'a>,
 248     counter: u8,
 249     cx: &'a LateContext<'tcx>,
 250 }
 251
 252 impl<'a, 'tcx> Visitor<'tcx> for ExprVisitor<'a, 'tcx> {
 253     type Map = Map<'tcx>;
 254
 255     fn visit_expr(&mut self, expr: &'tcx hir::Expr<'_>) {
 256         if SpanlessEq::new(self.cx).ignore_fn().eq_expr(self.assignee, expr) {
 257             self.counter += 1;
 258         }
 259
 260         walk_expr(self, expr);
 261     }
 262     fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> {
 263         NestedVisitorMap::None
 264     }
 265 }