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

Tracking issue for Option::deref, Result::deref, Result::deref_ok, and Result::deref_err #50264

Open
U007D opened this Issue Apr 27, 2018 · 24 comments

Comments

Projects
None yet
@U007D
Copy link
Contributor

U007D commented Apr 27, 2018

#50267 implemented these APIs:

impl<T, E> Result<T, E> {
    pub fn deref_ok(&self) -> Result<&T::Target, &E> where T: Deref {…}
    pub fn deref_err(&self) -> Result<&T, &E::Target> where E: Deref {…}
    pub fn deref(&self) -> Result<&T::Target, &E::Target> where T: Deref, E: Deref {…}
}
impl<T> Option<T> {
    pub fn deref(&self) -> Option<&T::Target> where T: Deref {…}
}

Original feature request:


&Option -> Option<&str> and related functionality for Result (&Result<T, E> -> Result<&T, &E>) seem to be missing in std.

Conversations with @lucasem @QuietMisdreavus, @durka and UndeadLeech on irc led us to create this issue: namely to have a single-shot Option/Result function to map through Deref.

@U007D

This comment has been minimized.

Copy link
Contributor Author

U007D commented Apr 27, 2018

closed prematurely

@U007D U007D reopened this Apr 27, 2018

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue Jul 30, 2018

bors added a commit that referenced this issue Jul 31, 2018

Auto merge of #50267 - humanenginuity:master, r=alexcrichton
Implement inner deref for Option and Result

tracking issue: #50264

bors added a commit that referenced this issue Jul 31, 2018

Auto merge of #50267 - humanenginuity:master, r=alexcrichton
Implement inner deref for Option and Result

tracking issue: #50264
@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Nov 25, 2018

I just found this useful! Thanks @Centril for pointing me here. :)

However, the name is a bit odd: It goes from &Option<T> to Option<T::Target>, e.g. from &Option<String> to Option<&str> -- that's just as much taking a reference as dereferencing one.

@U007D

This comment has been minimized.

Copy link
Contributor Author

U007D commented Dec 3, 2018

Ah, naming... :)

@uHOOCCOOHu

This comment has been minimized.

Copy link

uHOOCCOOHu commented Dec 21, 2018

Oh, the method name collides Deref::deref, quite misleading.

I think as_deref could be better name.

@HeroicKatora

This comment has been minimized.

Copy link

HeroicKatora commented Jan 13, 2019

What about the name deref_inner for the case of Option and both arguments of Result; although the more though I put into it the more as_deref seems correct? Compared to other ref-utils this is missing the mut variants such as deref_ok_mut. I have mixed feelings about the api as a whole though, is there enough expressiveness advantage over a plain result.as_ref().map(|t| &**t)?

@durka

This comment has been minimized.

Copy link
Contributor

durka commented Jan 14, 2019

@U007D

This comment has been minimized.

Copy link
Contributor Author

U007D commented Jan 14, 2019

@RalfJung correctly points out that there's as much ref going on as deref; As I think about this, it's about moving the outer ref onto the inner type, so how does as_inner_ref() sound to all of you? Avoids the name collisions that were pointed out, too.

Also agree with @HeroicKatora that the mut variations are currently missing. If this sounds good to the group, I'll get started on another pass over the next little while.

@HeroicKatora

This comment has been minimized.

Copy link

HeroicKatora commented Jan 14, 2019

Hm, I see how this could be troubling at first, even more so with result.as_mut().map(|t| &mut **t). But in this case maybe the name should not be deref related afterall. While this maybe correct, one of the simplest issues where this API comes up might be finding a function of this type:

fn problem<'a, 'b>(&'a Option<&'b SomeType>) -> Option<&'a SomeType>

Another possibility would be to instead give those methods the type of

impl<'a, T> Option<&'a T> {
    fn deref<T>(self) -> Option<&'a T::Target> where T: Deref {
        self.map(|t| &**t)
    }
}

with usage opt.as_ref().deref(). This would be comparable with how the name Option::as_ref collides in relation to std::convert::AsRef and maybe still yield the discoverability as the inner workings of the map are probably the really hard issues for beginners. And maybe mention this example and usage in the documentation for as_ref() as well?

@nox

This comment has been minimized.

Copy link
Contributor

nox commented Mar 9, 2019

I just started making use of inert and this crate provides an Inert<T> type which implements both Deref<Target = T::Output> and AsRef<T::Output>.

Using Option::deref let me avoid a quite verbose line of code.

I have a reference to an Inert<Option<Dom<Node>>, which derefs to Option<Inert<Dom<Node>>>, where Inert<Dom<Node>> derefs to InertNode. I want Option<&InertNode>.

let parent_node: &Inert<Option<Dom<Node>>;

// Without Option::deref:
(**parent_node).as_ref().map(|node| &**node);

// With Option::deref:
parent_node.deref();

To summarise, this method helps a lot if you have a reference to something which derefs to Option<T> and also implements AsRef<Option<T>> and you want Option<&T::Target>.

@SimonSapin SimonSapin changed the title &Option<String> -> Option<&str> Tracking issue for Option::deref, Result::deref, Result::deref_ok, and Result::deref_err Mar 10, 2019

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Mar 10, 2019

Turned into a tracking issue, since #[unstable] attributes point here.

@nox

This comment has been minimized.

Copy link
Contributor

nox commented Mar 11, 2019

About the name: As I'm mostly going to use it to replace a method named r in Servo, I would like to keep it short. The name deref is fine to me, and I could tolerate as_deref.

About the signature: I've yet to encounter a case where I'm fed something like Option<&String>, I usually take a reference to something owned somewhere, of type Option<String>. I would rather keep the current signature.

@joshtriplett

This comment has been minimized.

Copy link
Member

joshtriplett commented Mar 15, 2019

This seems great to me. This has been around for a long time; other than discussions around the name, is there any reason we shouldn't stabilize this?

@U007D

This comment has been minimized.

Copy link
Contributor Author

U007D commented Mar 16, 2019

@joshtriplett 1) the name and 2) the *Mut flavors of these

FYI, I've already started work on the *Mut side of things; I expect be able to have the new PR ready by the end of the month, if not before (sorry, my external commitments prevent me from being more precise than that at this point...).

In terms of naming, I've been kicking around (no particular order):

  • as_ref_deref/as_mut_deref (describes &**, without being too pedantic (i.e. avoid "as_ref_deref_deref/..."), and
  • as_inner_deref/as_inner_deref_mut (differentiates from deref/deref_mut and implies what kinds of transformations to expect)
  • as_inner_ref/as_inner_mut (same as above, but highlights the ref instead of the deref; also saves 2 chars of typing)

Personally, I feel the current deref/deref_mut is a bit misleading, given how it deref is (generally) used in the rest of std (I understand there are departures from this convention already, but I would prefer not to use that as a license to lower consistency further.) On the other hand, maybe this "convention" is all in my head... I'm open to your thoughts on this.

Given that, personally I am leaning slightly toward as_ref_deref/as_mut_deref because it is both distinct from deref and reasonably? descriptive about what it does.

Brevity is important, as is a (reasonably unambiguous) descriptive name. Please weigh in if you have thoughts any of these method names or ideas for others, and I'll try to put the zeitgeist into the upcoming PR.

@joshtriplett

This comment has been minimized.

Copy link
Member

joshtriplett commented Mar 16, 2019

@OddCoincidence

This comment has been minimized.

Copy link

OddCoincidence commented Mar 20, 2019

Given we have cloned for Clone and soon copied for Copy, what about derefed?

@joshtriplett

This comment has been minimized.

Copy link
Member

joshtriplett commented Mar 21, 2019

@QuietMisdreavus

This comment has been minimized.

Copy link
Member

QuietMisdreavus commented Mar 21, 2019

@OddCoincidence

This comment has been minimized.

Copy link

OddCoincidence commented Mar 21, 2019

Also Option will have copied (not on iterator) - see #59231. Given the existence of those two, it seems most intuitive and discoverable to follow that pattern.

@nox

This comment has been minimized.

Copy link
Contributor

nox commented Mar 21, 2019

I would expect derefed to take a Option<&T>, while the current version of deref takes a &Option<T>, which makes it closer to as_ref.

@OddCoincidence

This comment has been minimized.

Copy link

OddCoincidence commented Mar 21, 2019

Oh, I didn't realize that - I assumed it was Option<&T> because that makes more sense to me. If it was, the latter case could still be done with option.as_ref().derefed().

@nox

This comment has been minimized.

Copy link
Contributor

nox commented Mar 21, 2019

It has already been discussed earlier.

@HeroicKatora

This comment has been minimized.

Copy link

HeroicKatora commented Mar 21, 2019

@nox @OddCoincidence Yes, I had thrown that into the room for discussion. Ultimately, the gut feeling of myself, and apparently others, is that this makes it much less useful.

There's of course the interesting insight that the intermediate as_ref collides with an AsRef trait during auto-deref, see in this example, and a method taking &self should probably feel 'nice' in that regard.

Simply standing in for .map(Deref::deref) would be less useful, it more or less only avoids a single use std::ops::Deref statement then. Somewhat unfortunate that Deref is not in the prelude but understandable as calling .deref() directly is almost never what you want (This feels like a bit of an oversight: We can use trait methods without importing the trait by use Trait as _ but not the other way around).

In addition, I think that as_ methods on Option are definitely more useful if they don't simply have the same signature as a trait, but rather are inner options on the left side and outer ones on the right. See also Option::as_pin_mut which continues this pattern excellently. Now, I only ask myself where to stop, and how to determine which core traits benefit the most from this pattern. For example

impl<T> Option<T> {
    as_as_ref<U>(&self) -> Option<&U> where T: AsRef<U>;
}

feels wrong/would have me ponder my life choices and could be seen as an indicator that the name should not be strictly oriented around the trait it borrows from.

@U007D

This comment has been minimized.

Copy link
Contributor Author

U007D commented Mar 26, 2019

Indeed, naming is hard(TM). ;)
I'll submit the next pr with as_deref()/as_deref_mut() and we'll see what folks think.

@U007D

This comment has been minimized.

Copy link
Contributor Author

U007D commented Apr 2, 2019

PR submitted with name change and *_mut impls + sanity-check tests.

Centril added a commit to Centril/rust that referenced this issue Apr 4, 2019

Rollup merge of rust-lang#59628 - U007D:master, r=Kimundi
`as_deref()` and `as_deref_mut()` impls

addresses rust-lang#50264
renamed `deref()` -> `as_deref()`
added `deref_mut()` impls + tests
fixed breaking changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.