fix(swarm): display cause of denied listen error

[divagant-martian]

Description

The display of ListenError could be more helpful. The inner cause already implements Error which in turn requires Display. This is then just a matter of using said impl requirement to get an useful display

Notes & open questions

n/a

Change checklist

• I have performed a self-review of my own code
• I have made corresponding changes to the documentation
• I have added tests that prove my fix is effective or that my feature works
• A changelog entry has been made in the appropriate crates

[mxinden]

Looks good to me. Thanks @divagant-martian. Though needs a swarm/Cargo.toml version bump. Otherwise it will not be included in the next round of releases.

[mxinden]

Thank you.

[divagant-martian]

@mxinden do I need to continuously update with master or is this not an issue (as long as there are no conflicts) ?

[mxinden]

No need to keep it updated as long as there are no merge conflicts.

The pull request is part of the mergify merge queue now (see last check). Once it is first in the merge queue, mergify will merge latest master into your branch and on green merge.

Approvals have been dismissed because the PR was updated after the send-it label was applied.

[divagant-martian]

latest commit attends to this ci failure

[thomaseizinger]

This cause is already available as part of calling Error::source. Including it in the fmt::Display as well is kind of an anti-pattern because it leads to double-printing of errors.

If we print it here, we should remove it from Error::source. Also, if you use something like anyhow, the source would be printed which I personally find the better approach than printing it here because stringifies as "late" as possible.

[divagant-martian]

first let me explain the reason for this: we get an error, we print it and get no information at all about this. The implementation of Display comes before the one for Error (since Error: Display ) so it should be up to the Error implementation to deal with this differently if necessary.

If trait A depends on trait B where B on itself is useful and publicly available, I would argue it's an anti-pattern to decide B's implementation just to satisfy something about A. More so of if that comes down to removing important information

[thomaseizinger]

it should be up to the Error implementation to deal with this differently if necessary.

The issue with that is that it is quite hard to go from a string implementation back to structured data.

This is btw also the "official" recommendation, see rust-lang/project-error-handling#27 (comment).

What is missing the Rust ecosystem is a default way of printing errors. Yes, errors can be displayed but if I understand the error handling project group correctly, then at some point, there will be a std-built-in way of printing an error that will correctly chain them together. Until then, users are encouraged to use things like anyhow or eyre.

Personally, I mildly prefer to keep the display impls short and instead return sources from Error::source. This is still correct from an API point of view: This particular error itself says what happened, i.e. it explains one layer of the entire stack. But looking at errors out of context is seldomly meaningful which is why one should go down the chain of sources.

If trait A depends on trait B where B on itself is useful and publicly available, I would argue it's an anti-pattern to decide B's implementation just to satisfy something about A. More so of if that comes down to removing important information

I'd say it is exactly the opposite actually. By stringifying the error + its sources in the Display impl, the Display impl (which is more general than the Error trait), makes an assumption what it is going to be used for (printing chains of errors) when technically, it shouldn't know about the concept of a "source error".

---

I am not too fussed which way we go until there is a definite answer from the Rust project but what I want to avoid is double printing of errors when used with a library like anyhow. To avoid this, the inner error needs to be removed from source. Are you open to sending a patch for this?