Skip to content
This repository was archived by the owner on May 28, 2025. It is now read-only.

Commit 46687f1

Browse files
borsbors
committed
Auto merge of rust-lang#7896 - surechen:fix_manual_split_once, r=camsteffen
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.
2 parents 83ad511 + c051656 commit 46687f1

14 files changed

+276
-43
lines changed

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3039,6 +3039,7 @@ Released 2018-09-13
30393039
[`needless_question_mark`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_question_mark
30403040
[`needless_range_loop`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_range_loop
30413041
[`needless_return`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_return
3042+
[`needless_splitn`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_splitn
30423043
[`needless_update`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_update
30433044
[`neg_cmp_op_on_partial_ord`]: https://rust-lang.github.io/rust-clippy/master/index.html#neg_cmp_op_on_partial_ord
30443045
[`neg_multiply`]: https://rust-lang.github.io/rust-clippy/master/index.html#neg_multiply

‎clippy_lints/src/lib.register_all.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -159,6 +159,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![
159159
LintId::of(methods::MANUAL_STR_REPEAT),
160160
LintId::of(methods::MAP_COLLECT_RESULT_UNIT),
161161
LintId::of(methods::MAP_IDENTITY),
162+
LintId::of(methods::NEEDLESS_SPLITN),
162163
LintId::of(methods::NEW_RET_NO_SELF),
163164
LintId::of(methods::OK_EXPECT),
164165
LintId::of(methods::OPTION_AS_REF_DEREF),

‎clippy_lints/src/lib.register_complexity.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@ store.register_group(true, "clippy::complexity", Some("clippy_complexity"), vec!
4242
LintId::of(methods::MANUAL_FIND_MAP),
4343
LintId::of(methods::MANUAL_SPLIT_ONCE),
4444
LintId::of(methods::MAP_IDENTITY),
45+
LintId::of(methods::NEEDLESS_SPLITN),
4546
LintId::of(methods::OPTION_AS_REF_DEREF),
4647
LintId::of(methods::OPTION_FILTER_MAP),
4748
LintId::of(methods::SEARCH_IS_SOME),

‎clippy_lints/src/lib.register_lints.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -293,6 +293,7 @@ store.register_lints(&[
293293
methods::MAP_FLATTEN,
294294
methods::MAP_IDENTITY,
295295
methods::MAP_UNWRAP_OR,
296+
methods::NEEDLESS_SPLITN,
296297
methods::NEW_RET_NO_SELF,
297298
methods::OK_EXPECT,
298299
methods::OPTION_AS_REF_DEREF,

‎clippy_lints/src/methods/mod.rs‎

Lines changed: 31 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,6 @@ mod iter_nth_zero;
3333
mod iter_skip_next;
3434
mod iterator_step_by_zero;
3535
mod manual_saturating_arithmetic;
36-
mod manual_split_once;
3736
mod manual_str_repeat;
3837
mod map_collect_result_unit;
3938
mod map_flatten;
@@ -50,6 +49,7 @@ mod single_char_insert_string;
5049
mod single_char_pattern;
5150
mod single_char_push_string;
5251
mod skip_while_next;
52+
mod str_splitn;
5353
mod string_extend_chars;
5454
mod suspicious_map;
5555
mod suspicious_splitn;
@@ -1861,6 +1861,30 @@ declare_clippy_lint! {
18611861
"replace `.splitn(2, pat)` with `.split_once(pat)`"
18621862
}
18631863

1864+
declare_clippy_lint! {
1865+
/// ### What it does
1866+
/// Checks for usages of `str::splitn` (or `str::rsplitn`) where using `str::split` would be the same.
1867+
/// ### Why is this bad?
1868+
/// The function `split` is simpler and there is no performance difference in these cases, considering
1869+
/// that both functions return a lazy iterator.
1870+
/// ### Example
1871+
/// ```rust
1872+
/// // Bad
1873+
/// let str = "key=value=add";
1874+
/// let _ = str.splitn(3, '=').next().unwrap();
1875+
/// ```
1876+
/// Use instead:
1877+
/// ```rust
1878+
/// // Good
1879+
/// let str = "key=value=add";
1880+
/// let _ = str.split('=').next().unwrap();
1881+
/// ```
1882+
#[clippy::version = "1.58.0"]
1883+
pub NEEDLESS_SPLITN,
1884+
complexity,
1885+
"usages of `str::splitn` that can be replaced with `str::split`"
1886+
}
1887+
18641888
pub struct Methods {
18651889
avoid_breaking_exported_api: bool,
18661890
msrv: Option<RustcVersion>,
@@ -1939,7 +1963,8 @@ impl_lint_pass!(Methods => [
19391963
SUSPICIOUS_SPLITN,
19401964
MANUAL_STR_REPEAT,
19411965
EXTEND_WITH_DRAIN,
1942-
MANUAL_SPLIT_ONCE
1966+
MANUAL_SPLIT_ONCE,
1967+
NEEDLESS_SPLITN
19431968
]);
19441969

19451970
/// 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
22712296
if let Some((Constant::Int(count), _)) = constant(cx, cx.typeck_results(), count_arg) {
22722297
suspicious_splitn::check(cx, name, expr, recv, count);
22732298
if count == 2 && meets_msrv(msrv, &msrvs::STR_SPLIT_ONCE) {
2274-
manual_split_once::check(cx, name, expr, recv, pat_arg);
2299+
str_splitn::check_manual_split_once(cx, name, expr, recv, pat_arg);
2300+
}
2301+
if count >= 2 {
2302+
str_splitn::check_needless_splitn(cx, name, expr, recv, pat_arg, count);
22752303
}
22762304
}
22772305
},

‎clippy_lints/src/methods/manual_split_once.rs‎ renamed to ‎clippy_lints/src/methods/str_splitn.rs‎

Lines changed: 131 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,13 @@ use rustc_span::{symbol::sym, Span, SyntaxContext};
1111

1212
use super::MANUAL_SPLIT_ONCE;
1313

14-
pub(super) fn check(cx: &LateContext<'_>, method_name: &str, expr: &Expr<'_>, self_arg: &Expr<'_>, pat_arg: &Expr<'_>) {
14+
pub(super) fn check_manual_split_once(
15+
cx: &LateContext<'_>,
16+
method_name: &str,
17+
expr: &Expr<'_>,
18+
self_arg: &Expr<'_>,
19+
pat_arg: &Expr<'_>,
20+
) {
1521
if !cx.typeck_results().expr_ty_adjusted(self_arg).peel_refs().is_str() {
1622
return;
1723
}
@@ -36,7 +42,7 @@ pub(super) fn check(cx: &LateContext<'_>, method_name: &str, expr: &Expr<'_>, se
3642
format!("{}.{}({})", self_snip, method_name, pat_snip)
3743
},
3844
IterUsageKind::RNextTuple => format!("{}.{}({}).map(|(x, y)| (y, x))", self_snip, method_name, pat_snip),
39-
IterUsageKind::Next => {
45+
IterUsageKind::Next | IterUsageKind::Second => {
4046
let self_deref = {
4147
let adjust = cx.typeck_results().expr_adjustments(self_arg);
4248
if adjust.is_empty() {
@@ -51,26 +57,49 @@ pub(super) fn check(cx: &LateContext<'_>, method_name: &str, expr: &Expr<'_>, se
5157
"*".repeat(adjust.len() - 2)
5258
}
5359
};
54-
if usage.unwrap_kind.is_some() {
55-
format!(
56-
"{}.{}({}).map_or({}{}, |x| x.0)",
57-
&self_snip, method_name, pat_snip, self_deref, &self_snip
58-
)
60+
if matches!(usage.kind, IterUsageKind::Next) {
61+
match usage.unwrap_kind {
62+
Some(UnwrapKind::Unwrap) => {
63+
if reverse {
64+
format!("{}.{}({}).unwrap().0", self_snip, method_name, pat_snip)
65+
} else {
66+
format!(
67+
"{}.{}({}).map_or({}{}, |x| x.0)",
68+
self_snip, method_name, pat_snip, self_deref, &self_snip
69+
)
70+
}
71+
},
72+
Some(UnwrapKind::QuestionMark) => {
73+
format!(
74+
"{}.{}({}).map_or({}{}, |x| x.0)",
75+
self_snip, method_name, pat_snip, self_deref, &self_snip
76+
)
77+
},
78+
None => {
79+
format!(
80+
"Some({}.{}({}).map_or({}{}, |x| x.0))",
81+
&self_snip, method_name, pat_snip, self_deref, &self_snip
82+
)
83+
},
84+
}
5985
} else {
60-
format!(
61-
"Some({}.{}({}).map_or({}{}, |x| x.0))",
62-
&self_snip, method_name, pat_snip, self_deref, &self_snip
63-
)
86+
match usage.unwrap_kind {
87+
Some(UnwrapKind::Unwrap) => {
88+
if reverse {
89+
// In this case, no better suggestion is offered.
90+
return;
91+
}
92+
format!("{}.{}({}).unwrap().1", self_snip, method_name, pat_snip)
93+
},
94+
Some(UnwrapKind::QuestionMark) => {
95+
format!("{}.{}({})?.1", self_snip, method_name, pat_snip)
96+
},
97+
None => {
98+
format!("{}.{}({}).map(|x| x.1)", self_snip, method_name, pat_snip)
99+
},
100+
}
64101
}
65102
},
66-
IterUsageKind::Second => {
67-
let access_str = match usage.unwrap_kind {
68-
Some(UnwrapKind::Unwrap) => ".unwrap().1",
69-
Some(UnwrapKind::QuestionMark) => "?.1",
70-
None => ".map(|x| x.1)",
71-
};
72-
format!("{}.{}({}){}", self_snip, method_name, pat_snip, access_str)
73-
},
74103
};
75104

76105
span_lint_and_sugg(cx, MANUAL_SPLIT_ONCE, usage.span, msg, "try this", sugg, app);
@@ -209,3 +238,86 @@ fn parse_iter_usage(
209238
span,
210239
})
211240
}
241+
242+
use super::NEEDLESS_SPLITN;
243+
244+
pub(super) fn check_needless_splitn(
245+
cx: &LateContext<'_>,
246+
method_name: &str,
247+
expr: &Expr<'_>,
248+
self_arg: &Expr<'_>,
249+
pat_arg: &Expr<'_>,
250+
count: u128,
251+
) {
252+
if !cx.typeck_results().expr_ty_adjusted(self_arg).peel_refs().is_str() {
253+
return;
254+
}
255+
let ctxt = expr.span.ctxt();
256+
let mut app = Applicability::MachineApplicable;
257+
let (reverse, message) = if method_name == "splitn" {
258+
(false, "unnecessary use of `splitn`")
259+
} else {
260+
(true, "unnecessary use of `rsplitn`")
261+
};
262+
if_chain! {
263+
if count >= 2;
264+
if check_iter(cx, ctxt, cx.tcx.hir().parent_iter(expr.hir_id), count);
265+
then {
266+
span_lint_and_sugg(
267+
cx,
268+
NEEDLESS_SPLITN,
269+
expr.span,
270+
message,
271+
"try this",
272+
format!(
273+
"{}.{}({})",
274+
snippet_with_context(cx, self_arg.span, ctxt, "..", &mut app).0,
275+
if reverse {"rsplit"} else {"split"},
276+
snippet_with_context(cx, pat_arg.span, ctxt, "..", &mut app).0
277+
),
278+
app,
279+
);
280+
}
281+
}
282+
}
283+
284+
fn check_iter(
285+
cx: &LateContext<'tcx>,
286+
ctxt: SyntaxContext,
287+
mut iter: impl Iterator<Item = (HirId, Node<'tcx>)>,
288+
count: u128,
289+
) -> bool {
290+
match iter.next() {
291+
Some((_, Node::Expr(e))) if e.span.ctxt() == ctxt => {
292+
let (name, args) = if let ExprKind::MethodCall(name, _, [_, args @ ..], _) = e.kind {
293+
(name, args)
294+
} else {
295+
return false;
296+
};
297+
if_chain! {
298+
if let Some(did) = cx.typeck_results().type_dependent_def_id(e.hir_id);
299+
if let Some(iter_id) = cx.tcx.get_diagnostic_item(sym::Iterator);
300+
then {
301+
match (&*name.ident.as_str(), args) {
302+
("next", []) if cx.tcx.trait_of_item(did) == Some(iter_id) => {
303+
return true;
304+
},
305+
("next_tuple", []) if count > 2 => {
306+
return true;
307+
},
308+
("nth", [idx_expr]) if cx.tcx.trait_of_item(did) == Some(iter_id) => {
309+
if let Some((Constant::Int(idx), _)) = constant(cx, cx.typeck_results(), idx_expr) {
310+
if count > idx + 1 {
311+
return true;
312+
}
313+
}
314+
},
315+
_ => return false,
316+
}
317+
}
318+
}
319+
},
320+
_ => return false,
321+
};
322+
false
323+
}

‎tests/ui/manual_split_once.fixed‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22

33
#![feature(custom_inner_attributes)]
44
#![warn(clippy::manual_split_once)]
5-
#![allow(clippy::iter_skip_next, clippy::iter_nth_zero)]
5+
#![allow(clippy::iter_skip_next, clippy::iter_nth_zero, clippy::needless_splitn)]
66

77
extern crate itertools;
88

@@ -38,8 +38,8 @@ fn main() {
3838
let _ = [0, 1, 2].splitn(2, |&x| x == 1).nth(1).unwrap();
3939

4040
// `rsplitn` gives the results in the reverse order of `rsplit_once`
41-
let _ = "key=value".rsplit_once('=').unwrap().1;
42-
let _ = "key=value".rsplit_once('=').map_or("key=value", |x| x.0);
41+
let _ = "key=value".rsplitn(2, '=').next().unwrap();
42+
let _ = "key=value".rsplit_once('=').unwrap().0;
4343
let _ = "key=value".rsplit_once('=').map(|x| x.1);
4444
let (_, _) = "key=value".rsplit_once('=').map(|(x, y)| (y, x)).unwrap();
4545
}

‎tests/ui/manual_split_once.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22

33
#![feature(custom_inner_attributes)]
44
#![warn(clippy::manual_split_once)]
5-
#![allow(clippy::iter_skip_next, clippy::iter_nth_zero)]
5+
#![allow(clippy::iter_skip_next, clippy::iter_nth_zero, clippy::needless_splitn)]
66

77
extern crate itertools;
88

‎tests/ui/manual_split_once.stderr‎

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -72,17 +72,11 @@ error: manual implementation of `split_once`
7272
LL | let _ = s.splitn(2, "key=value").skip(1).next()?;
7373
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `s.split_once("key=value")?.1`
7474

75-
error: manual implementation of `rsplit_once`
76-
--> $DIR/manual_split_once.rs:41:13
77-
|
78-
LL | let _ = "key=value".rsplitn(2, '=').next().unwrap();
79-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `"key=value".rsplit_once('=').unwrap().1`
80-
8175
error: manual implementation of `rsplit_once`
8276
--> $DIR/manual_split_once.rs:42:13
8377
|
8478
LL | let _ = "key=value".rsplitn(2, '=').nth(1).unwrap();
85-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `"key=value".rsplit_once('=').map_or("key=value", |x| x.0)`
79+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `"key=value".rsplit_once('=').unwrap().0`
8680

8781
error: manual implementation of `rsplit_once`
8882
--> $DIR/manual_split_once.rs:43:13
@@ -102,5 +96,5 @@ error: manual implementation of `split_once`
10296
LL | let _ = "key=value".splitn(2, '=').nth(1).unwrap();
10397
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `"key=value".split_once('=').unwrap().1`
10498

105-
error: aborting due to 17 previous errors
99+
error: aborting due to 16 previous errors
106100

‎tests/ui/needless_splitn.fixed‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
// run-rustfix
2+
// edition:2018
3+
4+
#![feature(custom_inner_attributes)]
5+
#![warn(clippy::needless_splitn)]
6+
#![allow(clippy::iter_skip_next, clippy::iter_nth_zero, clippy::manual_split_once)]
7+
8+
extern crate itertools;
9+
10+
#[allow(unused_imports)]
11+
use itertools::Itertools;
12+
13+
fn main() {
14+
let str = "key=value=end";
15+
let _ = str.split('=').next();
16+
let _ = str.split('=').nth(0);
17+
let _ = str.splitn(2, '=').nth(1);
18+
let (_, _) = str.splitn(2, '=').next_tuple().unwrap();
19+
let (_, _) = str.split('=').next_tuple().unwrap();
20+
let _: Vec<&str> = str.splitn(3, '=').collect();
21+
22+
let _ = str.rsplit('=').next();
23+
let _ = str.rsplit('=').nth(0);
24+
let _ = str.rsplitn(2, '=').nth(1);
25+
let (_, _) = str.rsplitn(2, '=').next_tuple().unwrap();
26+
let (_, _) = str.rsplit('=').next_tuple().unwrap();
27+
}

0 commit comments

Comments
 (0)