diff --git a/CHANGELOG.md b/CHANGELOG.md index 1059f0ac7cd6..279dbc4175d2 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/clippy_lints/src/lib.register_all.rs b/clippy_lints/src/lib.register_all.rs index 15edb79d36c2..75ab1df018da 100644 --- a/clippy_lints/src/lib.register_all.rs +++ b/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), diff --git a/clippy_lints/src/lib.register_lints.rs b/clippy_lints/src/lib.register_lints.rs index 6d1d45f89000..8895af41097f 100644 --- a/clippy_lints/src/lib.register_lints.rs +++ b/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, diff --git a/clippy_lints/src/lib.register_style.rs b/clippy_lints/src/lib.register_style.rs index 744880bda3e6..b65d8af130da 100644 --- a/clippy_lints/src/lib.register_style.rs +++ b/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), diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 5cb849a56bc6..9e175edb5aad 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/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::>(); + /// ``` + /// Use instead: + /// ```rust + /// let mut iter = ['a', 'b', '.', 'd'].iter(); + /// let before_dot = iter.by_ref().take_while(|&&c| c != '.').collect::>(); + /// ``` + #[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, @@ -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 { diff --git a/clippy_lints/src/methods/ref_mut_iter_method_chain.rs b/clippy_lints/src/methods/ref_mut_iter_method_chain.rs new file mode 100644 index 000000000000..9d851a7bb3af --- /dev/null +++ b/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, + ); + } + } +} diff --git a/clippy_utils/src/source.rs b/clippy_utils/src/source.rs index 7f68cc388eb0..019e94248a1f 100644 --- a/clippy_utils/src/source.rs +++ b/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 diff --git a/tests/ui/ref_mut_iter_method_chain.fixed b/tests/ui/ref_mut_iter_method_chain.fixed new file mode 100644 index 000000000000..dafda7ed9c6d --- /dev/null +++ b/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); +} diff --git a/tests/ui/ref_mut_iter_method_chain.rs b/tests/ui/ref_mut_iter_method_chain.rs new file mode 100644 index 000000000000..caca9986ea8b --- /dev/null +++ b/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); +} diff --git a/tests/ui/ref_mut_iter_method_chain.stderr b/tests/ui/ref_mut_iter_method_chain.stderr new file mode 100644 index 000000000000..0fdea2a51a2a --- /dev/null +++ b/tests/ui/ref_mut_iter_method_chain.stderr @@ -0,0 +1,34 @@ +error: use of `&mut` on an iterator in a method chain + --> $DIR/ref_mut_iter_method_chain.rs:18:13 + | +LL | let _ = (&mut iter).find(|&&x| x == 1); + | ^^^^^^^^^^^ help: try: `iter.by_ref()` + | + = note: `-D clippy::ref-mut-iter-method-chain` implied by `-D warnings` + +error: use of `&mut` on an iterator in a method chain + --> $DIR/ref_mut_iter_method_chain.rs:19:13 + | +LL | let _ = (&mut m!(iter)).find(|&&x| x == 1); + | ^^^^^^^^^^^^^^^ help: try: `m!(iter).by_ref()` + +error: use of `&mut` on an iterator in a method chain + --> $DIR/ref_mut_iter_method_chain.rs:32:15 + | +LL | for &x in (&mut iter).filter(|&&x| x % 2 == 0) { + | ^^^^^^^^^^^ help: try: `iter.by_ref()` + +error: use of `&mut` on an iterator in a method chain + --> $DIR/ref_mut_iter_method_chain.rs:37:5 + | +LL | (&mut *iter).find(|&&x| x == 1); + | ^^^^^^^^^^^^ help: try: `iter.by_ref()` + +error: use of `&mut` on an iterator in a method chain + --> $DIR/ref_mut_iter_method_chain.rs:40:5 + | +LL | (&mut **iter).find(|&&x| x == 1); + | ^^^^^^^^^^^^^ help: try: `(*iter).by_ref()` + +error: aborting due to 5 previous errors +