Auto merge of rust-lang#10614 - bluthej:clear-with-drain, r=Manishearth

Clear with drain

Fixes rust-lang#10572: both the original intent of the issue (extending `clear_with_drain`) and the false negative for `collection_is_never_read` I found in the process are fixed by this PR.

changelog: [`clear_with_drain`]: extend to 5 other types of containers. [`collection_is_never_read`]: fix false negative for `String`s.

Author: bors

clippy_lints/src/collection_is_never_read.rs

@@ -1,9 +1,9 @@
 use clippy_utils::diagnostics::span_lint;
-use clippy_utils::ty::is_type_diagnostic_item;
+use clippy_utils::ty::{is_type_diagnostic_item, is_type_lang_item};
 use clippy_utils::visitors::for_each_expr_with_closures;
 use clippy_utils::{get_enclosing_block, get_parent_node, path_to_local_id};
 use core::ops::ControlFlow;
-use rustc_hir::{Block, ExprKind, HirId, Local, Node, PatKind};
+use rustc_hir::{Block, ExprKind, HirId, LangItem, Local, Node, PatKind};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_session::{declare_lint_pass, declare_tool_lint};
 use rustc_span::symbol::sym;
@@ -44,24 +44,23 @@ declare_clippy_lint! {
 }
 declare_lint_pass!(CollectionIsNeverRead => [COLLECTION_IS_NEVER_READ]);
 
-static COLLECTIONS: [Symbol; 10] = [
+// Add `String` here when it is added to diagnostic items
+static COLLECTIONS: [Symbol; 9] = [
     sym::BTreeMap,
     sym::BTreeSet,
     sym::BinaryHeap,
     sym::HashMap,
     sym::HashSet,
     sym::LinkedList,
     sym::Option,
-    sym::String,
     sym::Vec,
     sym::VecDeque,
 ];
 
 impl<'tcx> LateLintPass<'tcx> for CollectionIsNeverRead {
     fn check_local(&mut self, cx: &LateContext<'tcx>, local: &'tcx Local<'tcx>) {
         // Look for local variables whose type is a container. Search surrounding bock for read access.
-        let ty = cx.typeck_results().pat_ty(local.pat);
-        if COLLECTIONS.iter().any(|&sym| is_type_diagnostic_item(cx, ty, sym))
+        if match_acceptable_type(cx, local, &COLLECTIONS)
             && let PatKind::Binding(_, local_id, _, _) = local.pat.kind
             && let Some(enclosing_block) = get_enclosing_block(cx, local.hir_id)
             && has_no_read_access(cx, local_id, enclosing_block)
@@ -71,6 +70,13 @@ impl<'tcx> LateLintPass<'tcx> for CollectionIsNeverRead {
     }
 }
 
+fn match_acceptable_type(cx: &LateContext<'_>, local: &Local<'_>, collections: &[rustc_span::Symbol]) -> bool {
+    let ty = cx.typeck_results().pat_ty(local.pat);
+    collections.iter().any(|&sym| is_type_diagnostic_item(cx, ty, sym))
+    // String type is a lang item but not a diagnostic item for now so we need a separate check
+        || is_type_lang_item(cx, ty, LangItem::String)
+}
+
 fn has_no_read_access<'tcx>(cx: &LateContext<'tcx>, id: HirId, block: &'tcx Block<'tcx>) -> bool {
     let mut has_access = false;
     let mut has_read_access = false;

clippy_lints/src/methods/clear_with_drain.rs

@@ -1,25 +1,50 @@
 use clippy_utils::diagnostics::span_lint_and_sugg;
 use clippy_utils::is_range_full;
-use clippy_utils::ty::is_type_diagnostic_item;
+use clippy_utils::ty::{is_type_diagnostic_item, is_type_lang_item};
 use rustc_errors::Applicability;
-use rustc_hir::{Expr, ExprKind, QPath};
+use rustc_hir as hir;
+use rustc_hir::{Expr, ExprKind, LangItem, QPath};
 use rustc_lint::LateContext;
 use rustc_span::symbol::sym;
 use rustc_span::Span;
 
 use super::CLEAR_WITH_DRAIN;
 
-pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, recv: &Expr<'_>, span: Span, arg: &Expr<'_>) {
-    let ty = cx.typeck_results().expr_ty(recv);
-    if is_type_diagnostic_item(cx, ty, sym::Vec)
-        && let ExprKind::Path(QPath::Resolved(None, container_path)) = recv.kind
-        && is_range_full(cx, arg, Some(container_path))
+// Add `String` here when it is added to diagnostic items
+const ACCEPTABLE_TYPES_WITH_ARG: [rustc_span::Symbol; 2] = [sym::Vec, sym::VecDeque];
+
+const ACCEPTABLE_TYPES_WITHOUT_ARG: [rustc_span::Symbol; 3] = [sym::BinaryHeap, sym::HashMap, sym::HashSet];
+
+pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, recv: &Expr<'_>, span: Span, arg: Option<&Expr<'_>>) {
+    if let Some(arg) = arg {
+        if match_acceptable_type(cx, recv, &ACCEPTABLE_TYPES_WITH_ARG)
+            && let ExprKind::Path(QPath::Resolved(None, container_path)) = recv.kind
+            && is_range_full(cx, arg, Some(container_path))
+        {
+            suggest(cx, expr, recv, span);
+        }
+    } else if match_acceptable_type(cx, recv, &ACCEPTABLE_TYPES_WITHOUT_ARG) {
+        suggest(cx, expr, recv, span);
+    }
+}
+
+fn match_acceptable_type(cx: &LateContext<'_>, expr: &hir::Expr<'_>, types: &[rustc_span::Symbol]) -> bool {
+    let expr_ty = cx.typeck_results().expr_ty(expr).peel_refs();
+    types.iter().any(|&ty| is_type_diagnostic_item(cx, expr_ty, ty))
+    // String type is a lang item but not a diagnostic item for now so we need a separate check
+        || is_type_lang_item(cx, expr_ty, LangItem::String)
+}
+
+fn suggest(cx: &LateContext<'_>, expr: &Expr<'_>, recv: &Expr<'_>, span: Span) {
+    if let Some(adt) = cx.typeck_results().expr_ty(recv).ty_adt_def()
+    // Use `opt_item_name` while `String` is not a diagnostic item
+        && let Some(ty_name) = cx.tcx.opt_item_name(adt.did())
     {
         span_lint_and_sugg(
             cx,
             CLEAR_WITH_DRAIN,
             span.with_hi(expr.span.hi()),
-            "`drain` used to clear a `Vec`",
+            &format!("`drain` used to clear a `{ty_name}`"),
             "try",
             "clear()".to_string(),
             Applicability::MachineApplicable,

clippy_lints/src/methods/mod.rs

@@ -3193,7 +3193,7 @@ declare_clippy_lint! {
 
 declare_clippy_lint! {
     /// ### What it does
-    /// Checks for usage of `.drain(..)` for the sole purpose of clearing a `Vec`.
+    /// Checks for usage of `.drain(..)` for the sole purpose of clearing a container.
     ///
     /// ### Why is this bad?
     /// This creates an unnecessary iterator that is dropped immediately.
@@ -3213,7 +3213,7 @@ declare_clippy_lint! {
     #[clippy::version = "1.69.0"]
     pub CLEAR_WITH_DRAIN,
     nursery,
-    "calling `drain` in order to `clear` a `Vec`"
+    "calling `drain` in order to `clear` a container"
 }
 
 pub struct Methods {
@@ -3589,12 +3589,13 @@ impl Methods {
                     Some(("bytes", recv2, [], _, _)) => bytes_count_to_len::check(cx, expr, recv, recv2),
                     _ => {},
                 },
-                ("drain", [arg]) => {
-                if let Node::Stmt(Stmt { hir_id: _, kind, .. }) = cx.tcx.hir().get_parent(expr.hir_id)
-                    && matches!(kind, StmtKind::Semi(_))
+                ("drain", ..) => {
+                    if let Node::Stmt(Stmt { hir_id: _, kind, .. }) = cx.tcx.hir().get_parent(expr.hir_id)
+                        && matches!(kind, StmtKind::Semi(_))
+                        && args.len() <= 1
                     {
-                        clear_with_drain::check(cx, expr, recv, span, arg);
-                    } else {
+                        clear_with_drain::check(cx, expr, recv, span, args.first());
+                    } else if let [arg] = args {
                         iter_with_drain::check(cx, expr, recv, span, arg);
                     }
                 },