Not sure about "suspicious clone" lint

So I'm not sure I like this warning: added half support by KeKsBoTer · Pull Request #68 · ExpHP/npyz · GitHub

Basically I've got:

    pub fn to_vec(&self) -> Vec<T> {
        let mut reader = self.inner.reader().clone();
        (0..self.len())
            .map(|_| self.inner.type_reader.read_one(&mut reader).unwrap())
            .collect()
    }
warning: using `.clone()` on a double reference, which returns `&[u8]` instead of cloning the inner type
Warning:    --> src/read.rs:523:45
    |
523 |         let mut reader = self.inner.reader().clone();
    |                                             ^^^^^^^^
    |
    = note: `#[warn(suspicious_double_ref_op)]` on by default

Basically, it thinks a clone from &&[u8] to &[u8] is "suspicious." To me, let mut reader = some_reader.clone(); is the clearest way to indicate that you are making a copy of a reader to avoid mutating the original copy. The fact that the reader type happens to be &[u8] is incidental, and writing *self.inner.reader() would misdirect the person reading the code into focusing on the relatively unimportant fact that we have a borrow. (edited)

Perhaps this shouldn't warn for &&T -> &T when T: !Sized (or better, T: !Clone).


I posted about this on Discord. pie_flavor had this to respond:

it is not just the relatively unimportant fact that you have a borrow, it is the relatively important fact that you can move out of it
meaning it is not moving at all, but rather copying
if seeing this is not a reflex for you, that is fine, you will pick it up, but it is for those experienced.

And now honestly, I disagree. But to keep this thread on topic, I created another thread to discuss this: Stylistic practices on borrows of Copy values


Anyways, in this thread, I'm really wondering what the motivation for this warning is. What kind of mistakes does this catch in our code? And to be clear: I'm asking for real examples of mistakes that you know you've made, and not just hypotheticals. Because I can't think of any mistakes that this would catch that would not already run afowl of a type error or another diagnostic.

1 Like

The lint is right (unsurprisingly). You really ought to be using the dereferencing version.

The generic pattern is:

let mut thing = value.clone();
thing.mutate();

in which you do not expect value to be changed, but if it is actually a reference, then the pointed value will change. The fact that &[u8] is both Copy and a stateful io::Writer is an exception rather than the rule.

3 Likes

Interesting question. I’m having a hard time coming up with a convincing example on the spot, too.

Maybe the main motivation is actually not to catch any “miskates” as in “bugs”, but to avoid confusion. I can imagine someone having a borrow-checking error, adding a clone like the one suspicious_double_ref_op lints against, and wondering why that didn’t help.

Since it lints on working code, it does however seem reasonable to me that this lint could just as well…

  • …at least provide an actual suggestion what the code should be changed to. You personally don’t seem to prefer the dereferencing operator being used here, but at least that would be some, and with this lint in place arguably the “officially” best way to write this.

  • …be just a clippy lint. Though in order to demand this I would want to look into the actual criteria (assuming such criteria exist) for deciding whether something should be a clippy or rustc lint.


Another interesting observation for &&T -> &T in particular is that it’s an implicit coercion, if the desired target type is known. So one possibly nice way of writing this line could be simply

let mut reader: &[u8] = self.inner.reader();

with a type signature for reader. I know this might still go against your point of disliking the focus in the particular type at hand, as “the fact that the reader type happens to be &[u8] is incidental”, but at least it doesn’t need to go through an additional “this dereference op does a copy operation” kind of reasoning for readers of the code; and also adding type information rarely hurts (especially with a concise type like this), and mutable variables holding immutable references are special enough anyways that it doesn’t hurt to explicitly write “yes dear reader of this code, that’s indeed what we want to be doing here” in the form of that type signature.

5 Likes

It was a clippy lint. Apparently it was uplifted to provide more help with non-working code. Moreover, the clippy lint was aware this pattern is sometimes intentional.[1]

If it's too opinionated, perhaps there's a way to only fire on non-working code.


  1. The no-op lint in this PR dropped the clone-inner-reference logic since, well, that's not a no-op. ↩︎

3 Likes

Hm. That's a nice solution.

What I ended up going with was

let &(mut reader) = self.inner.reader();

which is consistent with techniques I use in other place to avoid dereferencing borrows of copy values, but it certainly ain't pretty.

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.