Adventures in maximum pedantic clippy

[blonk]

While reading this thread I learned it was possible to unlock new depths of torture with clippy. With this new knowledge I set up bacon's clippy targets to make my life as miserable as possible, and it has certainly been a learning experience.

I had two cases where a sub-library had previously lived in-crate, but once I split it out I had forgot to remove an unneeded dependency.

I had a few cases where I accidentally made Futures !Send. This one was actually a little surprising to me at first; for some reason I had imagined that enabling the multithreaded tokio runtime feature would have made using !Send Futures cause build errors.

I really like seeing ways to rewrite certain if-else statements and iterations. Plain default clippy does this as well, but enabling All The Things™️ makes it suggest even more. Not all of them are better, imho, but the majority definitely have been (at least for me).

However, there are some things that I find to be less useful -- like the "multiple versions of the same crate". I have yet to encounter a case of this where it wasn't out of my control, and the only way to solve it is to go back to older versions until they overlap, which I do not want to do.

I have come to the conclusion that #[must_use] should be the default for owned return values. It's not always that I agree with clippy's must_use suggestion, but most of them time I definitely do. And splashing these things all over pollutes the code.

There are a few lints that nag me because they suggest breaking changes. The most common one is probably that is keeps suggesting that passing ownership in s: impl ToString is bad, and that I should be using s: &impl ToString instead (which is clearly not a good suggestion). It's kind of annoying to have to manually disable it at each location (because I do want the lint globally).

Oh, also, I want to give a major kudos for the Clippy Lints database, where the link to the source that performs the lint is linked (see View source at the bottom right in this example) -- this is pure genius.

I give this experience a 5/5, not only because it appeals to the OCD part of my brain, but because I genuinely learned new things from it.

[meancoot]

There are a few lints that nag me because they suggest breaking changes. The most common one is probably that is keeps suggesting that passing ownership in s: impl ToString is bad, and that I should be using s: &impl ToString instead (which is clearly not a good suggestion). It's kind of annoying to have to manually disable it at each location (because I do want the lint globally).

It's a perfectly fine suggestion though. Because ToString::to_string never consumes its argument there's never a good reason to choose impl ToString rather than &impl ToString. The real suggestion should be to just take the String and let the caller create if however they see fit.

[steffahn]

I don’t really like your tone here at all. It’s hard (and IMO a bit dramatic) to say someone is “horrendously wrong”. Most things are a trade-off, as is this case.

@paramagnetic

There's nothing wrong with passing a Wrapper<'a>(&'a str), for example.

Yes, there is nothing wrong with passing Wrapper<'a>(&'a str) by-value, but there’s also nothing wrong with passing it by-reference. Assuming this is clippy::needless_pass_by_value we are talking about, it fires on a function whose implementation only uses the impl ToString by-reference. Which is quite common, because ToString is a trait whose method is a &self method. A reference is being created anyways, it’s just the question who creates it.

---

In the case of impl ToString, the real risk is that a user may forget that there’s no sense in passing an owned value, like an owned String. Unlike a (quite similarly looking) API of x: impl Into<String> which very much benefits from receiving an owned String, in case you have one anyways.

This means that a user that wants to call f twice can easily run into situations where they add a call to the methods: E.g. the code used to be

let s = String::from("string");

f(s);

and now they want to use s again later; e.g. for a second call to f:

let s = String::from("string");

f(s);
f(s);

bam, bad compiler suggestion:

help: consider cloning the value if the performance cost is acceptable
   |
9  |     f(s.clone());
   |        ++++++++

blindly applied (like, who memorizes the exact function signature of everything in their code; you’ll really have to pay attention to know that you can change f(s) to f(&s) instead!), and you gain a completely unnecessary clone.

---

Now only can impl ToString not benefit from owned arguments; other function signatures (like impl Into<String>, or just x: String can; if the function we’re calling just unconditionally calls the to_string method, it may easily be the smarter choice to just write x: String, like @meancoot.

---

@paramagnetic

By requiring the effective double indirection in the API, you are only making it annoying to use.

Yes, the API might be “annoying to use” if “annoying to use” means you have to add the & sign. But that’s being more true to representing what kind of value is actually needed and used.

It definitely is a minor disadvantage; which is why I’m calling this a trade-off.

---

Fun fact: The other (minor) downside of x: impl ToString is that it creates double indirection. Yes, that’s correct! This is the double-indirection case!! The lint fired because we create &x inside of the function. We still allow users to pass references because of the generic ToString for &T implementation. But that means that when a user passes &Foo, we create &&Foo!

[Schard]

I am note sure whether I understand the questionable impl ToString case correctly. But if the lint is triggered by the function only using it to convert the passed parameter to a String and then only using a reference of that String, passing a impl AsRef<str> might be the better alternative.

[steffahn]

@Schard

and then only using a reference of that String,

I don't think that second step is required to fire the lint, it should also fire when the String returned from x.to_string() is used by-value.

[paramagnetic]

@steffahn

the real risk is that a user may forget that there’s no sense in passing an owned value, like an owned String

It's no real risk. If you accidentally pass by value, without a subsequent use, it's just as well. If you do use the passed (and thus moved) value later, you'll get a compiler error, which will be trivially fixed by taking the address, now that it's actually needed.

[steffahn]

@paramagnetic

That's fine, as it's not specifically tailered to your liking.

Sorry if I wasn't clear. This isn't really just about my personal liking at all, but mostly in reference to the Code of Conduct, which participants of this forum should follow, in particular

• Respect that people have differences of opinion and that every design or implementation choice carries a trade-off and numerous costs. There is seldom a right answer.

This whole argument is a relatively abstract^[1] API design question, I can hardly imagine a situation where this rule (w.r.t. “every […] choice carries a trade-off”) applies more.

---

Besides, I’m not quite sure yet what “huge misunderstanding” you’re trying to correct here.

@paramagnetic

My goal was to be technically correct, point out the huge misunderstanding, and prevent the giving of bad advice

The many points in your response you are making seem to be responding to things @meancoot never talked about

For instance…

@paramagnetic

T: ToString implies &T: ToString, so all you do by requiring &impl ToString is disallow anything that is not a literal &-reference.

They never made any indication that would suggest they aren’t aware of this implementation. What I read them saying in

@meancoot

Because ToString::to_string never consumes its argument there's never a good reason to choose impl ToString rather than &impl ToString.

is that impl ToString-taking functions never actually need the ownership of the impl ToString^[2]; they are not saying that the impl ToString parameter in any way forces the caller to give up ownership unnecessarily.

It seems however like maybe you did write your answer on the assumption that @meancoot claimed s: impl ToString would force a user to give up ownership of their arguments.

@paramagnetic

No, no, no, no, a thousand times no. That is horrendously wrong, and outright false.

That being said, even if they had claimed such a thing,^[3] I still feel this kind of answer – to a first post of a new user – doesn’t help at all with another the goal, namely:

• We are committed to providing a friendly, safe and welcoming environment for all, regardless of level of experience

---

And your further bullet points…

@paramagnetic

• Passing an argument by-value is not expensive.

countering something that was not something that anyone claimed

@paramagnetic

• Passing an argument by-value does not incur automatic deep cloning; at most it's a memcpy.

also countering something that was not something that anyone claimed.

And funny that you mention “automatic deep cloning”, because s: impl ToString is actually the kind of API that feels a lot like “automatic deep cloning”. You may pass in an owned value and it gets (sort-of) “cloned” into creating a new String from it anyways; especially if it already was a String. And @meancoot literally also suggested the an alternative API design choice for avoiding the hidden cloning:

@meancoot

take the String and let the caller create if however they see fit

---

@paramagnetic

• Passing an argument by-value is cheaper for small types (size_of::<T>() < size_of::<usize>())) than indirection, and may be cheaper with some larger upper bound.

Again talking about the cost of passing the value; I’m really not sure how this became the topic of discussion; but it’s also not much of a relevant benefit after all. Even when the function call isn’t inlined, the cost of passing a single usize, i.e. data that fits into a single register, is already among the cheapest things imaginable.

And finally

@paramagnetic

• A generic type variable like T in fn do_it<T>(...) does not mean it's an "owned type". It may stand in for any type whatsoever, including references. It does not mean "every type except a reference".

Which once again only makes sense on the assumption that @meancoot claimed s: impl ToString would force a user to give up ownership of their arguments.

---

One remaining issue in the idea of writing s: &impl ToString is that it still contains a Sized bound, which can actually be somewhat more annoying (because AFAICT it doesn’t give good compiler suggestions like “add &_ here”). With that out of the way though, with s: &(impl ToString + ?Sized) – or equivalently s: &T where T: ?Sized + ToString, on wouldn’t be far off of countless instances of API that actually exists in the wild.

For example… Serialize is also a on-&self-methods sort of trait, and functions like serde_json::to_string tend to accept &T (where T: ?Sized + Serialize), not T; even though Serialize is also implemented for &T.

---

1. because we are not talking about concrete functions, it’s not even a concrete API design question; and even for those, many times it’s not a simple matter of right or wrong ↩︎

2. which isn’t true in full generality; but it’s true as far as the clippy lint applies ↩︎

3. i.e. the claim “s: impl ToString would force a user to give up ownership of their arguments”, which would indeed be clearly factually wrong ↩︎

[Vorpal]

I too tried the more extreme clippy lints. There were some useful ones, but also a lot that are about opinion (especially in pedantic, as is to be expected; even more in restriction of course). There were also a bunch with false positives (the unused dependency one for example, can't use it with projects).

[mattfbacon]

needless_pass_by_value for impl ToString is a false positive caused by the fact that T: ToString implies &T: ToString only because T: Display implies &T: Display and U: Display implies U: ToString. When the relationship is direct the warning is suppressed. Rust Playground

[meancoot]

Edit: Never mind. I was wrong here... had to go one step further Playground and indeed providing impl<T: Y> X for T will allow use_x to diverge based on whether it gets a value or reference. My original wrongness is below.

---

On the contrary, in your example, use_y lacking the lint is wholly justifiable, but absolutely a false negative.

The reason it doesn't trigger in that case is because the impl<T: Y> Y for &T implementation COULD diverge in behavior from a theoretical impl Y for OtherType implementation, though in the example you provided it doesn't. If it did diverge use_y(value) and use_y(&value) would have different behaviors and changing one to the other at the recommendation of a lint would be a mistake. Attempting to prove whether they diverge is obviously so complicated, and only even possible in the simplest of cases, that there's no point in even trying. Thus the lint is suppressed.

On the other hand, with no impl<T: X> X for &T, use_x(value) and use_x(&value) are guaranteed to have the same behavior aside from the fact that use_x(value) will drop value. Thus the only real advantage to fn use_x(x: impl X) is that the caller can omit the ampersand at the call site (in exchange for the potential extra monomorphized version of the function if you call it with both a u8 and &u8 for example) and in the rarest of rare cases may have to manually call mem::drop on the value.

Any performance benefits that could be gained from things like passing u8 rather than &u8 are going to be dwarfed by the downsides of accidentally passing an [[f32; 4]; 4] type matrix rather than &[[f32; 4]; 4]. At the same time, when giving ownership you want to keep, intuition and the compiler both lead to calling clone rather than just adding an ampersand, which is also an obvious performance sink.