Auto merge of rust-lang#7896 - surechen:fix_manual_split_once, r=cams…

…teffen Fix for rust-lang#7889 and add new lint needless_splitn fixes: rust-lang#7889 1. Fix the problem of manual_split_once changing the original behavior. 2. Add a new lint needless_splitn. changelog: Fix the problem of manual_split_once changing the original behavior and add a new lint needless_splitn.
flip1995 · Nov 17, 2021 · 46687f1 · 46687f1
2 parents 83ad511 + c051656
commit 46687f1
Show file tree

Hide file tree

Showing 14 changed files with 276 additions and 43 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -3039,6 +3039,7 @@ Released 2018-09-13
 [`needless_question_mark`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_question_mark
 [`needless_range_loop`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_range_loop
 [`needless_return`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_return
+[`needless_splitn`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_splitn
 [`needless_update`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_update
 [`neg_cmp_op_on_partial_ord`]: https://rust-lang.github.io/rust-clippy/master/index.html#neg_cmp_op_on_partial_ord
 [`neg_multiply`]: https://rust-lang.github.io/rust-clippy/master/index.html#neg_multiply

diff --git a/clippy_lints/src/lib.register_all.rs b/clippy_lints/src/lib.register_all.rs
@@ -159,6 +159,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![
     LintId::of(methods::MANUAL_STR_REPEAT),
     LintId::of(methods::MAP_COLLECT_RESULT_UNIT),
     LintId::of(methods::MAP_IDENTITY),
+    LintId::of(methods::NEEDLESS_SPLITN),
     LintId::of(methods::NEW_RET_NO_SELF),
     LintId::of(methods::OK_EXPECT),
     LintId::of(methods::OPTION_AS_REF_DEREF),

diff --git a/clippy_lints/src/lib.register_complexity.rs b/clippy_lints/src/lib.register_complexity.rs
@@ -42,6 +42,7 @@ store.register_group(true, "clippy::complexity", Some("clippy_complexity"), vec!
     LintId::of(methods::MANUAL_FIND_MAP),
     LintId::of(methods::MANUAL_SPLIT_ONCE),
     LintId::of(methods::MAP_IDENTITY),
+    LintId::of(methods::NEEDLESS_SPLITN),
     LintId::of(methods::OPTION_AS_REF_DEREF),
     LintId::of(methods::OPTION_FILTER_MAP),
     LintId::of(methods::SEARCH_IS_SOME),

diff --git a/clippy_lints/src/lib.register_lints.rs b/clippy_lints/src/lib.register_lints.rs
@@ -293,6 +293,7 @@ store.register_lints(&[
     methods::MAP_FLATTEN,
     methods::MAP_IDENTITY,
     methods::MAP_UNWRAP_OR,
+    methods::NEEDLESS_SPLITN,
     methods::NEW_RET_NO_SELF,
     methods::OK_EXPECT,
     methods::OPTION_AS_REF_DEREF,

diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs
@@ -33,7 +33,6 @@ mod iter_nth_zero;
 mod iter_skip_next;
 mod iterator_step_by_zero;
 mod manual_saturating_arithmetic;
-mod manual_split_once;
 mod manual_str_repeat;
 mod map_collect_result_unit;
 mod map_flatten;
@@ -50,6 +49,7 @@ mod single_char_insert_string;
 mod single_char_pattern;
 mod single_char_push_string;
 mod skip_while_next;
+mod str_splitn;
 mod string_extend_chars;
 mod suspicious_map;
 mod suspicious_splitn;
@@ -1861,6 +1861,30 @@ declare_clippy_lint! {
     "replace `.splitn(2, pat)` with `.split_once(pat)`"
 }
 
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for usages of `str::splitn` (or `str::rsplitn`) where using `str::split` would be the same.
+    /// ### Why is this bad?
+    /// The function `split` is simpler and there is no performance difference in these cases, considering
+    /// that both functions return a lazy iterator.
+    /// ### Example
+    /// ```rust
+    /// // Bad
+    /// let str = "key=value=add";
+    /// let _ = str.splitn(3, '=').next().unwrap();
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// // Good
+    /// let str = "key=value=add";
+    /// let _ = str.split('=').next().unwrap();
+    /// ```
+    #[clippy::version = "1.58.0"]
+    pub NEEDLESS_SPLITN,
+    complexity,
+    "usages of `str::splitn` that can be replaced with `str::split`"
+}
+
 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,
+    NEEDLESS_SPLITN
 ]);
 
 /// Extracts a method call name, args, and `Span` of the method name.
@@ -2271,7 +2296,10 @@ fn check_methods<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Optio
                 if let Some((Constant::Int(count), _)) = constant(cx, cx.typeck_results(), count_arg) {
                     suspicious_splitn::check(cx, name, expr, recv, count);
                     if count == 2 && meets_msrv(msrv, &msrvs::STR_SPLIT_ONCE) {
-                        manual_split_once::check(cx, name, expr, recv, pat_arg);
+                        str_splitn::check_manual_split_once(cx, name, expr, recv, pat_arg);
+                    }
+                    if count >= 2 {
+                        str_splitn::check_needless_splitn(cx, name, expr, recv, pat_arg, count);
                     }
                 }
             },

diff --git a/...py_lints/src/methods/manual_split_once.rs → clippy_lints/src/methods/str_splitn.rs b/...py_lints/src/methods/manual_split_once.rs → clippy_lints/src/methods/str_splitn.rs
@@ -11,7 +11,13 @@ use rustc_span::{symbol::sym, Span, SyntaxContext};
 
 use super::MANUAL_SPLIT_ONCE;
 
-pub(super) fn check(cx: &LateContext<'_>, method_name: &str, expr: &Expr<'_>, self_arg: &Expr<'_>, pat_arg: &Expr<'_>) {
+pub(super) fn check_manual_split_once(
+    cx: &LateContext<'_>,
+    method_name: &str,
+    expr: &Expr<'_>,
+    self_arg: &Expr<'_>,
+    pat_arg: &Expr<'_>,
+) {
     if !cx.typeck_results().expr_ty_adjusted(self_arg).peel_refs().is_str() {
         return;
     }
@@ -36,7 +42,7 @@ pub(super) fn check(cx: &LateContext<'_>, method_name: &str, expr: &Expr<'_>, se
             format!("{}.{}({})", self_snip, method_name, pat_snip)
         },
         IterUsageKind::RNextTuple => format!("{}.{}({}).map(|(x, y)| (y, x))", self_snip, method_name, pat_snip),
-        IterUsageKind::Next => {
+        IterUsageKind::Next | IterUsageKind::Second => {
             let self_deref = {
                 let adjust = cx.typeck_results().expr_adjustments(self_arg);
                 if adjust.is_empty() {
@@ -51,26 +57,49 @@ pub(super) fn check(cx: &LateContext<'_>, method_name: &str, expr: &Expr<'_>, se
                     "*".repeat(adjust.len() - 2)
                 }
             };
-            if usage.unwrap_kind.is_some() {
-                format!(
-                    "{}.{}({}).map_or({}{}, |x| x.0)",
-                    &self_snip, method_name, pat_snip, self_deref, &self_snip
-                )
+            if matches!(usage.kind, IterUsageKind::Next) {
+                match usage.unwrap_kind {
+                    Some(UnwrapKind::Unwrap) => {
+                        if reverse {
+                            format!("{}.{}({}).unwrap().0", self_snip, method_name, pat_snip)
+                        } else {
+                            format!(
+                                "{}.{}({}).map_or({}{}, |x| x.0)",
+                                self_snip, method_name, pat_snip, self_deref, &self_snip
+                            )
+                        }
+                    },
+                    Some(UnwrapKind::QuestionMark) => {
+                        format!(
+                            "{}.{}({}).map_or({}{}, |x| x.0)",
+                            self_snip, method_name, pat_snip, self_deref, &self_snip
+                        )
+                    },
+                    None => {
+                        format!(
+                            "Some({}.{}({}).map_or({}{}, |x| x.0))",
+                            &self_snip, method_name, pat_snip, self_deref, &self_snip
+                        )
+                    },
+                }
             } else {
-                format!(
-                    "Some({}.{}({}).map_or({}{}, |x| x.0))",
-                    &self_snip, method_name, pat_snip, self_deref, &self_snip
-                )
+                match usage.unwrap_kind {
+                    Some(UnwrapKind::Unwrap) => {
+                        if reverse {
+                            // In this case, no better suggestion is offered.
+                            return;
+                        }
+                        format!("{}.{}({}).unwrap().1", self_snip, method_name, pat_snip)
+                    },
+                    Some(UnwrapKind::QuestionMark) => {
+                        format!("{}.{}({})?.1", self_snip, method_name, pat_snip)
+                    },
+                    None => {
+                        format!("{}.{}({}).map(|x| x.1)", self_snip, method_name, pat_snip)
+                    },
+                }
             }
         },
-        IterUsageKind::Second => {
-            let access_str = match usage.unwrap_kind {
-                Some(UnwrapKind::Unwrap) => ".unwrap().1",
-                Some(UnwrapKind::QuestionMark) => "?.1",
-                None => ".map(|x| x.1)",
-            };
-            format!("{}.{}({}){}", self_snip, method_name, pat_snip, access_str)
-        },
     };
 
     span_lint_and_sugg(cx, MANUAL_SPLIT_ONCE, usage.span, msg, "try this", sugg, app);
@@ -209,3 +238,86 @@ fn parse_iter_usage(
         span,
     })
 }
+
+use super::NEEDLESS_SPLITN;
+
+pub(super) fn check_needless_splitn(
+    cx: &LateContext<'_>,
+    method_name: &str,
+    expr: &Expr<'_>,
+    self_arg: &Expr<'_>,
+    pat_arg: &Expr<'_>,
+    count: u128,
+) {
+    if !cx.typeck_results().expr_ty_adjusted(self_arg).peel_refs().is_str() {
+        return;
+    }
+    let ctxt = expr.span.ctxt();
+    let mut app = Applicability::MachineApplicable;
+    let (reverse, message) = if method_name == "splitn" {
+        (false, "unnecessary use of `splitn`")
+    } else {
+        (true, "unnecessary use of `rsplitn`")
+    };
+    if_chain! {
+        if count >= 2;
+        if check_iter(cx, ctxt, cx.tcx.hir().parent_iter(expr.hir_id), count);
+        then {
+            span_lint_and_sugg(
+                cx,
+                NEEDLESS_SPLITN,
+                expr.span,
+                message,
+                "try this",
+                format!(
+                    "{}.{}({})",
+                    snippet_with_context(cx, self_arg.span, ctxt, "..", &mut app).0,
+                    if reverse {"rsplit"} else {"split"},
+                    snippet_with_context(cx, pat_arg.span, ctxt, "..", &mut app).0
+                ),
+                app,
+            );
+        }
+    }
+}
+
+fn check_iter(
+    cx: &LateContext<'tcx>,
+    ctxt: SyntaxContext,
+    mut iter: impl Iterator<Item = (HirId, Node<'tcx>)>,
+    count: u128,
+) -> bool {
+    match iter.next() {
+        Some((_, Node::Expr(e))) if e.span.ctxt() == ctxt => {
+            let (name, args) = if let ExprKind::MethodCall(name, _, [_, args @ ..], _) = e.kind {
+                (name, args)
+            } else {
+                return false;
+            };
+            if_chain! {
+                if let Some(did) = cx.typeck_results().type_dependent_def_id(e.hir_id);
+                if let Some(iter_id) = cx.tcx.get_diagnostic_item(sym::Iterator);
+                then {
+                    match (&*name.ident.as_str(), args) {
+                        ("next", []) if cx.tcx.trait_of_item(did) == Some(iter_id) => {
+                            return true;
+                        },
+                        ("next_tuple", []) if count > 2 => {
+                            return true;
+                        },
+                        ("nth", [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) {
+                                if count > idx + 1 {
+                                    return true;
+                                }
+                            }
+                        },
+                        _ =>  return false,
+                    }
+                }
+            }
+        },
+        _ => return false,
+    };
+    false
+}
diff --git a/tests/ui/manual_split_once.fixed b/tests/ui/manual_split_once.fixed
@@ -2,7 +2,7 @@
 
 #![feature(custom_inner_attributes)]
 #![warn(clippy::manual_split_once)]
-#![allow(clippy::iter_skip_next, clippy::iter_nth_zero)]
+#![allow(clippy::iter_skip_next, clippy::iter_nth_zero, clippy::needless_splitn)]
 
 extern crate itertools;
 
@@ -38,8 +38,8 @@ fn main() {
     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".rsplitn(2, '=').next().unwrap();
+    let _ = "key=value".rsplit_once('=').unwrap().0;
     let _ = "key=value".rsplit_once('=').map(|x| x.1);
     let (_, _) = "key=value".rsplit_once('=').map(|(x, y)| (y, x)).unwrap();
 }

diff --git a/tests/ui/manual_split_once.rs b/tests/ui/manual_split_once.rs
@@ -2,7 +2,7 @@
 
 #![feature(custom_inner_attributes)]
 #![warn(clippy::manual_split_once)]
-#![allow(clippy::iter_skip_next, clippy::iter_nth_zero)]
+#![allow(clippy::iter_skip_next, clippy::iter_nth_zero, clippy::needless_splitn)]
 
 extern crate itertools;
 

diff --git a/tests/ui/manual_split_once.stderr b/tests/ui/manual_split_once.stderr
@@ -72,17 +72,11 @@ 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)`
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `"key=value".rsplit_once('=').unwrap().0`
 
 error: manual implementation of `rsplit_once`
   --> $DIR/manual_split_once.rs:43:13
@@ -102,5 +96,5 @@ error: manual implementation of `split_once`
 LL |     let _ = "key=value".splitn(2, '=').nth(1).unwrap();
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `"key=value".split_once('=').unwrap().1`
 
-error: aborting due to 17 previous errors
+error: aborting due to 16 previous errors
 
diff --git a/tests/ui/needless_splitn.fixed b/tests/ui/needless_splitn.fixed
@@ -0,0 +1,27 @@
+// run-rustfix
+// edition:2018
+
+#![feature(custom_inner_attributes)]
+#![warn(clippy::needless_splitn)]
+#![allow(clippy::iter_skip_next, clippy::iter_nth_zero, clippy::manual_split_once)]
+
+extern crate itertools;
+
+#[allow(unused_imports)]
+use itertools::Itertools;
+
+fn main() {
+    let str = "key=value=end";
+    let _ = str.split('=').next();
+    let _ = str.split('=').nth(0);
+    let _ = str.splitn(2, '=').nth(1);
+    let (_, _) = str.splitn(2, '=').next_tuple().unwrap();
+    let (_, _) = str.split('=').next_tuple().unwrap();
+    let _: Vec<&str> = str.splitn(3, '=').collect();
+
+    let _ = str.rsplit('=').next();
+    let _ = str.rsplit('=').nth(0);
+    let _ = str.rsplitn(2, '=').nth(1);
+    let (_, _) = str.rsplitn(2, '=').next_tuple().unwrap();
+    let (_, _) = str.rsplit('=').next_tuple().unwrap();
+}
diff --git a/tests/ui/needless_splitn.rs b/tests/ui/needless_splitn.rs
@@ -0,0 +1,27 @@
+// run-rustfix
+// edition:2018
+
+#![feature(custom_inner_attributes)]
+#![warn(clippy::needless_splitn)]
+#![allow(clippy::iter_skip_next, clippy::iter_nth_zero, clippy::manual_split_once)]
+
+extern crate itertools;
+
+#[allow(unused_imports)]
+use itertools::Itertools;
+
+fn main() {
+    let str = "key=value=end";
+    let _ = str.splitn(2, '=').next();
+    let _ = str.splitn(2, '=').nth(0);
+    let _ = str.splitn(2, '=').nth(1);
+    let (_, _) = str.splitn(2, '=').next_tuple().unwrap();
+    let (_, _) = str.splitn(3, '=').next_tuple().unwrap();
+    let _: Vec<&str> = str.splitn(3, '=').collect();
+
+    let _ = str.rsplitn(2, '=').next();
+    let _ = str.rsplitn(2, '=').nth(0);
+    let _ = str.rsplitn(2, '=').nth(1);
+    let (_, _) = str.rsplitn(2, '=').next_tuple().unwrap();
+    let (_, _) = str.rsplitn(3, '=').next_tuple().unwrap();
+}