diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index ddc4b139670..ff418092e4b 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -258,6 +258,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::loops::WHILE_IMMUTABLE_CONDITION_INFO, crate::loops::WHILE_LET_LOOP_INFO, crate::loops::WHILE_LET_ON_ITERATOR_INFO, + crate::loops::WHILE_POP_UNWRAP_INFO, crate::macro_use::MACRO_USE_IMPORTS_INFO, crate::main_recursion::MAIN_RECURSION_INFO, crate::manual_assert::MANUAL_ASSERT_INFO, @@ -645,7 +646,6 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::useless_conversion::USELESS_CONVERSION_INFO, crate::vec::USELESS_VEC_INFO, crate::vec_init_then_push::VEC_INIT_THEN_PUSH_INFO, - crate::while_pop_unwrap::WHILE_POP_UNWRAP_INFO, crate::wildcard_imports::ENUM_GLOB_USE_INFO, crate::wildcard_imports::WILDCARD_IMPORTS_INFO, crate::write::PRINTLN_EMPTY_STRING_INFO, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 0d594612b79..48dbecc9f6a 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -324,7 +324,6 @@ mod use_self; mod useless_conversion; mod vec; mod vec_init_then_push; -mod while_pop_unwrap; mod wildcard_imports; mod write; mod zero_div_zero; diff --git a/clippy_lints/src/loops/mod.rs b/clippy_lints/src/loops/mod.rs index 90012694062..02197e9c187 100644 --- a/clippy_lints/src/loops/mod.rs +++ b/clippy_lints/src/loops/mod.rs @@ -17,6 +17,7 @@ mod utils; mod while_immutable_condition; mod while_let_loop; mod while_let_on_iterator; +mod while_pop_unwrap; use clippy_utils::higher; use rustc_hir::{Expr, ExprKind, LoopSource, Pat}; @@ -25,8 +26,6 @@ use rustc_session::{declare_lint_pass, declare_tool_lint}; use rustc_span::source_map::Span; use utils::{make_iterator_snippet, IncrementVisitor, InitializeVisitor}; -use crate::while_pop_unwrap; - declare_clippy_lint! { /// ### What it does /// Checks for for-loops that manually copy items between @@ -577,6 +576,36 @@ declare_clippy_lint! { "manual implementation of `Iterator::find`" } +declare_clippy_lint! { + /// ### What it does + /// Looks for loops that check for emptiness of a `Vec` in the condition and pop an element + /// in the body as a separate operation. + /// + /// ### Why is this bad? + /// Such loops can be written in a more idiomatic way by using a while..let loop and directly + /// pattern matching on the return value of `Vec::pop()`. + /// + /// ### Example + /// ```rust + /// let mut numbers = vec![1, 2, 3, 4, 5]; + /// while !numbers.is_empty() { + /// let number = numbers.pop().unwrap(); + /// // use `number` + /// } + /// ``` + /// Use instead: + /// ```rust + /// let mut numbers = vec![1, 2, 3, 4, 5]; + /// while let Some(number) = numbers.pop() { + /// // use `number` + /// } + /// ``` + #[clippy::version = "1.70.0"] + pub WHILE_POP_UNWRAP, + style, + "checking for emptiness of a `Vec` in the loop condition and popping an element in the body" +} + declare_lint_pass!(Loops => [ MANUAL_MEMCPY, MANUAL_FLATTEN, @@ -596,6 +625,7 @@ declare_lint_pass!(Loops => [ SINGLE_ELEMENT_LOOP, MISSING_SPIN_LOOP, MANUAL_FIND, + WHILE_POP_UNWRAP ]); impl<'tcx> LateLintPass<'tcx> for Loops { @@ -642,10 +672,10 @@ impl<'tcx> LateLintPass<'tcx> for Loops { while_let_on_iterator::check(cx, expr); - if let Some(higher::While { condition, body }) = higher::While::hir(expr) { + if let Some(higher::While { condition, body, span }) = higher::While::hir(expr) { while_immutable_condition::check(cx, condition, body); missing_spin_loop::check(cx, condition, body); - while_pop_unwrap::check(cx, condition, body); + while_pop_unwrap::check(cx, condition, body, span); } } } diff --git a/clippy_lints/src/loops/while_pop_unwrap.rs b/clippy_lints/src/loops/while_pop_unwrap.rs new file mode 100644 index 00000000000..1e31184b34d --- /dev/null +++ b/clippy_lints/src/loops/while_pop_unwrap.rs @@ -0,0 +1,109 @@ +use clippy_utils::{ + diagnostics::{multispan_sugg_with_applicability, span_lint_and_then}, + match_def_path, paths, + source::snippet, + SpanlessEq, +}; +use rustc_errors::Applicability; +use rustc_hir::{Expr, ExprKind, Pat, Stmt, StmtKind, UnOp}; +use rustc_lint::LateContext; +use rustc_span::Span; +use std::borrow::Cow; + +use super::WHILE_POP_UNWRAP; + +/// The kind of statement that the `pop()` call appeared in. +/// +/// Depending on whether the value was assigned to a variable or not changes what pattern +/// we use for the suggestion. +enum PopStmt<'hir> { + /// `x.pop().unwrap()` was and assigned to a variable. + /// The pattern of this local variable will be used and the local statement + /// is deleted in the suggestion. + Local(&'hir Pat<'hir>), + /// `x.pop().unwrap()` appeared in an arbitrary expression and was not assigned to a variable. + /// The suggestion will use some placeholder identifier and the `x.pop().unwrap()` expression + /// is replaced with that identifier. + Anonymous, +} + +fn report_lint(cx: &LateContext<'_>, pop_span: Span, pop_stmt_kind: PopStmt<'_>, loop_span: Span, receiver_span: Span) { + span_lint_and_then( + cx, + WHILE_POP_UNWRAP, + pop_span, + "you seem to be trying to pop elements from a `Vec` in a loop", + |diag| { + let (pat, pop_replacement) = match &pop_stmt_kind { + PopStmt::Local(pat) => (snippet(cx, pat.span, ".."), String::new()), + PopStmt::Anonymous => (Cow::Borrowed("element"), "element".into()), + }; + + let loop_replacement = format!("while let Some({}) = {}.pop()", pat, snippet(cx, receiver_span, "..")); + multispan_sugg_with_applicability( + diag, + "consider using a `while..let` loop", + Applicability::MachineApplicable, + [(loop_span, loop_replacement), (pop_span, pop_replacement)], + ); + }, + ); +} + +fn match_method_call(cx: &LateContext<'_>, expr: &Expr<'_>, method: &[&str]) -> bool { + if let ExprKind::MethodCall(..) = expr.kind + && let Some(id) = cx.typeck_results().type_dependent_def_id(expr.hir_id) + { + match_def_path(cx, id, method) + } else { + false + } +} + +fn is_vec_pop_unwrap(cx: &LateContext<'_>, expr: &Expr<'_>, is_empty_recv: &Expr<'_>) -> bool { + if (match_method_call(cx, expr, &paths::OPTION_UNWRAP) || match_method_call(cx, expr, &paths::OPTION_EXPECT)) + && let ExprKind::MethodCall(_, unwrap_recv, ..) = expr.kind + && match_method_call(cx, unwrap_recv, &paths::VEC_POP) + && let ExprKind::MethodCall(_, pop_recv, ..) = unwrap_recv.kind + { + // make sure they're the same `Vec` + SpanlessEq::new(cx).eq_expr(pop_recv, is_empty_recv) + } else { + false + } +} + +fn check_local(cx: &LateContext<'_>, stmt: &Stmt<'_>, is_empty_recv: &Expr<'_>, loop_span: Span) { + if let StmtKind::Local(local) = stmt.kind + && let Some(init) = local.init + && is_vec_pop_unwrap(cx, init, is_empty_recv) + { + report_lint(cx, stmt.span, PopStmt::Local(&local.pat), loop_span, is_empty_recv.span); + } +} + +fn check_call_arguments(cx: &LateContext<'_>, stmt: &Stmt<'_>, is_empty_recv: &Expr<'_>, loop_span: Span) { + if let StmtKind::Semi(expr) | StmtKind::Expr(expr) = stmt.kind { + if let ExprKind::MethodCall(.., args, _) | ExprKind::Call(_, args) = expr.kind { + let offending_arg = args + .iter() + .find_map(|arg| is_vec_pop_unwrap(cx, arg, is_empty_recv).then_some(arg.span)); + + if let Some(offending_arg) = offending_arg { + report_lint(cx, offending_arg, PopStmt::Anonymous, loop_span, is_empty_recv.span); + } + } + } +} + +pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, full_cond: &'tcx Expr<'_>, body: &'tcx Expr<'_>, loop_span: Span) { + if let ExprKind::Unary(UnOp::Not, cond) = full_cond.kind + && let ExprKind::MethodCall(_, is_empty_recv, _, _) = cond.kind + && match_method_call(cx, cond, &paths::VEC_IS_EMPTY) + && let ExprKind::Block(body, _) = body.kind + && let Some(stmt) = body.stmts.first() + { + check_local(cx, stmt, is_empty_recv, loop_span); + check_call_arguments(cx, stmt, is_empty_recv, loop_span); + } +} diff --git a/clippy_lints/src/utils/author.rs b/clippy_lints/src/utils/author.rs index 1f632916c57..108077b9d15 100644 --- a/clippy_lints/src/utils/author.rs +++ b/clippy_lints/src/utils/author.rs @@ -333,7 +333,7 @@ impl<'a, 'tcx> PrintVisitor<'a, 'tcx> { #[allow(clippy::too_many_lines)] fn expr(&self, expr: &Binding<&hir::Expr<'_>>) { - if let Some(higher::While { condition, body }) = higher::While::hir(expr.value) { + if let Some(higher::While { condition, body, .. }) = higher::While::hir(expr.value) { bind!(self, condition, body); chain!( self, diff --git a/clippy_lints/src/while_pop_unwrap.rs b/clippy_lints/src/while_pop_unwrap.rs deleted file mode 100644 index 0a3f08888a3..00000000000 --- a/clippy_lints/src/while_pop_unwrap.rs +++ /dev/null @@ -1,122 +0,0 @@ -use clippy_utils::{diagnostics::span_lint_and_then, match_def_path, paths, source::snippet}; -use rustc_errors::Applicability; -use rustc_hir::{Expr, ExprKind, PatKind, Stmt, StmtKind, UnOp}; -use rustc_lint::LateContext; -use rustc_session::{declare_lint_pass, declare_tool_lint}; -use rustc_span::{symbol::Ident, Span}; - -declare_clippy_lint! { - /// ### What it does - /// Looks for loops that check for emptiness of a `Vec` in the condition and pop an element - /// in the body as a separate operation. - /// - /// ### Why is this bad? - /// Such loops can be written in a more idiomatic way by using a while..let loop and directly - /// pattern matching on the return value of `Vec::pop()`. - /// - /// ### Example - /// ```rust - /// let mut numbers = vec![1, 2, 3, 4, 5]; - /// while !numbers.is_empty() { - /// let number = numbers.pop().unwrap(); - /// // use `number` - /// } - /// ``` - /// Use instead: - /// ```rust - /// let mut numbers = vec![1, 2, 3, 4, 5]; - /// while let Some(number) = numbers.pop() { - /// // use `number` - /// } - /// ``` - #[clippy::version = "1.70.0"] - pub WHILE_POP_UNWRAP, - style, - "checking for emptiness of a `Vec` in the loop condition and popping an element in the body" -} -declare_lint_pass!(WhilePopUnwrap => [WHILE_POP_UNWRAP]); - -fn report_lint(cx: &LateContext<'_>, pop_span: Span, ident: Option, loop_span: Span, receiver_span: Span) { - span_lint_and_then( - cx, - WHILE_POP_UNWRAP, - pop_span, - "you seem to be trying to pop elements from a `Vec` in a loop", - |diag| { - diag.span_suggestion( - loop_span, - "try", - format!( - "while let Some({}) = {}.pop()", - ident.as_ref().map_or("element", Ident::as_str), - snippet(cx, receiver_span, "..") - ), - Applicability::MaybeIncorrect, - ) - .note("this while loop can be written in a more idiomatic way"); - }, - ); -} - -fn match_method_call(cx: &LateContext<'_>, expr: &Expr<'_>, method: &[&str]) -> bool { - if let ExprKind::MethodCall(..) = expr.kind - && let Some(id) = cx.typeck_results().type_dependent_def_id(expr.hir_id) - && match_def_path(cx, id, method) - { - true - } else { - false - } -} - -fn is_vec_pop(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { - match_method_call(cx, expr, &paths::VEC_POP) -} - -fn is_vec_pop_unwrap(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { - if let ExprKind::MethodCall(_, inner, ..) = expr.kind - && (match_method_call(cx, expr, &paths::OPTION_UNWRAP) || match_method_call(cx, expr, &paths::OPTION_EXPECT)) - && is_vec_pop(cx, inner) - { - true - } else { - false - } -} - -fn check_local(cx: &LateContext<'_>, stmt: &Stmt<'_>, loop_span: Span, recv_span: Span) { - if let StmtKind::Local(local) = stmt.kind - && let PatKind::Binding(.., ident, _) = local.pat.kind - && let Some(init) = local.init - && let ExprKind::MethodCall(_, inner, ..) = init.kind - && is_vec_pop_unwrap(cx, init) - { - report_lint(cx, init.span.to(inner.span), Some(ident), loop_span, recv_span); - } -} - -fn check_call_arguments(cx: &LateContext<'_>, stmt: &Stmt<'_>, loop_span: Span, recv_span: Span) { - if let StmtKind::Semi(expr) | StmtKind::Expr(expr) = stmt.kind { - if let ExprKind::MethodCall(_, _, args, _) | ExprKind::Call(_, args) = expr.kind { - let offending_arg = args - .iter() - .find_map(|arg| is_vec_pop_unwrap(cx, arg).then_some(arg.span)); - - if let Some(offending_arg) = offending_arg { - report_lint(cx, offending_arg, None, loop_span, recv_span); - } - } - } -} - -pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, cond: &'tcx Expr<'_>, body: &'tcx Expr<'_>) { - if let ExprKind::Unary(UnOp::Not, cond) = cond.kind - && let ExprKind::MethodCall(_, Expr { span: recv_span, .. }, _, _) = cond.kind - && match_method_call(cx, cond, &paths::VEC_IS_EMPTY) - && let ExprKind::Block(body, _) = body.kind - && let Some(stmt) = body.stmts.first() - { - check_local(cx, stmt, cond.span, *recv_span); - check_call_arguments(cx, stmt, cond.span, *recv_span); - } -} diff --git a/clippy_utils/src/higher.rs b/clippy_utils/src/higher.rs index 50bef370930..a61e4c38088 100644 --- a/clippy_utils/src/higher.rs +++ b/clippy_utils/src/higher.rs @@ -311,6 +311,8 @@ pub struct While<'hir> { pub condition: &'hir Expr<'hir>, /// `while` loop body pub body: &'hir Expr<'hir>, + /// Span of the loop header + pub span: Span, } impl<'hir> While<'hir> { @@ -336,10 +338,10 @@ impl<'hir> While<'hir> { }, _, LoopSource::While, - _, + span, ) = expr.kind { - return Some(Self { condition, body }); + return Some(Self { condition, body, span }); } None } diff --git a/tests/ui/while_pop_unwrap.fixed b/tests/ui/while_pop_unwrap.fixed new file mode 100644 index 00000000000..a635e054e6e --- /dev/null +++ b/tests/ui/while_pop_unwrap.fixed @@ -0,0 +1,93 @@ +// run-rustfix + +#![allow(unused)] +#![warn(clippy::while_pop_unwrap)] + +struct VecInStruct { + numbers: Vec, + unrelated: String, +} + +struct Foo { + a: i32, + b: i32, +} + +fn accept_i32(_: i32) {} +fn accept_optional_i32(_: Option) {} +fn accept_i32_tuple(_: (i32, i32)) {} + +fn main() { + let mut numbers = vec![1, 2, 3, 4, 5]; + while let Some(number) = numbers.pop() { + + } + + let mut val = VecInStruct { + numbers: vec![1, 2, 3, 4, 5], + unrelated: String::new(), + }; + while let Some(number) = val.numbers.pop() { + + } + + while let Some(element) = numbers.pop() { + accept_i32(element); + } + + while let Some(element) = numbers.pop() { + accept_i32(element); + } + + // This should not warn. It "conditionally" pops elements. + while !numbers.is_empty() { + if true { + accept_i32(numbers.pop().unwrap()); + } + } + + // This should also not warn. It conditionally pops elements. + while !numbers.is_empty() { + if false { + continue; + } + accept_i32(numbers.pop().unwrap()); + } + + // This should not warn. It pops elements, but does not unwrap it. + // Might handle the Option in some other arbitrary way. + while !numbers.is_empty() { + accept_optional_i32(numbers.pop()); + } + + let unrelated_vec: Vec = Vec::new(); + // This should not warn. It pops elements from a different vector. + while !unrelated_vec.is_empty() { + accept_i32(numbers.pop().unwrap()); + } + + macro_rules! generate_loop { + () => { + while !numbers.is_empty() { + accept_i32(numbers.pop().unwrap()); + } + }; + } + // Do not warn if the loop is in a macro. + generate_loop!(); + + // Try other kinds of patterns + let mut numbers = vec![(0, 0), (1, 1), (2, 2)]; + while let Some((a, b)) = numbers.pop() { + + } + + while let Some(element) = numbers.pop() { + accept_i32_tuple(element); + } + + let mut results = vec![Foo { a: 1, b: 2 }, Foo { a: 3, b: 4 }]; + while let Some(Foo { a, b }) = results.pop() { + + } +} diff --git a/tests/ui/while_pop_unwrap.rs b/tests/ui/while_pop_unwrap.rs index 1b6de8f1f89..820ff211086 100644 --- a/tests/ui/while_pop_unwrap.rs +++ b/tests/ui/while_pop_unwrap.rs @@ -1,3 +1,5 @@ +// run-rustfix + #![allow(unused)] #![warn(clippy::while_pop_unwrap)] @@ -6,7 +8,14 @@ struct VecInStruct { unrelated: String, } +struct Foo { + a: i32, + b: i32, +} + fn accept_i32(_: i32) {} +fn accept_optional_i32(_: Option) {} +fn accept_i32_tuple(_: (i32, i32)) {} fn main() { let mut numbers = vec![1, 2, 3, 4, 5]; @@ -44,4 +53,41 @@ fn main() { } accept_i32(numbers.pop().unwrap()); } + + // This should not warn. It pops elements, but does not unwrap it. + // Might handle the Option in some other arbitrary way. + while !numbers.is_empty() { + accept_optional_i32(numbers.pop()); + } + + let unrelated_vec: Vec = Vec::new(); + // This should not warn. It pops elements from a different vector. + while !unrelated_vec.is_empty() { + accept_i32(numbers.pop().unwrap()); + } + + macro_rules! generate_loop { + () => { + while !numbers.is_empty() { + accept_i32(numbers.pop().unwrap()); + } + }; + } + // Do not warn if the loop is in a macro. + generate_loop!(); + + // Try other kinds of patterns + let mut numbers = vec![(0, 0), (1, 1), (2, 2)]; + while !numbers.is_empty() { + let (a, b) = numbers.pop().unwrap(); + } + + while !numbers.is_empty() { + accept_i32_tuple(numbers.pop().unwrap()); + } + + let mut results = vec![Foo { a: 1, b: 2 }, Foo { a: 3, b: 4 }]; + while !results.is_empty() { + let Foo { a, b } = results.pop().unwrap(); + } } diff --git a/tests/ui/while_pop_unwrap.stderr b/tests/ui/while_pop_unwrap.stderr index 8dc77db8b5a..f8a6cfcda0e 100644 --- a/tests/ui/while_pop_unwrap.stderr +++ b/tests/ui/while_pop_unwrap.stderr @@ -1,43 +1,87 @@ error: you seem to be trying to pop elements from a `Vec` in a loop - --> $DIR/while_pop_unwrap.rs:14:22 + --> $DIR/while_pop_unwrap.rs:23:9 | -LL | while !numbers.is_empty() { - | ------------------ help: try: `while let Some(number) = numbers.pop()` LL | let number = numbers.pop().unwrap(); - | ^^^^^^^^^^^^^^^^^^^^^^ + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | - = note: this while loop can be written in a more idiomatic way = note: `-D clippy::while-pop-unwrap` implied by `-D warnings` +help: consider using a `while..let` loop + | +LL ~ while let Some(number) = numbers.pop() { +LL ~ + | error: you seem to be trying to pop elements from a `Vec` in a loop - --> $DIR/while_pop_unwrap.rs:22:22 + --> $DIR/while_pop_unwrap.rs:31:9 | -LL | while !val.numbers.is_empty() { - | ---------------------- help: try: `while let Some(number) = val.numbers.pop()` LL | let number = val.numbers.pop().unwrap(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^ + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: consider using a `while..let` loop + | +LL ~ while let Some(number) = val.numbers.pop() { +LL ~ | - = note: this while loop can be written in a more idiomatic way error: you seem to be trying to pop elements from a `Vec` in a loop - --> $DIR/while_pop_unwrap.rs:26:20 + --> $DIR/while_pop_unwrap.rs:35:20 | -LL | while !numbers.is_empty() { - | ------------------ help: try: `while let Some(element) = numbers.pop()` LL | accept_i32(numbers.pop().unwrap()); | ^^^^^^^^^^^^^^^^^^^^^^ | - = note: this while loop can be written in a more idiomatic way +help: consider using a `while..let` loop + | +LL ~ while let Some(element) = numbers.pop() { +LL ~ accept_i32(element); + | error: you seem to be trying to pop elements from a `Vec` in a loop - --> $DIR/while_pop_unwrap.rs:30:20 + --> $DIR/while_pop_unwrap.rs:39:20 | -LL | while !numbers.is_empty() { - | ------------------ help: try: `while let Some(element) = numbers.pop()` LL | accept_i32(numbers.pop().expect("")); | ^^^^^^^^^^^^^^^^^^^^^^^^ | - = note: this while loop can be written in a more idiomatic way +help: consider using a `while..let` loop + | +LL ~ while let Some(element) = numbers.pop() { +LL ~ accept_i32(element); + | -error: aborting due to 4 previous errors +error: you seem to be trying to pop elements from a `Vec` in a loop + --> $DIR/while_pop_unwrap.rs:82:9 + | +LL | let (a, b) = numbers.pop().unwrap(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: consider using a `while..let` loop + | +LL ~ while let Some((a, b)) = numbers.pop() { +LL ~ + | + +error: you seem to be trying to pop elements from a `Vec` in a loop + --> $DIR/while_pop_unwrap.rs:86:26 + | +LL | accept_i32_tuple(numbers.pop().unwrap()); + | ^^^^^^^^^^^^^^^^^^^^^^ + | +help: consider using a `while..let` loop + | +LL ~ while let Some(element) = numbers.pop() { +LL ~ accept_i32_tuple(element); + | + +error: you seem to be trying to pop elements from a `Vec` in a loop + --> $DIR/while_pop_unwrap.rs:91:9 + | +LL | let Foo { a, b } = results.pop().unwrap(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: consider using a `while..let` loop + | +LL ~ while let Some(Foo { a, b }) = results.pop() { +LL ~ + | + +error: aborting due to 7 previous errors