-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
The parameter ordering of Option::map_or_else
is unintuitive.
#1025
Comments
I agree that this ordering is unintuitive and it would be better if the |
Yes. I get this wrong every single time. 👍 |
To be honest I thought the order was always the other way around. Was it changed? |
@cmr I thought it was always the other way around too. That's what screwed me up and motivated me to open this issue. |
The order is currently chosen to be internally consistent with foo.map_or(some_value, |x| {
bar();
baz(x);
qux();
}) vs. foo.map_or(|x| {
bar();
baz(x);
qux();
}, some_value) This isn't to say that we shouldn't change it, but we shouldn't consider this in isolation. |
@huonw That's understandable but still rather unintuitive. I would prefer to make it look nicer with a minor style tweak: foo.map_or(|x| {
bar();
baz(x);
qux();
},
some_value
) or even: foo.map_or(
|x| {
bar();
baz(x);
qux();
},
some_value
) This does get a little more right-drift but I think it's worth the concession for cleaner, more intuitive code. I use the docs all the time but if I have to consult them to figure out the right argument ordering for something as simple as an operation on a monad, it's going to severely impact my efficiency. |
I don't think I've ever screwed this one up. It just seems natural to me that the default value comes first. Probably because |
@kballard, but even |
So obviously this can't be "fixed" in 1.x, and fixing it in 2.x would be a disaster. So... this bug isn't actionable? |
@gankro I believe deprecating this for an alternative with a name that does not cause wrong associations with parameter ordering might make it actionable. But in general, ditto. |
@gankro why would it be a disaster? People will get a compile error, and the fix is simple. |
This might be true, but it would be pretty devastating to have virtually all Rust projects break just because some people aren't satisfied with the |
@frewsxcv it's the re-ordering of arguments, not the renaming. Besides, people are free to stay with an ancient tool if doing a simple fix is too much effort. Some of these fixes could be automated even, especially simple ones like this. |
@tshepang This is not a good way for a language developer to act. Breaking client code in such a blatant way should not be taken so lightly. |
@gankro I think "fixing mistakes and oversights" is supposed to be part of 2.x. Some (more important) stuff will be broken anyways, so we might as well break the smaller things as well. I think making things more intuitive is worth the pain. |
As stated above in previous comments, the argument order is intentional; it is neither a mistake nor an oversight. |
I hear you, but a mistake can happen whether or not there is intent. And as this issue has proven, people do not like this choice. |
Triage: ⛵. |
Yup, this ship has sailed. |
<Rant> `clippy::map_unwrap_or` is a bad lint for the following reasons 0.) `map` => `unwrap_or_else` is two methods but that's not bad! It's the composition of two super important functions on `Result`, which leads to the more important point... 1.) `map_or_else` has it's arguments in the wrong order, which makes it unintuitive especially for a niche function on `Result` Original ```rust VerifiedBuiltin::<T>::from_str("default") .map(SettingsPtr::from) // <- what I'm mapping .unwrap_or_else(|_| SettingsPtr::from(T::default())) ``` Clippy Suggestion ```rust VerifiedBuiltin::<T>::from_str("default") .map_or_else( |_| SettingsPtr::from(T::default()), // <- the _else_ branch! SettingsPtr::from // <- What I'm mapping... ) ``` `map_or_else` is wildly unintuitive, and this dude agrees with me. rust-lang/rfcs#1025 </Rant>
These methods are confusing because the arguments are backwards. I feel like they should have never been added to `Option<T>` and that clippy should suggest rewriting to `map(...).unwrap_or(...)`/`map(...).unwrap_or_else(|| ...)` rust-lang/rfcs#1025
These methods are confusing because the arguments are backwards. I feel like they should have never been added to `Option<T>` and that clippy should suggest rewriting to `map(...).unwrap_or(...)`/`map(...).unwrap_or_else(|| ...)` rust-lang/rfcs#1025
Just coming here to say the "unintuitive" folks are right. Love that you're working on Rust. But I've now seen a few threads (forums and git) where this has been completely ignored (by many of the same folks). Please listen to the engineers using your language. They have a differing perspective from your own. With that said, dialogue has been halpful |
@who-biz as the person who opened this issue almost* ten years ago (!), yeah, this ship has sailed. The time to change it was before 1.0. To change this now would either require:
If you really want to, you can create an extension trait in your project to fix this yourself: pub trait OptionExt<T> {
fn map_or_else_v2<U>(
self,
map: impl FnOnce(T) -> U,
or_else: impl FnOnce() -> U
) -> Option<U>;
}
impl<T> OptionExt<T> for Option<T> {
fn map_or_else_v2<U>(
self,
map: impl FnOnce(T) -> U,
or_else: impl FnOnce() -> U
) -> Option<U> {
self.map_or_else(or_else, map)
}
} |
@who-biz it's not been ignored, it's simply not actionable. Rust has a commitment to backwards compatibility, and I can tell you engineers care far more about that than unintuitiveness. |
Yes, agreed on all points, at this point in time. |
The signature of
Option<T>::map_or_else
is as follows:fn map_or_else<U, D: FnOnce() -> U, F: FnOnce(T) -> U>(self, def: D, f: F) -> U
This is highly unintuitive as the two closure parameters are in the reverse order of that suggested by the function's name. This means that anyone who tries to invoke it from memory, including myself earlier today, is going to run into a compiler error on the first try.
Example:
The above makes sense, logically. If the value is there, map it and return the result. Otherwise (implying a secondary operation), call a closure which will produce a substitute value. Even the documentation supports this logical progression:
However, trying to invoke the method this way will result in a compiler error because the no-arg closure is required to be first. The particular reasoning behind this design is not given, but I have found precedent in Haskell's
maybe
function:However, I don't think Haskell's intuition for parameter ordering can extend to Rust because Rust doesn't have currying, partial application, or (global) lazy evaluation. And even in Haskell the ordering isn't necessarily intuitive.
I am aware that this method and
Option
itself have both been marked asStable
. However, I believe minor unintuities like this add up and ultimately reflect poorly on the user experience of the language as a whole, and should be addressed before Rust hits 1.0.I would like to note that I am willing to apply the effort to adjust this myself, as it's relatively trivial. However, because it's trivial, I am not sure if it requires an RFC or just a general community agreement.
The text was updated successfully, but these errors were encountered: