Proposal: Error conversion in FromIterator for Result

[emlun]

Hi! I recently tried to write some code like this:

struct Foo {}
struct Error1 {}
struct Error2 {}

impl From<Error1> for Error2 {
    fn from(other: Error1) -> Self {
        Error2 {}
    }
}

fn main() {
    let v1: Vec<Result<Foo, Error1>> = vec![];
    let r1: Result<Vec<Foo>, Error2> = v1.into_iter().map(|r| r.map_err(|e| e.into())).collect();

    let v2: Vec<Result<Foo, Error1>> = vec![];
    let r2: Result<Vec<Foo>, Error2> = v2.into_iter().collect();
}

Since the ? operator does error conversion automatically in cases like this when Error2: From<Error1>, I expected that the .collect() method would also figure out that r2 can be computed like r1, but this turned out not to be the case:

error[E0277]: a collection of type `std::result::Result<std::vec::Vec<Foo>, Error2>` cannot be built from an iterator over elements of type `std::result::Result<Foo, Error1>`
   = help: the trait `std::iter::FromIterator<std::result::Result<Foo, Error1>>` is not implemented for `std::result::Result<std::vec::Vec<Foo>, Error2>`

I tried implementing this in the standard libraries, and it turns out it can be done quite easily, although it does break some type inference since the output type becomes more ambiguous. In short, what I did was to change this:

impl<A, E, V: FromIterator<A>> FromIterator<Result<A, E>> for Result<V, E>

to this:

impl<A, E1, E2: From<E1>, V: FromIterator<A>> FromIterator<Result<A, E1>> for Result<V, E2>

Has this been considered before? Does it seem like a good idea?

[dhm]

I'll revive this thread since their author has been asking about it on Discord, and I guess the thread just went unnoticed.

The main "issue" with your otherwise interesting proposal is:

@emlun

it does break some type inference since the output type becomes more ambiguous

Going forward with something like this is then decided on a case-by-case basis:

1. a crater run seems essential to estimate the amount of breakage in the ecosystem;

2. if there isn't much, then this could be added, since, AFAIK, type inference breakage does not violate semver guarantees.

[emlun]

Thanks for the bump, I appreciate it!

Yeah, there definitely is a case for not doing this. I won't mind if the verdict ends up as not worth it, but I wanted it to at least be considered since it seems like a useful thing.

I'll see if I can do some local crater runs, but for starters: here is the change I made, along with fixes to 7 broken inferences in the standard libraries. Please let me know if that already seems like too much breakage.

Add error conversion to .collect()

[dhm]

Actually, now that I am looking at the changes required in code using that impl, it does feel a bit disappointing that now we'd have to "redundantly" (for those used to the current way of things) specify the error type.

Since most usages of .collect::<Result<_, _>>() seem to be followed by the ? operator, I have rewritten your example into that style:

    let classic: Vec<Result<Foo, Error2>> = vec![];
    classic
        .into_iter()
        .collect::<Result<Vec<Foo>, _>>()? // usage site can lead to `Vec<Foo>` being inferred
    ;

    let v1: Vec<Result<Foo, Error1>> = vec![];
    v1  .into_iter()
        .map(|r| r.map_err(|e| e.into()))
        .collect::<Result<Vec<Foo>, Error2>>()? // usage site can lead to `Vec<Foo>` being inferred
    ;

    let v2: Vec<Result<Foo, Error1>> = vec![];
    v2  .into_iter()
        .collect::<Result<Vec<Foo>, Error1>>()? // usage site can lead to `Vec<Foo>` being inferred
    ;

So classic is the current way of writing things, when the inner error type and the outer one match: we then do not need to specify the error type in the turbofish annotation.

v1 is you use case example, which currently seems to require an additional "mapmap" (.map(... .map_err...)) line, and a type annotation. If you change was accepted, then the "mapmap" line would not be necessary, but the type annotation would, and this requirement would "infect" the previous case.

However, if we look at the current state of things, we can alleviate the cumbersome "mapmap" by collecting into a Result<_, InnerError> first, and only then doing a .map_err. This is specially handy thanks to the ? operator performing an implicit .map_err(<Into<Error2>>::into) conversion, which leads to having no visible map_err line, only the added type annotation that would be required in all situations, which we can see with v2.

• this proves that in the case where .collect() is followed by ?, the current situation is strictly better than the one you suggest;

So I don't personally think that sometimes saving a "mapmap" line is worth the tradeoff of always having to explicit Err type annotations

[emlun]

Those are very good points! Sorry for the slow response, I wanted to try this out against my implementation before responding. I've done some cursory experiments but haven't had time to check all the things I want to quite yet. What I can say, though, is that your point about achieving the same thing by just sticking a ? after .collect() is spot on, I can't believe I didn't think of that. That alone makes my idea a lot less useful.