new lint: while_pop_unwrap

CHANGELOG.md

@@ -5160,6 +5160,7 @@ Released 2018-09-13
 [`while_immutable_condition`]: https://rust-lang.github.io/rust-clippy/master/index.html#while_immutable_condition
 [`while_let_loop`]: https://rust-lang.github.io/rust-clippy/master/index.html#while_let_loop
 [`while_let_on_iterator`]: https://rust-lang.github.io/rust-clippy/master/index.html#while_let_on_iterator
+[`while_pop_unwrap`]: https://rust-lang.github.io/rust-clippy/master/index.html#while_pop_unwrap
 [`wildcard_dependencies`]: https://rust-lang.github.io/rust-clippy/master/index.html#wildcard_dependencies
 [`wildcard_enum_match_arm`]: https://rust-lang.github.io/rust-clippy/master/index.html#wildcard_enum_match_arm
 [`wildcard_imports`]: https://rust-lang.github.io/rust-clippy/master/index.html#wildcard_imports

clippy_lints/src/declared_lints.rs

@@ -645,6 +645,7 @@
     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,

clippy_lints/src/lib.rs

@@ -324,6 +324,7 @@
 mod useless_conversion;
 mod vec;
 mod vec_init_then_push;
+mod while_pop_unwrap;
 mod wildcard_imports;
 mod write;
 mod zero_div_zero;

clippy_lints/src/loops/mod.rs

@@ -25,6 +25,8 @@
 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
@@ -643,6 +645,7 @@ fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
         if let Some(higher::While { condition, body }) = 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);
         }
     }
 }

clippy_lints/src/while_pop_unwrap.rs

@@ -0,0 +1,126 @@
+use clippy_utils::{diagnostics::span_lint_and_then, match_def_path, paths, source::snippet};
+use rustc_errors::Applicability;
+use rustc_hir::*;
+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<'tcx>(
+    cx: &LateContext<'tcx>,
+    pop_span: Span,
+    ident: Option<Ident>,
+    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(Ident::as_str).unwrap_or("element"),
+                    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<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'_>) -> bool {
+    match_method_call(cx, expr, &paths::VEC_POP)
+}
+
+fn is_vec_pop_unwrap<'tcx>(cx: &LateContext<'tcx>, 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<'tcx>(cx: &LateContext<'tcx>, 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<'tcx>(cx: &LateContext<'tcx>, 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(|| 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);
+    }
+}

clippy_utils/src/paths.rs

@@ -159,3 +159,7 @@
 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/while_pop_unwrap.rs

@@ -0,0 +1,47 @@
+#![allow(unused)]
+#![warn(clippy::while_pop_unwrap)]
+
+struct VecInStruct {
+    numbers: Vec<i32>,
+    unrelated: String,
+}
+
+fn accept_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());
+    }
+}

tests/ui/while_pop_unwrap.stderr

@@ -0,0 +1,43 @@
+error: you seem to be trying to pop elements from a `Vec` in a loop
+  --> $DIR/while_pop_unwrap.rs:14:22
+   |
+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`
+
+error: you seem to be trying to pop elements from a `Vec` in a loop
+  --> $DIR/while_pop_unwrap.rs:22:22
+   |
+LL |     while !val.numbers.is_empty() {
+   |            ---------------------- help: try: `while let Some(number) = val.numbers.pop()`
+LL |         let number = val.numbers.pop().unwrap();
+   |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = 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
+   |
+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
+
+error: you seem to be trying to pop elements from a `Vec` in a loop
+  --> $DIR/while_pop_unwrap.rs:30: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
+
+error: aborting due to 4 previous errors
+