Add a lint for redundant references to type params

[Alexendoo]

changelog: Add a lint for redundant references to type parameters

This lints for things like

fn f<R: Read>(reader: &mut Read) {
    ...
}

Which could be

fn f<R: Read>(reader: Read) {
    ...
}

I've seen it a couple times for different reasons now, some thinking it should be &mut because the methods on Read take &mut self. Others wanting to pass &mut [something that impls Read] who are unaware or forgot about impl<R: Read + ?Sized> Read for &mut R. So I thought it would be a good candidate to write my first lint

There's still some stuff unimplemented, it doesn't cover e.g. &mut impl Read and I haven't added many edge cases to the tests, but I wanted to ask for some feedback first

Mainly -- is this lint plausible? I worry I'm overlooking some case where you do need that signature

And currently I'm using a few hardcoded traits to check against, I found implements_trait which looks ideal, but I wasn't sure what should be passed in as ty_params

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @xFrednet (or someone else) soon.

Please see the contribution instructions for more information.

[xFrednet]

Hey and welcome to Clippy 📎. I haven't looked at the code yet, as I first want to address your question if this is plausible. To me, it's surprising that they should be equivalent, I would expect rustc to handle them differently. Anyway, that could just be something I'm missing.

I did a quick test, based on the lint documentation that you wrote, and came up with this piece of code:

use std::io::{self, Read};

fn before<R: Read>(reader: &mut R) {
    let mut buffer = [0; 10];
    let _res = reader.read(&mut buffer);
}
fn after<R: Read>(reader: R) {
    let mut buffer = [0; 10];
    let _res = reader.read(&mut buffer);
}

fn main() {
    let mut stdin = io::stdin();
    before(&mut stdin);
    
    // Still works with a reference due to `Read`
    // having `impl<R: Read + ?Sized> Read for &mut R`
    after(&mut stdin);
    // But no longer requires one
    after(stdin);
}

However, this example doesn't compile (See Playground). Is there something I'm missing? Or would this be a case that wouldn't be linted? 🙃

If this is resolved, I would like to also ask the rest of the team what they think. It could be an option to add this lint and just make it allow-by-default. (Like it currently would be in the nursery group). 🙃

[Alexendoo]

You're right, it should be including mut in the suggested change

fn after<R: Read>(mut reader: R) {
    let mut buffer = [0; 10];
    let _res = reader.read(&mut buffer);
}

This would be a case I'd expect it to lint

[bors]

☔ The latest upstream changes (presumably #7838) made this pull request unmergeable. Please resolve the merge conflicts.

[xFrednet]

You're right, it should be including mut in the suggested change

fn after<R: Read>(mut reader: R) {
    let mut buffer = [0; 10];
    let _res = reader.read(&mut buffer);
}

This would be a case I'd expect it to lint

Alright, I missed the mut keyword, thanks for pointing that out. 👍 Now my understanding is, that the after() function will consume the R instance, if it's not called with &mut reader. The before function forces the user to add the &mut reader annotation due to the function signature. I like having the explicit &mut T annotation to indicate that the instance is only borrowed and not consumed. My initial thought when I saw fn after<R: Read>(mut reader: R) { was: "The function is stealing my reader". It hasn't occurred to me that the constraint will also be fulfilled by &mut reader. However, that can be personal preference and even then we could add this as an allow-by-default lint. 🙃

Mainly -- is this lint plausible? I worry I'm overlooking some case where you do need that signature

I'm still not totally sure if this lint is entirely correct or if I'm also overlooking something. @rust-lang/clippy What are your thoughts on this? :)

[camsteffen]

I don't understand this lint TBH. Isn't &mut impl Read generally correct? Functions shouldn't require owned values if not needed.

[Manishearth]

Yeah I think &mut R is more self-documenting; unsure if this really is a style issue.

[camsteffen]

Actually looking at this again I think it does make more sense to not use &mut with Read because Read is implemented for all &mut R. It makes the function strictly more permissive since you can pass more types like &File instead of &mut &File. I'm not sure how to generalize this beyond Read - I guess it's because the trait generally is implemented for reference types like &File and all &mut impl Read.

[flip1995]

I'm definitely open to this lint. It's similar to the lint, that suggests to use &str over &String or &[_] over &Vec<_>. As long as you don't have to change the call-sites this should be fine.

I can imagine, that it may be hard to generalize this lint without any FP though. So we should be careful here and be creative when it comes to writing tests.

[flip1995]

Aren't there diagnostic items for this?

[Alexendoo]

For Debug there is, it was a few of the others that didn't have them. I think the fmt ones and Seek/BufRead

[Alexendoo]

I thought of a tricky false positive, things like

fn f<R: Read>(a: &mut R, b: &mut R) {
    mem::swap(a, b);
}

[flip1995]

Couldn't this be fixed with

fn f<R: Read>(mut a: R, mut b: R) {
    std::mem::swap(&mut a, &mut b);
}

producing an auto-applicable suggestion for this is nearly impossible, but that's why we have Applicability::MaybeIncorrect

[camsteffen]

I think the name should mention "generic" or "type params", like redundant_generic_param_refs.

[Manishearth]

So my concern with this lint is that &mut R is a bit of a documentation mechanism: people do not know about the implementation on &mut T, and &mut R gives folks the hint that they should use mutable references. Double-referencing is not a big deal: this lint shapes the API to accept code that is not necessarily faster. However, what it does do is make people look at R as owned which can lead to some bad coding style if they don't know about the &mut T impl.

[camsteffen]

Requiring a mutable reference could propagate through the rest of your code. That's bad.

fn with_file(file: &mut File) { // this shouldn't have to be a mutable reference
    with_read(file);
}

fn with_read(reader: &mut impl Read) {
    todo!();
}

[Alexendoo]

Couldn't this be fixed with

fn f<R: Read>(mut a: R, mut b: R) {
    std::mem::swap(&mut a, &mut b);
}

producing an auto-applicable suggestion for this is nearly impossible, but that's why we have Applicability::MaybeIncorrect

That would work for within the function, but wouldn't swap them at the callsite (playground)

use std::io::Read;

fn mut_ref<R: Read>(a: &mut R, b: &mut R) {
    std::mem::swap(a, b);
}
fn owned<R: Read>(mut a: R, mut b: R) {
    std::mem::swap(&mut a, &mut b);
}

fn main() {
    let mut a = &[1, 2, 3][..];
    let mut b = &[4, 5, 6][..];
    
    println!("{:?} | {:?}", a, b);
    owned(&mut a, &mut b);
    println!("{:?} | {:?}", a, b);
    mut_ref(&mut a, &mut b); // swaps our a & b
    println!("{:?} | {:?}", a, b);
}

[1, 2, 3] | [4, 5, 6]
[1, 2, 3] | [4, 5, 6]
[4, 5, 6] | [1, 2, 3]

[flip1995]

That would work for within the function, but wouldn't swap them at the callsite (playground)

Oh right. But doesn't this apply to every other case as well?

fn f<R: Read>(a: &mut R) {
    a.taking_ref_mut_self();
}

taking_ref_mut_self probably changes the state of a somehow. So changing the function signature to mut a: R would change the semantics. (?)

So I now think the lint is only applicable if the argument isn't really mutated in the function body.

Which brings me to a lint idea I had a few days ago: needless_ref_mut_args. Which would just lint if the &mut is just unnecessary.

[camsteffen]

mem::swap on the arguments is very niche/unusual.

taking_ref_mut_self probably changes the state of a somehow.

How? All that it can do is call Read::read.

---

use std::io::Read;

fn generic_read(mut reader: impl Read) {
    let _ = reader.read_to_end(&mut Vec::new());
}

fn generic_mut_ref_read(reader: &mut impl Read) {
    let _ = reader.read_to_end(&mut Vec::new());
}

fn main() {
    {
        // this will copy the slice ref and read it
        let slice = &[1u8, 2, 3][..];
        generic_read(slice);
    }
    {
        // this will update the slice
        let mut slice = &[1u8, 2, 3][..];
        generic_mut_ref_read(&mut slice);
        assert!(slice.is_empty());
    }
    {
        // this also updates the slice, but I am not forced to do so
        let mut slice = &[1u8, 2, 3][..];
        generic_read(&mut slice);
        assert!(slice.is_empty());
    }
}

[Manishearth]

Requiring a mutable reference could propagate through the rest of your code. That's bad.

I don't really see this as bad: the alternative being suggested is owned arguments, which is kinda worse? Owned arguments are more strict than mutable, you can always get &mut from an owned one if necessary

[camsteffen]

the alternative being suggested is owned arguments

I don't think that is really true looking at Read implementors. Every type allows you to use a reference, a mutable reference, or get a copy using something like io::stdin(). I think this is the nature of Read - it represents a reference to some data source - the Read itself is not the data source, it is the reader of the data source.

Owned arguments are more strict

&mut impl Read is more strict than impl Read because of impl Read for &mut R. If you take away &mut from the signature, the call site will always still compile AFAICT.

[flip1995]

How? All that it can do is call Read::read.

I meant in the general case. But what I was thinking of was something like this: Playground

use std::io::{self, Read};

#[derive(Default, Copy, Clone)]
struct S {
    num_used: u32,
}

impl Read for S {
    fn read(&mut self, _: &mut [u8]) -> io::Result<usize> {
        self.num_used += 1;
        Ok(0)
    }
}

fn f<R: Read>(r: &mut R) {
    let _ = r.read(&mut []);
}
fn g<R: Read>(mut r: R) {
    let _ = r.read(&mut []);
}

fn main() {
    let mut s = S::default();
    
    f(&mut s);
    g(&mut s);
    g(s); // s is copied and therefore "2" is printed at the end
    
    println!("{}", s.num_used);
}

But that isn't really a problem, since the reference is copied and therefore s is modified anyway.

But writing out this example gave me another finding: potential misuse of the API. With &mut R as the argument, you can only pass a mutable reference and it will work as expected. with mut _: R, you may also pass in s, which best case errors with used after move and worst case is just silently copied in (if the reader is Copy) and the state of your reader is not updated. This is already a concern for the Read trait, I can imagine that this is an even bigger concern for custom traits, if the lint should get generalized.

[Manishearth]

&mut impl Read is more strict than impl Read because of impl Read for &mut R. If you take away &mut from the signature, the call site will always still compile AFAICT.

Sure, but in terms of the types you are "forced" to use, if you require &mut R you can use owned types, but if you require R you can't

My point here is that most people don't intuitively know about the impl Read for &mut R, and so if an API is fn foo(impl Read), and if, say, they're reading from BufReader, they may write the following code:

fn do_something(r: BufReader) {
   foo(r);
}

without realizing that &mut r is acceptable here, because they saw the signature of foo() and noticed that it required an "owned reader".

I think that the convention is to use &mut impl Read because it makes this part really clear: you basically never need owned values for reading, even though they can work.
I think that people tend to use

[camsteffen]

The "used after move" case is unfortunate. IMO the solution to that is better rustc diagnostics. But without that, I can understand wanting avoid the footgun. I guess it's a matter of preference.

[camsteffen]

One more thought. impl Display is a common pattern instead of &impl Display, because Display is another trait that is expected to work with references.

[flip1995]

impl Display is a common pattern instead of &impl Display

I'm mostly concerned about removing &muts. Doing this most likely changes semantics somewhere. But also for &s I think this lint should be at most restriction for the reasons @Manishearth has given.

[xFrednet]

Having this as a restriction lint SGTM. The idea itself is definitely interesting, and it can be helpful. I'll get to review the code in the coming days. Thank you for the feedback everyone 🙃

[Alexendoo]

Yes, thank you for the input everybody!

I am convinced as well that the replacement is too unintuitive, and that &mut R is nice documentation wise

For example I noticed that std::io::copy has a signature this would have linted against 😄. However it feels right that copy uses &mut R rather than R

I'd be happy to just close this PR, but I can continue working on it to get it into restriction if you think it would see much use

[camsteffen]

Don't keep it open on my account 🙂

[Alexendoo]

👍