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

• Implementation: Add Option::inspect and Result::{inspect, inspect_err} #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 rust-lang/rfcs#3190 (comment) 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.

[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 (rust-lang/rfcs#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! 😆

[simonsan]

Even in a case where inspect's closure takes &T, that T might still use interior mutability. That is, there's no guarantee that .inspect(|x| x.foo()) doesn't modify data inside of x.

interior mutability is a whole other beast, IMHO. I personally wouldn't expect an inspection to change things without my consent in real life, e.g. on a car inspection. I would rather ask for what needs to be changed to explicitly change it, then in the next step. Furthermore, I think this is kind of the social contract that inspect as a word implies. Changing that, would be weird and contrary to the principles of least surprise.

[gubsey]

inspect is already used by iterators. Iterator::inspect does not provide mutability, so Option::inspect shouldn't either. There should be consistency in how things are named. I see zero reason to not just stabilize this feature now with the signature of pub fn inspect<F>(self, f: F) -> Option<T> where F: FnOnce(&T).

[gubsey]

How do we push this to get stabilized?

[janriemer]

Just as a counter voice, I would be fine if inspect's closure argument was of type &mut T.

To me, inspect is all about performing side effects. Those side effects could include:

* printing/logging

* modifying a captured variable

* modifying the closure's argument

Printing and logging modify global state, and people are generally OK with that inside of inspect. Indeed, that seems to be a huge usecase.

The closure is FnOnce as opposed to Fn, which allows us to modify (or even consume!) the variables captured by the closure. I haven't seen any complaints about that choice, even though it's been there for 75+ weeks.

Even in a case where inspect's closure takes &T, that T might still use interior mutability. That is, there's no guarantee that .inspect(|x| x.foo()) doesn't modify data inside of x.

tl;dr

inspect on Option/Result should take FnOnce(&T), which is the most consistent to Iterator.inspect.

Explanation of proposed suggestion

IMHO, one of the most important things when designing something is:

• how well it interacts with other features
• how consistent it is to other features
• how intuitive it is to learn/understand

Something is intuitive to learn, if you only need to learn a concept once and can transfer that knowledge to other parts of the system that should behave the same (according to what has been your "entry point" of learning the concept).

When we take those three points, we might come to the following conclusion:
Option/Result resembles the shape of an Iterator (like map, filter, zip) - they can also easily be turned into an Iterator (they implement IntoIterator after all).

=> When methods on Option/Result and Iterator with the same name exist, they should be as similar as possible.

In Iterator.inspect the generic closure has the type FnMut(&Self::Item):

• FnMut, because when we iterate over more than one element, we need to call the closure more than once (this is not necessary for Option/Result as they only contain at most one element)
• &Self::Item, because we don't want to change the value we are inspecting

Conclusion

inspect on Option/Result should take FnOnce(&T), which is the most consistent to Iterator.inspect.

[Amanieu]

Considering the points raised, let's try this again, this time keeping the closure argument as &T.

The fact that this function is not as general as it could be isn't a big issue: after all this is just a convenience method, you can always use map for the more complex but rarer cases where mutation is needed.

@rfcbot fcp merge

[rfcbot]

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

• @Amanieu
• @BurntSushi
• @dtolnay
• @joshtriplett
• @m-ou-se

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.

[rfcbot]

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

[slanterns]

I would like to work on a stabilization PR for it.

@rustbot claim

[SpriteOvO]

Since result_option_inspect has almost been accepted, perhaps we should reopen PR #94317 for adding Option::inspect_none?

Some discussions about it:

rust-lang/rfcs#3190 (comment)
#91345 (comment)
#91345 (comment)

[ppershing]

I would like to chime in on naming - I originally wanted to come argumenting for inspect_ok / inspect_some but since writing it down it is less clear now. Anyway, here are my arguments either way

1. inspect_ok/some it makes it explicit what I am inspecting. Explicit is always good in my opinion (downside is slightly longer code)
2. it makes it way easier to distinguish "inspect" calls on different types - this might be good or bad depending on whether one wants more clarity (inspect_ok vs inspect_some) vs ease of refactoring (e.g. when changing Option to Result or vice versa). On the other hand, we already have a precedent with .map() working on both types
3. most notably, I would like to have some general inspect / map call on every type in the future. Right now Rust is clunky because while many times you can use functional approach you also frequently need to drop down to more imperative style just to e.g. log something.
   In an ideal case, I would like to see something like

let x: i32 = some_computation().inspect(|x| info!("My result is {}", x))

as opposed to

let x: i32 = some_computation();
info!("my result is {}", x); 

The second version is harder to read because on the first glance it does not "tie" computation with logging and you need to go through the whole line looking for "x" to correlate it.
Similarly, using map for chaining operations would be very helpful (although tricky to add at this point).

While this RFC does not address either general inspect nor general map, I suspect reserving inspect instead of inspect_some/inspect_ok would make any such future efforts harder to push because we would need to either find a new name for such a method or live with a fact that general inspect on Option/Result would be clunky and sometimes inspecting whole value actually makes sense - consider quick debugging like my_result.inspect(|x| dbg!(x)) - in this case I want to see both Ok and Error paths.

[rfcbot]

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.

This will be merged soon.