From 2a9c254e23989df2faaaeb102e5fea8ce948f45e Mon Sep 17 00:00:00 2001 From: Michael Schubart Date: Thu, 16 Feb 2023 18:12:11 +0000 Subject: [PATCH 1/7] Add `collection_is_never_read` --- CHANGELOG.md | 1 + clippy_lints/src/collection_is_never_read.rs | 123 +++++++++++++++++++ clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/lib.rs | 2 + tests/ui/collection_is_never_read.rs | 116 +++++++++++++++++ tests/ui/collection_is_never_read.stderr | 40 ++++++ 6 files changed, 283 insertions(+) create mode 100644 clippy_lints/src/collection_is_never_read.rs create mode 100644 tests/ui/collection_is_never_read.rs create mode 100644 tests/ui/collection_is_never_read.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 765826ed867d0..f31e9825a17a4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4307,6 +4307,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 diff --git a/clippy_lints/src/collection_is_never_read.rs b/clippy_lints/src/collection_is_never_read.rs new file mode 100644 index 0000000000000..fbc66a2155f4b --- /dev/null +++ b/clippy_lints/src/collection_is_never_read.rs @@ -0,0 +1,123 @@ +use clippy_utils::diagnostics::span_lint; +use clippy_utils::get_enclosing_block; +use clippy_utils::get_parent_node; +use clippy_utils::path_to_local_id; +use clippy_utils::ty::is_type_diagnostic_item; +use clippy_utils::visitors::for_each_expr_with_closures; +use core::ops::ControlFlow; +use rustc_hir::Block; +use rustc_hir::ExprKind; +use rustc_hir::HirId; +use rustc_hir::Local; +use rustc_hir::Node; +use rustc_hir::PatKind; +use rustc_lint::LateContext; +use rustc_lint::LateLintPass; +use rustc_session::declare_lint_pass; +use rustc_session::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 mut sorted_samples = samples.clone(); + /// sorted_samples.sort(); + /// for sample in &samples { // Oops, meant to use `sorted_samples`. + /// println!("{sample}"); + /// } + /// ``` + /// Use instead: + /// ```rust + /// 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`. + 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) + { + 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 +} diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index cd5dd7a570653..470a2e79e4796 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/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, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 145cf524652f1..c6ad7940b81a1 100644 --- a/clippy_lints/src/lib.rs +++ b/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` } diff --git a/tests/ui/collection_is_never_read.rs b/tests/ui/collection_is_never_read.rs new file mode 100644 index 0000000000000..2c37fc212b033 --- /dev/null +++ b/tests/ui/collection_is_never_read.rs @@ -0,0 +1,116 @@ +#![allow(unused)] +#![warn(clippy::collection_is_never_read)] + +use std::collections::HashMap; + +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::::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::::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); +} diff --git a/tests/ui/collection_is_never_read.stderr b/tests/ui/collection_is_never_read.stderr new file mode 100644 index 0000000000000..43349f550a6cd --- /dev/null +++ b/tests/ui/collection_is_never_read.stderr @@ -0,0 +1,40 @@ +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: aborting due to 6 previous errors + From 08708198fbb8eb4bd8df76bded9fcdb32adb8163 Mon Sep 17 00:00:00 2001 From: Michael Schubart Date: Mon, 27 Feb 2023 19:31:59 +0000 Subject: [PATCH 2/7] Fix lint documentation --- clippy_lints/src/collection_is_never_read.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/clippy_lints/src/collection_is_never_read.rs b/clippy_lints/src/collection_is_never_read.rs index fbc66a2155f4b..f20c09a444821 100644 --- a/clippy_lints/src/collection_is_never_read.rs +++ b/clippy_lints/src/collection_is_never_read.rs @@ -30,6 +30,7 @@ declare_clippy_lint! { /// /// ### 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`. @@ -38,6 +39,7 @@ declare_clippy_lint! { /// ``` /// Use instead: /// ```rust + /// # let samples = vec![3, 1, 2]; /// let mut sorted_samples = samples.clone(); /// sorted_samples.sort(); /// for sample in &sorted_samples { From 5770d40d8ea3ba8d5a41db5e5914b45f32d80a9f Mon Sep 17 00:00:00 2001 From: Michael Schubart Date: Tue, 28 Feb 2023 21:47:07 +0000 Subject: [PATCH 3/7] More compact `use` statements --- clippy_lints/src/collection_is_never_read.rs | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/clippy_lints/src/collection_is_never_read.rs b/clippy_lints/src/collection_is_never_read.rs index f20c09a444821..c0c231bb2c79f 100644 --- a/clippy_lints/src/collection_is_never_read.rs +++ b/clippy_lints/src/collection_is_never_read.rs @@ -1,20 +1,11 @@ use clippy_utils::diagnostics::span_lint; -use clippy_utils::get_enclosing_block; -use clippy_utils::get_parent_node; -use clippy_utils::path_to_local_id; 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; -use rustc_hir::ExprKind; -use rustc_hir::HirId; -use rustc_hir::Local; -use rustc_hir::Node; -use rustc_hir::PatKind; -use rustc_lint::LateContext; -use rustc_lint::LateLintPass; -use rustc_session::declare_lint_pass; -use rustc_session::declare_tool_lint; +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; From cb3bb8a5b5449f330cba8544a30d1f7e1d5f16d3 Mon Sep 17 00:00:00 2001 From: Michael Schubart Date: Tue, 28 Feb 2023 22:01:48 +0000 Subject: [PATCH 4/7] Add tests that show: insert() can be a read or not --- tests/ui/collection_is_never_read.rs | 14 +++++++++++++- tests/ui/collection_is_never_read.stderr | 8 +++++++- 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/tests/ui/collection_is_never_read.rs b/tests/ui/collection_is_never_read.rs index 2c37fc212b033..28ec4bf0aeb47 100644 --- a/tests/ui/collection_is_never_read.rs +++ b/tests/ui/collection_is_never_read.rs @@ -1,7 +1,7 @@ #![allow(unused)] #![warn(clippy::collection_is_never_read)] -use std::collections::HashMap; +use std::collections::{HashSet, HashMap}; fn main() {} @@ -114,3 +114,15 @@ fn method_argument_but_not_target() { 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"); + } +} diff --git a/tests/ui/collection_is_never_read.stderr b/tests/ui/collection_is_never_read.stderr index 43349f550a6cd..d66b833c5225e 100644 --- a/tests/ui/collection_is_never_read.stderr +++ b/tests/ui/collection_is_never_read.stderr @@ -36,5 +36,11 @@ error: collection is never read LL | let mut x = vec![1, 2, 3]; // WARNING | ^^^^^^^^^^^^^^^^^^^^^^^^^^ -error: aborting due to 6 previous errors +error: collection is never read + --> $DIR/collection_is_never_read.rs:119:5 + | +LL | let mut x = HashSet::new(); // WARNING + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: aborting due to 7 previous errors From fbb7fd59c35095645fafc17ba573817cef68ffcf Mon Sep 17 00:00:00 2001 From: Michael Schubart Date: Tue, 28 Feb 2023 22:08:15 +0000 Subject: [PATCH 5/7] Add test for an interesting edge case --- tests/ui/collection_is_never_read.rs | 9 ++++++++- tests/ui/collection_is_never_read.stderr | 8 +++++++- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/tests/ui/collection_is_never_read.rs b/tests/ui/collection_is_never_read.rs index 28ec4bf0aeb47..8f5ceb06b898b 100644 --- a/tests/ui/collection_is_never_read.rs +++ b/tests/ui/collection_is_never_read.rs @@ -1,7 +1,7 @@ #![allow(unused)] #![warn(clippy::collection_is_never_read)] -use std::collections::{HashSet, HashMap}; +use std::collections::{HashMap, HashSet}; fn main() {} @@ -126,3 +126,10 @@ fn insert_is_a_read() { 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(); +} diff --git a/tests/ui/collection_is_never_read.stderr b/tests/ui/collection_is_never_read.stderr index d66b833c5225e..7654b74be3d17 100644 --- a/tests/ui/collection_is_never_read.stderr +++ b/tests/ui/collection_is_never_read.stderr @@ -42,5 +42,11 @@ error: collection is never read LL | let mut x = HashSet::new(); // WARNING | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ -error: aborting due to 7 previous errors +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 From 85ad8a6fdcd2b62b04219592966152196b0a98d7 Mon Sep 17 00:00:00 2001 From: Michael Schubart Date: Mon, 6 Mar 2023 07:16:36 +0000 Subject: [PATCH 6/7] Avoid false positives from extension traits --- clippy_lints/src/collection_is_never_read.rs | 6 ++++++ tests/ui/collection_is_never_read.rs | 20 ++++++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/clippy_lints/src/collection_is_never_read.rs b/clippy_lints/src/collection_is_never_read.rs index c0c231bb2c79f..10f2bef268a24 100644 --- a/clippy_lints/src/collection_is_never_read.rs +++ b/clippy_lints/src/collection_is_never_read.rs @@ -98,10 +98,16 @@ fn has_no_read_access<'tcx>(cx: &LateContext<'tcx>, id: HirId, block: &'tcx Bloc // 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(()); } diff --git a/tests/ui/collection_is_never_read.rs b/tests/ui/collection_is_never_read.rs index 8f5ceb06b898b..49c72e7eefec2 100644 --- a/tests/ui/collection_is_never_read.rs +++ b/tests/ui/collection_is_never_read.rs @@ -133,3 +133,23 @@ fn not_read_if_return_value_not_used() { let x = vec![1, 2, 3]; // WARNING x.is_empty(); } + +fn extension_traits() { + trait VecExt { + fn method_with_side_effect(&self); + fn method_without_side_effect(&self); + } + + impl VecExt for Vec { + 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(); +} From 4ee65535a708d6493b1a1d6276e6d06f8e156e14 Mon Sep 17 00:00:00 2001 From: Michael Schubart Date: Mon, 6 Mar 2023 22:19:34 +0000 Subject: [PATCH 7/7] Add test where container is passed to a function --- tests/ui/collection_is_never_read.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/ui/collection_is_never_read.rs b/tests/ui/collection_is_never_read.rs index 49c72e7eefec2..068a49486cf8a 100644 --- a/tests/ui/collection_is_never_read.rs +++ b/tests/ui/collection_is_never_read.rs @@ -153,3 +153,13 @@ fn extension_traits() { let y = vec![1, 2, 3]; // Ok (false negative) y.method_without_side_effect(); } + +fn function_argument() { + #[allow(clippy::ptr_arg)] + fn foo(v: &Vec) -> usize { + v.len() + } + + let x = vec![1, 2, 3]; // Ok + foo(&x); +}