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

Fix generics tokenization in Error derive macro #238

Merged
merged 4 commits into from
Feb 7, 2023
Merged

Fix generics tokenization in Error derive macro #238

merged 4 commits into from
Feb 7, 2023

Conversation

zohnannor
Copy link
Contributor

@zohnannor zohnannor commented Feb 6, 2023

Synopsis

#[derive(Error)] on enums or structs that have both const generics and generic types...

#[derive(Debug, Display, Error)]
enum ErrConstGeneric<const X: usize, E> {
    Inner(E),
}

...panics:

error: proc-macro derive panicked
   --> tests/error/derives_for_generic_enums_with_source.rs:270:30
    |
270 |     #[derive(Debug, Display, Error)]
    |                              ^^^^^
    |
    = help: message: expected one of: `for`, parentheses, `fn`, `unsafe`, `extern`, identifier, `::`, `<`, square brackets, `*`, `&`, `!`, `impl`, `_`, lifetime

Solution

For some reason, iterating over syn::Generics::params that consists of only const generics or only generic types, tokenizes them as identifiers only, but when we mix them, syn decides to output const generics like they were defined, with const keyword, type annotation, etc. So when we are rendering the trait impl, adding bounds for an item in where clause, we get this:

#[automatically_derived]
impl<const X: usize, E> ::std::error::Error for ErrConstGeneric<X, E>
where
    E: ::std::fmt::Debug + ::std::fmt::Display + ::std::error::Error + 'static,
    ErrConstGeneric<const X: usize, E>: ::std::fmt::Debug + ::std::fmt::Display, // obv wrong
{
    fn source(&self) -> Option<&(dyn ::std::error::Error + 'static)> {
        match self {
            ErrConstGeneric::Inner(source) => {
                Some(source as &(dyn ::std::error::Error + 'static))
            }
        }
    }
}

Use syn's built-in Generics::split_for_impl to obtain a type that properly tokenizes all generic parameters.

As for the tests, I've added a "compile fail" tests for Error derive macro on generic enums and structs with combinations of different generics. AFAIU, this happens only in Error macro, tell me if I should add tests for some others.

This should be listed in the changelog under Fixes section, am I right?

Checklist

  • Documentation is updated (if required)
  • Tests are added/updated (if required)
  • CHANGELOG entry is added (if required)

impl/src/error.rs Outdated Show resolved Hide resolved
Copy link
Owner

@JelteF JelteF left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution and nice catch. We use split_for_impl for this exact reason in most other places, but it seems this one was missed. Changelog should indeed be under the Fixes section.

@zohnannor
Copy link
Contributor Author

Thank you! I'll make corrections and re-request a review when I can.

@zohnannor zohnannor requested a review from JelteF February 7, 2023 09:14
@tyranron tyranron added this to the 1.0.0 milestone Feb 7, 2023
Copy link
Collaborator

@tyranron tyranron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zohnannor thanks!

@JelteF JelteF added bug and removed enhancement labels Feb 7, 2023
@JelteF JelteF merged commit 4816fd9 into JelteF:master Feb 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants