Add Option::inspect_none & minor example improvements to other new inspect methods

[cyqsimon]

See #91345

// core::option

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

And some minor changes to the examples of other new inspect methods to (hopefully) improve clarity.

[fee1-dead]

r? rust-lang/libs

[bors]

☔ The latest upstream changes (presumably #94824) made this pull request unmergeable. Please resolve the merge conflicts.

[dtolnay]

I have never been thrilled about any of the inspect methods, but this one seems the least appealing to me so far. I would prefer not to build this into the standard library.

I'll ping @rust-lang/libs-api in case someone else sees this being valuable.

[yaahc]

I have never been thrilled about any of the inspect methods, but this one seems the least appealing to me so far. I would prefer not to build this into the standard library.

I'll ping @rust-lang/libs-api in case someone else sees this being valuable.

I pretty much completely agree with you. I'm not against inspect methods as a concept but I don't think warning on None as described in #91345 (comment) is likely to be a common enough usecase to justify this. I'd be a lot more interested in a general solution like https://docs.rs/tap/latest/tap/ if anything, which lets you solve this like:

my_long_method_chain
   // ... many methods later
   .tap(|my_option| if my_option.is_none() { print_my_warning() })
  // continuing my method chain...

Even then, I've rarely if ever had a need for this crate.

[cyqsimon]

The particular reason I believe something like this is useful is cases like this:

    let arr2d = vec![vec![0, 1, 2], vec![3, 4, 5], vec![6, 7, 8]];
    let (x, y) = (1, 5);
    let el = arr2d
        .get(y)
        .inspect_none(|| warn!("y out of bound"))
        .and_then(|row| row.get(x).inspect_none(|| warn!("x out of bound")));

That being said, I am open to the idea of a better name than inspect_none. The advantage is that it keeps consistency with inspect and inspect_err in #91345; the problem is that it inspect_none really isn't "inspecting" anything, so it might confuse people.

[yaahc]

@cyqsimon think this is consistent with my expectation in my previous comment. What isn't clear to me is what advantage you get from inspect_none other than saving a few characters vs the tap alternative. Here's a side-by-side comparison with your example rewritten using the tap API, though with the tap fn renamed to inspect to make it read more consistently.

    // inspect_none(|| ... )
    let arr2d = vec![vec![0, 1, 2], vec![3, 4, 5], vec![6, 7, 8]];
    let (x, y) = (1, 5);
    let el = arr2d
        .get(y)
        .inspect_none(|| warn!("y out of bound"))
        .and_then(|row| row.get(x).inspect_none(|| warn!("x out of bound")));

    // inspect(|opt| if opt.is_none() { ... })
    let arr2d = vec![vec![0, 1, 2], vec![3, 4, 5], vec![6, 7, 8]];
    let (x, y) = (1, 5);
    let el = arr2d
        .get(y)
        .inspect(|opt| if opt.is_none() { warn!("y out of bound") })
        .and_then(|row| row.get(x).inspect(|opt| if opt.is_none() { warn!("x out of bound") }));

I feel like the more explicit API if anything directly resolves the confusion you pointed out with inspect_none not actually inspecting anything. If anything this APIs relationship with those APIs is pushing me towards wanting to not stabilize / remove those APIs and instead replace them with a general API based on tap.

[teor2345]

@cyqsimon think this is consistent with my expectation in my previous comment. What isn't clear to me is what advantage you get from inspect_none other than saving a few characters vs the tap alternative.

...

I feel like the more explicit API if anything directly resolves the confusion you pointed out with inspect_none not actually inspecting anything. If anything this APIs relationship with those APIs is pushing me towards wanting to not stabilize / remove those APIs and instead replace them with a general API based on tap.

I only use inspect and friends occasionally, so I'm not sure if we need a method for each different enum variant.

I would prefer a general solution like tap.

If tap is too verbose, a specific wrapper function can make the check both shorter and more explicit:

fn warn_y_out_of_bounds<T>(opt: Option<T>) {
    if opt.is_none() {
        warn!("y out of bound") 
    }
}

// inspect(|opt| if opt.is_none() { ... })
    let arr2d = vec![vec![0, 1, 2], vec![3, 4, 5], vec![6, 7, 8]];
    let (x, y) = (1, 5);
    let el = arr2d
        .get(y)
        .inspect(warn_y_out_of_bounds)
        .and_then(|row| row.get(x).inspect(|opt| if opt.is_none() { warn!("x out of bound") }));

Edit: I didn't notice the x check, but some sort of generic wrapper or macro would also work there.

[yaahc]

related to this I did some more digging into the tap crate and talked a bit with the author and realized that they do have some tap traits for Result and Option that they had intended to switch over to the Try trait once that's stable. I went ahead and trialed¹ that conversion to see how it would work out and really like the API. If we were to try to do some unified inspect API that is specific to try types this is much closer to what I think it should look like.

Applying that API to your example gives:

    // inspect_break(|None| ...)
    let arr2d = vec![vec![0, 1, 2], vec![3, 4, 5], vec![6, 7, 8]];
    let (x, y) = (1, 5);
    let el = arr2d
        .get(y)
        .inspect_break(|None| warn!("y out of bound") })
        .and_then(|row| row.get(x).inspect_break(|None| warn!("x out of bound")));

I hope you find this as aesthetically pleasing as I do.

Footnotes

1. https://github.com/myrrlyn/tap/pull/8 ↩

[dtolnay]

I'll close the PR because I would prefer not to build this API into the standard library as I wrote above, and there hasn't been a compelling argument in favor since then.

Thanks anyway for the PR!