Add helpful error message on derive syntax error

[jacklund]

Description

Panic on parse error of the NetworkBehaviour derive.

For a syntax error such as:

    #[derive(NetworkBehaviour)]
    #[behaviour(out_event = Event)]
    struct MyBehaviour {
        floodsub: Floodsub,
        mdns: Mdns,
    }

    enum Event {
        FloodsubEvent(FloodsubEvent),
        MdnsEvent(MdnsEvent),
    }

    impl From<FloodsubEvent> for Event {
        fn from(event: FloodsubEvent) -> Self {
            Self::FloodsubEvent(event)
        }
    }

    impl From<MdnsEvent> for Event {
        fn from(event: MdnsEvent) -> Self {
            Self::MdnsEvent(event)
        }
    }

The current derive macro gives:

% cargo build
   Compiling libp2p_testcase v0.1.0 (/home/jack/git/libp2p_testcase)
error parsing attribute metadata: expected literal
error parsing attribute metadata: expected literal
error parsing attribute metadata: expected literal
error[E0277]: the trait bound `(): From<MdnsEvent>` is not satisfied
  --> src/main.rs:10:14
   |
10 |     #[derive(NetworkBehaviour)]
   |              ^^^^^^^^^^^^^^^^ the trait `From<MdnsEvent>` is not implemented for `()`
   |
   = help: the following implementations were found:
             <(Vec<u8>, std::net::SocketAddr) as From<trust_dns_proto::xfer::serial_message::SerialMessage>>
             <(netlink_packet_core::message::NetlinkMessage<T>, netlink_sys::addr::SocketAddr, M) as From<netlink_proto::protocol::request::Request<T, M>>>
   = help: see issue #48214
   = note: this error originates in the derive macro `NetworkBehaviour` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0277]: the trait bound `(): From<FloodsubEvent>` is not satisfied
  --> src/main.rs:10:14
   |
10 |     #[derive(NetworkBehaviour)]
   |              ^^^^^^^^^^^^^^^^ the trait `From<FloodsubEvent>` is not implemented for `()`
   |
   = help: the following implementations were found:
             <(Vec<u8>, std::net::SocketAddr) as From<trust_dns_proto::xfer::serial_message::SerialMessage>>
             <(netlink_packet_core::message::NetlinkMessage<T>, netlink_sys::addr::SocketAddr, M) as From<netlink_proto::protocol::request::Request<T, M>>>
   = help: see issue #48214
   = note: this error originates in the derive macro `NetworkBehaviour` (in Nightly builds, run with -Z macro-backtrace for more info)

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

Whereas the code for this PR gives:

% cargo build
    Updating crates.io index
   Compiling libp2p_testcase v0.1.0 (/home/jack/git/libp2p_testcase)
error: proc-macro derive panicked
  --> src/main.rs:10:14
   |
10 |     #[derive(NetworkBehaviour)]
   |              ^^^^^^^^^^^^^^^^
   |
   = help: message: Error parsing '(out_event = Event)': expected literal

error: could not compile `libp2p_testcase` due to previous error

Links to any relevant issues

https://github.com/libp2p/rust-libp2p/issues/2663

Open Questions

• It'd be nice to be more specific as to where the error is.
• Couldn't find way to add test for this, given that it's a compiler error

Change checklist

• [x] 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
• [x] A changelog entry has been made in the appropriate crates

[thomaseizinger]

• It'd be nice to be more specific as to where the error is.

This is what Spans are for :)

• Couldn't find way to add test for this, given that it's a compiler error

A really good way for testing proc macros is trybuild.

[jacklund]

I am not convinced that this change is actually an improvement though. What if, instead of failing, we just accept this syntax? I am almost certain that the requirement for the type to be quoted is because historically, rustc didn't allow anything else within custom attributes!

That's a fair point, and one that I actually explored when trying to get this working, because the main reason I ran into this is that saying something like out_event = Event makes more sense to me, especially in the context of a macro or a derive.

However, I couldn't figure out how to make syn accept that syntax. The issue is that parse_meta expects only literals. To be fair, I've never worked with syn before, so I could be missing something really obvious, but I couldn't find anything that would parse the metadata and take anything but a literal.

If I am missing something, please let me know, because I would much prefer that syntax, honestly.

[jacklund]

This is what Spans are for :)

Y'know, I looked at using a span with this, because the error comes with one, but the span came back with something like "bytes 251-260", which I couldn't figure out how to turn into anything meaningful.

Again, syn n00b here, though.

A really good way for testing proc macros is trybuild.

Ah, cool, thanks!!!

[thomaseizinger]

This is what Spans are for :)

Y'know, I looked at using a span with this, because the error comes with one, but the span came back with something like "bytes 251-260", which I couldn't figure out how to turn into anything meaningful.

Again, syn n00b here, though.

syn has an Error type that you can feed the Span back into: https://docs.rs/syn/1.0.95/syn/struct.Error.html#method.new

This Error converts back into a Tokenstream which can then return from the macro function.

[thomaseizinger]

I am not convinced that this change is actually an improvement though. What if, instead of failing, we just accept this syntax? I am almost certain that the requirement for the type to be quoted is because historically, rustc didn't allow anything else within custom attributes!

That's a fair point, and one that I actually explored when trying to get this working, because the main reason I ran into this is that saying something like out_event = Event makes more sense to me, especially in the context of a macro or a derive.

However, I couldn't figure out how to make syn accept that syntax. The issue is that parse_meta expects only literals. To be fair, I've never worked with syn before, so I could be missing something really obvious, but I couldn't find anything that would parse the metadata and take anything but a literal.

If I am missing something, please let me know, because I would much prefer that syntax, honestly.

I am typing this from memory and I also don't know if that is the actual best-practise but I think if you ditch parse_meta and implement Parse yourself, you can control precisely what shape your input needs to have.

Again, this may not be the best practise so take it with a grain of salt. I don't want to send you on a wild-goose chase here :)

[jacklund]

I am typing this from memory and I also don't know if that is the actual best-practise but I think if you ditch parse_meta and implement Parse yourself, you can control precisely what shape your input needs to have.

Again, this may not be the best practise so take it with a grain of salt. I don't want to send you on a wild-goose chase here :)

As intriguing as that sounds, I'm kind of reticent to start rewriting the parsing when the ask was to just make the error message better, especially given that I'm not that familiar with the code or the library. Plus, this is my first PR for this library, so I don't want to get ahead of myself. :smile_cat:

I appreciate the pointers, though!

[thomaseizinger]

I am typing this from memory and I also don't know if that is the actual best-practise but I think if you ditch parse_meta and implement Parse yourself, you can control precisely what shape your input needs to have.

Again, this may not be the best practise so take it with a grain of salt. I don't want to send you on a wild-goose chase here :)

As intriguing as that sounds, I'm kind of reticent to start rewriting the parsing when the ask was to just make the error message better, especially given that I'm not that familiar with the code or the library. Plus, this is my first PR for this library, so I don't want to get ahead of myself. :smile_cat:

I appreciate the pointers, though!

That is okay :)

Would a spanned error be something that you are willing to do? Then the error message would at least be rendered at the right position. Plus, it would be nice to offer a help text that the user should try quoting the type?

[jacklund]

Sorry to leave this so long - things got kind of busy.

Honestly, as much as I'd love to dig into the spans thing, I don't really think I have time ATM.

Apologies.

[mxinden]

Closing here since the effort seems to have stalled. Help is still very much welcome. If you want to continue, please open a new pull request.