Lint unwrap_or_default is overly opinionated and likely wrong

[Kinrany]

Lint unwrap_or_default suggests replacing instances of unwrap_or and similar methods with unwrap_or_default.

This is a great change when the value or function (in the case of _or_else) being replaced is Default::default. However the lint appears to suggest replacing even constructors that have nothing to do with Default and merely happen to return the same value.

I believe this is not only not a useful suggestion but a misleading one. It propagates the expectation of the "default" value being somehow special, even though there is no specification for what exactly that specialness might mean.

---

From std::default::Default:

A trait for giving a type a useful default value.

Sometimes, you want to fall back to some kind of default value, and don’t particularly care what it is. This comes up often with structs that define a set of options: [...]

The purpose of the Default trait is to provide an arbitrary valid value. Almost every 'static type has at least one value that is valid regardless of context, so this is a very easy condition to satisfy, even when there is no particular reason to choose one value over others.

By design, the trait says nothing at all about the value. It would be unusual, but not incorrect to return a randomly chosen value or even choose a different value on every call.

Contrast this with various Zero traits that return an algebraic zero, such that x + 0 = x. It is quite reasonable for Default::default to return this zero value, but the number of types that can implement Zero is much lower. (Besides, sometimes a type cannot implement Zero because there are two different relevant operations with two different zero values, yet either value would work perfectly well as the output of Default::default.)

The motivating example described by the Default trait is various configuration types. It is common for configuration to include options that only become relevant in rare, even exceptional circumstances. Default allows explicitly deciding that those extra options don't matter.

#[derive(Default)] supports this design: in most cases it makes perfect sense to construct an arbitrary record by constructing arbitrary values for every field. On the other hand, it would have rarely been useful if Default::default carried some special but not standardized and, more importantly, not composable semantics.

Rust API Guidelines also support this design. They recommend implementing Default eagerly, among other default traits.

Despite all this, the misunderstanding of Default as being required to produce some undefined blessed value is common and causes friction across the ecosystem. Numerous crates were published to circumvent the issue of upstream types not implementing Default.

The most high-profile instance of this that I know of is chrono types not implementing Default, with the motivation of having no way to choose between multiple reasonable defaults, despite it being extremely common for aforementioned configuration types to include dates.

---

I hope I made a convincing case for not expecting impl Default to mean more that it is supposed to. It should now be obvious that replacing T::new with T::default is incorrect in the general case, even if returned value is the same.

[mina86]

‘Don’t particularly care what it is’ doesn’t necessarily mean ‘ok with any arbitrary value’. If I have configuration object with a timeout field, I don’t particularly care what the timeout is so long as it’s a sensible value that authors of the library considered a good default.

[Jarcho]

It replaces other functions only for std types which are known to produce the same value as default. If it's happening on any other type that should be treated as a bug.

As an aside, a reasonable argument could be made that T::new() and T::default() should always be made to return the same value. Neither function has any extra context as to what value should be returned.

[Kinrany]

‘Don’t particularly care what it is’ doesn’t necessarily mean ‘ok with any arbitrary value’.

For an implementation - sure, a type implementing Default can promise extra properties on top of the trait's contract. That's what doc comments on trait impls are for.

For generic code that consumes implementations of Default, there is no difference because it can only rely on the trait's contract. Which says nothing about the value, which means it can be any arbitrary value.

For a library, it's a good idea to choose a useful value and promise not to change it. But being unable to choose a perfect value is not a good reason to choose nothing.

For a consumer of the library, it's nice to be able to take a look at the defaults and see that they're the same thing that one would choose by hand.

It's reasonable to expect most libraries to set defaults that won't need to be changed. It is still incorrect to automatically change code with the assumption that all libraries will choose good defaults and that the library author's opinion on (unstandardized) good defaults will be the same as the user's opinion.

[Kinrany]

It replaces other functions only for std types which are known to produce the same value as default. If it's happening on any other type that should be treated as a bug.

@Jarcho in the specific case that triggered this post Default is implemented via new. The lint still triggers even though the Default impl could change without new changing. Is that expected?

I'll post a repro in a separate issue if necessary. Chose to make this post a "discussion" because it's really more about the semantics of Default than the clippy rule itself, an attempt to establish consensus. Though hopefully not entirely off topic, seeing how clippy is one of the ways people learn code style.

Edit: posted #11631

[djc]

In general I feel like this argument would be a lot stronger if you had specific examples where you think clippy's suggestion is wrong/misguided, which IMO is usually the standard "false positives" get judged against.

However the lint appears to suggest replacing even constructors that have nothing to do with Default and merely happen to return the same value.

In particular, how do you see this being applied?

[Kinrany]

Posted a repro as #11631. Specifically, it also replaces T::new when impl Default uses that constructor, even though the implementation of Default is opaque to the consumer and a change in Default could be reasonably considered backwards compatible.

[djc]

Honestly it's IMO a reasonable convention/assumption that, if there is a Default impl and a no-argument new() constructor, that these return the same thing.

[Kinrany]

I agree, but I think that should be a separate lint that checks exactly that.

[Kinrany]

Late correction: agree with the convention but not with the assumption.

[kornelski]

It would be good to have specific failure cases to discuss. Are there types that really have new() and default() different? Clippy already has a lint that suggests unifying them.

[Kinrany]

One example is UUID newtypes. There are two nice but incompatible properties: uniqueness and determinism. It's reasonable to generate new UUIDs in new but return a stable all-zeroes UUID from default.

[Jarcho]

generate or gen would be a better name than new in that case. Or what the uuid crate does with new_v1 and others, but I still would have preferred gen_v1. That being said I can see how someone would end up with new() doing something like that.

[aalekseyev]

Every single time I see this warning, I have to disable it or rewrite code to avoid the suggestion because it makes the code worse.
Relying on a vague concept of default is just not a good idea when you know exactly the value you want.
Most (all?) of my examples are just vec![] or an empty string, where it's not hard to spell out the value and in all cases so far I much prefer it spelled out.