Skip to content

Commit

Permalink
Auto merge of #10415 - schubart:collection_is_never_read, r=llogiq
Browse files Browse the repository at this point in the history
Add `collection_is_never_read`

Fixes #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 #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`]
[#10415](#10415)
<!-- changelog_checked -->
  • Loading branch information
bors committed Mar 7, 2023
2 parents 9035958 + 4ee6553 commit 41fa24c
Show file tree
Hide file tree
Showing 6 changed files with 343 additions and 0 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
122 changes: 122 additions & 0 deletions clippy_lints/src/collection_is_never_read.rs
Original file line number Diff line number Diff line change
@@ -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
}
1 change: 1 addition & 0 deletions clippy_lints/src/declared_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
2 changes: 2 additions & 0 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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`
}

Expand Down
165 changes: 165 additions & 0 deletions tests/ui/collection_is_never_read.rs
Original file line number Diff line number Diff line change
@@ -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);
}
52 changes: 52 additions & 0 deletions tests/ui/collection_is_never_read.stderr
Original file line number Diff line number Diff line change
@@ -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

0 comments on commit 41fa24c

Please sign in to comment.