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

Not all repr expressions in enums can be parsed with derive feature #1513

Closed
JelteF opened this issue Sep 15, 2023 · 6 comments · Fixed by #1572
Closed

Not all repr expressions in enums can be parsed with derive feature #1513

JelteF opened this issue Sep 15, 2023 · 6 comments · Fixed by #1572

Comments

@JelteF
Copy link

JelteF commented Sep 15, 2023

@ModProg reported an issue that a derive on one of his structs did not work in my derive_more crate:

#[derive(derive_more::Debug)]
enum Enum {
    A = if cfg!(unix) { 2 } else { 3 },
}
error: proc-macro derive panicked
 --> src/main.rs:1:10
  |
1 | #[derive(derive_more::Debug)]
  |          ^^^^^^^^^^^^^^^^^^
  |
  = help: message: called `Result::unwrap()` on an `Err` value: Error("unsupported expression; enable syn's features=[\"full\"]")

Source: JelteF/derive_more#301

Do you think there's a way to work around this. For this specific derive (and really for most) there's no need to actually inspect the expression, we only care about the field names. So even if syn would parse it to something like Expr::Unknown(TokenStream) that would be much better than completely failing the parsing (like it does now).

@JelteF
Copy link
Author

JelteF commented Dec 18, 2023

I would love to have this issue fixed in derive_more. Any thoughts on this? Is this fixable in syn?

@dtolnay
Copy link
Owner

dtolnay commented Dec 19, 2023

I don't think I would want to change this in the Parse impl for Expr, or DeriveInput. But it should be possible for derive_more to use syn::parse to parse the macro input directly itself using different parsing logic that behaves as you want.

@ModProg
Copy link
Contributor

ModProg commented Dec 20, 2023

But it should be possible for derive_more to use syn::parse to parse the macro input directly itself using different parsing logic that behaves as you want.

This would more or less require reimplementing a decent chunk of syn's AST.

Probably enabling syn.features = ["full"] is
the more reasonable approach.

@dtolnay
Copy link
Owner

dtolnay commented Dec 20, 2023

This would more or less require reimplementing a decent chunk of syn's AST.

It would not. My informed estimate is 2.25%.

The actual amount to be implemented is so small that anyhow parses the same chunk of syntax using macro_rules, and obviously that is without using syn at all, not even Type or Path which you would be reusing wholesale. That macro is 682 lines, of which over 40% is just for types and paths, and 27% is un/binops which are verbose to handle in macro_rules but <10 lines total in syn::parse.

My estimate is we are talking about under 90 lines of code to implement a correct expression parser with syn that recognizes all Rust syntax.

@JelteF
Copy link
Author

JelteF commented Dec 21, 2023

I don't think I would want to change this in the Parse impl for Expr, or DeriveInput.

My estimate is we are talking about under 90 lines of code to implement a correct expression parser with syn that recognizes all Rust syntax.

If it's really only 90 lines of code, then I'm not really sure why you wouldn't want to have it in the Parse impl for DeriveInput.

To be clear, I'm not opposed to derive_more taking ownership of writing this parser that recognizes rust syntax. I'm simply not understanding why that parser should then live in derive_more, instead of in syn. Because right now not all inputs to a derive can actually be parsed by DeriveInput its Parse impl. It seems to me that this is not a problem specific to derive_more.

@dtolnay
Copy link
Owner

dtolnay commented Jan 1, 2024

Fixed in 2.0.45.

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.

3 participants