Skip to content

Commit

Permalink
Auto merge of rust-lang#7663 - Jarcho:rsplit_once_order, r=llogiq
Browse files Browse the repository at this point in the history 
Fix result order for `manual_split_once` when `rsplitn` is used

fixes: rust-lang#7656

changelog: Fix result order for `manual_split_once` when `rsplitn` is used
  • Loading branch information
@bors
bors committed Sep 11, 2021
2 parents e8cd4e5 + 4c1b6a2 commit 2636a1b
Show file tree
Hide file tree
Showing 4 changed files with 72 additions and 38 deletions.
70 changes: 34 additions & 36 deletions clippy_lints/src/methods/manual_split_once.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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
Expand All @@ -68,33 +61,26 @@ 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 {
Some(UnwrapKind::Unwrap) => ".unwrap().1",
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 {
Expand All @@ -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<Item = (HirId, Node<'tcx>)>,
reverse: bool,
) -> Option<IterUsage> {
let (kind, span) = match iter.next() {
Some((_, Node::Expr(e))) if e.span.ctxt() == ctxt => {
Expand All @@ -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) {
Expand All @@ -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,
Expand Down
6 changes: 6 additions & 0 deletions tests/ui/manual_split_once.fixed
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
6 changes: 6 additions & 0 deletions tests/ui/manual_split_once.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
28 changes: 26 additions & 2 deletions tests/ui/manual_split_once.stderr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

0 comments on commit 2636a1b

Please sign in to comment.