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

Option::get_or_insert_default() #2876

Closed
asafigan opened this issue Mar 9, 2020 · 10 comments
Closed

Option::get_or_insert_default() #2876

asafigan opened this issue Mar 9, 2020 · 10 comments
Labels
T-libs-api Relevant to the library API team, which will review and decide on the RFC.

Comments

@asafigan
Copy link

asafigan commented Mar 9, 2020

For example:

let x = None;
*x.get_or_insert_default() += "Hello";

instead of:

let x = None;
*x.get_or_insert_with(Default::default) += "Hello";

This is similar to unwrap_or_default.

@RustyYato
Copy link

This seems really specialized, and I don't think that we really need this.

That said, there is some precedent for this:

Note, the most similar api, the HashSet's on nightly (rust-lang/rust#60896) doesn't include a get_or_insert_default, even though it has a get_or_insert_with. But this may be an oversight.

@hadronized
Copy link
Contributor

hadronized commented Mar 10, 2020

That seems like a fine addition. However, does not using it still imply a bigger object when we use trait objects?

@RustyYato
Copy link

@phaazon How are trait objects involved?

Note: Default::default does not involve trait objects (Default isn't even object-safe!), it is just sugar for <_ as Default>::default

@hadronized
Copy link
Contributor

hadronized commented Mar 11, 2020

If we add get_or_insert_default to Option, it means that we need to add a pointer to its vtable for when we pass a dyn Option<T>… right? So if we don’t use that function, we’re getting an extra cost there.

EDIT: I brainfarted.

I was just trying to brainstorm out loud. 😄

@burdges
Copy link

burdges commented Mar 11, 2020

There is no trait here @phaazon only the type Option along with this inherent method

impl<T: Default> Option<T> {
    pub fn get_or_insert_default(&mut self) -> &mut T {
        self.get_or_insert(Default::default())
    }
}

We've discussed expanding unsized type handling so that one could work more with bare trait objects, but I think here Option<dyn Trait> comes with different issues. In this case, you're maybe asking if one could use some vtable for Option<dyn Default> provided

impl<T, U> CoerceUnsized<Option<U>> for Option<T> where T: Unsize<U> + ?Sized, U: ?Sized, 

We've no CoerceUnsized enums right not, so maybe enum layout niches requires special care, but maybe it works since we already have

impl<T, U> CoerceUnsized<NonNull<U>> for NonNull<T> where T: Unsize<U> + ?Sized, U: ?Sized, 

Worse, there are presently no CoerceUnsized types that really "interact" with the underlying type, so we surely lack the expanded vtables for types like Option<dyn Default>. If we had them. then Result::ok must transform from the Result<dyn Default,_> vtable into the Option<dyn Default> vtable in this:

(dyn_result: Result<dyn Default,_>).ok().get_or_insert_default()

Awful lot of compiler complexity for quite a niche use case. ;)

@Pauan
Copy link

Pauan commented Mar 11, 2020

@phaazon If we add get_or_insert_default to Option, it means that we need to add a pointer to its vtable for when we pass a dyn Option… right?

Option isn't a trait, you cannot use dyn Option<T>, there is no pointer, there is no vtable, there is no runtime cost to adding this method.

@hadronized
Copy link
Contributor

Damn, what the heck was I thinking… 😆

@burdges
Copy link

burdges commented Mar 11, 2020

I agree with @KrishnaSannasi that this seems overly specialized because:

  • If you're programming generically then actually maybe writing out get_or_insert_with(Default::default) gives clarity.. maybe.
  • If not then you'd often enter the default manually for clarity.

There is however one argument for doing this: We'd often do get_or_insert(Default::default()) but we should prefer the lazy get_or_insert_with(Default::default) instead, so maybe this shifts that tendency. I'd think a comment about this in the docs suffices, but whatever.

@Ixrec
Copy link
Contributor

Ixrec commented Mar 11, 2020

The get_or_insert(Default::default()) mistake seems likely to happen whether or not this method exists, so I think the right solution for that is just a clippy lint. I believe they have at least one lint for "accidental non-laziness", but I don't know if that already covers this specific case.

@scottmcm scottmcm added the T-libs-api Relevant to the library API team, which will review and decide on the RFC. label Nov 10, 2020
@scottmcm
Copy link
Member

FWIW, Vec::resize_default was deprecated in favour of .resize_with(Default::default), so there's precedent in both directions.

I'm going to close this because small inherent API additions like this don't need an RFC, and this isn't a complicated method to implement. If anyone thinks it should be in core, make a PR and get the libs team's feedback that way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-libs-api Relevant to the library API team, which will review and decide on the RFC.
Projects
None yet
Development

No branches or pull requests

7 participants