Auto merge of #10647 - y21:while_pop_unwrap, r=llogiq

new lint: `manual_while_let_some`

This PR implements the lint I suggested [on zulip](https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/lint.20on.20while.20pop.20unwrap).
It looks for while loops like these:
```rs
let mut numbers = vec![0, 1, 2];
while !numbers.is_empty() {
  let number = numbers.pop().unwrap();
  // use `number`
}
```
and suggests replacing it with a while-let loop, like this:
```rs
let mut numbers = vec![0, 1, 2];
while let Some(number) = numbers.pop() {
  // use `number`
}
```
... which is more concise and idiomatic.

It only looks for `Vec::pop()` calls in the first statement of the loop body in an attempt to not trigger FPs (as pop might only be called conditionally).

changelog: new lint [`manual_while_let_some`]

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

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,

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);
+    }
+}

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);
         }
     }
 }

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,

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
     }

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"];

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<i32>,
+    unrelated: String,
+}
+
+struct Foo {
+    a: i32,
+    b: i32,
+}
+
+fn accept_i32(_: i32) {}
+fn accept_optional_i32(_: Option<i32>) {}
+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<String> = 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() {
+
+    }
+}