return inner error of std:io::Error as source/cause

[GlenDC]

Proposed solution to close #124513

Not sure if this should be seen as a breaking change or not.

See for more context #124513.

[rustbot]

r? @workingjubilee

rustbot has assigned @workingjubilee.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[workingjubilee]

cc @thomcc apparently you were the last to touch this.

[a1phyr]

As you noted in #124513, this would break a lot code, so I don't think that this can be merged.

Additionally, this makes an io::Error and its source have the same Display impl, which is... not great. The current state of affairs enable having an error with a custom message and while still choosing the source.

In your case, you probably need a wrapper (as you have shown works) or use into_inner/downcast.

The fact that you were confused by this may be sign that it be better documented, though.

[GlenDC]

@a1phyr downcast is not a universal thing though. So based on your answer I seem to understand that source does not at all work like I think it supposed to do...

But that also means that approaches such as the one taken by https://docs.rs/anyhow/latest/anyhow/struct.Error.html#method.chain (not something I use, but it's close enough), are flawed? As these rely upon source. And once you start to skip errors (persistent or at random) things get confusing very quickly.

Downcast sounds great, but it's not part of the Error contract, so no way to rely on that either.
So if not source is to be used, what is?!

[bjorn3]

For custom errors, io::Error effectively acts as transparent wrapper around the inner error. This means that the Display impl and Error impl delegates to the inner error (except the provide method seems to have been missed). The outer io::Error disappears entirely. Only for the errors returned by the standard library itself or io::Error::last_os_error()/io::Error::from_raw_os_error() does it show up as itself. In those cases there is no inner error to delegate to. Because of being a transparent wrapper around the inner error in case of the custom error case, the only data lost in the Display and Error impls is the ErrorKind.

[workingjubilee]

In practice, anyhow::Error::chain has no real flaw, or rather none that is particular and only emerges for io::Error. People do not try to present random errors as io::Error, and arbitrary implementations of Error::source can drop the chain of sources anyways.

[workingjubilee]

If you wish, you can open an issue against the anyhow library here: https://github.com/dtolnay/anyhow/issues

I have not really been presented with an argument for why this should change beyond "but anyhow uses it, and anyhow is supposedly more infallible than std, because ???"

[GlenDC]

Fair enough. I trust you all in this. Much respect.

Most important for me here is that I understand the intended behaviour behind ::source() as a concept.

I understand that no one is making random io errors and I get that there are no guarantees that people use the trait contract correctly.

Most important for me is just to know what source exactly is supposed to do. Are you always supposed to call inner.source? Should you never do it. Is it undefined and is there no real meaning to source? That's the thing I'm still confused about.

[workingjubilee]

It is more than "do people use the trait contract correctly", the issue is that Error::source is a provided method, so the default implementation is None, so many will simply implement it and not think about overriding it:

https://doc.rust-lang.org/1.77.2/src/core/error.rs.html#83-86

[GlenDC]

That's indeed the danger of provided trait methods. So ok, that is indeed a concern. And i get it is a reason why a chain iterator might anyway fail at any point.

Still. My question remains. Assuming the person did not forget to implement the trait method. What is the intended contract behaviour that person should take into account? Asking to aid my understanding and to know if perhaps there's room for additional documentation for that source method.

[workingjubilee]

When an implementation overrides Error::source, it should present the lower-level source of that error. The reason that the io::Error version "skips over" the io::Error and moves directly to the custom error is that when you present a custom error as an io::Error, that means it is the Error. It is adhering to the contract of io::Error::new. There is not intended to be any difference.

[GlenDC]

Ok that is super clear. Thanks a lot. Going to close this then. Thank you so much. Learned a lot from this ❤️🦀