Open sfackler opened 5 years ago
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)
.
I like the idea of a convenient interface to iterate sources. However, the interface feels unergonomic as mentioned in https://github.com/rust-lang/rust/pull/58289#issuecomment-463624435. Additionally I think this should not be stabilized before #53487
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_*()
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.
So, any changes needed here? I really want this to be stabilized
I think a few changes are needed before this can be stabilized:
.skip(1)
to that iterator. No one else has really expressed an opinion about that question..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).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.my 2¢: I think the simplest (for users) implementation is a single iter
method, with a note in the documentation that
>= 1
, that is, you can call iter.next().unwrap()
once without causing a panic.e.iter().skip(1)
.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)
).
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.
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:
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.
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.
@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)".
To give some more context on why I would like it to be called iter
:
When I'm reading the documentation for some object with methods, when I see an iter
method I think to myself "what iterator makes most sense for this object". So for a vector I would assume it returns refs to the elements, or for a linked list (which is what our error chain is essentially), I would expect it to walk the nodes and return a node per iteration. I don't think "how is FromIterator
implemented for the reference type for this object". Maybe I should be thinking that :P
I think
iter
violates our naming convention, in that I thinkiter
must return the same iterator returned by<&T as IntoIterator>::into_iter
, and I don't expect that impl to exist for error types.
Hmm, this works:
impl<'a> IntoIterator for &'a (dyn Error + 'static) {
type Item = &'a (dyn Error + 'static);
type IntoIter = ErrorIter<'a>;
fn into_iter(self) -> Self::IntoIter {
ErrorIter {
current: Some(self),
}
}
}
let mut iter = <&(dyn Error + 'static) as IntoIterator>::into_iter(&err);
or
for e in err.borrow() as &(dyn Error) {
eprintln!("{}", e);
}
and iter_chain()
(or iter()
how I would call it) is implemented for dyn Error
@haraldh We can add that impl, but I don't think we should (and it seems completely circular to add it just to justify naming the method iter
)
@haraldh We can add that impl, but I don't think we should ...
Agreed
I'd like to point to the issues discussing the addition (#48420) and stabilization (#50894) of std::path::Path::ancestors
. At first glance this API might seem unrelated but the situation back then was quite similar:
self
should be included.In the end, the rationale was:
self
should be included. It is easy to .skip(1)
it. Not including self
is harmful because it is harder to add self
to the iterator than to remove it. (I think, it is not a good idea to add two APIs just to cover both semantics because that will clutter both, the namespace and the human mind.)self
is included. Adding an "s" to the original method name is a good to start unless that leads to unwanted associations which, I think, is often the case. (I think, .iter()
is not a good choice, because it is natural to iterate over vector items but it is not natural to iterate over error sources. In theory, there may be other properties of an Error
that are a more or equally natural subject to iteration, e.g., descriptions provided in different languages. In particular, a type implementing Error
might also want to implement a method called .iter()
for whatever reason.)I'd like to throw two naming ideas into the ring that might be considered for further debate:
.ancestors()
, because the iterator iterates over the ancestors in a ancestry of errors which are cause to their respective children..chained()
in honor of error-chain
and because the iterator iterates over the chain of errors that is somehow included in self
.I'd also like to propose to rename the ErrorIter
struct. I think, it is good practice throughout the standard library to name the struct after the method creating it (example).
Sorry if this was brought up earlier, I tried to read the RFC/issues, and didn't see it brought up.
What is the intent of how to handle older code that overrides cause
but not source
? For example, IntoStringError
only implements cause
. If you iterate over sources (iter_sources
), then the cause is lost. In this case it is the Utf8Error
which provides useful detail like invalid utf-8 sequence of 1 bytes from index 0
.
Should these errors be changed to implement source
instead of cause
? Or maybe implement both?
IMO all new errors should only implement source
. All old code that want to stay relevant and usable should be updated to implement source
as well. I do not think we should focus the development of new features in libstd to cover deprecated items.
IMO all new errors should only implement source. All old code that want to stay relevant and usable should be updated to implement source as well. I do not think we should focus the development of new features in libstd to cover deprecated items.
If we were to do this, could we make an effort to submit PRs to popular crates using cause
.
More generally (and slightly OT), is there a mechanism for the community to help crate maintainers with stuff like this?
I'd like to point to the issues discussing the addition (#48420) and stabilization (#50894) of
std::path::Path::ancestors
. At first glance this API might seem unrelated but the situation back then was quite similar:1. The objective was to add an iterator that recursively calls an existing, well-known method. 2. There was a discussion if `self` should be included. 3. There was a lengthy discussion about the name.
In the end, the rationale was:
1. Such iterators are helpful. They should better be stabilized sooner than later. 2. `self` should be included. It is easy to `.skip(1)` it. Not including `self` is harmful because it is harder to add `self` to the iterator than to remove it. (I think, it is not a good idea to add two APIs just to cover both semantics because that will clutter both, the namespace and the human mind.) 3. The chosen name should be telling and reflect the fact that `self` is included. Adding an "s" to the original method name is a good to start unless that leads to unwanted associations which, I think, is often the case. (I think, `.iter()` is not a good choice, because it is natural to iterate over vector items but it is not natural to iterate over error sources. In theory, there may be other properties of an `Error` that are a more or equally natural subject to iteration, e.g., descriptions provided in different languages. In particular, a type implementing `Error` might also want to implement a method called `.iter()` for whatever reason.)
I'd like to throw two naming ideas into the ring that might be considered for further debate:
* `.ancestors()`, because the iterator iterates over the ancestors in a ancestry of errors which are cause to their respective children. * `.chained()` in honor of `error-chain` and because the iterator iterates over the chain of errors that is _somehow_ included in `self`.
I'd also like to propose to rename the
ErrorIter
struct. I think, it is good practice throughout the standard library to name the struct after the method creating it (example).
Opened a PR with this suggestion: https://github.com/rust-lang/rust/pull/65557
So, it seems, that even Path::ancestors() includes itself. So, to avoid confusion and simplify it more, I reduced PR #65557 to only have chained
and Chain
.
Rationale:
.chained()
was chosen in honor of error-chain and because the iterator iterates over the chain of errors that is somehow included in self.Chain
because the error::Chain
is what we want to have.I like the name chained
because it indicates that it includes the current error, but connects all the others.
The docs should mention the skip(1)
way of avoiding the original error. (This is currently the case in the PR 323f6a4)
What is the intent of how to handle older code that overrides
cause
but notsource
? For example,IntoStringError
only implementscause
. If you iterate over sources (iter_sources
), then the cause is lost. In this case it is theUtf8Error
which provides useful detail likeinvalid utf-8 sequence of 1 bytes from index 0
.
I fixed this for IntoStringError
in #65366. Now the only libstd error not properly implementing Error::source
is the TryLockError
. I might look into trying to fix that at some point. But it does not feel critical.
If we were to do this, could we make an effort to submit PRs to popular crates using cause.
I personally don't see why the source iterators should be blocked on helping the ecosystem start using source
instead of cause
. The decision to move over to source
as the preferred method was made back when that was introduced and cause
deprecated, not here. This issue just makes the new way more ergonomic/usable. Adding a source iterator does not make the crates still using cause
worse than they are today.
This iterator should not implement Copy
, as it was originally decided that Copy
iterators are confusing in practice.
This iterator should not implement
Copy
, as it was originally decided thatCopy
iterators are confusing in practice.
Addressed in https://github.com/rust-lang/rust/pull/66511
The other issue I see blocking stability (aside from the name) is the
T: Error
vsdyn Error
issue. For reasons that are beyond annoying,Box<dyn Error>
does not implementError
. This makes whether methods are implemented fordyn Error
orT: Error
matter in really annoying ways.Can this method be implemented on both
dyn Error
andT: Error
? If so, is it? If it can't be, why not? Have we chosen the right choice? I had to make these choices for failure once, and I'm not sure I made them right then (not that I even remember clearly what I did without checking). I would like to see some write up explaining our policy around where we add these convenience methods to convince me we've done it in the right way and not just introduced yet another papercut to the error API.
Originally posted by @withoutboats in https://github.com/rust-lang/rust/pull/66448#issuecomment-554431107
With:
pub trait Error: Debug + Display {
fn sources(&self) -> Chain<'_> {
Chain {
current: self.source(),
}
}
fn chain(&self) -> Chain<'_> {
Chain { current: None }
}
fn source(&self) -> Option<&(dyn Error + 'static)> {
None
}
}
We can let Errors implement Error::source()
and Error::chain()
.
Sadly we can't do:
fn chain(&self) -> Chain<'_> {
Chain {
current: Some(self),
}
}
in the trait definition, because Error
is not Sized
.
But with the upper trait, we could:
#[derive(Debug)]
struct B(Option<Box<dyn Error + 'static>>);
impl Error for B {
fn chain(&self) -> Chain<'_> {
Chain {
current: Some(self),
}
}
fn source(&self) -> Option<&(dyn Error + 'static)> {
self.0.as_ref().map(|e| e.as_ref())
}
}
This would save us from the impl dyn Error
and the unergonomic
let mut iter = (&b as &(dyn Error)).chain();
// or
let mut iter = <dyn Error>::chain(&b);
// or
let mut iter = Error::chain(&b);
Error::sources()
is fine though.
With:
pub trait Error: Debug + Display {
// …
fn chain(&self) -> Chain<'_>
where
Self: Sized + 'static,
{
Chain {
current: Some(self),
}
}
// …
}
we can't use for example:
let mut iter = b.source().unwrap().chain();
error: the `chain` method cannot be invoked on a trait object
--> src/main.rs:74:40
|
74 | let mut iter = b.source().unwrap().chain();
|
Annoyingly Error::sources()
is easier to implement than Error::chain()
.
Point for @withoutboats
Maybe we can get along with Error::sources()
and Chain::new()
Side note: once we have these, we should consider providing a simple type for boxed errors whose Debug implementation automatically prints the chain. That would make it do the right thing for question-mark-in-main by default.
Why would the Debug
impl print a bunch of Display
impls? Feels a bit weird, but maybe it makes sense.
But yeah, related, something I have wanted since forever is the equivalent of Error::display_chain()
from error-chain
or Error::display(separator)
from err-context
.
error.display("\nCaused by: ")
is how I print all my errors. It's been my motivation for having a source iterator from the start :)
Here's an issue that may be directly related to Error::chain()
or to another problem in rustc.
I've tried to use Error::chain()
on a borrowed Error
and while refactoring its usage into a smaller function I got a compiler error that my borrow and the borrowed type had to be 'static
.
error[E0621]: explicit lifetime required in the type of `cause`
--> src/error_handling.rs:94:34
|
91 | fn formatMultipleCauses(cause: &dyn StdError) -> String
| ------------- help: add explicit lifetime `'static` to the type of `cause`: `&'static (dyn std::error::Error + 'static)`
...
94 | for (n, causeEntry) in cause.chain().enumerate() {
| ^^^^^ lifetime `'static` required
I applied the first suggestion until I got to a point where the compiler declared my error didn't live long enough.
Thanks to the help on Rust's discord I was told the compiler suggestion was incorrect - the borrow didn't have to be static, but the borrowed type had to.
A question was raised if the Error::chain()
implementation was correct - right now it is implemented on dyn Error
, but do you think it should be done on dyn Error + 'static
instead?
Minimal reproduction of the static lifetime incorrect suggestion:
trait Foo { }
struct Bar<'a> {
inner: &'a (dyn Foo + 'static)
}
impl dyn Foo {
fn bar(&self) -> Bar<'_> {
Bar { inner: self }
}
}
fn f(x: &dyn Foo) {
x.bar();
}
For reference here are the original snippets that triggered the problem:
Here's an issue that may be directly related to
Error::chain()
or to another problem in rustc.I've tried to use
Error::chain()
on a borrowedError
and while refactoring its usage into a smaller function I got a compiler error that my borrow and the borrowed type had to be'static
.error[E0621]: explicit lifetime required in the type of `cause` --> src/error_handling.rs:94:34 | 91 | fn formatMultipleCauses(cause: &dyn StdError) -> String | ------------- help: add explicit lifetime `'static` to the type of `cause`: `&'static (dyn std::error::Error + 'static)` ... 94 | for (n, causeEntry) in cause.chain().enumerate() { | ^^^^^ lifetime `'static` required
I applied the first suggestion until I got to a point where the compiler declared my error didn't live long enough.
Thanks to the help on Rust's discord I was told the compiler suggestion was incorrect - the borrow didn't have to be static, but the borrowed type had to.
A question was raised if the
Error::chain()
implementation was correct - right now it is implemented ondyn Error
, but do you think it should be done ondyn Error + 'static
instead?Minimal reproduction of the static lifetime incorrect suggestion:
trait Foo { } struct Bar<'a> { inner: &'a (dyn Foo + 'static) } impl dyn Foo { fn bar(&self) -> Bar<'_> { Bar { inner: self } } } fn f(x: &dyn Foo) { x.bar(); }
For reference here are the original snippets that triggered the problem:
* The working version before refactoring: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=8246b0817606b96886f2fad00eee716a * The not working version with comments what changed: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=0cd335b97b920f0029a8ea6feacf99b6
try &(dyn StdError + 'static)
instead of &'static dyn StdError
@haraldh I already said I tried that and it worked. The point is - why did the compiler make a wrong suggestion and could it be avoided by changing the implementation of Error::chain()?
@haraldh I already said I tried that and it worked. The point is - why did the compiler make a wrong suggestion and could it be avoided by changing the implementation of Error::chain()?
Even if implemented only for dyn Error + 'static
the compiler message does not change.
Nevertheless, I now found a solution for having chain() on the Error trait as well as on dyn Error.
pub trait ErrorChain {
fn chain(&self) -> Chain<'_>;
}
impl<T: Error + 'static + Sized> ErrorChain for T {
fn chain(&self) -> Chain<'_> {
Chain {
current: Some(self),
}
}
}
impl ErrorChain for dyn Error + 'static {
fn chain(&self) -> Chain<'_> {
Chain {
current: Some(self),
}
}
}
This removes the non-ergonomic:
let my_error = MyError;
let mut iter = <dyn Error>::chain(&my_error);
// or
let mut iter = Error::chain(&my_error);
and you can now directly do:
let mut iter = my_error.chain();
Will make a PR soon.
Here we go: https://github.com/rust-lang/rust/pull/69163
Just found out about this API. I think it's a great idea. One thing that would be very useful is adding some more usability methods to Chain. Two ideas that would solve usability problems that people currently have:
Display
. This would then return all the displays, probably concatenated with :
. Then you could simply do println!("{}", error.chain())
to display the whole chain. This would solve the issue described here: https://github.com/rust-lang/rust/issues/53487#issuecomment-583957601backtrace()
method, to find the first Backtrace in the chain. This would fix the issue described here: https://github.com/JelteF/derive_more/pull/110#issuecomment-587425851. Since you could simply do error.chain().backtrace()
then.to find the first Backtrace in the chain
The last is the most useful one, probably. This is why SNAFU encourages “wrapper” errors to delegate backtrace
to the wrapped error, effectively creating a linked list of delegation.
What is the use case to default to returning an iterator that starts at self
rather than self.source()
? From personal experience, the only time I've ever iterated the source chain of an error is to pretty-print it, which means every error except the root has a caused by:
prefix and thus needs to be treated differently.
If this is the common use case for others too, then it seems to me the API should be designed such that the common use case does not have to write err.chain().skip(1)
. Rather the uncommon use case should require writing std::iter::once(&err).chain(err.chain())
What is the use case to default to returning an iterator that starts at
self
rather thanself.source()
? From personal experience, the only time I've ever iterated the source chain of an error is to pretty-print it, which means every error except the root has acaused by:
prefix and thus needs to be treated differently.If this is the common use case for others too, then it seems to me the API should be designed such that the common use case does not have to write
err.chain().skip(1)
. Rather the uncommon use case should require writingstd::iter::once(&err).chain(err.chain())
Hmm, I was more thinking about s.th. like:
if let Some(ioerror) = err
.chain()
.filter_map(Error::downcast_ref::<io::Error>)
.find(|ioerror| ioerror.kind() == io::ErrorKind::NotFound)
{
// …
}
for error handling rather than error reporting
@Arnavion You could actually use something like this on the chain
to achieve the pretty printing you talk about (untested code):
main_err.chain().map(|err| err.to_string()).join("\ncaused by:")
@haraldh Yes, that's true. I'm not a fan of chain introspection (*), but it's a valid use case.
@JelteF That creates a single String
that can be potentially quite large and is certainly very unnecessary. It's worse than writing err.chain().skip(1)
.
() It ought* to be done by matching strongly-typed enum variants, like Error::ReadConfig(inner) if inner.kind() == ...
rather than walking the chain()
and guessing that this std::io::Error
I found at an arbitrary depth in the chain relates to reading the config file. This is especially the case when trying to walk the chain of errors from third-party crates, because there is no guarantee of the chain being the same when you update the crate. But it's also true that sometimes there are too many Error
types involved in the chain that it would be a hassle to change them all to have the cause explicitly in the variant, or even impossible if they're in third-party crates.
Anything blocking this from stabilization?
see https://github.com/rust-lang/rust/issues/58520#issuecomment-554912556 and following
especially https://github.com/rust-lang/rust/issues/69161 is annoying
The current implementation of error source iterators is only implemented on dyn Error
as chain()
, which returns an iterator beginning with self
instead of self.source()
(which was discussed here).
This causes an un-ergonomic usage on non &dyn Error
types:
let mut iter = (&b as &(dyn Error)).chain();
// or
let mut iter = <dyn Error>::chain(&b);
// or
let mut iter = Error::chain(&b);
chain()
on the Error traitAn additional implementation on trait Error
could be done via:
pub trait Error: Debug + Display {
// […]
fn chain(&self) -> Chain<'_> where Self: Sized + 'static {
Chain {
current: Some(self),
}
}
}
but that leads to issue https://github.com/rust-lang/rust/issues/69161
ErrorChain
traitSee https://github.com/rust-lang/rust/issues/58520#issuecomment-583289404 and PR https://github.com/rust-lang/rust/pull/69163
chain()
and add sources()
insteadReturning an iterator not starting with self
but self.source()
can be done in the Error
trait directly and so it is implemented for all dyn Error
also.
pub trait Error: Debug + Display {
// […]
fn sources(&self) -> Chain<'_> {
Chain {
current: self.source(),
}
}
}
This covers the
<dyn Error>::iter_chain
and<dyn Error>::iter_sources
methods added in #58289.