Proposal: Add inspect and inspect_err methods to Result, and inspect method to Option

[cyqsimon]

When you have a function call that returns a Result type, it's a very common need to perform some immutable action using one of its contained values (such as printing an additional message), and then pass on the Result for other uses. I guess this is best described as the inspect operation of a wrapper type, similar to Iterator::inspect in its purpose.

Consider this pseudocode snippet:

1. Take a path, treat it as a Unix socket, and bind a listener to it
2. If error, print out a custom message, and propagate error
3. Do other things with the listener
4. Return if all successful

Ideally, I would want to write this:

fn start_listener<P: AsRef<Path>>(bind_addr: P) -> io::Result<()> {
    let listener = UnixListener::bind(&bind_addr)
        .inspect_err(|err| println!("Listener cannot bind: {}", err))?;

    // do things with listener
    Ok(())
}

Currently, we could write this code:

fn start_listener<P: AsRef<Path>>(bind_addr: P) -> io::Result<()> {
    let bind_res = UnixListener::bind(&bind_addr);
    if let Err(err) = &bind_res {
        println!("Listener cannot bind: {}", err);
    }

    let listener = bind_res?;
    // do things with listener
    Ok(())
}

Or this:

fn start_listener<P: AsRef<Path>>(bind_addr: P) -> io::Result<()> {
    let listener = match UnixListener::bind(&bind_addr) {
        ok_val @ Ok(_) => ok_val,
        Err(err) => {
            println!("Listener cannot bind: {}", err);
            Err(err)
        }
    }?;

    // do things with listener
    Ok(())
}

Or this:

fn start_listener<P: AsRef<Path>>(bind_addr: P) -> io::Result<()> {
    let listener = UnixListener::bind(&bind_addr).map_err(|err| {
        println!("Listener cannot bind: {}", err);
        err
    })?;

    // do things with listener
    Ok(())
}

The first two options are verbose; the third is overkill because we don't actually need to map in this case, so there's no need to take ownership of err. If we introduce an inspect_err method, it would make the code more concise, but more importantly it delivers the intent much more clearly. Also bear in mind that this is a very simple example - in many use cases method calls on Result and Option are often chained. In those situations the small ergonomics improvements would be more pronounced.

The proposed Result::inspect would have the signature:

pub fn inspect<F>(self, op: F) -> Self where F: Fn(&T) -> (),

Result::inspect_err would have the signature:

pub fn inspect_err<F>(self, op: F) -> Self where F: Fn(&E) -> (),

In the same spirit, Option::inspect would have the signature:

pub fn inspect<F>(self, op: F) -> Self where F: Fn(&T) -> (),

[y2kappa]

so it's map_err with a readable reference? is moving the err so expensive such that it's worth the effort and extra complexity (small admittedly) of adding an extra API to Result?

[cyqsimon]

It's not as much about the performance cost of moving the inner value than it is about code readability and ergonomics.

But you are right - it is a tiny change. If it's an obscure API that maybe 5% of rust users would ever use then I surely wouldn't have bothered. However, considering Result and Option are literally some of the most commonly used types in every Rust project, I would argue that every tiny bit of incremental improvement is worth the effort.

[Kixiron]

I'll pitch in that I've had multiple times where I wanted these methods for messing with results

[taufik-rama]

I think this is an improvements for ergonomics

As far as I understand, Result and Option is basically already an iterator, with 1 element if it's an Ok() or Some() respectively. A method of inspect() (or maybe peek()) makes sense for such iterator IMO

[cyqsimon]

Let me rewrite my deleted comment... I got confused by myself on what I wanted to say.

Yes I took inspiration from Iterator::inspect - it simply makes chaining calls much more comfortable, despite the existence of Iterator::map. I would like to do similar for Result and Option.

And just to get my bases covered, we cannot simply call iter on Result and use Iterator::inspect because the Err variant is unrecoverable. Maybe it's possible with Option but that's quite awkward.

[taufik-rama]

Yes, just trying to point out that the semantic already exists, so IMO it doesn't feel out of place

[LoganDark]

I believe that this is quite comprehensively covered by the tap crate (and tap_trait is quite fun too for those used to Kotlin's generic extension functions like let). I wouldn't be opposed to some functionality of tap being added to the standard library. (And I believe tap is a better name than inspect, anyway.)

[ibraheemdev]

Small API additions like this don't usually need an RFC, so I went ahead and opened a PR: rust-lang/rust#91346

[Veetaha]

I've stumbled with a compatibility warning on nightly that comes from our internal stdx crate which defines extension traits for Result and Option.

We've been successfully using the following extension methods:

impl<T> Option<T> {
    pub fn inspect_some(self, f: impl FnOnce(&T)) -> Self;
    pub fn inspect_none(self, f: impl FnOnce()) -> Self;
}

impl<Ok, Err> Result<Ok, Err> {
    pub fn inspect_ok(self, f: impl FnOnce(&Ok)) -> Self;
    pub fn inspect_err(self, f: impl FnOnce(&Err)) -> Self;   
}

Here is how they are defined and documented in our code (writing it separately under the spoiler to decrease verbosity):

Details

pub trait OptionExt<T> {
    /// Similar to [`std::iter::Iterator::inspect()`], but the closure is
    /// invoked if the [`Option`] contains [`Some`] value.
    ///
    /// ```
    /// use stdx::prelude::*;
    ///
    /// let mut invoked = false;
    ///
    /// None::<i32>.inspect_some(|_| {
    ///     invoked = true;
    /// });
    ///
    /// assert!(!invoked);
    ///
    /// Some(99).inspect_some(|&val| {
    ///     assert_eq!(val, 99);
    ///     invoked = true;
    /// });
    ///
    /// assert!(invoked);
    /// ```
    fn inspect_some(self, f: impl FnOnce(&T)) -> Self;

    /// Similar to [`std::iter::Iterator::inspect()`], but the closure is
    /// invoked if the [`Option`] contains [`None`] value.
    ///
    /// ```
    /// use stdx::prelude::*;
    ///
    /// let mut invoked = false;
    ///
    /// Some(99).inspect_none(|| {
    ///     invoked = true;
    /// });
    ///
    /// assert!(!invoked);
    ///
    /// None::<i32>.inspect_none(|| {
    ///     invoked = true;
    /// });
    ///
    /// assert!(invoked);
    /// ```
    fn inspect_none(self, f: impl FnOnce()) -> Self;
}

impl<T> OptionExt<T> for Option<T> {
    fn inspect_some(self, f: impl FnOnce(&T)) -> Self {
        if let Some(ref it) = self {
            f(it);
        }
        self
    }

    fn inspect_none(self, f: impl FnOnce()) -> Self {
        if self.is_none() {
            f();
        }
        self
    }
}

pub trait ResultExt<Ok, Err> {
    /// Similar to [`std::iter::Iterator::inspect()`], but the closure is
    /// invoked if [`Result`] contains [`Ok`] value.
    ///
    /// ```
    /// use stdx::prelude::*;
    /// use std::convert::Infallible as Never;
    ///
    /// let mut invoked = false;
    ///
    /// Err::<Never, i32>(99).inspect_ok(|val| {
    ///     // empty match means that the code is statically deduced unreachable
    ///     match *val {}
    /// });
    ///
    /// Ok::<i32, Never>(99).inspect_ok(|&val| {
    ///     invoked = true;
    ///     assert_eq!(val, 99);
    /// });
    ///
    /// assert!(invoked);
    /// ```
    fn inspect_ok(self, f: impl FnOnce(&Ok)) -> Self;

    /// Similar to [`std::iter::Iterator::inspect()`], but the closure is
    /// invoked if [`Result`] contains [`Err`] value.
    ///
    /// ```
    /// use stdx::prelude::*;
    /// use std::convert::Infallible as Never;
    ///
    /// let mut invoked = false;
    ///
    /// Ok::<i32, Never>(99).inspect_err(|err| {
    ///     // empty match means that the code is statically deduced unreachable
    ///     match *err {}
    /// });
    ///
    /// Err::<Never, i32>(99).inspect_err(|&err| {
    ///     invoked = true;
    ///     assert_eq!(err, 99);
    /// });
    ///
    /// assert!(invoked);
    /// ```
    fn inspect_err(self, f: impl FnOnce(&Err)) -> Self;
}

impl<Ok, Err> ResultExt<Ok, Err> for Result<Ok, Err> {
    fn inspect_ok(self, f: impl FnOnce(&Ok)) -> Self {
        if let Ok(ref it) = self {
            f(it);
        }
        self
    }

    fn inspect_err(self, f: impl FnOnce(&Err)) -> Self {
        if let Err(ref err) = self {
            f(err);
        }
        self
    }
}

I propose Rust's std to follow the same path. Having inspect_ok/err/some/none seems much more flexible, symmetric and intuitive. For instance, why wouldn't we introduce inspect_ok if inspect_err exists?

I propose block stabilization of this API until we take the proposed 4 methods into account.

cc @dtolnay

Btw. do you have any suggestions on how we could work around the unstable_name_collisions warning in our extension traits?
inspect_* methods sound too intuitive to replace them with something else...

[Kixiron]

Stabilizing any of these methods doesn't require stabilizing all of them, more methods shouldn't block each other

[VladasZ]

I agree with @Veetaha. In my opinion inspect_ok and inspect_some look more explicit and uniform than just inspect.

[cyqsimon]

Closing this issue. For any further comments please use tracking issue.