Add parenthesis when needed for suggestion in ref_mut_iter_method_chain

Author: Jarcho

clippy_lints/src/methods/ref_mut_iter_method_chain.rs

@@ -1,6 +1,5 @@
 use clippy_utils::diagnostics::span_lint_and_sugg;
-use clippy_utils::in_macro;
-use clippy_utils::source::snippet_with_context;
+use clippy_utils::source::{snippet_expr, TargetPrecedence};
 use clippy_utils::ty::implements_trait;
 use if_chain::if_chain;
 use rustc_errors::Applicability;
@@ -13,14 +12,14 @@ use super::REF_MUT_ITER_METHOD_CHAIN;
 pub(crate) fn check(cx: &LateContext<'_>, self_arg: &Expr<'_>) {
     if_chain! {
         if let ExprKind::AddrOf(BorrowKind::Ref, Mutability::Mut, base_expr) = self_arg.kind;
-        if !in_macro(self_arg.span);
-        if let Some(&iter_trait) = cx.tcx.all_diagnostic_items(()).get(&sym::Iterator);
+        if !self_arg.span.from_expansion();
+        if let Some(&iter_trait) = cx.tcx.all_diagnostic_items(()).name_to_id.get(&sym::Iterator);
         if implements_trait(cx, cx.typeck_results().expr_ty(base_expr).peel_refs(), iter_trait, &[]);
         then {
-            let snip_span = match base_expr.kind {
-                ExprKind::Unary(UnOp::Deref, e) if cx.typeck_results().expr_ty(e).is_ref() && !in_macro(base_expr.span)
-                    => e.span,
-                _ => base_expr.span,
+            let snip_expr = match base_expr.kind {
+                ExprKind::Unary(UnOp::Deref, e) if cx.typeck_results().expr_ty(e).is_ref() && !base_expr.span.from_expansion()
+                    => e,
+                _ => base_expr,
             };
             let mut app = Applicability::MachineApplicable;
             span_lint_and_sugg(
@@ -31,7 +30,7 @@ pub(crate) fn check(cx: &LateContext<'_>, self_arg: &Expr<'_>) {
                 "try",
                 format!(
                     "{}.by_ref()",
-                    snippet_with_context(cx, snip_span, self_arg.span.ctxt(), "..", &mut app).0,
+                    snippet_expr(cx, snip_expr, TargetPrecedence::Postfix, self_arg.span.ctxt(), &mut app),
                 ),
                 app,
             );

clippy_utils/src/source.rs

@@ -4,7 +4,7 @@
 
 use crate::line_span;
 use rustc_errors::Applicability;
-use rustc_hir::{Expr, ExprKind};
+use rustc_hir::{BinOpKind, Expr, ExprKind};
 use rustc_lint::{LateContext, LintContext};
 use rustc_span::hygiene;
 use rustc_span::{BytePos, Pos, Span, SyntaxContext};
@@ -306,6 +306,89 @@ pub fn snippet_with_context(
     )
 }
 
+#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord)]
+pub enum TargetPrecedence {
+    Closure,
+    Assignment,
+    Range,
+    Or,
+    And,
+    Eq,
+    BitOr,
+    BitAnd,
+    Shift,
+    BitXor,
+    Add,
+    Mul,
+    As,
+    Prefix,
+    Postfix,
+}
+
+/// Extracts a snippet of the given expression taking into account the `SyntaxContext` the snippet
+/// needs to be taken from. Parenthesis will be added if needed to place the snippet in the target
+/// precedence level. Returns a placeholder (`(..)`) if a snippet can't be extracted (e.g. an
+/// invalid span).
+///
+/// The `SyntaxContext` of the expression will be walked up to the given target context (usually
+/// from the parent expression) before extracting a snippet. This allows getting the call to a macro
+/// rather than the expression from expanding the macro. e.g. In the expression `&vec![]` taking a
+/// snippet of the chile of the borrow expression will get a snippet of what `vec![]` expands in to.
+/// With the target context set to the same as the borrow expression, this will get a snippet of the
+/// call to the macro.
+///
+/// The applicability will be modified in two ways:
+/// * If a snippet can't be extracted it will be changed from `MachineApplicable` or
+///   `MaybeIncorrect` to `HasPlaceholders`.
+/// * If the snippet is taken from a macro expansion then it will be changed from
+///   `MachineApplicable` to `MaybeIncorrect`.
+pub fn snippet_expr(
+    cx: &LateContext<'_>,
+    expr: &Expr<'_>,
+    target_precedence: TargetPrecedence,
+    ctxt: SyntaxContext,
+    app: &mut Applicability,
+) -> String {
+    let (snip, is_mac_call) = snippet_with_context(cx, expr.span, ctxt, "(..)", app);
+
+    match snip {
+        Cow::Borrowed(snip) => snip.to_owned(),
+        Cow::Owned(snip) if is_mac_call => snip,
+        Cow::Owned(mut snip) => {
+            let needs_paren = match expr.kind {
+                ExprKind::Binary(op, ..) => match op.node {
+                    BinOpKind::Add | BinOpKind::Sub => target_precedence > TargetPrecedence::Add,
+                    BinOpKind::Mul | BinOpKind::Div | BinOpKind::Rem => target_precedence > TargetPrecedence::Mul,
+                    BinOpKind::And => target_precedence > TargetPrecedence::And,
+                    BinOpKind::Or => target_precedence > TargetPrecedence::Or,
+                    BinOpKind::BitXor => target_precedence > TargetPrecedence::BitXor,
+                    BinOpKind::BitAnd => target_precedence > TargetPrecedence::BitAnd,
+                    BinOpKind::BitOr => target_precedence > TargetPrecedence::BitOr,
+                    BinOpKind::Shl | BinOpKind::Shr => target_precedence > TargetPrecedence::Shift,
+                    BinOpKind::Eq | BinOpKind::Lt | BinOpKind::Le | BinOpKind::Ne | BinOpKind::Gt | BinOpKind::Ge => {
+                        target_precedence > TargetPrecedence::Eq
+                    },
+                },
+                ExprKind::Unary(..) | ExprKind::AddrOf(..) => target_precedence > TargetPrecedence::Prefix,
+                ExprKind::Cast(..) => target_precedence > TargetPrecedence::As,
+                ExprKind::Box(..)
+                | ExprKind::Closure(..)
+                | ExprKind::Break(..)
+                | ExprKind::Ret(..)
+                | ExprKind::Yield(..) => target_precedence > TargetPrecedence::Closure,
+                ExprKind::Assign(..) | ExprKind::AssignOp(..) => target_precedence > TargetPrecedence::Assignment,
+                _ => false,
+            };
+
+            if needs_paren {
+                snip.insert(0, '(');
+                snip.push(')');
+            }
+            snip
+        },
+    }
+}
+
 /// Walks the span up to the target context, thereby returning the macro call site if the span is
 /// inside a macro expansion, or the original span if it is not. Note this will return `None` in the
 /// case of the span being in a macro expansion, but the target context is from expanding a macro

tests/ui/ref_mut_iter_method_chain.fixed

@@ -31,4 +31,7 @@ fn main() {
 
     let iter = &mut iter;
     iter.by_ref().find(|&&x| x == 1);
+    let mut iter = iter;
+    let iter = &mut iter;
+    (*iter).by_ref().find(|&&x| x == 1);
 }

tests/ui/ref_mut_iter_method_chain.rs

@@ -31,4 +31,7 @@ fn main() {
 
     let iter = &mut iter;
     (&mut *iter).find(|&&x| x == 1);
+    let mut iter = iter;
+    let iter = &mut iter;
+    (&mut **iter).find(|&&x| x == 1);
 }

tests/ui/ref_mut_iter_method_chain.stderr

@@ -18,5 +18,11 @@ error: use of `&mut` on an iterator in a method chain
 LL |     (&mut *iter).find(|&&x| x == 1);
    |     ^^^^^^^^^^^^ help: try: `iter.by_ref()`
 
-error: aborting due to 3 previous errors
+error: use of `&mut` on an iterator in a method chain
+  --> $DIR/ref_mut_iter_method_chain.rs:36:5
+   |
+LL |     (&mut **iter).find(|&&x| x == 1);
+   |     ^^^^^^^^^^^^^ help: try: `(*iter).by_ref()`
+
+error: aborting due to 4 previous errors