diff options
| -rw-r--r-- | CHANGELOG.md | 1 | ||||
| -rw-r--r-- | clippy_lints/src/collection_is_never_read.rs | 123 | ||||
| -rw-r--r-- | clippy_lints/src/declared_lints.rs | 1 | ||||
| -rw-r--r-- | clippy_lints/src/lib.rs | 2 | ||||
| -rw-r--r-- | tests/ui/collection_is_never_read.rs | 116 | ||||
| -rw-r--r-- | tests/ui/collection_is_never_read.stderr | 40 |
6 files changed, 283 insertions, 0 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index 765826ed867..f31e9825a17 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 00000000000..fbc66a2155f --- /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 cd5dd7a5706..470a2e79e47 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 145cf524652..c6ad7940b81 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 00000000000..2c37fc212b0 --- /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::<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); +} diff --git a/tests/ui/collection_is_never_read.stderr b/tests/ui/collection_is_never_read.stderr new file mode 100644 index 00000000000..43349f550a6 --- /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 + |