Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New lint: iter_next_slice #5597

Merged
merged 1 commit into from
Jun 2, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -1401,6 +1401,7 @@ Released 2018-09-13
[`items_after_statements`]: https://rust-lang.github.io/rust-clippy/master/index.html#items_after_statements
[`iter_cloned_collect`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_cloned_collect
[`iter_next_loop`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_next_loop
[`iter_next_slice`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_next_slice
[`iter_nth`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_nth
[`iter_nth_zero`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_nth_zero
[`iter_skip_next`]: https://rust-lang.github.io/rust-clippy/master/index.html#iter_skip_next
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 @@ -669,6 +669,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 @@ -1303,6 +1304,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
LintId::of(&methods::INTO_ITER_ON_REF),
LintId::of(&methods::ITERATOR_STEP_BY_ZERO),
LintId::of(&methods::ITER_CLONED_COLLECT),
LintId::of(&methods::ITER_NEXT_SLICE),
LintId::of(&methods::ITER_NTH),
LintId::of(&methods::ITER_NTH_ZERO),
LintId::of(&methods::ITER_SKIP_NEXT),
Expand Down Expand Up @@ -1483,6 +1485,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
LintId::of(&methods::CHARS_NEXT_CMP),
LintId::of(&methods::INTO_ITER_ON_REF),
LintId::of(&methods::ITER_CLONED_COLLECT),
LintId::of(&methods::ITER_NEXT_SLICE),
LintId::of(&methods::ITER_NTH_ZERO),
LintId::of(&methods::ITER_SKIP_NEXT),
LintId::of(&methods::MANUAL_SATURATING_ARITHMETIC),
Expand Down
26 changes: 13 additions & 13 deletions clippy_lints/src/loops.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ use rustc_middle::middle::region;
use rustc_middle::ty::{self, Ty};
use rustc_session::{declare_lint_pass, declare_tool_lint};
use rustc_span::source_map::Span;
use rustc_span::BytePos;
use rustc_span::symbol::Symbol;
use rustc_typeck::expr_use_visitor::{ConsumeMode, Delegate, ExprUseVisitor, Place, PlaceBase};
use std::iter::{once, Iterator};
use std::mem;
Expand Down Expand Up @@ -2381,32 +2381,32 @@ fn check_needless_collect<'a, 'tcx>(expr: &'tcx Expr<'_>, cx: &LateContext<'a, '
match_type(cx, ty, &paths::BTREEMAP) ||
is_type_diagnostic_item(cx, ty, sym!(hashmap_type)) {
if method.ident.name == sym!(len) {
let span = shorten_needless_collect_span(expr);
let span = shorten_span(expr, sym!(collect));
span_lint_and_sugg(
cx,
NEEDLESS_COLLECT,
span,
NEEDLESS_COLLECT_MSG,
"replace with",
".count()".to_string(),
"count()".to_string(),
Applicability::MachineApplicable,
);
}
if method.ident.name == sym!(is_empty) {
let span = shorten_needless_collect_span(expr);
let span = shorten_span(expr, sym!(iter));
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,
);
}
if method.ident.name == sym!(contains) {
let contains_arg = snippet(cx, args[1].span, "??");
let span = shorten_needless_collect_span(expr);
let span = shorten_span(expr, sym!(collect));
span_lint_and_then(
cx,
NEEDLESS_COLLECT,
Expand All @@ -2422,7 +2422,7 @@ fn check_needless_collect<'a, 'tcx>(expr: &'tcx Expr<'_>, cx: &LateContext<'a, '
span,
"replace with",
format!(
".any(|{}| x == {})",
"any(|{}| x == {})",
arg, pred
),
Applicability::MachineApplicable,
Expand All @@ -2435,13 +2435,13 @@ fn check_needless_collect<'a, 'tcx>(expr: &'tcx Expr<'_>, cx: &LateContext<'a, '
}
}

fn shorten_needless_collect_span(expr: &Expr<'_>) -> Span {
if_chain! {
if let ExprKind::MethodCall(_, _, ref args) = expr.kind;
if let ExprKind::MethodCall(_, ref span, _) = args[0].kind;
then {
return expr.span.with_lo(span.lo() - BytePos(1));
fn shorten_span(expr: &Expr<'_>, target_fn_name: Symbol) -> Span {
let mut current_expr = expr;
while let ExprKind::MethodCall(ref path, ref span, ref args) = current_expr.kind {
if path.ident.name == target_fn_name {
return expr.span.with_lo(span.lo());
}
current_expr = &args[0];
}
unreachable!()
}
84 changes: 83 additions & 1 deletion clippy_lints/src/methods/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,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 @@ -1242,6 +1242,32 @@ declare_clippy_lint! {
"using `as_ref().map(Deref::deref)`, which is more succinctly expressed as `as_deref()`"
}

declare_clippy_lint! {
/// **What it does:** Checks for usage of `iter().next()` on a Slice or an Array
///
/// **Why is this bad?** These can be shortened into `.get()`
///
/// **Known problems:** None.
///
/// **Example:**
/// ```rust
/// # let a = [1, 2, 3];
/// # let b = vec![1, 2, 3];
/// a[2..].iter().next();
/// b.iter().next();
/// ```
/// should be written as:
/// ```rust
/// # let a = [1, 2, 3];
/// # let b = vec![1, 2, 3];
/// a.get(2);
/// b.get(0);
/// ```
pub ITER_NEXT_SLICE,
style,
"using `.iter().next()` on a sliced array, which can be shortened to just `.get()`"
}

declare_lint_pass!(Methods => [
UNWRAP_USED,
EXPECT_USED,
Expand Down Expand Up @@ -1273,6 +1299,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 @@ -1320,6 +1347,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Methods {
},
["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 @@ -2184,6 +2212,60 @@ 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];

// Skip lint if the `iter().next()` expression is a for loop argument,
// since it is already covered by `&loops::ITER_NEXT_LOOP`
let mut parent_expr_opt = get_parent_expr(cx, expr);
flip1995 marked this conversation as resolved.
Show resolved Hide resolved
while let Some(parent_expr) = parent_expr_opt {
if higher::for_loop(parent_expr).is_some() {
return;
}
parent_expr_opt = get_parent_expr(cx, parent_expr);
}

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 @@ -934,6 +934,13 @@ pub static ref ALL_LINTS: Vec<Lint> = vec![
deprecation: None,
module: "loops",
},
Lint {
name: "iter_next_slice",
group: "style",
desc: "using `.iter().next()` on a sliced array, which can be shortened to just `.get()`",
deprecation: None,
module: "methods",
},
Lint {
name: "iter_nth",
group: "perf",
Expand Down
2 changes: 2 additions & 0 deletions tests/ui/into_iter_on_ref.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -40,4 +40,6 @@ fn main() {
let _ = (&HashSet::<i32>::new()).iter(); //~ WARN equivalent to .iter()
let _ = std::path::Path::new("12/34").iter(); //~ WARN equivalent to .iter()
let _ = std::path::PathBuf::from("12/34").iter(); //~ ERROR equivalent to .iter()

let _ = (&[1, 2, 3]).iter().next(); //~ WARN equivalent to .iter()
}
2 changes: 2 additions & 0 deletions tests/ui/into_iter_on_ref.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,4 +40,6 @@ fn main() {
let _ = (&HashSet::<i32>::new()).into_iter(); //~ WARN equivalent to .iter()
let _ = std::path::Path::new("12/34").into_iter(); //~ WARN equivalent to .iter()
let _ = std::path::PathBuf::from("12/34").into_iter(); //~ ERROR equivalent to .iter()

let _ = (&[1, 2, 3]).into_iter().next(); //~ WARN equivalent to .iter()
}
8 changes: 7 additions & 1 deletion tests/ui/into_iter_on_ref.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -156,5 +156,11 @@ error: this `.into_iter()` call is equivalent to `.iter()` and will not move the
LL | let _ = std::path::PathBuf::from("12/34").into_iter(); //~ ERROR equivalent to .iter()
| ^^^^^^^^^ help: call directly: `iter`

error: aborting due to 26 previous errors
error: this `.into_iter()` call is equivalent to `.iter()` and will not move the `array`
--> $DIR/into_iter_on_ref.rs:44:26
|
LL | let _ = (&[1, 2, 3]).into_iter().next(); //~ WARN equivalent to .iter()
| ^^^^^^^^^ help: call directly: `iter`

error: aborting due to 27 previous errors

24 changes: 24 additions & 0 deletions tests/ui/iter_next_slice.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// run-rustfix
#![warn(clippy::iter_next_slice)]

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

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

s.get(2);
// Should be replaced by s.get(2)

v.get(5);
// Should be replaced by v.get(5)

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

let o = Some(5);
o.iter().next();
// Shouldn't be linted since this is not a Slice or an Array
}
24 changes: 24 additions & 0 deletions tests/ui/iter_next_slice.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// run-rustfix
#![warn(clippy::iter_next_slice)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#![warn(clippy::iter_next_slice)]
// run-rustfix
#![warn(clippy::iter_next_slice)]

and a update-all-references.sh run and this PR should be finished.

Copy link
Contributor Author

@esamudera esamudera May 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I have added the run-rustfix marker and iter_next_slice.fixed in the new commit.

However, the test failed at Clippy Test, cargo build stage. I'm not sure about the errors listed, is it about toolchain version incompatibility? Should I rebase this branch to the latest master?

Copy link
Member

@flip1995 flip1995 May 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may have to rebase after #5665 is merged. In the meantime you could go ahead, remove the "WIP" prefixes from your commits and remove the "draft" status from this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I have squashed all the commits into one, and have updated this PR status to Open.


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

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

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

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

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

let o = Some(5);
o.iter().next();
// Shouldn't be linted since this is not a Slice or an Array
}
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:12: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:15:5
|
LL | v[5..].iter().next();
| ^^^^^^^^^^^^^^^^^^^^ help: try calling: `v.get(5)`

error: Using `.iter().next()` on an array
--> $DIR/iter_next_slice.rs:18: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
Loading