Skip to content
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

Implementing flatten for Option<&Option<T>> and Option<&mut Option<T>> #186

Open
Coca162 opened this issue Mar 2, 2023 · 7 comments
Open
Labels
api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api

Comments

@Coca162
Copy link

Coca162 commented Mar 2, 2023

Proposal

Problem statement

Flattening a Option<&Option<T>> is currently less intuitive then flattening a Option<Option>. This also comes in true if you want to flatten a Option<&mut Option<T>> to a mutable or immutable option.

Motivation, use-cases

This has the same use case as regular .flatten(), just for reference types. The current alternative is to use something close to .and_then(Option::as_ref) however it's rather unintuitive for somebody who hasn't seen it before to figure out what it does. This would be a solution that would improve the readability of code that may require it.

Solution sketches

A couple of the solution/decisions I went through:

Return Types

Option<&T>

Would require calling .as_ref()/as_mut() (or using a match equivalent) on the inner option. Much harder to go from Option<&T> to &Option<T> then vice versa.

&Option<T> (primary solution)

Intuitive return type for the flatten, can be turned into a Option<&T> easily with .as_ref. However a mutable flatten cannot be constant as a &mut Option<T> and so it would have to be kept as Option<&mut T>.

Function naming

.flatten() for both immutable and mutable references

Having the same function name for all flattens. Ideal for making it so people don't have to change the function name if the type changes back to being owned. However it makes it difficult to know how to turn a mutable reference into a immutable references (Option<&mut Option<T>> -> &Option<T>) idiomatically.

.flatten_ref() and .flatten_mut() (primary solution)

This allows allows for a distinction between all different return types, this way the return type is made clear since it doesn't change between the same function names. .flatten_ref() would be returned for both mutable and immutable references and only .flatten_mut() for mutable references.

I currently have done what It'd believe to be the ideal implementation in a pr.

Links and related work

Not much to say but thanks scottmcm in the Zulip for their thoughts on this idea!

@Coca162 Coca162 added api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api labels Mar 2, 2023
@Coca162 Coca162 changed the title Implementing flatten for Option<&Option<T>> Implementing flatten for Option<&Option<T>> and Option<&mut Option<T>> Mar 2, 2023
@programmerjake
Copy link
Member

&Option<T>

Since returning &None isn't possible in a constant functions this is not the ideal return type.

it is totally possible if you use an inline const to get around needing const Drop:
https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=285c82ef958abf1b5e64eeedadd29ac3

imho &Option<T> is the better return type since if you need Option<&T> it's simple to call as_ref()

@Coca162
Copy link
Author

Coca162 commented Mar 3, 2023

@programmerjake didn't know this worked! However when trying it for mutable references it gives the error: mutable references are not allowed in the final value of constants so I'm not sure what you would do there.

@programmerjake
Copy link
Member

@programmerjake didn't know this worked! However when trying it for mutable references it gives the error: mutable references are not allowed in the final value of constants so I'm not sure what you would do there.

it doesn't work with mutable references because what would happen here:

fn option_transmute<T>(v: Option<&mut Option<T>>) -> &mut Option<T> {
    ...
}

*option_transmute(None) = Some(123);
// where does it get a reference you can write to?!
// the only viable option is something like Box::leak,
// which is obviously not what we want
let a = option_transmute(None);
let b = option_transmute(None);
*a = Some(1);
*b = Some(2); // now what?

@programmerjake
Copy link
Member

so maybe return &Option<T> and Option<&mut T> and name the functions something like flatten_ref_opt and flatten_mut

@programmerjake
Copy link
Member

or have all of:

impl<'a, T> Option<&'a Option<T>> {
    pub fn flatten_ref(self) -> Option<&'a T> { ... }
    pub fn flatten_ref_opt(self) -> &'a Option<T> { ... }
}
impl<'a, T> Option<&'a mut Option<T>> {
    pub fn flatten_mut(self) -> Option<&'a mut T> { ... }
}

@Coca162
Copy link
Author

Coca162 commented Mar 3, 2023

I'd go with:

impl<'a, T> Option<&'a Option<T>> {
    pub fn flatten_ref(self) -> &'a Option<T> { ... }
}
impl<'a, T> Option<&'a mut Option<T>> {
    pub fn flatten_ref(self) -> &'a Option<T> { ... }
    pub fn flatten_opt_mut(self) -> Option<&'a mut T> { ... }
}

Since a &Option<T> is easily transformable to a Option<&T> with .as_ref I think it's better to not have methods for them, though I don't really like putting opt in the name

@Coca162
Copy link
Author

Coca162 commented Mar 3, 2023

I've edited the issue and pr to reflect these changes 👍, I'll be keeping .flatten_mut unless some alternative name is suggested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api
Projects
None yet
Development

No branches or pull requests

2 participants