Merge commit '371120bdbf' into clippyup

clippy_lints/src/loops/manual_memcpy.rs

@@ -15,7 +15,7 @@ use rustc_span::symbol::sym;
 use std::fmt::Display;
 use std::iter::Iterator;
 
-/// Checks for for loops that sequentially copy items from one slice-like
+/// Checks for `for` loops that sequentially copy items from one slice-like
 /// object to another.
 pub(super) fn check<'tcx>(
     cx: &LateContext<'tcx>,

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/loops/needless_range_loop.rs

@@ -208,7 +208,7 @@ fn is_end_eq_array_len<'tcx>(
     indexed_ty: Ty<'tcx>,
 ) -> bool {
     if_chain! {
-        if let ExprKind::Lit(ref lit) = end.kind;
+        if let ExprKind::Lit(lit) = end.kind;
         if let ast::LitKind::Int(end_int, _) = lit.node;
         if let ty::Array(_, arr_len_const) = indexed_ty.kind();
         if let Some(arr_len) = arr_len_const.try_eval_target_usize(cx.tcx, cx.param_env);