Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

derives panic when a discriminant is given by a macro #16

Closed
AdnoC opened this issue Jul 20, 2018 · 8 comments · Fixed by #39
Closed

derives panic when a discriminant is given by a macro #16

AdnoC opened this issue Jul 20, 2018 · 8 comments · Fixed by #39

Comments

@AdnoC
Copy link

AdnoC commented Jul 20, 2018

If you use set the discriminant of a C-like enum with a macro the derive panics. This happens with both ToPrimitive and FromPrimitive.

Minimal example:

#[macro_use]
extern crate num_derive;
extern crate num_traits;

macro_rules! get_an_isize {
    () => (0_isize)
}

#[derive(FromPrimitive)]
pub enum CLikeEnum {
    VarA = get_an_isize!(), // panics
    VarB = 2,
}

Errors with

error: proc-macro derive panicked
 --> src\lib.rs:9:10
  |
9 | #[derive(FromPrimitive)]
  |          ^^^^^^^^^^^^^
  |
  = help: message: called `Result::unwrap()` on an `Err` value: ParseError(Some("failed to parse derive input: failed to parse"))

Cargo.toml:

[dependencies]
num-traits = "0.2"
num-derive = "0.2"

Rust version:
rustc 1.27.0 (3eda71b00 2018-06-19)

@AdnoC
Copy link
Author

AdnoC commented Jul 20, 2018

Actually, seems like it is syn's fault, as syn::parse is returning an Err.

@AdnoC AdnoC closed this as completed Jul 20, 2018
@cuviper
Copy link
Member

cuviper commented Jul 20, 2018

You may need to enable the "full-syntax" feature here, which enables "syn/full".

@AdnoC
Copy link
Author

AdnoC commented Jul 20, 2018

Yeah, just figured that out. Going to make a ticket for syn since it should be able to parse valid custom derive input.

@bcantrill
Copy link

I hit this as well -- thank you @AdnoC and @cuviper for what ended up being a quick fix to an otherwise vexing problem! Was there ever an issue open on syn? If not, I think I'd like to get an issue open over there (and/or commented upon); the failure mode here is pretty brutal...

@cuviper
Copy link
Member

cuviper commented Jul 27, 2020

@bcantrill I don't know if there was ever a syn issue, but the error now seems worse:

error: proc-macro derive panicked
 --> src/lib.rs:9:10
  |
9 | #[derive(FromPrimitive)]
  |          ^^^^^^^^^^^^^
  |
  = help: message: called `Result::unwrap()` on an `Err` value: Error("expected `,`")

I guess it's sort of more specific than "failed to parse", but there's not enough context to really be useful.

Maybe instead of syn::parse(input).unwrap(), we should try the syn::parse_macro_input! macro. If that prints span info, it will be a lot more helpful. Or maybe we can manually do the same while combining a message about the "full-syntax" feature.

@cuviper
Copy link
Member

cuviper commented Jul 27, 2020

Re-opening since I think we can indeed do better on num-derive's part.

I have it looking like this now:

error: expected `,`
  --> src/test.rs:12:24
   |
10 |     VarA = get_an_isize!(), // panics
   |                        ^

error: this might need the "full-syntax" feature of `num-derive`
  --> src/test.rs:12:24
   |
10 |     VarA = get_an_isize!(), // panics
   |                        ^

error: aborting due to 2 previous errors

But I'd like to connect the second message back to the derive(FromPrimitive) span if possible.

@cuviper cuviper reopened this Jul 27, 2020
@cuviper
Copy link
Member

cuviper commented Jul 27, 2020

Aha, Span::call_site() is what I want. I'll get a PR up shortly, once I figure out how to test this...

@bcantrill
Copy link

Thanks for opening this back up! The failure mode was indeed really confusing -- but your new error message is great, and would have sent me to the right spot more or less immediately; thanks again!

@bors bors bot closed this as completed in 77c551f Jul 29, 2020
@bors bors bot closed this as completed in #39 Jul 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants
@cuviper @bcantrill @AdnoC and others