diff --git a/CHANGELOG.md b/CHANGELOG.md index 23f4f97ee07c..d1e98a93940d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4797,6 +4797,7 @@ Released 2018-09-13 [`manual_strip`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_strip [`manual_swap`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_swap [`manual_unwrap_or`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_unwrap_or +[`manual_while_let_some`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_while_let_some [`many_single_char_names`]: https://rust-lang.github.io/rust-clippy/master/index.html#many_single_char_names [`map_clone`]: https://rust-lang.github.io/rust-clippy/master/index.html#map_clone [`map_collect_result_unit`]: https://rust-lang.github.io/rust-clippy/master/index.html#map_collect_result_unit diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 4aebd0b7d011..44e725be710e 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -249,6 +249,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::loops::MANUAL_FIND_INFO, crate::loops::MANUAL_FLATTEN_INFO, crate::loops::MANUAL_MEMCPY_INFO, + crate::loops::MANUAL_WHILE_LET_SOME_INFO, crate::loops::MISSING_SPIN_LOOP_INFO, crate::loops::MUT_RANGE_BOUND_INFO, crate::loops::NEEDLESS_RANGE_LOOP_INFO, diff --git a/clippy_lints/src/loops/manual_while_let_some.rs b/clippy_lints/src/loops/manual_while_let_some.rs new file mode 100644 index 000000000000..cb9c84be4c7a --- /dev/null +++ b/clippy_lints/src/loops/manual_while_let_some.rs @@ -0,0 +1,110 @@ +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::MANUAL_WHILE_LET_SOME; + +/// 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. +#[derive(Copy, Clone)] +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, + MANUAL_WHILE_LET_SOME, + 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/loops/mod.rs b/clippy_lints/src/loops/mod.rs index 610a0233eee1..f83ad388a742 100644 --- a/clippy_lints/src/loops/mod.rs +++ b/clippy_lints/src/loops/mod.rs @@ -7,6 +7,7 @@ mod iter_next_loop; mod manual_find; mod manual_flatten; mod manual_memcpy; +mod manual_while_let_some; mod missing_spin_loop; mod mut_range_bound; mod needless_range_loop; @@ -575,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 MANUAL_WHILE_LET_SOME, + 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, @@ -594,6 +625,7 @@ declare_lint_pass!(Loops => [ SINGLE_ELEMENT_LOOP, MISSING_SPIN_LOOP, MANUAL_FIND, + MANUAL_WHILE_LET_SOME ]); impl<'tcx> LateLintPass<'tcx> for Loops { @@ -640,9 +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); + manual_while_let_some::check(cx, condition, body, span); } } } diff --git a/clippy_lints/src/utils/author.rs b/clippy_lints/src/utils/author.rs index 1f632916c570..108077b9d158 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_utils/src/higher.rs b/clippy_utils/src/higher.rs index 50bef3709309..a61e4c380886 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/clippy_utils/src/paths.rs b/clippy_utils/src/paths.rs index 9be2d0eae80a..0f0792fdaa96 100644 --- a/clippy_utils/src/paths.rs +++ b/clippy_utils/src/paths.rs @@ -159,3 +159,7 @@ pub const WEAK_RC: [&str; 3] = ["alloc", "rc", "Weak"]; pub const PTR_NON_NULL: [&str; 4] = ["core", "ptr", "non_null", "NonNull"]; pub const INSTANT_NOW: [&str; 4] = ["std", "time", "Instant", "now"]; pub const INSTANT: [&str; 3] = ["std", "time", "Instant"]; +pub const VEC_IS_EMPTY: [&str; 4] = ["alloc", "vec", "Vec", "is_empty"]; +pub const VEC_POP: [&str; 4] = ["alloc", "vec", "Vec", "pop"]; +pub const OPTION_UNWRAP: [&str; 4] = ["core", "option", "Option", "unwrap"]; +pub const OPTION_EXPECT: [&str; 4] = ["core", "option", "Option", "expect"]; diff --git a/tests/ui/manual_while_let_some.fixed b/tests/ui/manual_while_let_some.fixed new file mode 100644 index 000000000000..8b610919536c --- /dev/null +++ b/tests/ui/manual_while_let_some.fixed @@ -0,0 +1,93 @@ +//@run-rustfix + +#![allow(unused)] +#![warn(clippy::manual_while_let_some)] + +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 comes from 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/manual_while_let_some.rs b/tests/ui/manual_while_let_some.rs new file mode 100644 index 000000000000..85a0a084a424 --- /dev/null +++ b/tests/ui/manual_while_let_some.rs @@ -0,0 +1,93 @@ +//@run-rustfix + +#![allow(unused)] +#![warn(clippy::manual_while_let_some)] + +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 !numbers.is_empty() { + let number = numbers.pop().unwrap(); + } + + let mut val = VecInStruct { + numbers: vec![1, 2, 3, 4, 5], + unrelated: String::new(), + }; + while !val.numbers.is_empty() { + let number = val.numbers.pop().unwrap(); + } + + while !numbers.is_empty() { + accept_i32(numbers.pop().unwrap()); + } + + while !numbers.is_empty() { + accept_i32(numbers.pop().expect("")); + } + + // 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 comes from 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/manual_while_let_some.stderr b/tests/ui/manual_while_let_some.stderr new file mode 100644 index 000000000000..633fe05c49b8 --- /dev/null +++ b/tests/ui/manual_while_let_some.stderr @@ -0,0 +1,87 @@ +error: you seem to be trying to pop elements from a `Vec` in a loop + --> $DIR/manual_while_let_some.rs:23:9 + | +LL | let number = numbers.pop().unwrap(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: `-D clippy::manual-while-let-some` 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/manual_while_let_some.rs:31:9 + | +LL | let number = val.numbers.pop().unwrap(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: consider using a `while..let` loop + | +LL ~ while let Some(number) = val.numbers.pop() { +LL ~ + | + +error: you seem to be trying to pop elements from a `Vec` in a loop + --> $DIR/manual_while_let_some.rs:35:20 + | +LL | accept_i32(numbers.pop().unwrap()); + | ^^^^^^^^^^^^^^^^^^^^^^ + | +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/manual_while_let_some.rs:39:20 + | +LL | accept_i32(numbers.pop().expect("")); + | ^^^^^^^^^^^^^^^^^^^^^^^^ + | +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/manual_while_let_some.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/manual_while_let_some.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/manual_while_let_some.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 +