Allow ?-converting from Result<T, E> in functions returning Option<Result<T, E>>

[GoldsteinE]

This PR adds implementation FromResidual<Result<Infallible, E>> for Option<Result<T, E>>, allowing to use ? operator on Result<_, E> in functions returning Option<Result<T, E>>.

Interestingly, this is already the case for Poll<Option<Result<T, E>>>, so this implementation seems to be accidentally missing and not deliberately left out.

This is particularly useful for Iterator implementations where Item = Result<T, E>.

This implementation is const when underlying From<_> impl is const. This is not the case for Poll<_> implementations and I’m not sure why. Should I add missing const to Poll<_> impls or there’s a reason for that?

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @scottmcm (or someone else) soon.

Please see the contribution instructions for more information.

[rustbot]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

[GoldsteinE]

I’m not sure if this needs an API change proposal, but I’m happy to create one if needed.

[GoldsteinE]

@rustbot label +T-libs-api -T-libs

[joshtriplett]

The Poll implementations of ? aren't something to take inspiration from; we've had a fair bit of trouble with those, with the benefit of hindsight. cc @scottmcm

Nonetheless, this does seem potentially useful.

The main downside I could imagine would be a context in which some errors need to get processed in a different way and then turned into None. (For instance, failures that will prevent iterating at all; if those get turned into Some(Err(...)) then that could result in an infinite loop.)

[GoldsteinE]

The Poll implementations of ? aren't something to take inspiration from; we've had a fair bit of trouble with those, with the benefit of hindsight. cc @scottmcm

I think that even if Poll<_> implementations were a mistake, it’s surprising to have implementation for Poll<Option<Result<_, _>>> but not for Option<Result<_, _>>.

The main downside I could imagine would be a context in which some errors need to get processed in a different way and then turned into None. (For instance, failures that will prevent iterating at all; if those get turned into Some(Err(...)) then that could result in an infinite loop.)

I thought about it. I think this conversion as implemented follows the general pattern of “implicit conversions do not lose data”. It’s also very simple to go from Result<_, _> to Option<_> with .ok(), but returning an error requires something cumbersome like

match res {
    Ok(val) => val,
    Err(err) => return Some(Err(err)),
}

and can’t be abstracted into a method.

[scottmcm]

Convenient that I got randomly picked for this review 🙃

I was going to say the same thing that Josh did -- that I don't think experience has been kind to these complex implementations on Poll. (cc @yaahc, who might have more to say on this.) Note also that trait implementations are insta-stable, so we can't really experiment with them. That generally means that they can't be accepted without a "they seem pretty obviously what we want" sentiment. (Things like new inherent methods, where #[unstable] works, are much easier to accept provisionally and see how they go.)

I think the core problem is that the nestings don't permute well. ? on an Option<Result<T, E>> in a function returning Result<T, E> isn't going to ever work without violating the "the value produced by ? doesn't depend on the context" rule that rust-lang/rfcs#1859 decided to have which, AFAIK, is still popular. So if the other way around doesn't work, it's not clear to me that this way 'round should be supported.

---

This is particularly useful for Iterator implementations where Item = Result<T, E>.

Hmm, interesting. (And coincidentally reminds me of https://swatinem.de/blog/fallible-iterators/ from this week's TWiR).

Can you show some examples of the code you're thinking of using this? I'd like to experiment with other possibilities, like whether try{} blocks would help. (If not, that'd be particularly good information for the error handling story.)

[GoldsteinE]

I don’t have public examples off the top of my head, but quick rg in rust-lang/rust shows a couple of places where this could be used: std::io::Lines and std::io::Split. They are not very interesting though, since it’s only a single usage.

GitHub search returns some more results:

• https://github.com/nix-rust/nix/blob/5f859d10dfd21b99405054d80392649adc392fd0/src/dir.rs#L97
• https://github.com/m-ou-se/gpio-cdev/blob/bd58cf1a8f1438c0597de23b1b5823342dc48412/src/lib.rs#L179 and also lower on line 1017
• https://github.com/tmerr/i3ipc-rs/blob/0a06e74d7a4ab448b081ce897e15c0cb273ba56d/src/lib.rs#L233 — this one could be simplified significantly (though it’d require adding From<_> impls for the error type).

So if the other way around doesn't work, it's not clear to me that this way 'round should be supported.

I’d like to note that ? is already asymmetric, since it relies on From<_> to convert errors.

[c410-f3r]

Such implementation is extremely useful when you are dealing with fallible iterators that return both Errs and Oks.

use core::num::TryFromIntError;
let array: [Result<i16, TryFromIntError>; 4] = [Ok(1), Ok(-256), Ok(3), Ok(4)];
let mut iter = array.into_iter().filter_map(|rslt| {
    let elem = match rslt {
        Err(err) => return Some(Err(err)),
        Ok(elem) => elem
    };
    let _u8 = match u8::try_from(elem) {
        Err(err) => return Some(Err(err)),
        Ok(elem) => elem
    };
    (_u8 % 2 == 0).then_some(Ok::<_, TryFromIntError>(_u8))
});
assert!(matches!(iter.next().unwrap(), Err(_)));
assert!(matches!(iter.next().unwrap(), Ok(4)));

Just an example but there can be other combinators that involve Option. As seen above, it is very cumbersome to always manually match in every Result.

use core::num::TryFromIntError;
let array: [Result<i16, TryFromIntError>; 4] = [Ok(1), Ok(-256), Ok(3), Ok(4)];
let mut iter = array.into_iter().filter_map(|rslt| {
    let _u8 = u8::try_from(rslt?)?;
    (_u8 % 2 == 0).then_some(Ok::<_, TryFromIntError>(_u8))
});
assert!(matches!(iter.next().unwrap(), Err(_)));
assert!(matches!(iter.next().unwrap(), Ok(4)));

Taking aside macros, I am not aware of any other thing that improves the first snippet, therefore, this PR seems worthwhile.

[Numenorean]

I have recenlty faced with the same issue while it's more convenient to use built-in implementation instead of writing the same code each time. Having such need as writing plenty of iterators (in my case it's scrapers) I obviously want to use fallible iterators because there is no other way to throw errors away. Sorry for not reproducible examples, it's just from the my current project

let mut owners = asset.into_owners_crawler(self.graphql_client).into_iter();
let owner = match owners.next() {
    Some(Ok(owner)) => match self.database.owner_exists_or_add(&owner, &asset_id) {
        Ok(false) => owner,
        Ok(true) => return Some(Err(ScrapeError::OwnerRepeat)),
        Err(err) => return Some(Err(err.into())),
    },
    Some(Err(error)) => return Some(Err(error)),
    None => unreachable!("owners seems to be empty, should not happened: {asset_id}"),
};

And after implementing Try on Result<_, _> in functions returning Option<Result<_, _>>

let mut owners = asset.into_owners_crawler(self.graphql_client).into_iter();
let owner = match owners.next().transpose()? {
    None => unreachable!("owners seems to be empty, should not happened: {asset_id}"),
    Some(owner) => {
        if self.database.owner_exists_or_add(&owner, &asset_id)? {
            return Some(Err(ScrapeError::OwnerRepeat));
        }
        owner
    }
};

Modified version seems to be more readable and convinient to work with. Moreover after modifing owner_exists_or_add so it returns just a result we have got even shorter logic

let mut owners = asset.into_owners_crawler(self.graphql_client).into_iter();
let owner = match owners.next().transpose()? {
    None => unreachable!("owners seems to be empty, should not happened: {asset_id}"),
    Some(owner) => {
        self.database.add_owner(&owner, &asset_id)?;
        owner
    }
};

At this stage we have got rid of all unwanted pattern matching. If an our codebase built on top of the iterators, we write lots of unnecessary code, though, in a way, it could be solved with macros

[m-ou-se]

The Poll implementations of ? aren't something to take inspiration from; we've had a fair bit of trouble with those, with the benefit of hindsight.

The implementation proposed in this PR is less problematic though. The issue with the controversial Poll implementations was that it's not obvious whether a ? applied to a Poll<Result> expression applies to the Poll or the Result. The implementation in this PR is just about applying ? to a Result, so there's no ambiguity. The Option<Result> is the return type of the function, not of the expression that the ? gets applied to.

[m-ou-se]

I am wondering if this implementation would be too specific, and if it'd make sense to try to find a way to be more generic. Probably not, because a more generic version would also apply to Option<Option>, which conflicts with the existing implementation.

[yaahc]

I think that even if Poll<_> implementations were a mistake, it’s surprising to have implementation for Poll<Option<Result<_, _>>> but not for Option<Result<_, _>>.

An unfortunate price to pay for stability. We already have inconsistencies like this such as how Box<dyn Error> does not impl Error but Arc<dyn Error> does. We tend to prefer inconsistencies over repeating mistakes. That said, I'm not positive the Poll impls were a mistake, I know some people swear by them, and I'm not in the habit of writing manual impls of Future so I haven't had much occasion to use them.

---

Replying to both @scottmcm and @c410-f3r, here's a version of the example @c410-f3r gave using try blocks like @scottmcm mentioned:

use core::num::TryFromIntError;
let array: [Result<i16, TryFromIntError>; 4] = [Ok(1), Ok(-256), Ok(3), Ok(4)];
let mut iter = array
    .into_iter()
    .filter_map(|rslt| -> Option<Result<_, TryFromIntError>> {
        try {
            try {
                let elem = rslt?;
                let _u8 = u8::try_from(elem)?;
                if _u8 % 2 != 0 {
                    return None;
                }
                _u8
            }
        }
    });
assert!(matches!(iter.next().unwrap(), Err(_)));
assert!(matches!(iter.next().unwrap(), Ok(4)));

vs

use core::num::TryFromIntError;
let array: [Result<i16, TryFromIntError>; 4] = [Ok(1), Ok(-256), Ok(3), Ok(4)];
let mut iter = array.into_iter().filter_map(|rslt| {
    let _u8 = u8::try_from(rslt?)?;
    (_u8 % 2 == 0).then_some(Ok::<_, TryFromIntError>(_u8))
});
assert!(matches!(iter.next().unwrap(), Err(_)));
assert!(matches!(iter.next().unwrap(), Ok(4)));

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=54f9c428da373dcc993adde3c92aef5c

---

And to be clear, I'm not opposed, just hesitant. I think Mara's reasoning here is sound but I do want to be particularly cautious before introducing new Try impls.

[joshtriplett]

We discussed this in today's @rust-lang/libs-api meeting. Based on @yaahc's suggested use of try, we generally felt that this wouldn't add as much value as it would add the chance for confusion. Nonetheless, we're leaving this open for further discussion to see if there are further use cases that try wouldn't help with.

[scottmcm]

Hmm, the try { try { doesn't fill me with joy.

Another version of this would use transpose:

use core::num::TryFromIntError;
let array: [Result<i16, TryFromIntError>; 4] = [Ok(1), Ok(-256), Ok(3), Ok(4)];
let mut iter = array
    .into_iter()
    .filter_map(|rslt| -> Option<Result<_, TryFromIntError>> {
        let x: Result<_, _> = try {
            let u8 = u8::try_from(rslt?)?;
            (u8 % 2 == 0).then_some(u8)
        };
        x.transpose()
    });
assert!(matches!(iter.next().unwrap(), Err(_)));
assert!(matches!(iter.next().unwrap(), Ok(4)));

And, helpfully, if we get #98417 then I think that could simplify down to

let mut iter = array
    .into_iter()
    .filter_map(|rslt| {
        try {
            let u8 = u8::try_from(rslt?)?;
            (u8 % 2 == 0).then_some(u8)
        }.transpose()
    });

which might be sufficient.

(The important thing there is that try { … }.transpose() will never work on current nightly, since it blocks the type information, but that alternative try form can solve that.)

[yaahc]

Independent of the try blocks issue, I believe we should do a crater run if we do decide to proceed with this. My primary concerns at the moment are:

• breaking existing inference
• compiler diagnostic regressions
• locking us out of future try impls we may prefer
• allowing code that previously didn't compile to compile in unexpected ways.

[yaahc]

• the one-line idiom of “bind success value to local name, propagate error outwards” using ? has been very successful in making error-aware code readable

My problem is that I don't think this is what ? really means. It is not exclusive to propagating errors, it's more general than that, and it's about propagating break conditions. With this impl I worry that we run into the problem where ? in a function that returns an option will sometimes return Some and will sometimes return None, and you have to know the type you're calling ? on to know whether or not you're propagating a break or a continue(break). I worry that this could end in surprising refactor breakages where a function signature is changed and suddenly an existing ? starts propagating the opposite state of what it used to propagate in an unintended way¹. This is similar to my complaint in the poll.ready()? tracking issue which makes it so that ? sometimes propagates Pending in contradiction of the existing stable behavior which always propagates Ready².

the above suggestions of using try blocks do not achieve this for the specific use-case of implementing or working with a fallible iterator — the point is to get rid of the visual clutter and focus on the logic (indentation depth of nested try also is a problem)

I also don't agree with this point at all, and I feel that it's purely subjective, and likely based in a lack of familiarity because try blocks are still unstable.

Footnotes

1. I want to noodle on this one some more and see if I can't come up with a realistic example for how this could cause problems ↩

2. https://github.com/rust-lang/rust/issues/89780#issuecomment-1075570747 ↩

[yaahc]

I don't want to reject this outright, but I do think we should hold off on possibly adding this until after try blocks have been stabilized for a while. Then we will be in a much better position to judge how necessary this impl is.

[rkuhn]

Thanks for those concerns, if there is indeed a silent refactoring failure hidden inside this addition then I agree we shouldn’t do it.

As far as I see it, and assuming no change to the function body:

• if we start with fn(...)->Result<T, E> then all ? will now propagate Some(Err(T))
• if we start with fn(...)->Option<Result<T, E>> then ? on Result will still do what it did before and on Option it will fail to compile

Only the first case is a potential problem. I’d argue that due to Option<Result<T, E>> being isomorphic to Result<Option<T>, E> the behaviour is not actually changed. The desired effect of the refactoring may well be that some errors should be turned into None instead, leaving others alone (otherwise there would be no need for Result anymore). This is a manual process in any case, where each potential break propagation needs to be inspected — it can’t be driven by types since there is only a single E type. The argument against this proposed addition then is that flagging all ? as type errors after the refactoring will ensure that all are inspected by the programmer; after to my own experience I wouldn’t overrate this, though, since adding whatever decorum is chosen (e.g. a macro) is often done with an editor’s bulk change facilities.

I also don't agree with this point at all, and I feel that it's purely subjective, and likely based in a lack of familiarity because try blocks are still unstable.

None of the examples above customarily fit on a single line — default and common formatting rules require three lines and added indentation for a block — which is an objective disadvantage compared to ? syntax. If I’m missing some visual optimisations due to unfamiliarity, would you please add some examples that may reasonably be expected to be stabilised soon?

[camelid]

ping from triage: What's the status of this PR? It sounds like maybe it should be closed for now while try blocks are in the works?

[rkuhn]

As far as I’m concerned no proposal has been put forward that offers to solve the issue using try blocks, so in that sense I don’t agree with the assessment, @camelid.

[GoldsteinE]

@m-ou-se I’m not sure what the status of it is. It feels rejected, but there was no formal rejection from libs team, so I’m kinda confused. What’s the verdict on this?

[yaahc]

@m-ou-se I’m not sure what the status of it is. It feels rejected, but there was no formal rejection from libs team, so I’m kinda confused. What’s the verdict on this?

not currently a libs team member and I'm not recently in the loop on this, but I can clarify the stance I was holding last time I commented on this PR.

Given that this is an irreversible decision I think we need to maximize the amount of information we have before moving forward with this. I raised a blocking objection that is conditional upon try blocks being stabilized. Once we have them and have more information I think we could reconsider this issue and it may be justified, but until that time I think the uncertainty, risk, and potential cost are too high to move forward.

[GoldsteinE]

I still don't see a good way to solve the issue with try blocks. What information would we get from stabilization that we don't have now?

[joshtriplett]

By way of one example, if we had generators and try, it'd be feasible to write yield try { ... expr()? ... } as a construct to return Some(Err(...)). That seems clearer and more explicit than just expr()?.

[joshtriplett]

We discussed this in today's @rust-lang/libs-api meeting.

We were generally in agreement with @scottmcm's comment at #99333 (comment).

We did look at the code samples for code that would be shortened by these ? impls, but felt that even those examples did not seem sufficient to add this additional complexity.

We're hoping that some combination of try and gen and the existing ? will be able to simplify those use cases. But even if there are cases that those do not handle, we still felt that having this ? impl would make subsequent reading of the code less clear.

@rfcbot close

[rfcbot]

Team member @joshtriplett has proposed to close 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. 🔔

[GoldsteinE]

Thanks for clearing up the status on this PR. I hope generators will remove the Iterator<Item = Result<_, _>> usecase sometime in the future. Sadly, .filter_map() usecase is not solved by this, so will continue to require a manual match on every fallible function call (unless maybe some version of “alternative try” is accepted + try { try {?).

[traviscross]

+1 on close and to the concerns that @scottmcm raised here. In particular, we should add this impl only if we're also willing to support "? on an Option<Result<T, E>> in a function returning Result<T, E>".

Perhaps it goes without saying, but when this is needed, note that macros are always an option.¹ E.g., we can of course write:

macro_rules! otry {
    ($e:expr) => { match $e {
        Err(e) => return Some(Err(e)),
        Ok(x) => x,
    }}
}

use core::num::TryFromIntError;
let array: [Result<i16, TryFromIntError>; 4] =
    [Ok(1), Ok(-256), Ok(3), Ok(4)];
let mut iter = array.into_iter().filter_map(|rslt| {
    let _u8 = otry!(u8::try_from(otry!(rslt)));
    (_u8 % 2 == 0).then_some(Ok::<_, TryFromIntError>(_u8))
});
assert!(matches!(iter.next().unwrap(), Err(_)));
assert!(matches!(iter.next().unwrap(), Ok(4)));

Playground link

That has the same shape as the desired code in this comment.

Footnotes

1. This also raises the point that postfix macros would be another path forward to better supporting these use cases. [Edit: This footnote was added concurrently with the comment by @the8472 below.] ↩

[the8472]

Perhaps it goes without saying, but when this is needed, note that macros are always an option.

And with postfix macros it could exist in the same position as ?

[GoldsteinE]

macros are always an option

Yeah, but macros kill autocompletion.

I’m more hopeful about the gen/try approach:

use core::num::TryFromIntError;
let array: [Result<i16, TryFromIntError>; 4] =
    [Ok(1), Ok(-256), Ok(3), Ok(4)];
let mut iter = gen {
    for rslt in array {
        let _u8 = try { u8::try_from(rslt?)? };
        if _u8.is_ok_and(|x| x % 2 == 0) {
            yield _u8;
        }
    }
});
assert!(matches!(iter.next().unwrap(), Err(_)));
assert!(matches!(iter.next().unwrap(), Ok(4)));

In simple cases like this you could just replace .filter_map() with an inline generator. It doesn’t exactly generalize to more complex cases (notably, it won’t work with rayon’s parallel iterators, would require async gen to work with streams and won’t work with lending iterators at all), but it handles the most common case without compromising on IDE experience.

Postfix macros would also solve this problem, because they should work with autocompletion, but I’m not sure that postfix macros will happen anytime soon, and there seems to be some progress on generators.

[rfcbot]

The final comment period, with a disposition to close, 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.