From 4c1b6a28e4c7b932b077c03e7a0f100b791391d7 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Fri, 10 Sep 2021 12:20:39 -0400 Subject: [PATCH] Fix result order for `manual_split_once` when `rsplitn` is used --- clippy_lints/src/methods/manual_split_once.rs | 70 +++++++++---------- tests/ui/manual_split_once.fixed | 6 ++ tests/ui/manual_split_once.rs | 6 ++ tests/ui/manual_split_once.stderr | 28 +++++++- 4 files changed, 72 insertions(+), 38 deletions(-) diff --git a/clippy_lints/src/methods/manual_split_once.rs b/clippy_lints/src/methods/manual_split_once.rs index e273186d0519..8440f2918592 100644 --- a/clippy_lints/src/methods/manual_split_once.rs +++ b/clippy_lints/src/methods/manual_split_once.rs @@ -17,32 +17,25 @@ pub(super) fn check(cx: &LateContext<'_>, method_name: &str, expr: &Expr<'_>, se } let ctxt = expr.span.ctxt(); - let usage = match parse_iter_usage(cx, ctxt, cx.tcx.hir().parent_iter(expr.hir_id)) { + let (method_name, msg, reverse) = if method_name == "splitn" { + ("split_once", "manual implementation of `split_once`", false) + } else { + ("rsplit_once", "manual implementation of `rsplit_once`", true) + }; + let usage = match parse_iter_usage(cx, ctxt, cx.tcx.hir().parent_iter(expr.hir_id), reverse) { Some(x) => x, None => return, }; - let (method_name, msg) = if method_name == "splitn" { - ("split_once", "manual implementation of `split_once`") - } else { - ("rsplit_once", "manual implementation of `rsplit_once`") - }; let mut app = Applicability::MachineApplicable; let self_snip = snippet_with_context(cx, self_arg.span, ctxt, "..", &mut app).0; let pat_snip = snippet_with_context(cx, pat_arg.span, ctxt, "..", &mut app).0; - match usage.kind { + let sugg = match usage.kind { IterUsageKind::NextTuple => { - span_lint_and_sugg( - cx, - MANUAL_SPLIT_ONCE, - usage.span, - msg, - "try this", - format!("{}.{}({})", self_snip, method_name, pat_snip), - app, - ); + format!("{}.{}({})", self_snip, method_name, pat_snip) }, + IterUsageKind::RNextTuple => format!("{}.{}({}).map(|(x, y)| (y, x))", self_snip, method_name, pat_snip), IterUsageKind::Next => { let self_deref = { let adjust = cx.typeck_results().expr_adjustments(self_arg); @@ -58,7 +51,7 @@ pub(super) fn check(cx: &LateContext<'_>, method_name: &str, expr: &Expr<'_>, se "*".repeat(adjust.len() - 2) } }; - let sugg = if usage.unwrap_kind.is_some() { + if usage.unwrap_kind.is_some() { format!( "{}.{}({}).map_or({}{}, |x| x.0)", &self_snip, method_name, pat_snip, self_deref, &self_snip @@ -68,9 +61,7 @@ pub(super) fn check(cx: &LateContext<'_>, method_name: &str, expr: &Expr<'_>, se "Some({}.{}({}).map_or({}{}, |x| x.0))", &self_snip, method_name, pat_snip, self_deref, &self_snip ) - }; - - span_lint_and_sugg(cx, MANUAL_SPLIT_ONCE, usage.span, msg, "try this", sugg, app); + } }, IterUsageKind::Second => { let access_str = match usage.unwrap_kind { @@ -78,23 +69,18 @@ pub(super) fn check(cx: &LateContext<'_>, method_name: &str, expr: &Expr<'_>, se Some(UnwrapKind::QuestionMark) => "?.1", None => ".map(|x| x.1)", }; - span_lint_and_sugg( - cx, - MANUAL_SPLIT_ONCE, - usage.span, - msg, - "try this", - format!("{}.{}({}){}", self_snip, method_name, pat_snip, access_str), - app, - ); + format!("{}.{}({}){}", self_snip, method_name, pat_snip, access_str) }, - } + }; + + span_lint_and_sugg(cx, MANUAL_SPLIT_ONCE, usage.span, msg, "try this", sugg, app); } enum IterUsageKind { Next, Second, NextTuple, + RNextTuple, } enum UnwrapKind { @@ -108,10 +94,12 @@ struct IterUsage { span: Span, } +#[allow(clippy::too_many_lines)] fn parse_iter_usage( cx: &LateContext<'tcx>, ctxt: SyntaxContext, mut iter: impl Iterator)>, + reverse: bool, ) -> Option { let (kind, span) = match iter.next() { Some((_, Node::Expr(e))) if e.span.ctxt() == ctxt => { @@ -124,20 +112,30 @@ fn parse_iter_usage( let iter_id = cx.tcx.get_diagnostic_item(sym::Iterator)?; match (&*name.ident.as_str(), args) { - ("next", []) if cx.tcx.trait_of_item(did) == Some(iter_id) => (IterUsageKind::Next, e.span), + ("next", []) if cx.tcx.trait_of_item(did) == Some(iter_id) => { + if reverse { + (IterUsageKind::Second, e.span) + } else { + (IterUsageKind::Next, e.span) + } + }, ("next_tuple", []) => { - if_chain! { + return if_chain! { if match_def_path(cx, did, &paths::ITERTOOLS_NEXT_TUPLE); if let ty::Adt(adt_def, subs) = cx.typeck_results().expr_ty(e).kind(); if cx.tcx.is_diagnostic_item(sym::option_type, adt_def.did); if let ty::Tuple(subs) = subs.type_at(0).kind(); if subs.len() == 2; then { - return Some(IterUsage { kind: IterUsageKind::NextTuple, span: e.span, unwrap_kind: None }); + Some(IterUsage { + kind: if reverse { IterUsageKind::RNextTuple } else { IterUsageKind::NextTuple }, + span: e.span, + unwrap_kind: None + }) } else { - return None; + None } - } + }; }, ("nth" | "skip", [idx_expr]) if cx.tcx.trait_of_item(did) == Some(iter_id) => { if let Some((Constant::Int(idx), _)) = constant(cx, cx.typeck_results(), idx_expr) { @@ -158,7 +156,7 @@ fn parse_iter_usage( } } }; - match idx { + match if reverse { idx ^ 1 } else { idx } { 0 => (IterUsageKind::Next, span), 1 => (IterUsageKind::Second, span), _ => return None, diff --git a/tests/ui/manual_split_once.fixed b/tests/ui/manual_split_once.fixed index 3a0332939d40..992baf1f185a 100644 --- a/tests/ui/manual_split_once.fixed +++ b/tests/ui/manual_split_once.fixed @@ -36,6 +36,12 @@ fn main() { // Don't lint, slices don't have `split_once` let _ = [0, 1, 2].splitn(2, |&x| x == 1).nth(1).unwrap(); + + // `rsplitn` gives the results in the reverse order of `rsplit_once` + let _ = "key=value".rsplit_once('=').unwrap().1; + let _ = "key=value".rsplit_once('=').map_or("key=value", |x| x.0); + let _ = "key=value".rsplit_once('=').map(|x| x.1); + let (_, _) = "key=value".rsplit_once('=').map(|(x, y)| (y, x)).unwrap(); } fn _msrv_1_51() { diff --git a/tests/ui/manual_split_once.rs b/tests/ui/manual_split_once.rs index e6093b63fe8d..4f92ab6b812b 100644 --- a/tests/ui/manual_split_once.rs +++ b/tests/ui/manual_split_once.rs @@ -36,6 +36,12 @@ fn main() { // Don't lint, slices don't have `split_once` let _ = [0, 1, 2].splitn(2, |&x| x == 1).nth(1).unwrap(); + + // `rsplitn` gives the results in the reverse order of `rsplit_once` + let _ = "key=value".rsplitn(2, '=').next().unwrap(); + let _ = "key=value".rsplitn(2, '=').nth(1).unwrap(); + let _ = "key=value".rsplitn(2, '=').nth(0); + let (_, _) = "key=value".rsplitn(2, '=').next_tuple().unwrap(); } fn _msrv_1_51() { diff --git a/tests/ui/manual_split_once.stderr b/tests/ui/manual_split_once.stderr index 4f15196b469e..7bea2303d921 100644 --- a/tests/ui/manual_split_once.stderr +++ b/tests/ui/manual_split_once.stderr @@ -72,11 +72,35 @@ error: manual implementation of `split_once` LL | let _ = s.splitn(2, "key=value").skip(1).next()?; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `s.split_once("key=value")?.1` +error: manual implementation of `rsplit_once` + --> $DIR/manual_split_once.rs:41:13 + | +LL | let _ = "key=value".rsplitn(2, '=').next().unwrap(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `"key=value".rsplit_once('=').unwrap().1` + +error: manual implementation of `rsplit_once` + --> $DIR/manual_split_once.rs:42:13 + | +LL | let _ = "key=value".rsplitn(2, '=').nth(1).unwrap(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `"key=value".rsplit_once('=').map_or("key=value", |x| x.0)` + +error: manual implementation of `rsplit_once` + --> $DIR/manual_split_once.rs:43:13 + | +LL | let _ = "key=value".rsplitn(2, '=').nth(0); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `"key=value".rsplit_once('=').map(|x| x.1)` + +error: manual implementation of `rsplit_once` + --> $DIR/manual_split_once.rs:44:18 + | +LL | let (_, _) = "key=value".rsplitn(2, '=').next_tuple().unwrap(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `"key=value".rsplit_once('=').map(|(x, y)| (y, x))` + error: manual implementation of `split_once` - --> $DIR/manual_split_once.rs:49:13 + --> $DIR/manual_split_once.rs:55:13 | LL | let _ = "key=value".splitn(2, '=').nth(1).unwrap(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `"key=value".split_once('=').unwrap().1` -error: aborting due to 13 previous errors +error: aborting due to 17 previous errors