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
• [x] Final comment period (FCP)
• [x] 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.

[DesmondWillowbrook]

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

[DesmondWillowbrook]

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.

[real-felix]

Any news? It's extremely common to log an error before using the try operator, something like:

let foo = fallible_call().inspect_err(|error| eprintln!("Oops: {error}"))?;

Instead of the verbose current equivalent:

let foo = match fallible_call() {
    Ok(foo) => foo,
    Err(error) => {
        eprintln!("Oops: {error}");
        return Err(error.into())
    }
}

[pravic]

@Boiethios that can be shorten a bit, but your point is valid nonetheless:

let foo = fallible_call().map_err(|e| { 
  log::error!("{e}");
  e
})?;

[dbofmmbt]

For those who are following this closely, there's been some discussion in a recent Libs-API Meeting:

https://hackmd.io/RiZHW71SRmy_fGfwGP2PyA#nominated-rusttf91345-Tracking-Issue-for-result_option_inspect

I'd like to share my experience with this issue.

I discovered inspect while I was trying to perform some side effects in the middle of a chain. I needed to inspect the intermediary result to perform some computation. I use stable Rust on VS Code + Rust-Analyzer, and I got some completions from Rust-Analyzer for inspect, which did exactly what I needed: It gives me a way to perform a side effect without changing the processing chain. I was a bit sad to see it be unstable, as I 'stumbled' on it out of need.

Also, I think inspect idioms would be easier to read. When I read .map, I don't usually expect side effects happening. I usually expect a type being transformed into another. It's a little surprising to see a .map being used but not performing a mapping at all:

items.iter().map(|item| {
          do_some_side_effect_with(item);
          item;
})

If this issue ends up not being accepted, then we could have some "go-to" crate to provide this extension and see if more people see the need for this idiom over time. With this issue open without definition, people (including myself) may wait for a decision before going to a third-party solution.

[interruptinuse]

If this issue ends up not being accepted, then we could have some "go-to" crate to provide this extension and see if more people see the need for this idiom over time

@dbofmmbt For this specific purpose, I found the already-mentioned tap crate very useful, more specifically, TapFallible's tap_ok and tap_err.

[m-ou-se]

We discussed this in a recent libs-api meeting, but most of us had trouble forming strong opinions (in favor or against). An FCP would probably not get enough support at this point.

In general, we tend to be quite careful with adding more methods to types like Option and Result, since they form part of the 'vocabulary' one needs to know to write and read Rust code. There is value in keeping that vocabulary small, of course, but it's always a trade-off.

---

Personally, I wonder if .inspect() is worth it as an additional way to something similar to what if let (or let else) already does. I think it could perhaps be cool if basically everything in Rust could be written as part of a chain, e.g. .dbg!() postfix macro, or postfix .match, etc, but without such (language) features, I'm not sure if .inspect() significantly changes how much can be written as a chain, but I'm happy to be convinced otherwise.

Tangentally, I think it's a shame that a simple type such as Option has so many methods. It means that if I make my own enum I need to consider adding all those methods to make it as ergonomic as Option or Result, adding a higher bar for using custom types. Ideally, there would be some kind of language feature that would cover all common enum functionality like map, inspect, filter, and_then and so on with some short and easy syntax, but I can't think of anything that would work well.

I am afraid that .inspect() and especially .inspect_err() will often lead to patterns where you'd want the ? inside the closure to apply to the surrounding function, which it doesn't. A language feature that would not be based on closures wouldn't have this problem. (Just like if let Some(_) = _ { _ }, although that feature is more (too?) verbose of course.)

Anyway, I'm a bit on the edge on whether we should stabilize this or not, but I could be convinced with some more (real world!) examples, such as places where similar methods from community crates are already used, or places where .inspect() could make code easier to read. I think the same holds for other members of the team. :)

[jplatte]

A problem that has come up a few times for me that I think is closely related is logging an error and returning with a default return value that is no Result::Err – common values being false, None and (). Currently I would say the most convenient way of handling these is let-else, but that throws away the error message:

let Ok(value) = some_fallible_thing() else {
    error!("something went wrong.");
    return None;
};

If the default return value is None like in the example above, a good alternative that does include the error message in the logs would be

let value = some_fallible_thing()
    .inspect_err(|e| error!("something went wrong: {e}"))
    .ok()?;

However, as it's only a partial solution (doesn't work for other default return values) I agree that it doesn't feel great. I also agree that postfix macros might be the answer here. Specifically, unwrap_or_else! would allow:

let value = some_fallible_thing().unwrap_or_else!(e => {
    error!("something went wrong: {e}");
    return None;
});

which seems much better than my current go-to of

let value = match some_fallible_thing() {
    Ok(value) => value,
    Err(e) => {
        error!("something went wrong: {e}");
        return None;
    }
};

(an if let where the success branch just evals to a binding from the pattern is practically equivalent in terms of redundancy / noise)

[meowjesty]

We're using inspect_err to log stuff in quite a few places in mirrord. It feels more right than map_err, and it's more convenient than let else, as we're chaining stuff. Couple of examples:

https://github.com/metalbear-co/mirrord/blob/669a92422d59406cbc9726ad494dbbaf6142dd39/mirrord/auth/src/credential_store.rs#L120-L123

https://github.com/metalbear-co/mirrord/blob/669a92422d59406cbc9726ad494dbbaf6142dd39/mirrord/layer/src/debugger_ports.rs#L188C8-L200

[aslilac]

I can understand if it's not the most compelling use-case, but like @meowjesty, logging stuff is definitely the main reason I want this.

[m-ou-se]

In both of those examples, map_err would have worked just fine, right? In both cases, the error value is not used afterwards.

I'm not saying that I think map_err is the perfect method for those use cases, but I find it interesting to see that while inspect_err is designed to leave the error value in tact, that value is immediately thrown away afterwards.

[aslilac]

that value is immediately thrown away afterwards

nah, I wouldn't use inspect_err for this at the end of a method chain, I'd just dbg!(x) if it's being put in a variable or wrap the whole thing in a dbg! if it's a return position expression or something

I think the discussion on this issue has ended up asking for something different than what I actually wanted when I decided to subscribe to the discussion. I wouldn't want an inspect method that gets &T and an inspect_err method that gets &E. I wanted an inspect method that would get Result<&T, &E>.

Here's really what I had in mind:

• I can dbg! in here and get the full picture
• I could potentially do something side-effecting based on an intermediate value without "mapping" a value and needing to return the correct thing
• if I want a (potentially different) side effect regardless of whether the value is Ok or Err, I can do that in one place to make it clear that they are linked

this is a slightly half hearted defence, because obviously we've all been getting along just fine without any of this. it's probably better to just split the method chain by storing the first half in a variable and making a non-owning copy with my_result.as_ref(), do your side effects, and then resume the method chain on the owned value.

and an intermediate dbg! could just look like...

- a
+ dbg!(a
    .and_then(...)
-   .flatten()
+   .flatten())
    .whatever()

Also this kind of diverges from what inspect usually means (for an iterator) and what flatten means for a result.

Yeah ok, this has turned into a very long winded "I don't actually want this anymore" 😅

[WaffleLapkin]

Adding my example of code where I wanted to use Result::inspect (it's a bit cursed to I'm not sure it's a good example but still 😅):

       self.inner
            .compare_exchange(current as _, new as _, Ordering::Relaxed, Ordering::Relaxed)
            .map(ShutdownState::from_u8)
            .map_err(ShutdownState::from_u8)
            .map(|st| {
                self.notify.notify_waiters();
                st
            })
            // That would be
            // .inspect(|_| self.notify.notify_waiters())

[WaffleLapkin]

But speaking of more powerful alternatives, I quite like the idea of an extension that allows to inspect any value:

trait Also: Sized {
    fn also(mut self, f: impl FnOnce(&mut Self)) -> Self { f(&mut self); self }
    // `f` could also return a value that is always dropped,
    // but this may be error prone...
}

impl<T> Also for T {}

With that you can easily simulate Result::inspect (play):

.inspect(|ok| eprintln!("{ok}"))
// becomes
.also(|res| if let Ok(ok) = res { eprintln!("{ok}") })

But you could also chain anything, really. It's really nice for chaining &mut self -> () methods for example (play):

let res = Vec::new()
    .also(|v| v.extend(values_a))
    .also(|v| v.extend(values_b))
    .also(|v| v.sort())
    .also(|v| v.extend(values_sorted_tail))
    .also(|v| debug_assert!(v.is_sorted()))
    .also(|v| tracing::trace!(?v));

Upd: did not notice that the OP mentioned Tap, which is basically the same thing. Oops 😅

[meowjesty]

In both of those examples, map_err would have worked just fine, right? In both cases, the error value is not used afterwards.

Wow, I managed to pick up 2 examples where we throw out the error, but that's not always the case, my bad.

Here we don't discard it, and this should give a feel for how often we reach for it.

btw: I'm not strongly in favor of the feature, as I agree with your notion of "too much stuff in Result/Option" (especially since we implemented our own Result-ish thing). IMO, this is a nice to have mostly for looks (as it's implemented, not talking about the tap discussion), where map_err looks a bit less fancy.

[sassman]

On argument regarding the vocabulary that one need to learn to write rust code.

I found it myself counter intuitive that iterator chains support .inspect but result and option not.

For me it would feel more consistent to find the same or similar idioms all around the std lib. Which is of course often the case, here in particular not.

[CBenoit]

Anyway, I'm a bit on the edge on whether we should stabilize this or not, but I could be convinced with some more (real world!) examples, such as places where similar methods from community crates are already used, or places where .inspect() could make code easier to read. I think the same holds for other members of the team. :)

We're using inspect_err to log stuff in quite a few places in mirrord. It feels more right than map_err, and it's more convenient than let else, as we're chaining stuff.

I can understand if it's not the most compelling use-case, but like @meowjesty, logging stuff is definitely the main reason I want this.

Sadly, I can’t put a link to the actual code because the repository is not public, but we are also using it at work for simple data inspection and logging. Here is a simplified excerpt that I’m allowed to share (using the tap crate):

fn step(input: &[u8], output: &mut Vec<u8>) -> Result<usize, SomeError> {
    /* State machine  */

    // Handling the handshake result
    let args = match HandshakeResult::decode(input)?.tap(|message| debug!(?message, "Received")) {
        HandshakeResult::Success { /* … */ } => /* … */,
        HandshakeResult::Failure { /* … */ } => /* … */,
    };
    StartActiveStageRequest::from(args).tap(|message| debug!(?message, "Send")).encode_buf(output)
}

Without tap, I would write the following:

fn step(input: &[u8], output: &mut Vec<u8>) -> Result<usize, SomeError> {
    /* State machine  */

    // Handling the handshake result

    let handshake_result = HandshakeResult::decode(input)?;
    debug!(message = ?handshake_result, "Received"); // We record fields named `message` using tracing
    let some_args = match handshake_result {
        HandshakeResult::Success { /* … */ } => /* … */,
        HandshakeResult::Failure { /* … */ } => /* … */,
    };

    let start_active_stage_request = StartActiveStageRequest::from(some_args);
    debug!(message = ?start_active_stage_request, "Send");
    start_active_stage_request.encode_buf(output)
}

I don’t feel the latter is particularly more readable than the former.

Note that:

• This is somewhat repetitive, boring code and a shorter way to write it was much welcomed.
• We didn’t add tap to all our crates and I often end up writing code without "taping", because I wanted to stay focused on my task without breaking the flow. That’s because tap, while being a nice quality of life tool, is not absolutely essential to tackle the task.

Note also that instead of short-lived bindings all named message, I gave full explicit names in the latter because I don’t want the reader to confuse the binding for the handshake result with the binding for the start active stage request. Indeed, it’s arguably easier to make a typo and log the wrong value:

let message = HandshakeResult::decode(input)?;
debug!(?message, "Received");
let some_args = match message {
    HandshakeResult::Success { /* … */ } => /* … */,
    HandshakeResult::Failure { /* … */ } => /* … */,
};

let messge = StartActiveStageRequest::from(some_args); // Oops, previous binding is not shadowed
debug!(?message, "Send");
messge.encode_buf(output)

If not for that, I don’t feel like there was a need to have a long explicit name for the bindings given that the value itself is short-lived.

An alternative would be to introduce shorter scopes:

let some_args = {
    let message = HandshakeResult::decode(input)?;
    debug!(?message, "Received");
    match message {
        HandshakeResult::Success { /* … */ } => /* … */,
        HandshakeResult::Failure { /* … */ } => /* … */,
    }
};

{
    let messge = StartActiveStageRequest::from(some_args);
    debug!(?message, "Send"); // ERROR
    messge.encode_buf(output)
}

I don’t worry about all that when "tapping" because, precisely, the bindings are clearly short-lived and no shadowing is happening.

EDIT: The above was assuming some kind of tap-like API which result_option_inspect is not exactly covering. Only half of the "problem" is addressed by result_option_inspect:

fn step(input: &[u8], output: &mut Vec<u8>) -> Result<usize, SomeError> {
    /* State machine  */

    // Using the `inspect` API
    let args = match HandshakeResult::decode(input).inspect(|message| debug!(?message, "Received"))? {
        HandshakeResult::Success { /* … */ } => /* … */,
        HandshakeResult::Failure { /* … */ } => /* … */,
    };

    // The `inspect` API is only for `Option` and `Result`, so  this would not be addressed yet:
    let start_active_stage_request = StartActiveStageRequest::from(some_args);
    debug!(message = ?start_active_stage_request, "Send");
    start_active_stage_request.encode_buf(output)
}

As mentioned by others, the next logical step would be a Tap trait, but I believe this still demonstrates how "similar methods" are actually used in practice

[sassman]

One more thing regarding

Should there be a more general Tap trait in std?

The std lib has a lot of places where into_inner() is used. That however is also not standardized via a common trait. Hence I wonder if a generalized trait like Tap really must be coupled to the stabilization of this very feature.

IMHO a trait for a common seen behavior could also be introduced late. Later than the here discussed feature is implemented.

[CBenoit]

I wonder if a generalized trait like Tap really must be coupled to the stabilization of this very feature.

Indeed, my understanding is that a separate RFC is required for that.

The Tap trait is not even generalizing the inspect / inspect_err methods. The "tapping" methods are simply giving a view to the value as-is while inspect / inspect_err is performing pattern matching, so that’s really different.

[Amanieu]

We discussed this in the libs-api meeting today, and the consensus was that we're mostly happy to add this. However we would like to change the signature to give a &mut T to the closure instead of a &T.

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

• [x] @Amanieu
• [ ] @BurntSushi
• [x] @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.

[fintelia]

No strong opinions either way, but to clarify, taking an &mut T is an intentional inconsistency with Iterator::inspect whose closure takes an &Self::Item?