Add collection_is_never_read

[schubart]

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]
• Added passing UI tests (including committed .stderr file)
• cargo test passes locally
• Executed cargo dev update_lints
• Added lint documentation
• 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

[rustbot]

r? @giraffate

(rustbot has picked a reviewer for you, use r? to override)

[xFrednet]

r? @llogiq

giraffate seems to be busy and you seem to know about this lint based on the description. You are welcome to reassign as well :)

[llogiq]

I like the general direction. The code in general looks OK, I have only a small suggestion. However, I'd like to see more tests and perhaps docs about possible known problems.

[schubart]

Thanks for your comments. Regarding docs about possible known problems: I honestly can't think of any. Using lintcheck found some false positives in earlier versions of the lint (e.g. not searching through closures) but I've addressed everything I found or could think of. I'm very open to ideas as to what might be missing.

[schubart]

I came up with a false positive:

The lint assumes that a statement x.foo(args); is not reading the collection x and that any x that is only used in this way is pointless. This assumption is wrong if foo has side effects or if it modifies args.

I'm not aware of any methods like that in the container types I'm targeting, but such methods can be added using extension traits:

    trait VecExt<T> {
        fn print_self(&self);
    }

    impl<T> VecExt<T> for Vec<T> {
        fn print_self(&self) {
            println!("my length: {}", self.len());
        }
    }

    let mut x = vec![1, 2, 3]; // Wrong warning: `collection_is_never_read`
    x.print_self();

Perhaps I should check if foo is defined via an extension trait and if so, pessimistically assume that it has side effects.

Is there a way to check where foo is defined?

[llogiq]

Regarding the possible false positive via extension trait, if the trait is defined in the local crate, we might be able to get the method by def_id. I would advise to avoid that because it only pushes the boundary one step: If your trait method calls another one, you're back to step one unless you can follow the whole chain of methods, at which point you will run the risk of a) a general performance nightmare and b) possible infinite recursion if methods call themselves. Even if you collect all the needed data on visit_crate(..), you'll recreate most of the information that rustc already has for very little gain; if only one function call is outside of your crate, the chain is broken.

So I would suggest we add that possibility under ### Known Problems in the docs and then merge this.

[schubart]

My idea is simpler than what I think you describe: I don't want to look into the definitions of any methods to see if they have side effects. I only want to identify calls to methods that are not part of the "official" collection type. Such calls can be pessimistically considered reads. This avoids the false positive above (but might create false negatives, which seems acceptable).

I think I can identify such methods via the orphan rule: Any method on, say, Vec is either part of its definition in std, or it's local.

I've pushed a commit that implements this idea. What do you think?

[llogiq]

In that case, you'd need to check for all kinds of unknown external methods taking the collection as an argument, right? The current commit only checks for extension trait methods.

E.g. if mycrate defines fn foo(v: &mut Vec) -> usize;, which is called from yourcrate via mycrate::foo(x), the lint should consider the x argument as read.

[schubart]

How the lint works: For a local variable x that is of a container type, raise a warning if there is no read access (but there is some access). The lint considers almost every access of x to be a read access, so by default the risk of false positives seems low.

The only way of accessing x that the lint does not consider a read access is a statement of the form x.foo(args);, i.e. a method call with receiver x where the return value is not used. If all access to x is of this form, then the lint raises a warning.

In your example, foo is not a method, so the lint does consider foo(x) a read access and no warning is raised, which we seem to agree is the right behaviour:

    fn foo<T>(v: &Vec<T>) -> usize {
        v.len()
    }

    let x = vec![1, 2, 3]; // Ok, no warning
    foo(&x);

Note that this particular example could be considered a false negative because nothing useful is ever done with the vector. But that's beyond the scope of the lint. If the collection is passed to a function (local or not), then the lint assumes that that's meaningful and necessary. It does not try to look into the method. Since foo in this example is just a wrapper around len(), it should probably be annotated as must_use and different lints would catch the pointlessness of this code.

[llogiq]

Ah, I missed that. Thank you for writing this lint!

@bors r+

[bors]

📌 Commit 4ee6553 has been approved by llogiq

It is now in the queue for this repository.

[bors]

⌛ Testing commit 4ee6553 with merge 41fa24c...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: llogiq
Pushing 41fa24c to master...

[schubart]

Thanks!

A few things I was hoping to discuss before merging:

• Is the name OK?
• It's currently in nursery. Could it be in suspicious instead?
• Shall I squash the commits before merging? I guess that's a moot point now.