nordic-dev.net/rust
nordic-dev.net/rust
2
0
Fork 0
mirror of https://github.com/rust-lang/rust.git synced 2024-12-14 17:48:10 +00:00
Code Issues Packages Projects Releases Wiki Activity

Auto merge of #8717 - Alexendoo:manual-split-once-manual-iter, r=dswij,xFrednet

Browse Source 
`manual_split_once`: lint manual iteration of `SplitN`

changelog: `manual_split_once`: lint manual iteration of `SplitN`

Now lints:

```rust
let mut iter = "a.b.c".splitn(2, '.');
let first = iter.next().unwrap();
let second = iter.next().unwrap();

let mut iter = "a.b.c".splitn(2, '.');
let first = iter.next()?;
let second = iter.next()?;

let mut iter = "a.b.c".rsplitn(2, '.');
let first = iter.next().unwrap();
let second = iter.next().unwrap();

let mut iter = "a.b.c".rsplitn(2, '.');
let first = iter.next()?;
let second = iter.next()?;
```

It suggests (minus leftover whitespace):

```rust
let (first, second) = "a.b.c".split_once('.').unwrap();

let (first, second) = "a.b.c".split_once('.')?;

let (second, first) = "a.b.c".rsplit_once('.').unwrap();

let (second, first) = "a.b.c".rsplit_once('.')?;
```

Currently only lints if the statements are next to each other, as detecting the various kinds of shadowing was tricky, so the following won't lint

```rust
let mut iter = "a.b.c".splitn(2, '.');
let something_else = 1;
let first = iter.next()?;
let second = iter.next()?;
```
This commit is contained in:
bors 2022-04-22 09:57:00 +00:00
commit ed22428b72
5 changed files with 528 additions and 40 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

24
clippy_lints/src/methods/mod.rs
View File

@ -2009,13 +2009,27 @@ declare_clippy_lint! {
/// ### Example
/// ```rust,ignore
/// // Bad
/// let (key, value) = _.splitn(2, '=').next_tuple()?;
/// let value = _.splitn(2, '=').nth(1)?;
/// let s = "key=value=add";
/// let (key, value) = s.splitn(2, '=').next_tuple()?;
/// let value = s.splitn(2, '=').nth(1)?;
///
/// // Good
/// let (key, value) = _.split_once('=')?;
/// let value = _.split_once('=')?.1;
/// let mut parts = s.splitn(2, '=');
/// let key = parts.next()?;
/// let value = parts.next()?;
/// ```
/// Use instead:
/// ```rust,ignore
/// // Good
/// let s = "key=value=add";
/// let (key, value) = s.split_once('=')?;
/// let value = s.split_once('=')?.1;
///
/// let (key, value) = s.split_once('=')?;
/// ```
///
/// ### Limitations
/// The multiple statement variant currently only detects `iter.next()?`/`iter.next().unwrap()`
/// in two separate `let` statements that immediately follow the `splitn()`
#[clippy::version = "1.57.0"]
pub MANUAL_SPLIT_ONCE,
complexity,

223
clippy_lints/src/methods/str_splitn.rs
View File

@ -1,14 +1,19 @@
use clippy_utils::consts::{constant, Constant};
use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then};
use clippy_utils::source::snippet_with_context;
use clippy_utils::{is_diag_item_method, match_def_path, meets_msrv, msrvs, paths};
use clippy_utils::usage::local_used_after_expr;
use clippy_utils::visitors::expr_visitor;
use clippy_utils::{is_diag_item_method, match_def_path, meets_msrv, msrvs, path_to_local_id, paths};
use if_chain::if_chain;
use rustc_errors::Applicability;
use rustc_hir::{Expr, ExprKind, HirId, LangItem, Node, QPath};
use rustc_hir::intravisit::Visitor;
use rustc_hir::{
BindingAnnotation, Expr, ExprKind, HirId, LangItem, Local, MatchSource, Node, Pat, PatKind, QPath, Stmt, StmtKind,
};
use rustc_lint::LateContext;
use rustc_middle::ty;
use rustc_semver::RustcVersion;
use rustc_span::{symbol::sym, Span, SyntaxContext};
use rustc_span::{sym, Span, Symbol, SyntaxContext};
use super::{MANUAL_SPLIT_ONCE, NEEDLESS_SPLITN};
@ -25,40 +30,41 @@ pub(super) fn check(
return;
}
let ctxt = expr.span.ctxt();
let Some(usage) = parse_iter_usage(cx, ctxt, cx.tcx.hir().parent_iter(expr.hir_id)) else { return };
let needless = match usage.kind {
let needless = |usage_kind| match usage_kind {
IterUsageKind::Nth(n) => count > n + 1,
IterUsageKind::NextTuple => count > 2,
};
let manual = count == 2 && meets_msrv(msrv, &msrvs::STR_SPLIT_ONCE);
if needless {
let mut app = Applicability::MachineApplicable;
let (r, message) = if method_name == "splitn" {
("", "unnecessary use of `splitn`")
} else {
("r", "unnecessary use of `rsplitn`")
};
span_lint_and_sugg(
cx,
NEEDLESS_SPLITN,
expr.span,
message,
"try this",
format!(
"{}.{r}split({})",
snippet_with_context(cx, self_arg.span, ctxt, "..", &mut app).0,
snippet_with_context(cx, pat_arg.span, ctxt, "..", &mut app).0,
),
app,
);
} else if count == 2 && meets_msrv(msrv, &msrvs::STR_SPLIT_ONCE) {
check_manual_split_once(cx, method_name, expr, self_arg, pat_arg, &usage);
match parse_iter_usage(cx, expr.span.ctxt(), cx.tcx.hir().parent_iter(expr.hir_id)) {
Some(usage) if needless(usage.kind) => lint_needless(cx, method_name, expr, self_arg, pat_arg),
Some(usage) if manual => check_manual_split_once(cx, method_name, expr, self_arg, pat_arg, &usage),
None if manual => {
check_manual_split_once_indirect(cx, method_name, expr, self_arg, pat_arg);
},
_ => {},
}
}
fn lint_needless(cx: &LateContext<'_>, method_name: &str, expr: &Expr<'_>, self_arg: &Expr<'_>, pat_arg: &Expr<'_>) {
let mut app = Applicability::MachineApplicable;
let r = if method_name == "splitn" { "" } else { "r" };
span_lint_and_sugg(
cx,
NEEDLESS_SPLITN,
expr.span,
&format!("unnecessary use of `{r}splitn`"),
"try this",
format!(
"{}.{r}split({})",
snippet_with_context(cx, self_arg.span, expr.span.ctxt(), "..", &mut app).0,
snippet_with_context(cx, pat_arg.span, expr.span.ctxt(), "..", &mut app).0,
),
app,
);
}
fn check_manual_split_once(
cx: &LateContext<'_>,
method_name: &str,
@ -107,16 +113,171 @@ fn check_manual_split_once(
span_lint_and_sugg(cx, MANUAL_SPLIT_ONCE, usage.span, msg, "try this", sugg, app);
}
/// checks for
///
/// ```
/// let mut iter = "a.b.c".splitn(2, '.');
/// let a = iter.next();
/// let b = iter.next();
/// ```
fn check_manual_split_once_indirect(
cx: &LateContext<'_>,
method_name: &str,
expr: &Expr<'_>,
self_arg: &Expr<'_>,
pat_arg: &Expr<'_>,
) -> Option<()> {
let ctxt = expr.span.ctxt();
let mut parents = cx.tcx.hir().parent_iter(expr.hir_id);
if let (_, Node::Local(local)) = parents.next()?
&& let PatKind::Binding(BindingAnnotation::Mutable, iter_binding_id, iter_ident, None) = local.pat.kind
&& let (iter_stmt_id, Node::Stmt(_)) = parents.next()?
&& let (_, Node::Block(enclosing_block)) = parents.next()?
&& let mut stmts = enclosing_block
.stmts
.iter()
.skip_while(|stmt| stmt.hir_id != iter_stmt_id)
.skip(1)
&& let first = indirect_usage(cx, stmts.next()?, iter_binding_id, ctxt)?
&& let second = indirect_usage(cx, stmts.next()?, iter_binding_id, ctxt)?
&& first.unwrap_kind == second.unwrap_kind
&& first.name != second.name
&& !local_used_after_expr(cx, iter_binding_id, second.init_expr)
{
let (r, lhs, rhs) = if method_name == "splitn" {
("", first.name, second.name)
} else {
("r", second.name, first.name)
};
let msg = format!("manual implementation of `{r}split_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;
span_lint_and_then(cx, MANUAL_SPLIT_ONCE, local.span, &msg, |diag| {
diag.span_label(first.span, "first usage here");
diag.span_label(second.span, "second usage here");
let unwrap = match first.unwrap_kind {
UnwrapKind::Unwrap => ".unwrap()",
UnwrapKind::QuestionMark => "?",
};
diag.span_suggestion_verbose(
local.span,
&format!("try `{r}split_once`"),
format!("let ({lhs}, {rhs}) = {self_snip}.{r}split_once({pat_snip}){unwrap};"),
app,
);
let remove_msg = format!("remove the `{iter_ident}` usages");
diag.span_suggestion(
first.span,
&remove_msg,
String::new(),
app,
);
diag.span_suggestion(
second.span,
&remove_msg,
String::new(),
app,
);
});
}
Some(())
}
#[derive(Debug)]
struct IndirectUsage<'a> {
name: Symbol,
span: Span,
init_expr: &'a Expr<'a>,
unwrap_kind: UnwrapKind,
}
/// returns `Some(IndirectUsage)` for e.g.
///
/// ```ignore
/// let name = binding.next()?;
/// let name = binding.next().unwrap();
/// ```
fn indirect_usage<'tcx>(
cx: &LateContext<'tcx>,
stmt: &Stmt<'tcx>,
binding: HirId,
ctxt: SyntaxContext,
) -> Option<IndirectUsage<'tcx>> {
if let StmtKind::Local(Local {
pat:
Pat {
kind: PatKind::Binding(BindingAnnotation::Unannotated, _, ident, None),
..
},
init: Some(init_expr),
hir_id: local_hir_id,
..
}) = stmt.kind
{
let mut path_to_binding = None;
expr_visitor(cx, |expr| {
if path_to_local_id(expr, binding) {
path_to_binding = Some(expr);
}
path_to_binding.is_none()
})
.visit_expr(init_expr);
let mut parents = cx.tcx.hir().parent_iter(path_to_binding?.hir_id);
let iter_usage = parse_iter_usage(cx, ctxt, &mut parents)?;
let (parent_id, _) = parents.find(|(_, node)| {
!matches!(
node,
Node::Expr(Expr {
kind: ExprKind::Match(.., MatchSource::TryDesugar),
..
})
)
})?;
if let IterUsage {
kind: IterUsageKind::Nth(0),
unwrap_kind: Some(unwrap_kind),
..
} = iter_usage
{
if parent_id == *local_hir_id {
return Some(IndirectUsage {
name: ident.name,
span: stmt.span,
init_expr,
unwrap_kind,
});
}
}
}
None
}
#[derive(Debug, Clone, Copy)]
enum IterUsageKind {
Nth(u128),
NextTuple,
}
#[derive(Debug, PartialEq)]
enum UnwrapKind {
Unwrap,
QuestionMark,
}
#[derive(Debug)]
struct IterUsage {
kind: IterUsageKind,
unwrap_kind: Option<UnwrapKind>,

96
tests/ui/manual_split_once.fixed
View File

@ -2,7 +2,7 @@
#![feature(custom_inner_attributes)]
#![warn(clippy::manual_split_once)]
#![allow(clippy::iter_skip_next, clippy::iter_nth_zero)]
#![allow(unused, clippy::iter_skip_next, clippy::iter_nth_zero)]
extern crate itertools;
@ -41,13 +41,107 @@ fn main() {
let _ = s.rsplit_once('=').map(|x| x.0);
}
fn indirect() -> Option<()> {
let (l, r) = "a.b.c".split_once('.').unwrap();
let (l, r) = "a.b.c".split_once('.')?;
let (l, r) = "a.b.c".rsplit_once('.').unwrap();
let (l, r) = "a.b.c".rsplit_once('.')?;
// could lint, currently doesn't
let mut iter = "a.b.c".splitn(2, '.');
let other = 1;
let l = iter.next()?;
let r = iter.next()?;
let mut iter = "a.b.c".splitn(2, '.');
let mut mut_binding = iter.next()?;
let r = iter.next()?;
let mut iter = "a.b.c".splitn(2, '.');
let tuple = (iter.next()?, iter.next()?);
// should not lint
let mut missing_unwrap = "a.b.c".splitn(2, '.');
let l = missing_unwrap.next();
let r = missing_unwrap.next();
let mut mixed_unrap = "a.b.c".splitn(2, '.');
let unwrap = mixed_unrap.next().unwrap();
let question_mark = mixed_unrap.next()?;
let mut iter = "a.b.c".splitn(2, '.');
let same_name = iter.next()?;
let same_name = iter.next()?;
let mut iter = "a.b.c".splitn(2, '.');
let shadows_existing = "d";
let shadows_existing = iter.next()?;
let r = iter.next()?;
let mut iter = "a.b.c".splitn(2, '.');
let becomes_shadowed = iter.next()?;
let becomes_shadowed = "d";
let r = iter.next()?;
let mut iter = "a.b.c".splitn(2, '.');
let l = iter.next()?;
let r = iter.next()?;
let third_usage = iter.next()?;
let mut n_three = "a.b.c".splitn(3, '.');
let l = n_three.next()?;
let r = n_three.next()?;
let mut iter = "a.b.c".splitn(2, '.');
{
let in_block = iter.next()?;
}
let r = iter.next()?;
let mut lacks_binding = "a.b.c".splitn(2, '.');
let _ = lacks_binding.next()?;
let r = lacks_binding.next()?;
let mut mapped = "a.b.c".splitn(2, '.').map(|_| "~");
let l = iter.next()?;
let r = iter.next()?;
let mut assigned = "";
let mut iter = "a.b.c".splitn(2, '.');
let l = iter.next()?;
assigned = iter.next()?;
None
}
fn _msrv_1_51() {
#![clippy::msrv = "1.51"]
// `str::split_once` was stabilized in 1.52. Do not lint this
let _ = "key=value".splitn(2, '=').nth(1).unwrap();
let mut iter = "a.b.c".splitn(2, '.');
let a = iter.next().unwrap();
let b = iter.next().unwrap();
}
fn _msrv_1_52() {
#![clippy::msrv = "1.52"]
let _ = "key=value".split_once('=').unwrap().1;
let (a, b) = "a.b.c".split_once('.').unwrap();
}

96
tests/ui/manual_split_once.rs
View File

@ -2,7 +2,7 @@
#![feature(custom_inner_attributes)]
#![warn(clippy::manual_split_once)]
#![allow(clippy::iter_skip_next, clippy::iter_nth_zero)]
#![allow(unused, clippy::iter_skip_next, clippy::iter_nth_zero)]
extern crate itertools;
@ -41,13 +41,107 @@ fn main() {
let _ = s.rsplitn(2, '=').nth(1);
}
fn indirect() -> Option<()> {
let mut iter = "a.b.c".splitn(2, '.');
let l = iter.next().unwrap();
let r = iter.next().unwrap();
let mut iter = "a.b.c".splitn(2, '.');
let l = iter.next()?;
let r = iter.next()?;
let mut iter = "a.b.c".rsplitn(2, '.');
let r = iter.next().unwrap();
let l = iter.next().unwrap();
let mut iter = "a.b.c".rsplitn(2, '.');
let r = iter.next()?;
let l = iter.next()?;
// could lint, currently doesn't
let mut iter = "a.b.c".splitn(2, '.');
let other = 1;
let l = iter.next()?;
let r = iter.next()?;
let mut iter = "a.b.c".splitn(2, '.');
let mut mut_binding = iter.next()?;
let r = iter.next()?;
let mut iter = "a.b.c".splitn(2, '.');
let tuple = (iter.next()?, iter.next()?);
// should not lint
let mut missing_unwrap = "a.b.c".splitn(2, '.');
let l = missing_unwrap.next();
let r = missing_unwrap.next();
let mut mixed_unrap = "a.b.c".splitn(2, '.');
let unwrap = mixed_unrap.next().unwrap();
let question_mark = mixed_unrap.next()?;
let mut iter = "a.b.c".splitn(2, '.');
let same_name = iter.next()?;
let same_name = iter.next()?;
let mut iter = "a.b.c".splitn(2, '.');
let shadows_existing = "d";
let shadows_existing = iter.next()?;
let r = iter.next()?;
let mut iter = "a.b.c".splitn(2, '.');
let becomes_shadowed = iter.next()?;
let becomes_shadowed = "d";
let r = iter.next()?;
let mut iter = "a.b.c".splitn(2, '.');
let l = iter.next()?;
let r = iter.next()?;
let third_usage = iter.next()?;
let mut n_three = "a.b.c".splitn(3, '.');
let l = n_three.next()?;
let r = n_three.next()?;
let mut iter = "a.b.c".splitn(2, '.');
{
let in_block = iter.next()?;
}
let r = iter.next()?;
let mut lacks_binding = "a.b.c".splitn(2, '.');
let _ = lacks_binding.next()?;
let r = lacks_binding.next()?;
let mut mapped = "a.b.c".splitn(2, '.').map(|_| "~");
let l = iter.next()?;
let r = iter.next()?;
let mut assigned = "";
let mut iter = "a.b.c".splitn(2, '.');
let l = iter.next()?;
assigned = iter.next()?;
None
}
fn _msrv_1_51() {
#![clippy::msrv = "1.51"]
// `str::split_once` was stabilized in 1.52. Do not lint this
let _ = "key=value".splitn(2, '=').nth(1).unwrap();
let mut iter = "a.b.c".splitn(2, '.');
let a = iter.next().unwrap();
let b = iter.next().unwrap();
}
fn _msrv_1_52() {
#![clippy::msrv = "1.52"]
let _ = "key=value".splitn(2, '=').nth(1).unwrap();
let mut iter = "a.b.c".splitn(2, '.');
let a = iter.next().unwrap();
let b = iter.next().unwrap();
}

129
tests/ui/manual_split_once.stderr
View File

@ -79,10 +79,135 @@ LL | let _ = s.rsplitn(2, '=').nth(1);
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `s.rsplit_once('=').map(|x| x.0)`
error: manual implementation of `split_once`
--> $DIR/manual_split_once.rs:52:13
--> $DIR/manual_split_once.rs:45:5
|
LL | let mut iter = "a.b.c".splitn(2, '.');
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
LL | let l = iter.next().unwrap();
| ----------------------------- first usage here
LL | let r = iter.next().unwrap();
| ----------------------------- second usage here
|
help: try `split_once`
|
LL | let (l, r) = "a.b.c".split_once('.').unwrap();
|
help: remove the `iter` usages
|
LL - let l = iter.next().unwrap();
LL +
|
help: remove the `iter` usages
|
LL - let r = iter.next().unwrap();
LL +
|
error: manual implementation of `split_once`
--> $DIR/manual_split_once.rs:49:5
|
LL | let mut iter = "a.b.c".splitn(2, '.');
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
LL | let l = iter.next()?;
| --------------------- first usage here
LL | let r = iter.next()?;
| --------------------- second usage here
|
help: try `split_once`
|
LL | let (l, r) = "a.b.c".split_once('.')?;
|
help: remove the `iter` usages
|
LL - let l = iter.next()?;
LL +
|
help: remove the `iter` usages
|
LL - let r = iter.next()?;
LL +
|
error: manual implementation of `rsplit_once`
--> $DIR/manual_split_once.rs:53:5
|
LL | let mut iter = "a.b.c".rsplitn(2, '.');
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
LL | let r = iter.next().unwrap();
| ----------------------------- first usage here
LL | let l = iter.next().unwrap();
| ----------------------------- second usage here
|
help: try `rsplit_once`
|
LL | let (l, r) = "a.b.c".rsplit_once('.').unwrap();
|
help: remove the `iter` usages
|
LL - let r = iter.next().unwrap();
LL +
|
help: remove the `iter` usages
|
LL - let l = iter.next().unwrap();
LL +
|
error: manual implementation of `rsplit_once`
--> $DIR/manual_split_once.rs:57:5
|
LL | let mut iter = "a.b.c".rsplitn(2, '.');
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
LL | let r = iter.next()?;
| --------------------- first usage here
LL | let l = iter.next()?;
| --------------------- second usage here
|
help: try `rsplit_once`
|
LL | let (l, r) = "a.b.c".rsplit_once('.')?;
|
help: remove the `iter` usages
|
LL - let r = iter.next()?;
LL +
|
help: remove the `iter` usages
|
LL - let l = iter.next()?;
LL +
|
error: manual implementation of `split_once`
--> $DIR/manual_split_once.rs:142:13
|
LL | let _ = "key=value".splitn(2, '=').nth(1).unwrap();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `"key=value".split_once('=').unwrap().1`
error: aborting due to 14 previous errors
error: manual implementation of `split_once`
--> $DIR/manual_split_once.rs:144:5
|
LL | let mut iter = "a.b.c".splitn(2, '.');
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
LL | let a = iter.next().unwrap();
| ----------------------------- first usage here
LL | let b = iter.next().unwrap();
| ----------------------------- second usage here
|
help: try `split_once`
|
LL | let (a, b) = "a.b.c".split_once('.').unwrap();
|
help: remove the `iter` usages
|
LL - let a = iter.next().unwrap();
LL +
|
help: remove the `iter` usages
|
LL - let b = iter.next().unwrap();
LL +
|
error: aborting due to 19 previous errors