From 71f51ade1b9e89314d5caf03f2bf414db91d049c Mon Sep 17 00:00:00 2001 From: Ericko Samudera Date: Tue, 12 May 2020 22:18:23 +0700 Subject: [PATCH] WIP - implement iter next slice --- CHANGELOG.md | 1 + clippy_lints/src/lib.rs | 3 + clippy_lints/src/loops.rs | 16 ++++- clippy_lints/src/methods/mod.rs | 67 ++++++++++++++++++++- clippy_lints/src/needless_continue.rs | 2 +- src/lintlist/mod.rs | 7 +++ tests/ui/for_loop_over_option_result.rs | 6 +- tests/ui/for_loop_over_option_result.stderr | 24 ++++---- tests/ui/iter_next_slice.rs | 39 ++++++++++++ tests/ui/iter_next_slice.stderr | 28 +++++++++ tests/ui/needless_collect.fixed | 2 +- tests/ui/needless_collect.stderr | 4 +- 12 files changed, 177 insertions(+), 22 deletions(-) create mode 100644 tests/ui/iter_next_slice.rs create mode 100644 tests/ui/iter_next_slice.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 8457d6ad05cf..3644e3cf464c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 51b5401da7d0..0a3aae440829 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -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, @@ -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), @@ -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), diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index 2bbf4dba614b..ba5d6906abba 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -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, ); } @@ -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!() +} diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 3676dc5b09d2..cead179d876c 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -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, @@ -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, @@ -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, @@ -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]), @@ -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<'_>, diff --git a/clippy_lints/src/needless_continue.rs b/clippy_lints/src/needless_continue.rs index 28183810df48..a971d041ca66 100644 --- a/clippy_lints/src/needless_continue.rs +++ b/clippy_lints/src/needless_continue.rs @@ -424,7 +424,7 @@ fn erode_from_back(s: &str) -> String { } fn span_of_first_expr_in_block(block: &ast::Block) -> Option { - block.stmts.iter().next().map(|stmt| stmt.span) + block.stmts.get(0).map(|stmt| stmt.span) } #[cfg(test)] diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index 51d1cb2216a9..312614756cbf 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -2026,6 +2026,13 @@ pub static ref ALL_LINTS: Vec = 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", diff --git a/tests/ui/for_loop_over_option_result.rs b/tests/ui/for_loop_over_option_result.rs index 6b207b26b6b7..5e578e9d558a 100644 --- a/tests/ui/for_loop_over_option_result.rs +++ b/tests/ui/for_loop_over_option_result.rs @@ -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); } diff --git a/tests/ui/for_loop_over_option_result.stderr b/tests/ui/for_loop_over_option_result.stderr index 194a0bfec5b3..bd9c55448513 100644 --- a/tests/ui/for_loop_over_option_result.stderr +++ b/tests/ui/for_loop_over_option_result.stderr @@ -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 diff --git a/tests/ui/iter_next_slice.rs b/tests/ui/iter_next_slice.rs new file mode 100644 index 000000000000..b9651d6a77da --- /dev/null +++ b/tests/ui/iter_next_slice.rs @@ -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 +} diff --git a/tests/ui/iter_next_slice.stderr b/tests/ui/iter_next_slice.stderr new file mode 100644 index 000000000000..c74c4bde6a23 --- /dev/null +++ b/tests/ui/iter_next_slice.stderr @@ -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 + diff --git a/tests/ui/needless_collect.fixed b/tests/ui/needless_collect.fixed index b4227eaf2f8b..be37dc16b9a3 100644 --- a/tests/ui/needless_collect.fixed +++ b/tests/ui/needless_collect.fixed @@ -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); diff --git a/tests/ui/needless_collect.stderr b/tests/ui/needless_collect.stderr index 8884c8e16129..ae8903ea9412 100644 --- a/tests/ui/needless_collect.stderr +++ b/tests/ui/needless_collect.stderr @@ -7,10 +7,10 @@ LL | let len = sample.iter().collect::>().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::>().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