Auto merge of rust-lang#10415 - schubart:collection_is_never_read, r=…

…llogiq

Add `collection_is_never_read`

Fixes rust-lang#9267

`@flip1995` and `@llogiq,` I talked with you about this one at Rust Nation in London last week. :-)

This is my first contribution to Clippy, so lots of feedback would be greatly appreciated.

- \[ ] Followed [lint naming conventions][lint_naming]
- \[x] Added passing UI tests (including committed `.stderr` file)
- \[x] `cargo test` passes locally
- \[x] Executed `cargo dev update_lints`
- \[x] Added lint documentation
- \[x] Run `cargo dev fmt`

`dogfood` found one true positive (see rust-lang#9509) and no false positives.

`lintcheck` found no (true or false) positives, even when running on an extended set of crates.

---

changelog: new lint [`collection_is_never_read`]
[rust-lang#10415](rust-lang/rust-clippy#10415)
<!-- changelog_checked -->

CHANGELOG.md

@@ -4452,6 +4452,7 @@ Released 2018-09-13
 [`collapsible_if`]: https://rust-lang.github.io/rust-clippy/master/index.html#collapsible_if
 [`collapsible_match`]: https://rust-lang.github.io/rust-clippy/master/index.html#collapsible_match
 [`collapsible_str_replace`]: https://rust-lang.github.io/rust-clippy/master/index.html#collapsible_str_replace
+[`collection_is_never_read`]: https://rust-lang.github.io/rust-clippy/master/index.html#collection_is_never_read
 [`comparison_chain`]: https://rust-lang.github.io/rust-clippy/master/index.html#comparison_chain
 [`comparison_to_empty`]: https://rust-lang.github.io/rust-clippy/master/index.html#comparison_to_empty
 [`const_static_lifetime`]: https://rust-lang.github.io/rust-clippy/master/index.html#const_static_lifetime

clippy_lints/src/collection_is_never_read.rs

@@ -0,0 +1,122 @@
+use clippy_utils::diagnostics::span_lint;
+use clippy_utils::ty::is_type_diagnostic_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_lint::{LateContext, LateLintPass};
+use rustc_session::{declare_lint_pass, declare_tool_lint};
+use rustc_span::symbol::sym;
+use rustc_span::Symbol;
+
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for collections that are never queried.
+    ///
+    /// ### Why is this bad?
+    /// Putting effort into constructing a collection but then never querying it might indicate that
+    /// the author forgot to do whatever they intended to do with the collection. Example: Clone
+    /// a vector, sort it for iteration, but then mistakenly iterate the original vector
+    /// instead.
+    ///
+    /// ### Example
+    /// ```rust
+    /// # let samples = vec![3, 1, 2];
+    /// let mut sorted_samples = samples.clone();
+    /// sorted_samples.sort();
+    /// for sample in &samples { // Oops, meant to use `sorted_samples`.
+    ///     println!("{sample}");
+    /// }
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// # let samples = vec![3, 1, 2];
+    /// let mut sorted_samples = samples.clone();
+    /// sorted_samples.sort();
+    /// for sample in &sorted_samples {
+    ///     println!("{sample}");
+    /// }
+    /// ```
+    #[clippy::version = "1.69.0"]
+    pub COLLECTION_IS_NEVER_READ,
+    nursery,
+    "a collection is never queried"
+}
+declare_lint_pass!(CollectionIsNeverRead => [COLLECTION_IS_NEVER_READ]);
+
+static COLLECTIONS: [Symbol; 10] = [
+    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))
+            && 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)
+        {
+            span_lint(cx, COLLECTION_IS_NEVER_READ, local.span, "collection is never read");
+        }
+    }
+}
+
+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;
+
+    // Inspect all expressions and sub-expressions in the block.
+    for_each_expr_with_closures(cx, block, |expr| {
+        // Ignore expressions that are not simply `id`.
+        if !path_to_local_id(expr, id) {
+            return ControlFlow::Continue(());
+        }
+
+        // `id` is being accessed. Investigate if it's a read access.
+        has_access = true;
+
+        // `id` appearing in the left-hand side of an assignment is not a read access:
+        //
+        // id = ...; // Not reading `id`.
+        if let Some(Node::Expr(parent)) = get_parent_node(cx.tcx, expr.hir_id)
+            && let ExprKind::Assign(lhs, ..) = parent.kind
+            && path_to_local_id(lhs, id)
+        {
+            return ControlFlow::Continue(());
+        }
+
+        // Method call on `id` in a statement ignores any return value, so it's not a read access:
+        //
+        // id.foo(...); // Not reading `id`.
+        //
+        // Only assuming this for "official" methods defined on the type. For methods defined in extension
+        // traits (identified as local, based on the orphan rule), pessimistically assume that they might
+        // have side effects, so consider them a read.
+        if let Some(Node::Expr(parent)) = get_parent_node(cx.tcx, expr.hir_id)
+            && let ExprKind::MethodCall(_, receiver, _, _) = parent.kind
+            && path_to_local_id(receiver, id)
+            && let Some(Node::Stmt(..)) = get_parent_node(cx.tcx, parent.hir_id)
+            && let Some(method_def_id) = cx.typeck_results().type_dependent_def_id(parent.hir_id)
+            && !method_def_id.is_local()
+        {
+            return ControlFlow::Continue(());
+        }
+
+        // Any other access to `id` is a read access. Stop searching.
+        has_read_access = true;
+        ControlFlow::Break(())
+    });
+
+    // Ignore collections that have no access at all. Other lints should catch them.
+    has_access && !has_read_access
+}

clippy_lints/src/declared_lints.rs

@@ -92,6 +92,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
     crate::cognitive_complexity::COGNITIVE_COMPLEXITY_INFO,
     crate::collapsible_if::COLLAPSIBLE_ELSE_IF_INFO,
     crate::collapsible_if::COLLAPSIBLE_IF_INFO,
+    crate::collection_is_never_read::COLLECTION_IS_NEVER_READ_INFO,
     crate::comparison_chain::COMPARISON_CHAIN_INFO,
     crate::copies::BRANCHES_SHARING_CODE_INFO,
     crate::copies::IFS_SAME_COND_INFO,

clippy_lints/src/lib.rs

@@ -87,6 +87,7 @@ mod casts;
 mod checked_conversions;
 mod cognitive_complexity;
 mod collapsible_if;
+mod collection_is_never_read;
 mod comparison_chain;
 mod copies;
 mod copy_iterator;
@@ -924,6 +925,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         ))
     });
     store.register_late_pass(|_| Box::new(no_mangle_with_rust_abi::NoMangleWithRustAbi));
+    store.register_late_pass(|_| Box::new(collection_is_never_read::CollectionIsNeverRead));
     // add lints here, do not remove this comment, it's used in `new_lint`
 }
 

tests/ui/collection_is_never_read.rs

@@ -0,0 +1,165 @@
+#![allow(unused)]
+#![warn(clippy::collection_is_never_read)]
+
+use std::collections::{HashMap, HashSet};
+
+fn main() {}
+
+fn not_a_collection() {
+    // TODO: Expand `collection_is_never_read` beyond collections?
+    let mut x = 10; // Ok
+    x += 1;
+}
+
+fn no_access_at_all() {
+    // Other lints should catch this.
+    let x = vec![1, 2, 3]; // Ok
+}
+
+fn write_without_read() {
+    // The main use case for `collection_is_never_read`.
+    let mut x = HashMap::new(); // WARNING
+    x.insert(1, 2);
+}
+
+fn read_without_write() {
+    let mut x = vec![1, 2, 3]; // Ok
+    let _ = x.len();
+}
+
+fn write_and_read() {
+    let mut x = vec![1, 2, 3]; // Ok
+    x.push(4);
+    let _ = x.len();
+}
+
+fn write_after_read() {
+    // TODO: Warn here, but this requires more extensive data flow analysis.
+    let mut x = vec![1, 2, 3]; // Ok
+    let _ = x.len();
+    x.push(4); // Pointless
+}
+
+fn write_before_reassign() {
+    // TODO: Warn here, but this requires more extensive data flow analysis.
+    let mut x = HashMap::new(); // Ok
+    x.insert(1, 2); // Pointless
+    x = HashMap::new();
+    let _ = x.len();
+}
+
+fn read_in_closure() {
+    let mut x = HashMap::new(); // Ok
+    x.insert(1, 2);
+    let _ = || {
+        let _ = x.len();
+    };
+}
+
+fn write_in_closure() {
+    let mut x = vec![1, 2, 3]; // WARNING
+    let _ = || {
+        x.push(4);
+    };
+}
+
+fn read_in_format() {
+    let mut x = HashMap::new(); // Ok
+    x.insert(1, 2);
+    format!("{x:?}");
+}
+
+fn shadowing_1() {
+    let x = HashMap::<usize, usize>::new(); // Ok
+    let _ = x.len();
+    let mut x = HashMap::new(); // WARNING
+    x.insert(1, 2);
+}
+
+fn shadowing_2() {
+    let mut x = HashMap::new(); // WARNING
+    x.insert(1, 2);
+    let x = HashMap::<usize, usize>::new(); // Ok
+    let _ = x.len();
+}
+
+#[allow(clippy::let_unit_value)]
+fn fake_read() {
+    let mut x = vec![1, 2, 3]; // Ok
+    x.reverse();
+    // `collection_is_never_read` gets fooled, but other lints should catch this.
+    let _: () = x.clear();
+}
+
+fn assignment() {
+    let mut x = vec![1, 2, 3]; // WARNING
+    let y = vec![4, 5, 6]; // Ok
+    x = y;
+}
+
+#[allow(clippy::self_assignment)]
+fn self_assignment() {
+    let mut x = vec![1, 2, 3]; // WARNING
+    x = x;
+}
+
+fn method_argument_but_not_target() {
+    struct MyStruct;
+    impl MyStruct {
+        fn my_method(&self, _argument: &[usize]) {}
+    }
+    let my_struct = MyStruct;
+
+    let mut x = vec![1, 2, 3]; // Ok
+    x.reverse();
+    my_struct.my_method(&x);
+}
+
+fn insert_is_not_a_read() {
+    let mut x = HashSet::new(); // WARNING
+    x.insert(5);
+}
+
+fn insert_is_a_read() {
+    let mut x = HashSet::new(); // Ok
+    if x.insert(5) {
+        println!("5 was inserted");
+    }
+}
+
+fn not_read_if_return_value_not_used() {
+    // `is_empty` does not modify the set, so it's a query. But since the return value is not used, the
+    // lint does not consider it a read here.
+    let x = vec![1, 2, 3]; // WARNING
+    x.is_empty();
+}
+
+fn extension_traits() {
+    trait VecExt<T> {
+        fn method_with_side_effect(&self);
+        fn method_without_side_effect(&self);
+    }
+
+    impl<T> VecExt<T> for Vec<T> {
+        fn method_with_side_effect(&self) {
+            println!("my length: {}", self.len());
+        }
+        fn method_without_side_effect(&self) {}
+    }
+
+    let x = vec![1, 2, 3]; // Ok
+    x.method_with_side_effect();
+
+    let y = vec![1, 2, 3]; // Ok (false negative)
+    y.method_without_side_effect();
+}
+
+fn function_argument() {
+    #[allow(clippy::ptr_arg)]
+    fn foo<T>(v: &Vec<T>) -> usize {
+        v.len()
+    }
+
+    let x = vec![1, 2, 3]; // Ok
+    foo(&x);
+}

tests/ui/collection_is_never_read.stderr

@@ -0,0 +1,52 @@
+error: collection is never read
+  --> $DIR/collection_is_never_read.rs:21:5
+   |
+LL |     let mut x = HashMap::new(); // WARNING
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: `-D clippy::collection-is-never-read` implied by `-D warnings`
+
+error: collection is never read
+  --> $DIR/collection_is_never_read.rs:60:5
+   |
+LL |     let mut x = vec![1, 2, 3]; // WARNING
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: collection is never read
+  --> $DIR/collection_is_never_read.rs:75:5
+   |
+LL |     let mut x = HashMap::new(); // WARNING
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: collection is never read
+  --> $DIR/collection_is_never_read.rs:80:5
+   |
+LL |     let mut x = HashMap::new(); // WARNING
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: collection is never read
+  --> $DIR/collection_is_never_read.rs:95:5
+   |
+LL |     let mut x = vec![1, 2, 3]; // WARNING
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: collection is never read
+  --> $DIR/collection_is_never_read.rs:102:5
+   |
+LL |     let mut x = vec![1, 2, 3]; // WARNING
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: collection is never read
+  --> $DIR/collection_is_never_read.rs:119:5
+   |
+LL |     let mut x = HashSet::new(); // WARNING
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: collection is never read
+  --> $DIR/collection_is_never_read.rs:133:5
+   |
+LL |     let x = vec![1, 2, 3]; // WARNING
+   |     ^^^^^^^^^^^^^^^^^^^^^^
+
+error: aborting due to 8 previous errors
+