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

Migrate to syn version 2.0 #130

Merged
merged 6 commits into from
Feb 14, 2024
Merged

Migrate to syn version 2.0 #130

merged 6 commits into from
Feb 14, 2024

Conversation

czocher
Copy link
Contributor

@czocher czocher commented Jun 20, 2023

This is my take on migrating typeshare to syn version 2.0. This will allow to use the serde-derive-internals library in the future.

TODO

  • Fix failing tests, currently the type override does not work due to the fact it uses the type keyword to indicate the new type. It would be great if we could rename it to new_type but that would be a major breaking change, so instead I need to write my own parser.
  • Deduplicate methods such as get_typeshare_meta_items and get_serde_meta_items etc.
  • Change some of the helper methods into extension traits, e.g. expr_to_string, remove_dash_from_identifier, rename_all_to_case

Closes #120

@czocher czocher force-pushed the syn2 branch 4 times, most recently from bcc407f to b2e7217 Compare June 20, 2023 21:07
@czocher
Copy link
Contributor Author

czocher commented Jun 20, 2023

This PR is ready for review @Lucretiel @inquisitivepenguin.

@czocher czocher changed the title DRAFT: Migrate to syn version 2.0 Migrate to syn version 2.0 Jun 20, 2023
@czocher czocher force-pushed the syn2 branch 3 times, most recently from 24590a5 to 97a5ade Compare June 20, 2023 22:38
@Lucretiel Lucretiel self-requested a review June 22, 2023 20:53
Copy link
Contributor

@Lucretiel Lucretiel 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!

I wanted to ask about this:

currently the type override does not work due to the fact it uses the type keyword to indicate the new type. It would be great if we could rename it to new_type but that would be a major breaking change, so instead I need to write my own parser.

I'm not sure I follow where this problem is occurring, could you elaborate? If this is about the use of type as an identifier in the typeshare code itself, it can usually be circumvented by using r#type, the raw identifier syntax. I'm surprised that this would require the addition of so much duplicated parse code.

@czocher
Copy link
Contributor Author

czocher commented Jun 24, 2023

I'm not sure I follow where this problem is occurring, could you elaborate? If this is about the use of type as an identifier in the typeshare code itself, it can usually be circumvented by using r#type, the raw identifier syntax. I'm surprised that this would require the addition of so much duplicated parse code.

@Lucretiel let me explain, cause I see the misunderstanding here. This PR consists of 3 commits:

  1. Extracting the helper functions to helper.rs and implementing syn 2.0.
  2. Removing the duplicated methods between the serde annotation parsing and typeshare annotation parsing (which after the change were the same, the only difference being the name of the annotation).
  3. A final cleanup which consisted of migrating the large parser.rs file into a parser module and multiple, much smaller files, each with a separate responsibility.

Here's my motivation:
While migrating the syn crate version I noticed that the parser.rs file structure was rather messy. It dealt with two separate things: parsing and helper functions (it even had a // helpers comment on line 420, which immediately told me it needed to be divided into two files), it had a lot of duplication.
I decided to extract all the parsing helpers to helpers.rs as a first pass, and later implement the other refactorings in two separate PRs, so we can easily revert them if you found it too big of a change.

The part about the type, which I mentioned in my first TODO task was due to the fact that type is a keyword in Rust (as you already know). syn 1 didn't treat it in any special way, but syn 2 failed parsing and required me to use a different method to parse it as an identifier (Ident::parse_any instead of just the default Ident::parse). This is the part I needed to implement as my own parser. If that wasn't the case I could just use: attr.parse_args_with(Punctuated::<Meta, Token![,]>::parse_terminated) instead of that whole closure.

The rest is mostly the same code, just migrated to the new syn version and extracted to more, smaller files for readability ;).

@Lucretiel
Copy link
Contributor

A final cleanup which consisted of migrating the large parser.rs file into a parser module and multiple, much smaller files, each with a separate responsibility.

Ah, GitHub was hiding this part from me, it looked like you were adding just a huge amount of new code (cause it made unclear that the old parser.rs was deleted. Re-reviewing with this in mind.

Copy link
Contributor

@Lucretiel Lucretiel left a comment

Choose a reason for hiding this comment

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

It looks like syn 1 is still coming in from a few places; it looks like they're all trivial to fix:

  • typeshare-annotation: mostly a no-op, should be a trivial fix
  • serde: bump to at least 1.0.157, though probably just go to latest for some recent build speed improvements
  • thiserror: bump to 1.0.40

@Lucretiel
Copy link
Contributor

(for reference, you can use cargo tree --invert --package syn@1.0.100 to track down where these dependencies are coming from)

@czocher
Copy link
Contributor Author

czocher commented Jul 1, 2023

Ah, GitHub was hiding this part from me, it looked like you were adding just a huge amount of new code (cause it made unclear that the old parser.rs was deleted. Re-reviewing with this in mind.

My fault, should have described my motivations better from the get-go ;).

@czocher
Copy link
Contributor Author

czocher commented Jul 1, 2023

Updated the dependencies, this task should be mergable @Lucretiel

Copy link
Contributor

@Lucretiel Lucretiel left a comment

Choose a reason for hiding this comment

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

Sorry for the additional delays. I'd like to revert the parser changes and re-review them in a different PR, since they involve implementation changes, rather than being a direct verbatim copy of the existing code from parser.rs.

Additionally, run cargo update -p wasm-bindgen in the project root; this will remove syn 1.0 entirely from the Cargo.lock.

Thanks again for all your work on this!

core/src/parser/mod.rs Outdated Show resolved Hide resolved
@czocher
Copy link
Contributor Author

czocher commented Jul 8, 2023

@Lucretiel I reverted the commit but left the helpers extraction (can revert that too if you deem it necessary). Feel free to do a review.

@czocher
Copy link
Contributor Author

czocher commented Aug 2, 2023

The review fixes has been implemented and I believe this PR is ready to be merged @Lucretiel.

@andymac4182
Copy link

@Lucretiel Any update on this? Its conflicting now due to the length of time.

@czocher
Copy link
Contributor Author

czocher commented Jan 24, 2024

I'm still around to do a rebase whenever it's necessary. Probably gonna do one soon just to make it easier in the future.

@andymac4182
Copy link

@inquisitivepenguin @CerulanLumina Any thoughts on this from you?

@czocher
Copy link
Contributor Author

czocher commented Jan 30, 2024

Rebased it.

@anish-1p
Copy link
Contributor

anish-1p commented Feb 1, 2024

@czocher Thanks for all your work on this! Could we also get the helpers extraction reverted as well, so the PR exclusively contains changes related to the migration to syn 2? We'd be happy to take a look at the helpers extraction in a separate PR.

@czocher
Copy link
Contributor Author

czocher commented Feb 1, 2024

Sure @anish-1p, give me a moment.

@czocher
Copy link
Contributor Author

czocher commented Feb 1, 2024

@anish-1p the helper extraction has been reverted.

@anish-1p
Copy link
Contributor

anish-1p commented Feb 1, 2024

@anish-1p the helper extraction has been reverted.

Thanks again! Will get this reviewed soon

Copy link
Contributor

@anish-1p anish-1p left a comment

Choose a reason for hiding this comment

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

This looks good to me. Only change I'd request is to make some utility functions in core/src/parser.rs private (I don't see a reason why get_meta_items, get_ident, rename_all_to_case, is_skipped, serde_default, serde_flatten and get_decorators are public to the crate).

@czocher
Copy link
Contributor Author

czocher commented Feb 7, 2024

@anish-1p I made the functions private.

@anish-1p anish-1p merged commit 3f7527a into 1Password:main Feb 14, 2024
8 checks passed
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.

Move to syn 2.0
4 participants