Add lint ref_mut_iter_method_chain

CHANGELOG.md

@@ -3107,6 +3107,7 @@ Released 2018-09-13
 [`redundant_static_lifetimes`]: https://rust-lang.github.io/rust-clippy/master/index.html#redundant_static_lifetimes
 [`ref_binding_to_reference`]: https://rust-lang.github.io/rust-clippy/master/index.html#ref_binding_to_reference
 [`ref_in_deref`]: https://rust-lang.github.io/rust-clippy/master/index.html#ref_in_deref
+[`ref_mut_iter_method_chain`]: https://rust-lang.github.io/rust-clippy/master/index.html#ref_mut_iter_method_chain
 [`ref_option_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#ref_option_ref
 [`regex_macro`]: https://rust-lang.github.io/rust-clippy/master/index.html#regex_macro
 [`repeat_once`]: https://rust-lang.github.io/rust-clippy/master/index.html#repeat_once

clippy_lints/src/lib.register_all.rs

@@ -165,6 +165,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![
     LintId::of(methods::OPTION_FILTER_MAP),
     LintId::of(methods::OPTION_MAP_OR_NONE),
     LintId::of(methods::OR_FUN_CALL),
+    LintId::of(methods::REF_MUT_ITER_METHOD_CHAIN),
     LintId::of(methods::RESULT_MAP_OR_INTO_OPTION),
     LintId::of(methods::SEARCH_IS_SOME),
     LintId::of(methods::SHOULD_IMPLEMENT_TRAIT),

clippy_lints/src/lib.register_lints.rs

@@ -299,6 +299,7 @@ store.register_lints(&[
     methods::OPTION_FILTER_MAP,
     methods::OPTION_MAP_OR_NONE,
     methods::OR_FUN_CALL,
+    methods::REF_MUT_ITER_METHOD_CHAIN,
     methods::RESULT_MAP_OR_INTO_OPTION,
     methods::SEARCH_IS_SOME,
     methods::SHOULD_IMPLEMENT_TRAIT,

clippy_lints/src/lib.register_style.rs

@@ -64,6 +64,7 @@ store.register_group(true, "clippy::style", Some("clippy_style"), vec![
     LintId::of(methods::NEW_RET_NO_SELF),
     LintId::of(methods::OK_EXPECT),
     LintId::of(methods::OPTION_MAP_OR_NONE),
+    LintId::of(methods::REF_MUT_ITER_METHOD_CHAIN),
     LintId::of(methods::RESULT_MAP_OR_INTO_OPTION),
     LintId::of(methods::SHOULD_IMPLEMENT_TRAIT),
     LintId::of(methods::SINGLE_CHAR_ADD_STR),

clippy_lints/src/methods/mod.rs

@@ -44,6 +44,7 @@ mod option_as_ref_deref;
 mod option_map_or_none;
 mod option_map_unwrap_or;
 mod or_fun_call;
+mod ref_mut_iter_method_chain;
 mod search_is_some;
 mod single_char_add_str;
 mod single_char_insert_string;
@@ -1861,6 +1862,29 @@ declare_clippy_lint! {
     "replace `.splitn(2, pat)` with `.split_once(pat)`"
 }
 
+declare_clippy_lint! {
+    /// ### What it does
+    /// Check for `&mut iter` followed by a method call.
+    ///
+    /// ### Why is this bad?
+    /// Using `(&mut iter)._` requires using parenthesis to signify precedence.
+    ///
+    /// ### Example
+    /// ```rust
+    /// let mut iter = ['a', 'b', '.', 'd'].iter();
+    /// let before_dot = (&mut iter).take_while(|&&c| c != '.').collect::<Vec<_>>();
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// let mut iter = ['a', 'b', '.', 'd'].iter();
+    /// let before_dot = iter.by_ref().take_while(|&&c| c != '.').collect::<Vec<_>>();
+    /// ```
+    #[clippy::version = "1.58.0"]
+    pub REF_MUT_ITER_METHOD_CHAIN,
+    style,
+    "`&mut iter` used in a method chain"
+}
+
 pub struct Methods {
     avoid_breaking_exported_api: bool,
     msrv: Option<RustcVersion>,
@@ -1939,7 +1963,8 @@ impl_lint_pass!(Methods => [
     SUSPICIOUS_SPLITN,
     MANUAL_STR_REPEAT,
     EXTEND_WITH_DRAIN,
-    MANUAL_SPLIT_ONCE
+    MANUAL_SPLIT_ONCE,
+    REF_MUT_ITER_METHOD_CHAIN
 ]);
 
 /// Extracts a method call name, args, and `Span` of the method name.
@@ -1973,7 +1998,7 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
             hir::ExprKind::Call(func, args) => {
                 from_iter_instead_of_collect::check(cx, expr, args, func);
             },
-            hir::ExprKind::MethodCall(method_call, ref method_span, args, _) => {
+            hir::ExprKind::MethodCall(method_call, ref method_span, args @ [self_arg, ..], _) => {
                 or_fun_call::check(cx, expr, *method_span, &method_call.ident.as_str(), args);
                 expect_fun_call::check(cx, expr, *method_span, &method_call.ident.as_str(), args);
                 clone_on_copy::check(cx, expr, method_call.ident.name, args);
@@ -1982,6 +2007,7 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
                 single_char_add_str::check(cx, expr, args);
                 into_iter_on_ref::check(cx, expr, *method_span, method_call.ident.name, args);
                 single_char_pattern::check(cx, expr, method_call.ident.name, args);
+                ref_mut_iter_method_chain::check(cx, self_arg);
             },
             hir::ExprKind::Binary(op, lhs, rhs) if op.node == hir::BinOpKind::Eq || op.node == hir::BinOpKind::Ne => {
                 let mut info = BinaryExprInfo {

clippy_lints/src/methods/ref_mut_iter_method_chain.rs

@@ -0,0 +1,40 @@
+use clippy_utils::diagnostics::span_lint_and_sugg;
+use clippy_utils::source::{snippet_expr, ExprPosition};
+use clippy_utils::ty::implements_trait;
+use if_chain::if_chain;
+use rustc_errors::Applicability;
+use rustc_hir::{BorrowKind, Expr, ExprKind, Mutability, UnOp};
+use rustc_lint::LateContext;
+use rustc_span::sym;
+
+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 !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_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(
+                cx,
+                REF_MUT_ITER_METHOD_CHAIN,
+                self_arg.span,
+                "use of `&mut` on an iterator in a method chain",
+                "try",
+                format!(
+                    "{}.by_ref()",
+                    snippet_expr(cx, snip_expr, ExprPosition::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,147 @@ pub fn snippet_with_context(
     )
 }
 
+#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord)]
+pub enum ExprPosition {
+    // Also includes `return`, `yield`, `break` and closures
+    Paren,
+    AssignmentRhs,
+    AssignmentLhs,
+    RangeLhs,
+    RangeRhs,
+    OrLhs,
+    OrRhs,
+    AndLhs,
+    AndRhs,
+    Let,
+    EqLhs,
+    EqRhs,
+    BitOrLhs,
+    BitOrRhs,
+    BitXorLhs,
+    BitXorRhs,
+    BitAndLhs,
+    BitAndRhs,
+    ShiftLhs,
+    ShiftRhs,
+    AddLhs,
+    AddRhs,
+    MulLhs,
+    MulRhs,
+    // Also includes type ascription
+    Cast,
+    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_position: ExprPosition,
+    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 ctxt = expr.span.ctxt();
+
+            // Attempt to determine if parenthesis are needed base on the target position. The snippet may have
+            // parenthesis already, so attempt to find those.
+            // TODO: Remove parenthesis if they aren't needed at the target position.
+            let needs_paren = match expr.peel_drop_temps().kind {
+                ExprKind::Binary(_, lhs, rhs)
+                    if (ctxt == lhs.span.ctxt() && expr.span.lo() != lhs.span.lo())
+                        || (ctxt == rhs.span.ctxt() && expr.span.hi() != rhs.span.hi()) =>
+                {
+                    false
+                },
+                ExprKind::Binary(op, ..) => match op.node {
+                    BinOpKind::Add | BinOpKind::Sub => target_position > ExprPosition::AddLhs,
+                    BinOpKind::Mul | BinOpKind::Div | BinOpKind::Rem => target_position > ExprPosition::MulLhs,
+                    BinOpKind::And => target_position > ExprPosition::AndLhs,
+                    BinOpKind::Or => target_position > ExprPosition::OrLhs,
+                    BinOpKind::BitXor => target_position > ExprPosition::BitXorLhs,
+                    BinOpKind::BitAnd => target_position > ExprPosition::BitAndLhs,
+                    BinOpKind::BitOr => target_position > ExprPosition::BitOrLhs,
+                    BinOpKind::Shl | BinOpKind::Shr => target_position > ExprPosition::ShiftLhs,
+                    BinOpKind::Eq | BinOpKind::Lt | BinOpKind::Le | BinOpKind::Ne | BinOpKind::Gt | BinOpKind::Ge => {
+                        target_position > ExprPosition::EqLhs
+                    },
+                },
+                ExprKind::Box(..) | ExprKind::Unary(..) | ExprKind::AddrOf(..) if snip.starts_with('(') => false,
+                ExprKind::Box(..) | ExprKind::Unary(..) | ExprKind::AddrOf(..) => {
+                    target_position > ExprPosition::Prefix
+                },
+                ExprKind::Let(..) if snip.starts_with('(') => false,
+                ExprKind::Let(..) => target_position > ExprPosition::Let,
+                ExprKind::Cast(lhs, rhs)
+                    if (ctxt == lhs.span.ctxt() && expr.span.lo() != lhs.span.lo())
+                        || (ctxt == rhs.span.ctxt() && expr.span.hi() != rhs.span.hi()) =>
+                {
+                    false
+                },
+                ExprKind::Cast(..) | ExprKind::Type(..) => target_position > ExprPosition::Cast,
+
+                ExprKind::Closure(..)
+                | ExprKind::Break(..)
+                | ExprKind::Ret(..)
+                | ExprKind::Yield(..)
+                | ExprKind::Assign(..)
+                | ExprKind::AssignOp(..) => target_position > ExprPosition::AssignmentRhs,
+
+                // Postfix operators, or expression with braces of some form
+                ExprKind::Array(_)
+                | ExprKind::Call(..)
+                | ExprKind::ConstBlock(_)
+                | ExprKind::MethodCall(..)
+                | ExprKind::Tup(..)
+                | ExprKind::Lit(..)
+                | ExprKind::DropTemps(_)
+                | ExprKind::If(..)
+                | ExprKind::Loop(..)
+                | ExprKind::Match(..)
+                | ExprKind::Block(..)
+                | ExprKind::Field(..)
+                | ExprKind::Index(..)
+                | ExprKind::Path(_)
+                | ExprKind::Continue(_)
+                | ExprKind::InlineAsm(_)
+                | ExprKind::LlvmInlineAsm(_)
+                | ExprKind::Struct(..)
+                | ExprKind::Repeat(..)
+                | ExprKind::Err => 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

@@ -0,0 +1,41 @@
+// run-rustfix
+#![warn(clippy::ref_mut_iter_method_chain)]
+
+macro_rules! m {
+    ($i:ident) => {
+        $i
+    };
+    (&mut $i:ident) => {
+        &mut $i
+    };
+    (($i:expr).$m:ident($arg:expr)) => {
+        ($i).$m($arg)
+    };
+}
+
+fn main() {
+    let mut iter = [0, 1, 2].iter();
+    let _ = iter.by_ref().find(|&&x| x == 1);
+    let _ = m!(iter).by_ref().find(|&&x| x == 1);
+
+    // Don't lint. `&mut` comes from macro expansion.
+    let _ = m!(&mut iter).find(|&&x| x == 1);
+
+    // Don't lint. Method call from expansion
+    let _ = m!((&mut iter).find(|&&x| x == 1));
+
+    // Don't lint. No method chain.
+    for &x in &mut iter {
+        print!("{}", x);
+    }
+
+    for &x in iter.by_ref().filter(|&&x| x % 2 == 0) {
+        print!("{}", x);
+    }
+
+    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

@@ -0,0 +1,41 @@
+// run-rustfix
+#![warn(clippy::ref_mut_iter_method_chain)]
+
+macro_rules! m {
+    ($i:ident) => {
+        $i
+    };
+    (&mut $i:ident) => {
+        &mut $i
+    };
+    (($i:expr).$m:ident($arg:expr)) => {
+        ($i).$m($arg)
+    };
+}
+
+fn main() {
+    let mut iter = [0, 1, 2].iter();
+    let _ = (&mut iter).find(|&&x| x == 1);
+    let _ = (&mut m!(iter)).find(|&&x| x == 1);
+
+    // Don't lint. `&mut` comes from macro expansion.
+    let _ = m!(&mut iter).find(|&&x| x == 1);
+
+    // Don't lint. Method call from expansion
+    let _ = m!((&mut iter).find(|&&x| x == 1));
+
+    // Don't lint. No method chain.
+    for &x in &mut iter {
+        print!("{}", x);
+    }
+
+    for &x in (&mut iter).filter(|&&x| x % 2 == 0) {
+        print!("{}", x);
+    }
+
+    let iter = &mut iter;
+    (&mut *iter).find(|&&x| x == 1);
+    let mut iter = iter;
+    let iter = &mut iter;
+    (&mut **iter).find(|&&x| x == 1);
+}