Tracking issue for error source iterators

[sfackler]

View all comments

This covers the <dyn Error>::iter_chain and <dyn Error>::iter_sources methods added in #58289.

[withoutboats]

My preference before stabilizing would be to make changes to make it more like the original failure design, which I still prefer. There should be one iterator, it should begin from this error type, and its name should be sources. If you want all the sources of error less this layer, you can write .skip(1).

[TimDiekmann]

I like the idea of a convenient interface to iterate sources. However, the interface feels unergonomic as mentioned in #58289 (comment). Additionally I think this should not be stabilized before #53487

[haraldh]

My preference before stabilizing would be to make changes to make it more like the original failure design, which I still prefer. There should be one iterator, it should begin from this error type, and its name should be sources. If you want all the sources of error less this layer, you can write .skip(1).

Yeah, my first iteration (pun) had only .iter(), but then @sfackler requested the other iter_*()

[faern]

These iterators are really good. I have long relied on the display_chain method in error-chain. As I move away from that lib I find it cumbersome to transform my entire chain of errors into a good string representation.

With these iterators I can get the equivalent of display_chain this way:

let display_chain: String = error
    .iter_sources()
    .fold(format!("Error: {}", error), |mut s, e| {
        s.push_str(&format!("\nCaused by: {}", e));
        s
    });

These iterators does not solve the entire problem. But they make it a bit easier to implement.

[haraldh]

So, any changes needed here? I really want this to be stabilized

[withoutboats]

I think a few changes are needed before this can be stabilized:

1. First, we need to actually reach a consensus about whether we have both of these APIs. I've been consistent that I think the changes made to failure regarding this issue since I stopped maintaining it are a mistake: we should have one iterator which begins with self and calls cause until it returns None. If users want to skip the self value, its just as simple as adding .skip(1) to that iterator. No one else has really expressed an opinion about that question.
2. Even if we do have both, I think the names right now are not well-chosen. It would be better to change them to something like .errors() and .sources() (or keep one and just call it one of these two names). We tend to avoid the name iter except for iterating through elements of a collection, modified by its ownership type; for iterators which are semantic values pulled out of a type, we prefer just using the name of that kind of value (as in lines, keys, chars, etc).
3. We need to decide what to do about the trait object vs provided method problem, discussed on the PR. This involves the struggle around the fact that Box<Error> doesn't implement Error. I think the best solution available to us now is to just have the method repeated in both places.

[richard-uk1]

my 2¢: I think the simplest (for users) implementation is a single iter method, with a note in the documentation that

1. The iterator length is always >= 1, that is, you can call iter.next().unwrap() once without causing a panic.
2. If you just want the sources, use e.iter().skip(1).
3. An example like

   fn print_error(e: impl Error) { // or Box<Error> or whatever other actual type this is implemented for
     let mut chain = e.iter();
     eprintln!("error: {}", chain.next().unwrap());
     for source in chain {
       eprintln!("caused by {}", source);
     }

This leaves the option open of an iter_sources method later if it's decided that it's really needed (that can just be implemented as iter().skip(1)).

[BurntSushi]

I agree with @withoutboats and @derekdreery. I had to carefully read the docs for each method before I figured out how they differed. Moreover, I suspect it will be quite difficult to pick names for both methods that are easy to remember and distinguish. Having one method and requiring the caller to use skip(1) if they don't want the current error seems much nicer IMO.

[withoutboats]

It does seem that many users found the terminology causes confusing for an iterator that returns self (myself, it seemed intuitive, if the documentation says so, that this error can be considered a member of the chain of causality). So a better name than sources might be preferred here. However, both of the other options I can think of are also imperfect:

• I think iter violates our naming convention, in that I think iter must return the same iterator returned by <&T as IntoIterator>::into_iter, and I don't expect that impl to exist for error types.
• Error::errors does not seem great either, just because it can be confusing to say that this error contains many errors.

As I said previously, I don't think names like iter_chain are a good choice either, they're pretty far outside of our general style for iterator names.

[faern]

I agree causes/sources would be confusing if the iterator contains the error it's called on. In err.causes() I want the errors that caused err. err did not cause itself, nor is it part of the sources for this error. It would not be consistent with the existing err.source() which returns the first error under err not err itself.

I do fully agree that err is part of the chain of causality however, which is why I think something like chain would work.

[richard-uk1]

@withoutboats

I think iter violates our naming convention, in that I think iter must return the same iterator returned by <&T as IntoIterator>::into_iter, and I don't expect that impl to exist for error types.

Is this set in stone. I think iter is the best name, because it feels familiar, in that the iter() method should return an iterator that makes most sense for the object. I wouldn't expect FromIterator to be implemented for &Error, but if it were to be implemented then it should look like this (i.e. include the base error).

Could the naming convention be changed to "iter must return the same iterator returned by <&T as IntoIterator>::into_iter, or if this is not implemented, it must return the canonical iterator for the object (implying that such a canonical iterator must exist)".