Rust: Tracking issue for Option::deref, Result::deref, Result::deref_ok, and Result::deref_err (feature: inner_deref)

Created on 27 Apr 2018  Â·  40Comments  Â·  Source: rust-lang/rust

API

impl<T, E> Result<T, E> {
    fn as_deref(&self) -> Result<&T::Target, &E> where T: Deref;
    fn as_deref_err(&self) -> Result<&T, &E::Target> where E: Deref;
    fn as_deref_mut(&mut self) -> Result<&mut T::Target, &mut E> where T: DerefMut;
    fn as_deref_mut_err(&mut self) -> Result<&mut T, &mut E::Target> where E: DerefMut;
}


Implementation history

  • Implemented in https://github.com/rust-lang/rust/pull/50267 as:

    impl<T> Option<T> {
        fn deref(&self) -> Option<&T::Target> where T: Deref;
    }
    impl<T, E> Result<T, E> {
        fn deref(&self) -> Result<&T::Target, &E::Target> where T: Deref, E: Deref;
        fn deref_ok(&self) -> Result<&T::Target, &E> where T: Deref;
        fn deref_err(&self) -> Result<&T, &E::Target> where E: Deref;
    }
    
  • Renamed and extended in #59628 / #62421:

    impl<T> Option<T> {
        fn as_deref(&self) -> Option<&T::Target> where T: Deref;
        fn as_deref_mut(&mut self) -> Option<&mut T::Target> where T: DerefMut;
    }
    impl<T, E> Result<T, E> {
        fn as_deref(&self) -> Result<&T::Target, &E::Target> where T: Deref, E: Deref;
        fn as_deref_ok(&self) -> Result<&T::Target, &E> where T: Deref;
        fn as_deref_err(&self) -> Result<&T, &E::Target> where E: Deref;
        fn as_deref_mut(&mut self) -> Result<&mut T::Target, &mut E::Target> where T: DerefMut, E: DerefMut;
        fn as_deref_mut_ok(&mut self) -> Result<&mut T::Target, &mut E> where T: DerefMut;
        fn as_deref_mut_err(&mut self) -> Result<&mut T, &mut E::Target> where E: DerefMut;
    }
    
  • Option methods stabilized in #64708:

    impl<T> Option<T> {
        fn as_deref(&self) -> Option<&T::Target> where T: Deref;
        fn as_deref_mut(&mut self) -> Option<&mut T::Target> where T: DerefMut;
    }
    
  • Result methods pared down and renamed in #67930:

    impl<T, E> Result<T, E> {
        fn as_deref(&self) -> Result<&T::Target, &E> where T: Deref;
        fn as_deref_err(&self) -> Result<&T, &E::Target> where E: Deref;
        fn as_deref_mut(&mut self) -> Result<&mut T::Target, &mut E> where T: DerefMut;
        fn as_deref_mut_err(&mut self) -> Result<&mut T, &mut E::Target> where E: DerefMut;
    }
    
A-result-option B-unstable C-tracking-issue Libs-Tracked T-libs disposition-merge finished-final-comment-period

Most helpful comment

I would prefer dropping Result::as_deref and renaming as_deref_ok to as_deref, similar for mut. This matches the way we have Result::map to operate on the Ok value and Result::map_err to operate on the Err value.

All 40 comments

closed prematurely

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.

Ah, naming... :)

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

I think as_deref could be better name.

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)?

It's less about expressiveness than discoverability I feel. I know the
as_ref + &** trick but it's hard to land on as a learner.

On Sun, Jan 13, 2019, 18:57 HeroicKatora <[email protected] wrote:

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)?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/rust-lang/rust/issues/50264#issuecomment-453877643,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAC3nxzC19t6xTAx6Rj5F44SzM-XmPliks5vC8fbgaJpZM4TpOU5
.

@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.

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?

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>.

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

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.

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?

@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.

On March 15, 2019 11:06:02 PM PDT, Brad Gibson notifications@github.com wrote:

@joshtriplett the name and 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 specific
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)

Personally, I feel the current deref/deref_mut is a bit misleading,
given how it is (generally) used in the rest of std (I understand
there are departures from this convention already, but I do not want to
use that as a license to lower consistency further.)

Personally, I am leaning slightly toward as_ref_deref/as_mut_deref
because it is both distinct and descriptive.

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

Shorter seems preferable here; as_deref seems as long as it should get.

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

On March 20, 2019 4:07:49 PM PDT, Joey notifications@github.com wrote:

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

Those are for iterators. I'd be all for an iterator operation to deref elements and calling that derefed. However, for Option, I think we should use "deref" or "as_deref".

@joshtriplett Option has a cloned method as well: https://doc.rust-lang.org/nightly/std/option/enum.Option.html#method.cloned

Also Option will have copied (not on iterator) - see https://github.com/rust-lang/rust/pull/59231. Given the existence of those two, it seems most intuitive and discoverable to follow that pattern.

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.

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().

It has already been discussed earlier.

@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.

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

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

These APIs now point here:

impl<T> Option<T> {
    pub fn as_deref(&self) -> Option<&T::Target> where T: Deref {…}
    pub fn as_deref_mut(&mut self) -> Option<&mut T::Target> where T: DerefMut {…}
}

impl<T, E> Result<T, E> {
    pub fn as_deref_ok(&self) -> Result<&T::Target, &E> where T: Deref {…}
    pub fn as_deref_err(&self) -> Result<&T, &E::Target> where E: Deref {…}
    pub fn as_deref(&self) -> Result<&T::Target, &E::Target> where T: Deref, E: Deref {…}

    pub fn as_deref_mut_ok(&mut self) -> Result<&mut T::Target, &mut E> where T: DerefMut {…}
    pub fn as_deref_mut_err(&mut self) -> Result<&mut T, &mut E::Target> where E: DerefMut {…}
    pub fn as_deref_mut(&mut self) -> Result<&mut T::Target, &mut E::Target> where T: DerefMut, E: DerefMut {…}
}

Any remaining issue with them? Or shall we

@rfcbot fcp merge

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

  • [ ] @Amanieu
  • [ ] @Kimundi
  • [x] @SimonSapin
  • [x] @alexcrichton
  • [ ] @dtolnay
  • [x] @sfackler
  • [ ] @withoutboats

Concerns:

  • do we need the Result methods that deref both (https://github.com/rust-lang/rust/issues/50264#issuecomment-533952247)

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.

We dislike simply forwarding the Deref and DerefMut impls somehow?

impl Deref for Option<T> where T: Deref {
    type Target = <T as Deref>::Target;
    pub fn deref(&self) -> Option<&<T as Deref>::Target> {
        self.as_ref().map(|t| t.deref())
    }
}
impl DerefMut for Option<T> where T: Deref+DerefMut {
    pub fn deref_mut(&self) -> Option<&<T as Deref>::Target> {
        self.as_mut().map(|t| t.deref_mut())
    }
}

It cannot be made consistent for Result but..

Deref::deref and DerefMut::deref_mut must return references, what you suggested is not possible.

Do we have any known uses for Result::as_deref or Result::as_deref_mut? It seems like it would be quite rare that those would be what you want.

@rfcbot concern do we need the Result methods that deref both

I would prefer dropping Result::as_deref and renaming as_deref_ok to as_deref, similar for mut. This matches the way we have Result::map to operate on the Ok value and Result::map_err to operate on the Err value.

This is neat! Personally, my interest has mainly been for Option::as_deref, which I've been happily using in the compiler.

I made a stabilization PR and separate FCP for just the Option methods: https://github.com/rust-lang/rust/pull/64708

The changes proposed by @dtolnay in https://github.com/rust-lang/rust/issues/50264#issuecomment-533952881 sound to me like a good way to resolve the concern https://github.com/rust-lang/rust/issues/50264#issuecomment-533952247. It’s been a while and there hasn’t been an objection.

So, anyone wants to make a PR for that change?

@rfcbot fcp cancel

The Option methods were stabilized in #64708, and the Result methods are still waiting for a PR that implements https://github.com/rust-lang/rust/issues/50264#issuecomment-533952881.

@dtolnay proposal cancelled.

@rust-lang/libs:
@rfcbot fcp merge

Judging by https://grep.app/search?q=as_deref%28_mut%29%3F%5C%28%5C%29&regexp=true&filter[lang][0]=Rust, the Option::as_deref and Option::as_deref_mut methods have seen a fair amount of use.

I am interested in stabilizing these corresponding two methods on Result. The first was added in #50267 under the name deref_ok, then in #62421 renamed to as_deref_ok and augmented with as_deref_mut_ok, then in #67930 renamed to the current names as requested by https://github.com/rust-lang/rust/issues/50264#issuecomment-533952881.

fn as_deref(&self) -> Result<&T::Target, &E> where T: Deref;
fn as_deref_mut(&mut self) -> Result<&mut T::Target, &mut E> where T: DerefMut;

Additionally I propose removing the remaining two methods tracked by this tracking issue. I expect these to be much less commonly applicable and they can be written instead as as_ref().map_err(Deref::deref) and as_mut().map_err(DerefMut::deref_mut).

fn as_deref_err(&self) -> Result<&T, &E::Target> where E: Deref;
fn as_deref_mut_err(&mut self) -> Result<&mut T, &mut E::Target> where E: DerefMut;

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

  • [x] @Amanieu
  • [x] @SimonSapin
  • [x] @dtolnay
  • [ ] @sfackler
  • [ ] @withoutboats

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.

they can be written instead as as_ref().map_err(Deref::deref) and as_mut().map_err(DerefMut::deref_mut).

How discoverable is that, though? After all, the exact same argument applies to the methods that are being stabilized. Only stabilizing the Ok methods seems weirdly asymmetric.

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

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.

The RFC will be merged soon.

Was this page helpful?
0 / 5 - 0 ratings

Related issues

nikomatsakis picture nikomatsakis  Â·  274Comments

withoutboats picture withoutboats  Â·  213Comments

Centril picture Centril  Â·  382Comments

nikomatsakis picture nikomatsakis  Â·  259Comments

nikomatsakis picture nikomatsakis  Â·  236Comments