Tracking Issue for `result_option_inspect`

[ibraheemdev]

Feature gate: #![feature(result_option_inspect)]

This is a tracking issue for Option::inspect and Result::{inspect, inspect_err}.

Public API

// core::result

impl Result<T, E> {
    pub fn inspect<F: FnOnce(&T)>(self, f: F) -> Self;
    pub fn inspect_err<F: FnOnce(&E)>(self, f: F) -> Self;
}

// core::option

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

Steps / History

• [x] Implementation: #91346
• [ ] Final comment period (FCP)
• [ ] Stabilization PR

Unresolved Questions

• Should there be a more general Tap trait in std?

[Veetaha]

I propose to discuss adding Option::inspect_some, Option::inspect_none, Result::inspect_ok, Result::inspect_err methods as described in https://github.com/rust-lang/rfcs/issues/3190#issuecomment-986236562 before stabilizing

[cyqsimon]

I kind of like the idea of Option::inspect_none. It could be very useful in long method call chains to, for example, spit out a warning.

On the other hand, I don't think it's necessary to rename Option::inspect into Option::inspect_some and Result::inspect into Result::inspect_ok. Considering unwrap is not named unwrap_ok and unwrap_some and map is not named map_ok and map_some, I think using the simple inspect name maintains more consistency.

[sassman]

what are the next steps to stabilize this proposal?

[Elias-Graf]

Could this somehow enable the following?

struct Cookie {
    size: usize,
};

let maybe_cookie = Some(Cookie { size: 5 });

maybe_cookie.inspect(|mut c| c.size = 3);

I suppose the current alternative is:

// The formatter put it onto multiple lines
maybe_cookie.map(|mut c| {
    c.size = 3;
    c
});

But that is not really pretty, especially when assigning to fields or directly in a function call:

eat(maybe_cookie.inspect(|mut c| c.size = 3));

fn eat(cookie: Option<Cookie>) {
    // 
}

[TrolledWoods]

@Elias-Graf You can already do this with map, with something like maybe_cookie.as_mut().map(|c| c.size = 3); (or |c| { c.size = 3; c } to get access to the value later), inspect wouldn't work I think since it gives you an immutable reference to the item. Alternatively we could add inspect_mut or something, but it doesn't feel like there's a whole lot of usecases for it.

[rami3l]

@Elias-Graf You might be looking for something like tap_mut instead of inspect. As the name suggests, inspecting an object doesn't change it.

Whether std accepts this convenient method is entirely another topic, which falls into the question of whether we want to include tapping or not.

[kartva]

Any progress on an FCP?

[elpiel]

Any progress on an FCP?

Another set of hands for stabilizing this feature. I think it is very useful for e.g. logging a given error too and it's a nice addition in general

[kartva]

Ok, from what I understand of FCP protocol, we should first make sure there are no outstanding concerns. The correct thing to do should be to ping one of the libs team members like @joshtriplett to give their comments and then potentially file an FCP.

[Antikyth]

I don't know how much help I can be to the process, if at all (please let me know if there are things to read about how I can help at all^), but I would like to mention that I have found myself wanting to use this functionality quite often. It makes for clean code in a pretty common situation.

^I'm relatively new to Rust, and haven't contributed to it before. I would love to learn more about what that involves, though.

[benwis]

I think this would be a nice addition, not sure how to push it forward though.

[traviscross]

Most of the time when I want inspect for Result (or Option), I want the closure to be passed the entire thing (i.e. &Self), rather than just the Ok or Err (or Some or None) value. That is, I want to write:

use tracing::debug;
client
    .protocol("https")
    .method("GET")
    .send()
    .await // returns `Result<_, _>
    .inspect(|result| debug!(?result))
    .map_err(Error::from)

rather than:

use tracing::debug;
client
    .protocol("https")
    .method("GET")
    .send()
    .await // returns `Result<_, _>
    .inspect(|ok_val| debug!(result = ?Ok::<_, ()>(ok_val)))
    .inspect_err(|err_val| debug!(result = ?Err::<(), _>(err_val)))
    .map_err(Error::from)

If we wanted, we could still add inspect_ok, inspect_err, inspect_some, and inspect_none for the cases where they might be more convenient. But we would not need to, as this version of inspect would have the power of all of them.

Arguably, this definition is more consistent with Iterator::inspect, as that method sees every item. I would find it a bit surprising if inspect didn't see everything that's passing through the method chain.

For concreteness, here is the API I propose:

// core::result

impl<T, E> Result<T, E> {
    pub fn inspect<F: FnOnce(&Self)>(self, f: F) -> Self;
}

// core::option

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

[Antikyth]

Most of the time when I want inspect for Result (or Option), I want the closure to be passed the entire thing (i.e. &Self), rather than just the Ok or Err (or Some or None) value. That is, I want to write:

use tracing::debug;
client
    .protocol("https")
    .method("GET")
    .send()
    .await // returns `Result<_, _>
    .inspect(|result| debug!(?result))
    .map_err(Error::from)

rather than:

use tracing::debug;
client
    .protocol("https")
    .method("GET")
    .send()
    .await // returns `Result<_, _>
    .inspect(|ok_val| debug!(result = ?Ok::<_, ()>(ok_val)))
    .inspect_err(|err_val| debug!(result = ?Err::<(), _>(err_val)))
    .map_err(Error::from)

If we wanted, we could still add inspect_ok, inspect_err, inspect_some, and inspect_none for the cases where they might be more convenient. But we would not need to, as this version of inspect would have the power of all of them.

Arguably, this definition is more consistent with Iterator::inspect, as that method sees every item. I would find it a bit surprising if inspect didn't see everything that's passing through the method chain.

For concreteness, here is the API I propose:

// core::result

impl<T, E> Result<T, E> {
    pub fn inspect<F: FnOnce(&Self)>(self, f: F) -> Self;
}

// core::option

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

That makes sense to me. I would definitely say that adding inspect_ok, inspect_some, inspect_err, inspect_none for convenience would be a good idea to go with this proposal - I think there are many times when you want to inspect the 'whole' Result or Option, but also many times when you wouldn't.

[shepmaster]

There’s nothing special about an inspect that has the whole value that should be limited to Result — any type whatsoever could apply.

That’s the tap crate https://docs.rs/tap/latest/tap/

[traviscross]

@shepmaster. That's true. Fair enough. Thanks for that.

Above, the body of this ticket has an unresolved question:

• Should there be a more general Tap trait in std?

This discussion suggests that, yes, there probably should be.

[ibraheemdev]

Instead of a trait, tap could be a postfix macro which would allow for things like:

let x: Option<i32> = option
    .tap!(|x| dbg!(x))
    .tap_matches!(|Some(x)| Some(x + 1))
    .inspect!(|x| println!("{x}"))
    .inspect_matches!(|None| println!("x is None"));

Where tap logically takes an FnOnce(T) -> T, inspect takes a Fn(&T) -> T, and the _matches versions only run if the value matches the pattern.

[mibmo]

I definitely second emulating the tap crate's behavior, favoring @ibraheemdev's approach. Having a _matches variant could be super convenient, especially with both mutable (tap) and immutable (inspect) variants.

[simonsan]

I don't necessarily like the addition of a macro for this, tbh, and would love to see this going the Tap trait in std way. Also, with postfix macros it would depend on another RFC (https://github.com/rust-lang/rfcs/pull/2442). That being said, anything needed for bringing this forward?

[rursprung]

i just ran into this because i found the feature inspect_err and it'd be quite useful for me. as pointed out by others, being able to call that would clean up a lot of logging code, even in otherwise non-fluent code.

e.g. for this use-case here:

fn foo() -> Result<Foo, SomeError> { /* ... */ }

fn bar() -> Result<Foo, SomeError> {
  foo().inspect_err(|e| warn!("encountered error {} while performing task xyz", e))
}

or, even simpler, when you don't actually need the result afterwards:

fn foo() -> Result<(), SomeError> { /* ... */ }

fn bar() {
  foo().inspect_err(|e| warn!("encountered error {} while performing task xyz", e));
}

and i think that this is distinct from the tap use-case where you want access to the complete object. so i think the two can be looked at separately:

• the APIs introduced by this feature here
• a tap-like API in std (or, hopefully, core for no_std support?) as a separate feature

[lkts]

I completely agree, tap API seems like a different feature.

inspect and inspect_err are common in code bases i work on because they exist on TryFuture (https://docs.rs/futures/latest/futures/future/trait.TryFutureExt.html#method.inspect_err). Searching on GitHub also shows quite a bit of hand-rolled implementations. I think this is a pretty good indicator of the usefulness of API.

[TmLev]

What's blocking this from being stabilised?

[matthiasbeyer]

I would also like to see this stable, so I can deprecate my option-inspect and result-inspect crates! :laughing:

[SignalWhisperer]

As many have mentioned, this would be a very useful feature. I have some errors that are soft failures with a fallback, but I still want to log them.

Here's how I currently do it:

let result = perform_action()
    .map_err(|e| { error!(?e, "Action failed, falling back."); e })
    .unwrap_or(default_value);

And here's how simple it would be to do with inspect_err:

let result = perform_action()
    .inspect_err(|e| error!(?e, "Action failed, falling back."))
    .unwrap_or(default_value);

The difference is small, but I feel like syntactically it is wrong to say that I am "mapping the error".

We should leave inspect to work on Ok rather than the entire Result to match the behavior of expect, map, unwrap... Those all have an equivalent _err function that works on Err. I'd say inspect should work on Ok and inspect_err on Err.

[kitsuneninetails]

Sorry to chime in late, but I just saw the suggestion to have inspect closure take the whole object. I have to say, I'm having trouble making sense of the idea. Wouldn't you just take the object as-is, then? Especially since inspect is a const function that just prints or "inspects" the item without mapping or changing.

For example, the code listed above:

use tracing::debug;
client
    .protocol("https")
    .method("GET")
    .send()
    .await // returns `Result<_, _>
    .inspect(|result| debug!(?result))
    .map_err(Error::from)

would seem more straightforward, rustic, transparent, and simple as:

use tracing::debug;
let res = client
    .protocol("https")
    .method("GET")
    .send()
    .await; // returns `Result<_, _>
debug!(?res);
res.map_err(Error::from)

would it not?

Do we need to add a whole implementation just to inspect the object itself when we have the object? Just to make it perhaps appear more "functional" (although I'm not sure I'd call it more functional in truth since it adds or subtracts nothing from functional principles)?

I'd vote to keep inspect as the "Ok/Some" version and inspect_err for Err() (I don't really think an inspect_none is adding anything to the Option class, to be honest).

Anyways, just my two cents...

[gubsey]

What still needs to be done for this to be stabilized?

[SpriteOvO]

If a more general trait will be added, and the method will be named as inspect, should we reserve the name for it from {Result,Option}::inspect? This is a point of view that supports .inspect_{ok,some}. Although I personally like {Result,Option}::inspect a bit more, since it has a consistent naming style with the existing .map, .unwrap, etc.

[photino]

Any progress on this feature?

[john-h-k]

Yeah, this would be really nice

[dbofmmbt]

I asked about it on this zulip topic but got no answers. Maybe we should ping someone from libs team to help us set a direction for this feature.

[photino]

Since we already have inspect() for an iterator, it is consistent to keep inspect as the Some version.