From 050dd3b3c838a8e7d3a9403d861cd61364df3654 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patrick=20Jos=C3=A9=20Pereira?= Date: Sat, 3 Oct 2020 15:46:28 -0300 Subject: [PATCH] Add linter for a single element for loop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Patrick José Pereira --- CHANGELOG.md | 1 + clippy_lints/src/lib.rs | 3 ++ clippy_lints/src/loops.rs | 62 ++++++++++++++++++++++++++++- src/lintlist/mod.rs | 7 ++++ tests/ui/single_element_loop.rs | 10 +++++ tests/ui/single_element_loop.stderr | 10 +++++ 6 files changed, 91 insertions(+), 2 deletions(-) create mode 100644 tests/ui/single_element_loop.rs create mode 100644 tests/ui/single_element_loop.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index d82f970b8bf2..1bf25bcd0f96 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1936,6 +1936,7 @@ Released 2018-09-13 [`single_char_pattern`]: https://rust-lang.github.io/rust-clippy/master/index.html#single_char_pattern [`single_char_push_str`]: https://rust-lang.github.io/rust-clippy/master/index.html#single_char_push_str [`single_component_path_imports`]: https://rust-lang.github.io/rust-clippy/master/index.html#single_component_path_imports +[`single_element_loop`]: https://rust-lang.github.io/rust-clippy/master/index.html#single_element_loop [`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 diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index d4d2f92a6a69..84ea2d5ca78c 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -633,6 +633,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &loops::NEEDLESS_RANGE_LOOP, &loops::NEVER_LOOP, &loops::SAME_ITEM_PUSH, + &loops::SINGLE_ELEMENT_LOOP, &loops::WHILE_IMMUTABLE_CONDITION, &loops::WHILE_LET_LOOP, &loops::WHILE_LET_ON_ITERATOR, @@ -1363,6 +1364,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&loops::NEEDLESS_RANGE_LOOP), LintId::of(&loops::NEVER_LOOP), LintId::of(&loops::SAME_ITEM_PUSH), + LintId::of(&loops::SINGLE_ELEMENT_LOOP), LintId::of(&loops::WHILE_IMMUTABLE_CONDITION), LintId::of(&loops::WHILE_LET_LOOP), LintId::of(&loops::WHILE_LET_ON_ITERATOR), @@ -1664,6 +1666,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&lifetimes::NEEDLESS_LIFETIMES), LintId::of(&loops::EXPLICIT_COUNTER_LOOP), LintId::of(&loops::MUT_RANGE_BOUND), + LintId::of(&loops::SINGLE_ELEMENT_LOOP), LintId::of(&loops::WHILE_LET_LOOP), LintId::of(&manual_strip::MANUAL_STRIP), LintId::of(&manual_unwrap_or::MANUAL_UNWRAP_OR), diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index 63d7e3176b10..6bf98f89b30f 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -5,8 +5,9 @@ use crate::utils::usage::{is_unused, mutated_variables}; use crate::utils::{ contains_name, get_enclosing_block, get_parent_expr, get_trait_def_id, has_iter_method, higher, implements_trait, is_integer_const, is_no_std_crate, is_refutable, is_type_diagnostic_item, last_path_segment, match_trait_method, - match_type, match_var, multispan_sugg, qpath_res, snippet, snippet_with_applicability, snippet_with_macro_callsite, - span_lint, span_lint_and_help, span_lint_and_sugg, span_lint_and_then, sugg, SpanlessEq, + match_type, match_var, multispan_sugg, qpath_res, single_segment_path, snippet, snippet_with_applicability, + snippet_with_macro_callsite, span_lint, span_lint_and_help, span_lint_and_sugg, span_lint_and_then, sugg, + SpanlessEq, }; use if_chain::if_chain; use rustc_ast::ast; @@ -452,6 +453,31 @@ declare_clippy_lint! { "the same item is pushed inside of a for loop" } +declare_clippy_lint! { + /// **What it does:** Checks whether a for loop has a single element. + /// + /// **Why is this bad?** There is no reason to have a loop of a + /// single element. + /// **Known problems:** None + /// + /// **Example:** + /// ```rust + /// let item1 = 2; + /// for item in &[item1] { + /// println!("{}", item); + /// } + /// ``` + /// could be written as + /// ```rust + /// let item1 = 2; + /// let item = &item1; + /// println!("{}", item); + /// ``` + pub SINGLE_ELEMENT_LOOP, + complexity, + "there is no reason to have a single element loop" +} + declare_lint_pass!(Loops => [ MANUAL_MEMCPY, NEEDLESS_RANGE_LOOP, @@ -469,6 +495,7 @@ declare_lint_pass!(Loops => [ MUT_RANGE_BOUND, WHILE_IMMUTABLE_CONDITION, SAME_ITEM_PUSH, + SINGLE_ELEMENT_LOOP, ]); impl<'tcx> LateLintPass<'tcx> for Loops { @@ -777,6 +804,7 @@ fn check_for_loop<'tcx>( check_for_loop_arg(cx, pat, arg, expr); check_for_loop_over_map_kv(cx, pat, arg, body, expr); check_for_mut_range_bound(cx, arg, body); + check_for_single_element_loop(cx, pat, arg, expr); detect_same_item_push(cx, pat, arg, body, expr); } @@ -1866,6 +1894,36 @@ fn check_for_loop_over_map_kv<'tcx>( } } +fn check_for_single_element_loop<'tcx>( + cx: &LateContext<'tcx>, + pat: &'tcx Pat<'_>, + arg: &'tcx Expr<'_>, + expr: &'tcx Expr<'_>, +) { + if_chain! { + if let ExprKind::AddrOf(BorrowKind::Ref, _, ref arg_expr) = arg.kind; + if let PatKind::Binding(.., target, _) = pat.kind; + if let ExprKind::Array(ref arg_expr_list) = arg_expr.kind; + if let [arg_expression] = arg_expr_list; + if let ExprKind::Path(ref list_item) = arg_expression.kind; + if let Some(list_item_name) = single_segment_path(list_item).map(|ps| ps.ident.name); + + then { + let for_span = get_span_of_entire_for_loop(expr); + + span_lint_and_sugg( + cx, + SINGLE_ELEMENT_LOOP, + for_span.with_hi(expr.span.hi()), + "for loop over a single element", + "try", + format!("let {} = &{};", target.name, list_item_name), + Applicability::MachineApplicable + ) + } + } +} + struct MutatePairDelegate<'a, 'tcx> { cx: &'a LateContext<'tcx>, hir_id_low: Option, diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index 6301d623a2b1..70369dc582a1 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -2125,6 +2125,13 @@ vec![ deprecation: None, module: "single_component_path_imports", }, + Lint { + name: "single_element_loop", + group: "complexity", + desc: "there is no reason to have a single element loop", + deprecation: None, + module: "loops", + }, Lint { name: "single_match", group: "style", diff --git a/tests/ui/single_element_loop.rs b/tests/ui/single_element_loop.rs new file mode 100644 index 000000000000..57e9336a31fc --- /dev/null +++ b/tests/ui/single_element_loop.rs @@ -0,0 +1,10 @@ +// run-rustfix +// Tests from for_loop.rs that don't have suggestions + +#[warn(clippy::single_element_loop)] +fn main() { + let item1 = 2; + for item in &[item1] { + println!("{}", item); + } +} diff --git a/tests/ui/single_element_loop.stderr b/tests/ui/single_element_loop.stderr new file mode 100644 index 000000000000..3984fc679022 --- /dev/null +++ b/tests/ui/single_element_loop.stderr @@ -0,0 +1,10 @@ +error: for loop over a single element + --> $DIR/single_element_loop.rs:7:5 + | +LL | for item in &[item1] { + | ^^^^^^^^^^^^^^^^^^^^ help: try: `let item = &item1;` + | + = note: `-D clippy::single-element-loop` implied by `-D warnings` + +error: aborting due to previous error +