Use of `ErrorIterator` for unsized types

dcormier commented 2 weeks ago

I've got a case where I have a &dyn std::error::Error passed in, which is unsized, so error_iter::ErrorIter (in the current release; ErrorExt in main) isn't implemented and I can't call ErrorIter::sources. However, Sized is not a requirement for ErrorIterator itself. Having a public constructor of some sort for ErrorIterator would allow me to take advantage of it.

Additionally, this would require droppinging the 'static requirement. That requirement doesn't appear to be needed for anything else. All tests still pass with it removed.

Both of these compile, and don't require Sized:

pub fn iter<'a>(err: &'a dyn std::error::Error) -> ErrorIterator<'a> {
    ErrorIterator { inner: Some(err) }
}

impl<'a> ErrorIterator<'a> {
    pub fn new(err: &'a dyn std::error::Error) -> Self {
        Self { inner: Some(err) }
    }
}

parasyte commented 1 week ago

Do these same issues exist on nightly with Error::sources()? Looks like it wouldn't, since sources() is implemented on dyn Error in the standard library. This could be a clue on what to do here.

dcormier commented 6 days ago

I do have the same problem on nightly, yes. Here's an example.

The use case I have is that I have a method that takes &dyn std::error::Error to add error context to a logger. I include the chain of errors (as Vec<String>) in the logs. I have something to do this manually today, but it seems a perfect use case for an iterator over error sources. But it would need to work for &dyn std::error::Error, without the 'static or Sized requirements.

This is basically what I'd like to be able to do:

fn chain(err: &dyn Error) -> Vec<String> {
    err.sources().map(ToString::to_string).collect()
}

parasyte commented 6 days ago

That's an excellent reproduction case, thank you!

After some looking around, this might be hitting one of the rough spots in the compiler. It's technically a bug, since the compiler could provide more information about what is going wrong and provide a suggested solution. The test case and its solution are very similar to these:

(None of these issues have much activity, which is a bit concerning.)

The solution is, for at least this test case, to include the + 'static constraint on the trait object in the chain function signature:

fn chain(err: &(dyn Error + 'static)) -> Vec<String> {
    //         ^---------^^^^^^^^^^^ Add this constraint.
    err.sources().map(ToString::to_string).collect()
}

The reason it works is because your error types do not contain any internal lifetimes (which would be a very odd thing to do and would be difficult to use in its own right). So, you can get away with just adding the constraint and moving on.

(Note that you could also add Send + Sync constraints since your trait objects already have these in their definition: https://gist.github.com/rust-play/b5612cf76347ff6003a732815279f221#file-playground-rs-L38)

That's what leads me to believe this is a compiler bug. It could detect this situation and offer the constraint as a possible solution. The chain function doesn't need to accept any lifetime based on its call sites. I don't expect the compiler does this kind of full-program analysis though. Suggesting this constraint may be completely wrong in some cases, I don't know for sure.

Anyway, I hope this is enough to help you with the issue that spawned this report. It's possibly just stuff that you are already aware of, though.

parasyte / error-iter

Use of `ErrorIterator` for unsized types #17