Skip to content

Commit

Permalink
WIP - implement iter next slice
Browse files Browse the repository at this point in the history
  • Loading branch information
esamudera committed May 14, 2020
1 parent 7147068 commit 71f51ad
Show file tree
Hide file tree
Showing 12 changed files with 177 additions and 22 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -1561,6 +1561,7 @@ Released 2018-09-13
[`single_match`]: https://rust-lang.github.io/rust-clippy/master/index.html#single_match
[`single_match_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#single_match_else
[`skip_while_next`]: https://rust-lang.github.io/rust-clippy/master/index.html#skip_while_next
[`iter_next_slice`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_next_slice
[`slow_vector_initialization`]: https://rust-lang.github.io/rust-clippy/master/index.html#slow_vector_initialization
[`str_to_string`]: https://rust-lang.github.io/rust-clippy/master/index.html#str_to_string
[`string_add`]: https://rust-lang.github.io/rust-clippy/master/index.html#string_add
Expand Down
3 changes: 3 additions & 0 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -670,6 +670,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
&methods::INTO_ITER_ON_REF,
&methods::ITERATOR_STEP_BY_ZERO,
&methods::ITER_CLONED_COLLECT,
&methods::ITER_NEXT_SLICE,
&methods::ITER_NTH,
&methods::ITER_NTH_ZERO,
&methods::ITER_SKIP_NEXT,
Expand Down Expand Up @@ -1330,6 +1331,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
LintId::of(&methods::SHOULD_IMPLEMENT_TRAIT),
LintId::of(&methods::SINGLE_CHAR_PATTERN),
LintId::of(&methods::SKIP_WHILE_NEXT),
LintId::of(&methods::ITER_NEXT_SLICE),
LintId::of(&methods::STRING_EXTEND_CHARS),
LintId::of(&methods::SUSPICIOUS_MAP),
LintId::of(&methods::TEMPORARY_CSTRING_AS_PTR),
Expand Down Expand Up @@ -1505,6 +1507,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
LintId::of(&methods::OPTION_MAP_OR_NONE),
LintId::of(&methods::RESULT_MAP_OR_INTO_OPTION),
LintId::of(&methods::SHOULD_IMPLEMENT_TRAIT),
LintId::of(&methods::ITER_NEXT_SLICE),
LintId::of(&methods::STRING_EXTEND_CHARS),
LintId::of(&methods::UNNECESSARY_FOLD),
LintId::of(&methods::WRONG_SELF_CONVENTION),
Expand Down
16 changes: 14 additions & 2 deletions clippy_lints/src/loops.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2487,14 +2487,14 @@ fn check_needless_collect<'a, 'tcx>(expr: &'tcx Expr<'_>, cx: &LateContext<'a, '
);
}
if method.ident.name == sym!(is_empty) {
let span = shorten_needless_collect_span(expr);
let span = shorten_needless_collect_span_to_iter(expr);
span_lint_and_sugg(
cx,
NEEDLESS_COLLECT,
span,
NEEDLESS_COLLECT_MSG,
"replace with",
".next().is_none()".to_string(),
".get(0).is_none()".to_string(),
Applicability::MachineApplicable,
);
}
Expand Down Expand Up @@ -2539,3 +2539,15 @@ fn shorten_needless_collect_span(expr: &Expr<'_>) -> Span {
}
unreachable!()
}

fn shorten_needless_collect_span_to_iter(expr: &Expr<'_>) -> Span {
if_chain! {
if let ExprKind::MethodCall(_, _, ref args) = expr.kind;
if let ExprKind::MethodCall(_, _, ref args2) = args[0].kind;
if let ExprKind::MethodCall(_, ref span, _) = args2[0].kind;
then {
return expr.span.with_lo(span.lo() - BytePos(1));
}
}
unreachable!()
}
67 changes: 66 additions & 1 deletion clippy_lints/src/methods/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ use rustc_span::symbol::{sym, SymbolStr};
use crate::consts::{constant, Constant};
use crate::utils::usage::mutated_variables;
use crate::utils::{
get_arg_name, get_parent_expr, get_trait_def_id, has_iter_method, implements_trait, in_macro, is_copy,
get_arg_name, get_parent_expr, get_trait_def_id, has_iter_method, higher, implements_trait, in_macro, is_copy,
is_ctor_or_promotable_const_function, is_expn_of, is_type_diagnostic_item, iter_input_pats, last_path_segment,
match_def_path, match_qpath, match_trait_method, match_type, match_var, method_calls, method_chain_args, paths,
remove_blocks, return_ty, same_tys, single_segment_path, snippet, snippet_with_applicability,
Expand Down Expand Up @@ -1285,6 +1285,26 @@ declare_clippy_lint! {
"using `as_ref().map(Deref::deref)`, which is more succinctly expressed as `as_deref()`"
}

declare_clippy_lint! {
/// **What it does:** TODO
///
/// **Why is this bad?** TODO
///
/// **Known problems:** None.
///
/// **Example:**
/// ```rust
/// TODO
/// ```
/// The correct use would be:
/// ```rust
/// TODO
/// ```
pub ITER_NEXT_SLICE,
style,
"using `.iter().next()` on a sliced array, which can be shortened to just `.get()`"
}

declare_lint_pass!(Methods => [
OPTION_UNWRAP_USED,
RESULT_UNWRAP_USED,
Expand Down Expand Up @@ -1320,6 +1340,7 @@ declare_lint_pass!(Methods => [
FIND_MAP,
MAP_FLATTEN,
ITERATOR_STEP_BY_ZERO,
ITER_NEXT_SLICE,
ITER_NTH,
ITER_NTH_ZERO,
ITER_SKIP_NEXT,
Expand Down Expand Up @@ -1361,6 +1382,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Methods {
["and_then", ..] => lint_option_and_then_some(cx, expr, arg_lists[0]),
["next", "filter"] => lint_filter_next(cx, expr, arg_lists[1]),
["next", "skip_while"] => lint_skip_while_next(cx, expr, arg_lists[1]),
["next", "iter"] => lint_iter_next(cx, expr, arg_lists[1]),
["map", "filter"] => lint_filter_map(cx, expr, arg_lists[1], arg_lists[0]),
["map", "filter_map"] => lint_filter_map_map(cx, expr, arg_lists[1], arg_lists[0]),
["next", "filter_map"] => lint_filter_map_next(cx, expr, arg_lists[1]),
Expand Down Expand Up @@ -2230,6 +2252,49 @@ fn lint_step_by<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &hir::Expr<'_>, args
}
}

fn lint_iter_next<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'tcx hir::Expr<'_>, iter_args: &'tcx [hir::Expr<'_>]) {
let caller_expr = &iter_args[0];
if derefs_to_slice(cx, caller_expr, cx.tables.expr_ty(caller_expr)).is_some() {
// caller is a Slice
if_chain! {
if let hir::ExprKind::Index(ref caller_var, ref index_expr) = &caller_expr.kind;
if let Some(higher::Range { start: Some(start_expr), end: None, limits: ast::RangeLimits::HalfOpen })
= higher::range(cx, index_expr);
if let hir::ExprKind::Lit(ref start_lit) = &start_expr.kind;
if let ast::LitKind::Int(start_idx, _) = start_lit.node;
then {
let mut applicability = Applicability::MachineApplicable;
span_lint_and_sugg(
cx,
ITER_NEXT_SLICE,
expr.span,
"Using `.iter().next()` on a Slice without end index.",
"try calling",
format!("{}.get({})", snippet_with_applicability(cx, caller_var.span, "..", &mut applicability), start_idx),
applicability,
);
}
}
} else if is_type_diagnostic_item(cx, cx.tables.expr_ty(caller_expr), sym!(vec_type))
|| matches!(&walk_ptrs_ty(cx.tables.expr_ty(caller_expr)).kind, ty::Array(_, _))
{
// caller is a Vec or an Array
let mut applicability = Applicability::MachineApplicable;
span_lint_and_sugg(
cx,
ITER_NEXT_SLICE,
expr.span,
"Using `.iter().next()` on an array",
"try calling",
format!(
"{}.get(0)",
snippet_with_applicability(cx, caller_expr.span, "..", &mut applicability)
),
applicability,
);
}
}

fn lint_iter_nth<'a, 'tcx>(
cx: &LateContext<'a, 'tcx>,
expr: &hir::Expr<'_>,
Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/needless_continue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -424,7 +424,7 @@ fn erode_from_back(s: &str) -> String {
}

fn span_of_first_expr_in_block(block: &ast::Block) -> Option<Span> {
block.stmts.iter().next().map(|stmt| stmt.span)
block.stmts.get(0).map(|stmt| stmt.span)
}

#[cfg(test)]
Expand Down
7 changes: 7 additions & 0 deletions src/lintlist/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2026,6 +2026,13 @@ pub static ref ALL_LINTS: Vec<Lint> = vec![
deprecation: None,
module: "methods",
},
Lint {
name: "iter_next_slice",
group: "style",
desc: "default lint description",
deprecation: None,
module: "methods",
},
Lint {
name: "slow_vector_initialization",
group: "perf",
Expand Down
6 changes: 3 additions & 3 deletions tests/ui/for_loop_over_option_result.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,16 +23,16 @@ fn for_loop_over_option_and_result() {

// make sure LOOP_OVER_NEXT lint takes clippy::precedence when next() is the last call
// in the chain
for x in v.iter().next() {
for x in v.get(0) {
println!("{}", x);
}

// make sure we lint when next() is not the last call in the chain
for x in v.iter().next().and(Some(0)) {
for x in v.get(0).and(Some(0)) {
println!("{}", x);
}

for x in v.iter().next().ok_or("x not found") {
for x in v.get(0).ok_or("x not found") {
println!("{}", x);
}

Expand Down
24 changes: 12 additions & 12 deletions tests/ui/for_loop_over_option_result.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -24,29 +24,29 @@ LL | for x in option.ok_or("x not found") {
|
= help: consider replacing `for x in option.ok_or("x not found")` with `if let Ok(x) = option.ok_or("x not found")`

error: you are iterating over `Iterator::next()` which is an Option; this will compile but is probably not what you want
error: for loop over `v.get(0)`, which is an `Option`. This is more readably written as an `if let` statement.
--> $DIR/for_loop_over_option_result.rs:26:14
|
LL | for x in v.iter().next() {
| ^^^^^^^^^^^^^^^
LL | for x in v.get(0) {
| ^^^^^^^^
|
= note: `#[deny(clippy::iter_next_loop)]` on by default
= help: consider replacing `for x in v.get(0)` with `if let Some(x) = v.get(0)`

error: for loop over `v.iter().next().and(Some(0))`, which is an `Option`. This is more readably written as an `if let` statement.
error: for loop over `v.get(0).and(Some(0))`, which is an `Option`. This is more readably written as an `if let` statement.
--> $DIR/for_loop_over_option_result.rs:31:14
|
LL | for x in v.iter().next().and(Some(0)) {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
LL | for x in v.get(0).and(Some(0)) {
| ^^^^^^^^^^^^^^^^^^^^^
|
= help: consider replacing `for x in v.iter().next().and(Some(0))` with `if let Some(x) = v.iter().next().and(Some(0))`
= help: consider replacing `for x in v.get(0).and(Some(0))` with `if let Some(x) = v.get(0).and(Some(0))`

error: for loop over `v.iter().next().ok_or("x not found")`, which is a `Result`. This is more readably written as an `if let` statement.
error: for loop over `v.get(0).ok_or("x not found")`, which is a `Result`. This is more readably written as an `if let` statement.
--> $DIR/for_loop_over_option_result.rs:35:14
|
LL | for x in v.iter().next().ok_or("x not found") {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
LL | for x in v.get(0).ok_or("x not found") {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: consider replacing `for x in v.iter().next().ok_or("x not found")` with `if let Ok(x) = v.iter().next().ok_or("x not found")`
= help: consider replacing `for x in v.get(0).ok_or("x not found")` with `if let Ok(x) = v.get(0).ok_or("x not found")`

error: this loop never actually loops
--> $DIR/for_loop_over_option_result.rs:47:5
Expand Down
39 changes: 39 additions & 0 deletions tests/ui/iter_next_slice.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
#![warn(clippy::iter_next_slice)]

fn main() {
// test code goes here
let s = [1, 2, 3];
let v = vec![1, 2, 3];
let o = Some(5);

s.iter().next();
// Should be replaced by s.get(0)

// s.into_iter().next();
// Probaby can rely on the default `array_into_iter`/`clippy::into_iter_on_ref` lint

s[2..].iter().next();
// Should be replaced by s.get(2)

// s[1..].into_iter().next();
// Probaby can rely on the default `clippy::into_iter_on_ref` lint

v[5..].iter().next();
// Should be replaced by v.get(5)

// v[1..].into_iter().next();
// Probaby can rely on the default `clippy::into_iter_on_ref` lint

v.iter().next();
// Should be replaced by v.get(0)

// v.into_iter().next();
// Tricky one—I don't think there's any other (short) way to say this (take ownership of first value)
// So this should probably not get any warnings

o.iter().next();
// Replace with o.as_ref()

// o.into_iter().next();
// Replace with o
}
28 changes: 28 additions & 0 deletions tests/ui/iter_next_slice.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
error: Using `.iter().next()` on an array
--> $DIR/iter_next_slice.rs:9:5
|
LL | s.iter().next();
| ^^^^^^^^^^^^^^^ help: try calling: `s.get(0)`
|
= note: `-D clippy::iter-next-slice` implied by `-D warnings`

error: Using `.iter().next()` on a Slice without end index.
--> $DIR/iter_next_slice.rs:15:5
|
LL | s[2..].iter().next();
| ^^^^^^^^^^^^^^^^^^^^ help: try calling: `s.get(2)`

error: Using `.iter().next()` on a Slice without end index.
--> $DIR/iter_next_slice.rs:21:5
|
LL | v[5..].iter().next();
| ^^^^^^^^^^^^^^^^^^^^ help: try calling: `v.get(5)`

error: Using `.iter().next()` on an array
--> $DIR/iter_next_slice.rs:27:5
|
LL | v.iter().next();
| ^^^^^^^^^^^^^^^ help: try calling: `v.get(0)`

error: aborting due to 4 previous errors

2 changes: 1 addition & 1 deletion tests/ui/needless_collect.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use std::collections::{BTreeSet, HashMap, HashSet};
fn main() {
let sample = [1; 5];
let len = sample.iter().count();
if sample.iter().next().is_none() {
if sample.get(0).is_none() {
// Empty
}
sample.iter().cloned().any(|x| x == 1);
Expand Down
4 changes: 2 additions & 2 deletions tests/ui/needless_collect.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,10 @@ LL | let len = sample.iter().collect::<Vec<_>>().len();
= note: `-D clippy::needless-collect` implied by `-D warnings`

error: avoid using `collect()` when not needed
--> $DIR/needless_collect.rs:12:21
--> $DIR/needless_collect.rs:12:14
|
LL | if sample.iter().collect::<Vec<_>>().is_empty() {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `.next().is_none()`
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `.get(0).is_none()`

error: avoid using `collect()` when not needed
--> $DIR/needless_collect.rs:15:27
Expand Down

0 comments on commit 71f51ad

Please sign in to comment.