rust-lang
Suggest using `unwrap_or_default()` instead of `unwrap_or(<default>)`
Closes https://github.com/rust-lang/rust-clippy/issues/9435
changelog: [or_fun_call]: Suggest using unwrap_or_default() instead of unwrap_or(<default>)
r? @giraffate
(rust-highfive has picked a reviewer for you, use r? to override)
I don't like this because it ads an unnecessary layer of abstraction to the code without benefit.
:umbrella: The latest upstream changes (presumably #9447) made this pull request unmergeable. Please resolve the merge conflicts.
I think using unwrap_or(specific_value) is generally better than unwrap_or_default() because it's clearer and doesn't require the reader of that line to know what the default value of the relevant type is.
I don't like this because it ads an unnecessary layer of abstraction to the code without benefit.
By "layer of abstraction", do you mean syntax-wise or is there a performance penalty when using unwrap_or_default
I think using
unwrap_or(specific_value)is generally better thanunwrap_or_default()because it's clearer and doesn't require the reader of that line to know what the default value of the relevant type is.
Makes sense. But then, why do we have a unwrap_or_default in the first place?
Hey @erickt , I went through the history and it seems like you're the one who introduced unwrap_or_default syntax. (Correct me if I'm wrong)
Would you care to comment on this?
unwrap_or_default can cause issues with type inference, because previously the type was determined by the inner expression.
For primitives, this also looks needlessly complex and obfuscated. The .unwrap_or(0) is shorter, specifies the type (so no inference errors) and is explicit about the value. While the default 0: uN is simple, it's one more thing to remember, and doesn't carry its weight for this simple expression.
It also seems that the lint performs constant evaluation (i.e. folds .unwrap_or(FOO) to .unwrap_or(0) and lints it). This is just wrong. The entire point of constants is to abstract away the specific value, so that it can easily be changed later, and to give that value a clear and meaningful name in the current context. This lint violates both of those goals.
Finally, I expect this to create a lot of churn, so if added, this lint must be restriction or pedantic.
I agree with @afetisov. If I remember correctly, I added unwrap_or_default a long time before 1.0, back when we were just feeling out how APIs would work with traits. Nowadays after reading a lot of rust code I’m not sure if it’s really saving reducing that much cognitive load, and I can’t remember ever actually using it.
I don't have an issue with unwrap_or_default when it's used for complex types, and the type is clear for the context (foremost to preserve type inference). For example, I don't think the explicit type expression adds anything in this case:
let map: HashMap<u64, String> = make_map();
let s = map.get(idx).unwrap_or_else(String::new);
The expression is more complex, you need to remember to use unwrap_or_else instead of unwrap_or and to pass a function (otherwise Clippy will somewhat reasonably complain). If you change the type of values from String to Vec<Stuff>, then you need to change the default value expressions for no good reason, and if you use Default::default as that expression, then it just unwrap_or_default but longer and clunkier.
But the for primitive types an explicit initializer is much more clear.
@xphoniex As we talked about this before (I think in DMs?), I agree with all the others here: unwrap_or_default doesn't really make sense with primitives. Just writing the primitive in unwrap_or(0) is shorter and contains more information. In addition to all the other arguments that were brought up here.
The reason this lint exists for more complex types is the exact reason @afetisov brought up in the above comment: https://github.com/rust-lang/rust-clippy/pull/9436#issuecomment-1270515858
We can add an extra lint that just lints this for primitives. But then again, I don't see who would use a lint like that.
So I'm strongly leaning towards not merging this PR.
