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 some missing syn invisible group handling in FromMeta implementations #263

Merged

Conversation

Ten0
Copy link
Contributor

@Ten0 Ten0 commented Feb 8, 2024

This was supposed to be fixed by c036162 but it looks like these implementations were forgotten.

This was supposed to be fixed by c036162 but it looks like this implementation was forgotten.
Ten0 added a commit to Ten0/darling that referenced this pull request Feb 8, 2024
It's currently named "Unexpected litteral type", but it can also be instantiated when not parsing a litteral, e.g. https://github.com/TedDriggs/darling/blob/224a866727cd9e40c2cd80430852b1ea0deeb40e/core/src/error/mod.rs#L152
This specifically made investigation of TedDriggs#263 more difficult.
@Ten0 Ten0 force-pushed the fix_missing_syn_invisible_group_handling branch from 2ccf283 to aecdfec Compare February 8, 2024 17:27
@Ten0 Ten0 changed the title Fix missing syn invisible group handling in FromMeta for Path Fix some missing syn invisible group handling in FromMeta implementations Feb 8, 2024
Copy link
Owner

@TedDriggs TedDriggs left a comment

Choose a reason for hiding this comment

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

Can you please add some unit tests to src/from_meta that show inputs which rely on this change? That will make it easier to understand the change, and to prevent any future regressions.

TedDriggs pushed a commit that referenced this pull request Feb 12, 2024
It's currently named "Unexpected litteral type", but it can also be instantiated when not parsing a litteral, e.g. https://github.com/TedDriggs/darling/blob/224a866727cd9e40c2cd80430852b1ea0deeb40e/core/src/error/mod.rs#L152
This specifically made investigation of #263 more difficult.
@Ten0
Copy link
Contributor Author

Ten0 commented Feb 12, 2024

As of syn 2.40, syn may generate this invisible token when the input to the darling proc macro (specifically, the attributes) are generated by a macro_rules! (e.g. propagating a macro_rules!'s expr), so this doesn't fit well within the existing test framework (as there would have to be tests for a crate that expands a such macro, that depends on the proc_macro user crate that depends on darling's macro crate).
However I did test that this resolves the parsing issue in such cases (We're actually relying on a fork for this ATM).
A similar breakage happened within Diesel: diesel-rs/diesel#3873

@TedDriggs
Copy link
Owner

Okay, can you add that as a comment in the source code with each of these changes so that it's clear later why this is going on?

@TedDriggs TedDriggs merged commit 3ee36d9 into TedDriggs:master Feb 12, 2024
12 checks passed
@Ten0 Ten0 deleted the fix_missing_syn_invisible_group_handling branch February 12, 2024 20:18
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 this pull request may close these issues.

2 participants