Skip to content

RFC: map_or_default in Option and Result #3148

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

orlp
Copy link
Contributor

@orlp orlp commented Jul 14, 2021

This adds Option::map_or_default and Result::map_or_default which behave exactly like .map_or_else(Default::default, _) would, similarly to .unwrap_or_default().

Rendered.

@joshtriplett
Copy link
Member

Echoing a comment from the internals thread: once we have the free function default, we could write this as map_or_else(default, ...).

I think we should weigh that when deciding if we want to add this. It may still make sense to add this.

@jhpratt
Copy link
Member

jhpratt commented Jul 14, 2021

As was also started in the internals thread, I am of the opinion this doesn't need an RFC.

@orlp
Copy link
Contributor Author

orlp commented Jul 14, 2021

As was also started in the internals thread, I am of the opinion this doesn't need an RFC.

It's said here to submit an RFC if it is a new API. Maybe it could be argued it's a "Obvious API hole patch", but I'm not so sure. The RFC was easy enough to write, if the libs team thinks it does not need an RFC no harm done and I can make a pull request.

@scottmcm
Copy link
Member

Note that resize_default was deemed unnecessary in rust-lang/rust#41758 (comment) -- I think the same "just pass the function" may well apply here.

@jhpratt
Copy link
Member

jhpratt commented Jul 14, 2021

Note that resize_default was deemed unnecessary in rust-lang/rust#41758 (comment) -- I think the same "just pass the function" may well apply here.

I've got a feeling this would be used far more often than resize_with_default.

@steffahn
Copy link
Member

I don't know how much of an argument this is, but in my opinion the methods map_or and map_or_else are terribly named anyways, because they don't actually constitute a “mapping” operation. They are just a method form of a general by-value match statement. The only relation to “map” comes from the fact that you can write them as a chain of calls .map(…).unwrap_or[_else](…).

So my argument would be: if map_or[_else] are badly named, we don't need more methods with similarly bad names.

Im not having a great suggestion for alternative names right now, and I'm not sure if there was previous discussion about this.

If a new alternative name for map_or_else was shorter (maybe by dropping the pattern of making the version taking a closure the longer name, similar how bool::then did although the proposed then_some has a weird name, too, because I don't know how that name translates to “like then but doesn't take a closure”) then the or_default alternative would be needed even less. As things stand now, it seems like the main advantage of map_or_default would be in dnot needing to write or_else. E. g. if a new pair of names was .cases (for map_or_else) and .cases_with_value (for map_or) then an alternative .cases_with_default for .cases(default, …) would be highly redundant since the alternative using default would even be shorter. Consider these names mostly place-holders.

@ehuss ehuss added the T-libs-api Relevant to the library API team, which will review and decide on the RFC. label Oct 5, 2021
@arniu
Copy link

arniu commented Jul 28, 2022

Shall we deprecate map_or / map_or_else?

@BurntSushi
Copy link
Member

No.

@arniu
Copy link

arniu commented Jul 28, 2022

Then why not add map_or_default?

@tkr-sh
Copy link

tkr-sh commented Feb 11, 2025

Hello!
I just stumbled upon this issue when looking for it today.
It seems to me that this would be:

  • A great & useful addition to the language std
  • Making the language std more coherent. As cited previously: unwrap_or/map_or, unwrap_or_else/map_or_else, unwrap_or_default/map_or_default.

Now that the issue has been opened for almost 3.5 years, I think that some arguments against .map_or_default don't really work now.

  1. In response to RFC: map_or_default in Option and Result #3148 (comment) => map_or_else(Default::default is way more verbose map_or_default and make the code less readable. (Same reason that there is unwrap_or_default). And Tracking issue for std::default::default() rust#73014 doesn't seem to be evolving quickly.
  2. In response to RFC: map_or_default in Option and Result #3148 (comment) => I agree that map_or.* methods don't have the best naming. Tho, they're here, and used in a lot of projects, if not the majority. In my opinion, deprecating them / making them legacy, would be a really bad idea.
    I agree that creating some brand new functions (something like .map_with_never_seen_feature) would not be the greatest idea. Tho, in this case, it's porting an already existing functionality (in .unwrap_or_default()) to .map.
    If, one day, map_or.*s are deprecated, map_or_default could be migrated with other methods. But I don't think that it's going to happen anytime soon, nor that it would be ( hard & time consuming ) to migrate.

For all of these reasons, I think that it's pertinent to re-"open" the debate around this "RFC" and to add this to the std if validated . Happy to contribute to it if needed.

What do you think about it @steffahn ?

@kennytm
Copy link
Member

kennytm commented Feb 13, 2025

I'd suggest close this RFC and resubmit this as an ACP to go forward.

@Amanieu
Copy link
Member

Amanieu commented May 3, 2025

@rfcbot merge

@rfcbot
Copy link
Collaborator

rfcbot commented May 3, 2025

Team member @Amanieu has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. labels May 3, 2025
@joshtriplett
Copy link
Member

While I have mixed feelings about Option combinators, I feel like almost every use of map_or_else is unnecessarily complex (due to taking two closures, and having to remember which order they come in), and map_or_default only takes one argument, which will be an improvement almost everywhere it could be used.

Add tracking issue and rename file.
@rfcbot rfcbot added the final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label May 3, 2025
@rfcbot
Copy link
Collaborator

rfcbot commented May 3, 2025

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot removed the proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. label May 3, 2025
@rfcbot rfcbot added the finished-final-comment-period The final comment period is finished for this RFC. label May 13, 2025
@rfcbot rfcbot removed the final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label May 13, 2025
@rfcbot
Copy link
Collaborator

rfcbot commented May 13, 2025

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this RFC. T-libs-api Relevant to the library API team, which will review and decide on the RFC. to-announce
Projects
None yet
Development

Successfully merging this pull request may close these issues.