core/: Add display implementation for DialError<THandler> #2456

Frederik-Baetens · 2022-01-29T16:02:13Z

Deriving debug was impossible because of THandler not having a Debug/Display trait bound on it. Adding that might be another way of solving this, but that seemed to require more extensive changes throughout the codebase.

This broke several examples, such as ipfs-kad. The problem seems to have been introduced in commit 74f31f1

thomaseizinger · 2022-01-31T01:28:29Z

core/src/network.rs

+impl<THandler> fmt::Display for DialError<THandler> {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        write!(f, "{:?}", self)
+    }
+}


The Display implementation of an error should be a nice string IMO. For most other errors in the codebase, we match on self here and return something expressive.

I've implemented a prettier display implementation. The output for ConnectionLimit and LocalPeerId are inspired by the docs, but there was no description for InvalidPeerId, so I wrote something myself there, I hope that's okay, but would be happy with suggestions for something better.

If PeerIds map one to one to multihashes, it might be cleaner to say "The dialing attempt was rejected because the PeerId was invalid" instead of what I wrote now. Let me know if you think that's better.

thomaseizinger

That looks good to me, thank you!

@mxinden anything to add?

Frederik-Baetens · 2022-02-01T01:27:20Z

Thanks for the review. What's still curious to me though, is that the CI didn't catch this issue.

In this run on the offending commit, cargo test --workspace --no-default-features passes without issues. (Just some unrelated warnings it seems).

Checking out that commit locally, and running cargo test --workspace --no-default-features definitely fails before running any tests. Same with running cargo check. I wonder why this is. Possibly because of the caching?

core/src/network.rs

mxinden · 2022-02-01T14:17:13Z

Thanks for the review. What's still curious to me though, is that the CI didn't catch this issue.

In this run on the offending commit, cargo test --workspace --no-default-features passes without issues. (Just some unrelated warnings it seems).

Checking out that commit locally, and running cargo test --workspace --no-default-features definitely fails before running any tests. Same with running cargo check. I wonder why this is. Possibly because of the caching?

I can not reproduce the failure locally either. Neither with current master, nor with 74f31f1266f784f65ae69755d52c2127dcdcf126. Could you post more details on the failure you are seeing here @Frederik-Baetens?

Frederik-Baetens · 2022-02-01T15:36:04Z

~/Programmeren/rust/rust-libp2p/examples : cargo check
   Compiling libp2p-relay v0.6.0 (/home/frederik/Programmeren/rust/rust-libp2p/protocols/relay)
   Compiling libp2p-autonat v0.1.0 (/home/frederik/Programmeren/rust/rust-libp2p/protocols/autonat)
   Compiling libp2p-core v0.31.0 (/home/frederik/Programmeren/rust/rust-libp2p/core)
   Compiling libp2p-identify v0.33.0 (/home/frederik/Programmeren/rust/rust-libp2p/protocols/identify)
   Compiling libp2p-gossipsub v0.35.0 (/home/frederik/Programmeren/rust/rust-libp2p/protocols/gossipsub)
   Compiling libp2p-kad v0.34.0 (/home/frederik/Programmeren/rust/rust-libp2p/protocols/kad)
   Compiling libp2p-noise v0.34.0 (/home/frederik/Programmeren/rust/rust-libp2p/transports/noise)
   Compiling libp2p-rendezvous v0.3.0 (/home/frederik/Programmeren/rust/rust-libp2p/protocols/rendezvous)
   Compiling libp2p-floodsub v0.33.0 (/home/frederik/Programmeren/rust/rust-libp2p/protocols/floodsub)
   Compiling libp2p-plaintext v0.31.0 (/home/frederik/Programmeren/rust/rust-libp2p/transports/plaintext)
error[E0277]: `DialError<THandler>` doesn't implement `std::fmt::Display`
   --> core/src/network.rs:573:1
    |
573 | pub enum DialError<THandler> {
    | ^^^^^^^^^^^^^^^^^^ `DialError<THandler>` cannot be formatted with the default formatter
    |
    = help: the trait `std::fmt::Display` is not implemented for `DialError<THandler>`
    = note: in format strings you may be able to use `{:?}` (or {:#?} for pretty-print) instead
note: required by a bound in `std::error::Error`
   --> /home/frederik/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/error.rs:55:26
    |
55  | pub trait Error: Debug + Display {
    |                          ^^^^^^^ required by this bound in `std::error::Error`

error[E0277]: `THandler` doesn't implement `std::fmt::Debug`
   --> core/src/network.rs:573:1
    |
573 | pub enum DialError<THandler> {
    | ^^^^^^^^^^^^^^^^^^ `THandler` cannot be formatted using `{:?}` because it doesn't implement `std::fmt::Debug`
    |
note: required because of the requirements on the impl of `std::fmt::Debug` for `DialError<THandler>`
   --> core/src/network.rs:572:10
    |
572 | #[derive(Debug, Clone, Error)]
    |          ^^^^^
note: required by a bound in `std::error::Error`
   --> /home/frederik/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/error.rs:55:18
    |
55  | pub trait Error: Debug + Display {
    |                  ^^^^^ required by this bound in `std::error::Error`
    = note: this error originates in the derive macro `Debug` (in Nightly builds, run with -Z macro-backtrace for more info)
help: consider restricting type parameter `THandler`
    |
573 | pub enum DialError<THandler: std::fmt::Debug> {
    |                            +++++++++++++++++

For more information about this error, try `rustc --explain E0277`.
error: could not compile `libp2p-core` due to 2 previous errors

Also made a screen recording with some more info:

debug.mp4

Frederik-Baetens · 2022-02-01T15:54:00Z

🤔 running cargo update fixed it for me on current upstream master, but that seems really strange. I don't know why it works now. I'd think that deriving debug shouldn't be possible if THandler doesn't implement debug?

And I really don't understand why updating the dependencies with cargo update would make a difference. I thought that #[derive(Debug)] was offered through the stdlib, which cargo update doesn't affect?

Anyways, if this all makes sense to you, I can transform this PR into one which derives debug again, but still adds the documentation string to InvalidPeerId, and provides a display impl through thiserror.

Frederik-Baetens · 2022-02-02T01:07:54Z

Apparently it was a bug in thiserror: dtolnay/thiserror#79

No idea how I ended up with a thiserror version that old though.

This PR now derives debug again, but still adds the docstring & provides a display implementation through thiserror.

mxinden

Looks good to me. Thanks for bearing with us. Congrats on debugging the issue all the way upstream!

Would you mind including the merge commit and the changelog/version bump commit from https://github.com/mxinden/rust-libp2p/tree/display-dial-error here?

Frederik-Baetens · 2022-02-03T01:09:36Z

I rebased on your branch instead, I hope that's fine. Just cherry picking your last 3 commits resulted in a lot of merge conflicts.

thomaseizinger · 2022-02-03T01:17:22Z

I triggered CI again. Let's see what it thinks :)

mxinden · 2022-02-03T11:49:36Z

Merged via #2473. @Frederik-Baetens would have pushed to your branch instead, but I don't seem to have permissions.

add debug & display implementation for DialError<THandler>

5dfb4b9

Frederik-Baetens mentioned this pull request Jan 29, 2022

add peer-discovery example with identify & kademlia #2457

Draft

thomaseizinger requested changes Jan 31, 2022

View reviewed changes

Frederik-Baetens added 2 commits January 31, 2022 15:18

add prettier display impl for DialError

46ed56f

add documentation string to DialError::InvalidPeerId

dd149b4

thomaseizinger approved these changes Feb 1, 2022

View reviewed changes

mxinden reviewed Feb 1, 2022

View reviewed changes

core/src/network.rs Outdated Show resolved Hide resolved

implement Dialerror display through thiserror

22b6ad7

derive debug again

65d1949

mxinden changed the title ~~add debug & display implementation for DialError<THandler>~~ core/: Add display implementation for DialError<THandler> Feb 2, 2022

mxinden added 2 commits February 2, 2022 21:35

Merge branch 'libp2p/master' into display-dial-error

a64a041

*: Add changelog entries and bump versions

a316797

mxinden reviewed Feb 2, 2022

View reviewed changes

thomaseizinger approved these changes Feb 3, 2022

View reviewed changes

mxinden mentioned this pull request Feb 3, 2022

core/: Add Display implementation for DialError #2473

Merged

mxinden closed this Feb 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core/: Add display implementation for DialError<THandler> #2456

core/: Add display implementation for DialError<THandler> #2456

Frederik-Baetens commented Jan 29, 2022 •

edited

Loading

thomaseizinger Jan 31, 2022

Frederik-Baetens Jan 31, 2022

Frederik-Baetens Jan 31, 2022 •

edited

Loading

thomaseizinger left a comment

Frederik-Baetens commented Feb 1, 2022 •

edited

Loading

mxinden commented Feb 1, 2022

Frederik-Baetens commented Feb 1, 2022

Frederik-Baetens commented Feb 1, 2022 •

edited

Loading

Frederik-Baetens commented Feb 2, 2022

mxinden left a comment

Frederik-Baetens commented Feb 3, 2022 •

edited

Loading

thomaseizinger commented Feb 3, 2022

mxinden commented Feb 3, 2022

core/: Add display implementation for DialError<THandler> #2456

core/: Add display implementation for DialError<THandler> #2456

Conversation

Frederik-Baetens commented Jan 29, 2022 • edited Loading

thomaseizinger Jan 31, 2022

Choose a reason for hiding this comment

Frederik-Baetens Jan 31, 2022

Choose a reason for hiding this comment

Frederik-Baetens Jan 31, 2022 • edited Loading

Choose a reason for hiding this comment

thomaseizinger left a comment

Choose a reason for hiding this comment

Frederik-Baetens commented Feb 1, 2022 • edited Loading

mxinden commented Feb 1, 2022

Frederik-Baetens commented Feb 1, 2022

Frederik-Baetens commented Feb 1, 2022 • edited Loading

Frederik-Baetens commented Feb 2, 2022

mxinden left a comment

Choose a reason for hiding this comment

Frederik-Baetens commented Feb 3, 2022 • edited Loading

thomaseizinger commented Feb 3, 2022

mxinden commented Feb 3, 2022

Frederik-Baetens commented Jan 29, 2022 •

edited

Loading

Frederik-Baetens Jan 31, 2022 •

edited

Loading

Frederik-Baetens commented Feb 1, 2022 •

edited

Loading

Frederik-Baetens commented Feb 1, 2022 •

edited

Loading

Frederik-Baetens commented Feb 3, 2022 •

edited

Loading