From 6b48c3abc79f73ec885b0d9a8c17e3e9ad74a2bf Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Sat, 18 Sep 2021 16:49:25 -0400 Subject: [PATCH 1/5] Add lint `ref_mut_iter_method_chain` --- CHANGELOG.md | 1 + clippy_lints/src/lib.register_all.rs | 1 + clippy_lints/src/lib.register_lints.rs | 1 + clippy_lints/src/lib.register_style.rs | 1 + clippy_lints/src/methods/mod.rs | 31 +++++++++++++- .../src/methods/ref_mut_iter_method_chain.rs | 40 +++++++++++++++++++ tests/ui/ref_mut_iter_method_chain.fixed | 34 ++++++++++++++++ tests/ui/ref_mut_iter_method_chain.rs | 34 ++++++++++++++++ tests/ui/ref_mut_iter_method_chain.stderr | 22 ++++++++++ 9 files changed, 163 insertions(+), 2 deletions(-) create mode 100644 clippy_lints/src/methods/ref_mut_iter_method_chain.rs create mode 100644 tests/ui/ref_mut_iter_method_chain.fixed create mode 100644 tests/ui/ref_mut_iter_method_chain.rs create mode 100644 tests/ui/ref_mut_iter_method_chain.stderr 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..1ed98a265726 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,30 @@ 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? + /// This 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::>(); + /// let after_dot = iter.collect::>(); + /// ``` + /// Use instead: + /// ```rust + /// let mut iter = ['a', 'b', '.', 'd'].iter(); + /// let before_dot = iter.by_ref().take_while(|&&c| c != '.').collect::>(); + /// let after_dot = iter.collect::>(); + /// ``` + 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 +1964,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 +1999,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 +2008,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..a68129317827 --- /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::in_macro; +use clippy_utils::source::snippet_with_context; +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 !in_macro(self_arg.span); + if let Some(&iter_trait) = cx.tcx.all_diagnostic_items(()).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 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_with_context(cx, snip_span, self_arg.span.ctxt(), "..", &mut app).0, + ), + app, + ); + } + } +} 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..dc30a16c0e5a --- /dev/null +++ b/tests/ui/ref_mut_iter_method_chain.fixed @@ -0,0 +1,34 @@ +// 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) + } + + 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..8414cf8c6039 --- /dev/null +++ b/tests/ui/ref_mut_iter_method_chain.rs @@ -0,0 +1,34 @@ +// 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) + } + + 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..8a10888fe669 --- /dev/null +++ b/tests/ui/ref_mut_iter_method_chain.stderr @@ -0,0 +1,22 @@ +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:33:5 + | +LL | (&mut *iter).find(|&&x| x == 1); + | ^^^^^^^^^^^^ help: try: `iter.by_ref()` + +error: aborting due to 3 previous errors + From 6aa878cab972d8da2bb6c84bd6bc4d78d57dfa8e Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Fri, 12 Nov 2021 11:13:00 -0500 Subject: [PATCH 2/5] Add parenthesis when needed for suggestion in `ref_mut_iter_method_chain` --- .../src/methods/ref_mut_iter_method_chain.rs | 17 ++-- clippy_utils/src/source.rs | 85 ++++++++++++++++++- tests/ui/ref_mut_iter_method_chain.fixed | 3 + tests/ui/ref_mut_iter_method_chain.rs | 3 + tests/ui/ref_mut_iter_method_chain.stderr | 8 +- 5 files changed, 105 insertions(+), 11 deletions(-) diff --git a/clippy_lints/src/methods/ref_mut_iter_method_chain.rs b/clippy_lints/src/methods/ref_mut_iter_method_chain.rs index a68129317827..7744c5dc7a3a 100644 --- a/clippy_lints/src/methods/ref_mut_iter_method_chain.rs +++ b/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, ); diff --git a/clippy_utils/src/source.rs b/clippy_utils/src/source.rs index 7f68cc388eb0..dd414f0e60f3 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,89 @@ pub fn snippet_with_context( ) } +#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] +pub enum TargetPrecedence { + Closure, + Assignment, + Range, + Or, + And, + Eq, + BitOr, + BitXor, + BitAnd, + Shift, + 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 diff --git a/tests/ui/ref_mut_iter_method_chain.fixed b/tests/ui/ref_mut_iter_method_chain.fixed index dc30a16c0e5a..3a93fdd0ebb6 100644 --- a/tests/ui/ref_mut_iter_method_chain.fixed +++ b/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); } diff --git a/tests/ui/ref_mut_iter_method_chain.rs b/tests/ui/ref_mut_iter_method_chain.rs index 8414cf8c6039..68946f4aafce 100644 --- a/tests/ui/ref_mut_iter_method_chain.rs +++ b/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); } diff --git a/tests/ui/ref_mut_iter_method_chain.stderr b/tests/ui/ref_mut_iter_method_chain.stderr index 8a10888fe669..555bf45e0c84 100644 --- a/tests/ui/ref_mut_iter_method_chain.stderr +++ b/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 From c959d5e54bb18e8e5eade96a609e7cdb4c9fa070 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Fri, 12 Nov 2021 11:21:28 -0500 Subject: [PATCH 3/5] Cleanup for `ref_mut_iter_method_chain` --- clippy_lints/src/methods/mod.rs | 4 +--- tests/ui/ref_mut_iter_method_chain.fixed | 6 +++++- tests/ui/ref_mut_iter_method_chain.rs | 6 +++++- tests/ui/ref_mut_iter_method_chain.stderr | 12 +++++++++--- 4 files changed, 20 insertions(+), 8 deletions(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 1ed98a265726..6d2a329e9d9f 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -1867,19 +1867,17 @@ declare_clippy_lint! { /// Check for `&mut iter` followed by a method call. /// /// ### Why is this bad? - /// This requires using parenthesis to signify precedence. + /// 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::>(); - /// let after_dot = iter.collect::>(); /// ``` /// Use instead: /// ```rust /// let mut iter = ['a', 'b', '.', 'd'].iter(); /// let before_dot = iter.by_ref().take_while(|&&c| c != '.').collect::>(); - /// let after_dot = iter.collect::>(); /// ``` pub REF_MUT_ITER_METHOD_CHAIN, style, diff --git a/tests/ui/ref_mut_iter_method_chain.fixed b/tests/ui/ref_mut_iter_method_chain.fixed index 3a93fdd0ebb6..dafda7ed9c6d 100644 --- a/tests/ui/ref_mut_iter_method_chain.fixed +++ b/tests/ui/ref_mut_iter_method_chain.fixed @@ -26,7 +26,11 @@ fn main() { // Don't lint. No method chain. for &x in &mut iter { - print!("{}", x) + print!("{}", x); + } + + for &x in iter.by_ref().filter(|&&x| x % 2 == 0) { + print!("{}", x); } let iter = &mut iter; diff --git a/tests/ui/ref_mut_iter_method_chain.rs b/tests/ui/ref_mut_iter_method_chain.rs index 68946f4aafce..caca9986ea8b 100644 --- a/tests/ui/ref_mut_iter_method_chain.rs +++ b/tests/ui/ref_mut_iter_method_chain.rs @@ -26,7 +26,11 @@ fn main() { // Don't lint. No method chain. for &x in &mut iter { - print!("{}", x) + print!("{}", x); + } + + for &x in (&mut iter).filter(|&&x| x % 2 == 0) { + print!("{}", x); } let iter = &mut iter; diff --git a/tests/ui/ref_mut_iter_method_chain.stderr b/tests/ui/ref_mut_iter_method_chain.stderr index 555bf45e0c84..0fdea2a51a2a 100644 --- a/tests/ui/ref_mut_iter_method_chain.stderr +++ b/tests/ui/ref_mut_iter_method_chain.stderr @@ -13,16 +13,22 @@ 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:33:5 + --> $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:36:5 + --> $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 4 previous errors +error: aborting due to 5 previous errors From 453c07b4eb506f9cab1054c828442ebef1056bba Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Fri, 12 Nov 2021 11:29:53 -0500 Subject: [PATCH 4/5] Add version to `ref_mut_iter_method_chain` --- clippy_lints/src/methods/mod.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 6d2a329e9d9f..9e175edb5aad 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -1879,6 +1879,7 @@ declare_clippy_lint! { /// 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" From 31dc4abb6a87ec8cd158c3487294814ead66e6e9 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Fri, 12 Nov 2021 12:33:54 -0500 Subject: [PATCH 5/5] Fix formatting for `ref_mut_iter_method_chain` --- .../src/methods/ref_mut_iter_method_chain.rs | 7 +- clippy_utils/src/source.rs | 122 +++++++++++++----- 2 files changed, 94 insertions(+), 35 deletions(-) diff --git a/clippy_lints/src/methods/ref_mut_iter_method_chain.rs b/clippy_lints/src/methods/ref_mut_iter_method_chain.rs index 7744c5dc7a3a..9d851a7bb3af 100644 --- a/clippy_lints/src/methods/ref_mut_iter_method_chain.rs +++ b/clippy_lints/src/methods/ref_mut_iter_method_chain.rs @@ -1,5 +1,5 @@ use clippy_utils::diagnostics::span_lint_and_sugg; -use clippy_utils::source::{snippet_expr, TargetPrecedence}; +use clippy_utils::source::{snippet_expr, ExprPosition}; use clippy_utils::ty::implements_trait; use if_chain::if_chain; use rustc_errors::Applicability; @@ -17,7 +17,8 @@ pub(crate) fn check(cx: &LateContext<'_>, self_arg: &Expr<'_>) { 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() + ExprKind::Unary(UnOp::Deref, e) + if cx.typeck_results().expr_ty(e).is_ref() && !base_expr.span.from_expansion() => e, _ => base_expr, }; @@ -30,7 +31,7 @@ pub(crate) fn check(cx: &LateContext<'_>, self_arg: &Expr<'_>) { "try", format!( "{}.by_ref()", - snippet_expr(cx, snip_expr, TargetPrecedence::Postfix, self_arg.span.ctxt(), &mut app), + 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 dd414f0e60f3..019e94248a1f 100644 --- a/clippy_utils/src/source.rs +++ b/clippy_utils/src/source.rs @@ -307,20 +307,34 @@ pub fn snippet_with_context( } #[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] -pub enum TargetPrecedence { - Closure, - Assignment, - Range, - Or, - And, - Eq, - BitOr, - BitXor, - BitAnd, - Shift, - Add, - Mul, - As, +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, } @@ -345,7 +359,7 @@ pub enum TargetPrecedence { pub fn snippet_expr( cx: &LateContext<'_>, expr: &Expr<'_>, - target_precedence: TargetPrecedence, + target_position: ExprPosition, ctxt: SyntaxContext, app: &mut Applicability, ) -> String { @@ -355,29 +369,73 @@ pub fn snippet_expr( Cow::Borrowed(snip) => snip.to_owned(), Cow::Owned(snip) if is_mac_call => snip, Cow::Owned(mut snip) => { - let needs_paren = match expr.kind { + 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_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::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_precedence > TargetPrecedence::Eq + target_position > ExprPosition::EqLhs }, }, - ExprKind::Unary(..) | ExprKind::AddrOf(..) => target_precedence > TargetPrecedence::Prefix, - ExprKind::Cast(..) => target_precedence > TargetPrecedence::As, - ExprKind::Box(..) - | ExprKind::Closure(..) + 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(..) => target_precedence > TargetPrecedence::Closure, - ExprKind::Assign(..) | ExprKind::AssignOp(..) => target_precedence > TargetPrecedence::Assignment, - _ => false, + | 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 {