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

RFC: glib-macros: port attribute parsing to use deluxe crate #970

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

jf2048
Copy link
Member

@jf2048 jf2048 commented Feb 7, 2023

I have this crate that can generate parsers for attributes using a derive macro. It automatically throws errors (and spelling suggestions) on invalid attributes. We can get better errors messages and remove some code this way so here is a port. The tests are passing for me without any changes.

Also made some changes to avoid erroring out early. Before it would just stop on first error, now any compile errors from inner attributes like #[enum_value(name = ...)] will be aggregated and included in the final output.

Fixes #943

@GuillaumeGomez
Copy link
Member

I'm not very convinced by the added value. It makes the code more complicated to understand and adds more proc-macro.

@jf2048
Copy link
Member Author

jf2048 commented Feb 8, 2023

Yes, this was more needed for the class macro where there are lot of attributes. And the properties macro too if wanted.

The idea though is to make it so actually running the proc macro is faster due to the generated parser, but I have not done any benchmarks yet.

@GuillaumeGomez
Copy link
Member

Might be better to have some numbers indeed (not just for this PR).

@jf2048 jf2048 changed the title glib-macros: port attribute parsing to use deluxe crate RFC: glib-macros: port attribute parsing to use deluxe crate Feb 8, 2023
@jf2048
Copy link
Member Author

jf2048 commented Feb 8, 2023

btw @ranfdev I will be using this for the class macro and it will be duplicating the same attributes from the properties macro

@jf2048
Copy link
Member Author

jf2048 commented Feb 14, 2023

@GuillaumeGomez PR now updated with a test to see how porting of one of the wrapper macros to a proc macro would be like

@jf2048
Copy link
Member Author

jf2048 commented Feb 14, 2023

Some thinking here: the current syntax of the wrapper is really unfortunate. It takes a tuple struct but then it turns it into a normal struct with a single field inner. If we want to add other fields to it they can't be added without using another awkward syntax. So I turned it into an attribute macro that works on any struct, it always takes the first field and infers the type from it. Also now you can turn off generation of any trait to manually implement it.

@jf2048
Copy link
Member Author

jf2048 commented Feb 14, 2023

Also now includes a port of clone_block to use attributes as a replacement for clone! and closure! @Hofer-Julian

@ranfdev
Copy link
Member

ranfdev commented Feb 14, 2023

The number of changes is impressive.

I agree with @GuillaumeGomez that we should evaluate if it's worth porting everything to this macro or not.

Mainly because contributing to glib-macros would then require another layer of knowledge: knowing the deluxe macro.

For example, if the properties macro gets rewritten, I would need a bit of time to learn deluxe to get comfortable working on the properties macro again.

I think it would be better to keep the number of changes to a minimum, so that we can better evaluate the changes introduced by using deluxe.

Unless clone_block is strictly required to port the macros to deluxe, it's probably better if we introduce a separate PR for that.

@jf2048
Copy link
Member Author

jf2048 commented Feb 14, 2023

These are some changes I have had in the back burner for at least several months now in various states of completeness... clone_block is not needed, all of them are separate. I've just been putting all my macros together in this PR for now. Note only the parsing gets rewritten here, the main reason I am doing this is:

  • To remove syn::parse::Parse impls and equivalents and just autogenerate them. The token output stays the same. Except for the wrapper macro, that one was rewritten when ported to a proc macro.
  • To standardize the error messages. I have done a lot of work on deluxe to make it more like darling where parsing can continue after an error and the errors are all aggregated, and you get spelling suggestions on unknown attributes; currently all the existing macros just exit on the first parse error.
  • To standardize on normal rust syntax to make it easier to use and to make clippy and rustfmt work again, those will probably never work correctly with function-like macros.

@jf2048
Copy link
Member Author

jf2048 commented Feb 14, 2023

An alternative here is to try and bring back some of the utils and techniques from darling/deluxe into glib-macros and implement them manually that way, but my hope is the autogenerated parsers are less error prone

@Hofer-Julian
Copy link
Contributor

Hofer-Julian commented Feb 14, 2023

Also now includes a port of clone_block to use attributes as a replacement for clone! and closure! @Hofer-Julian

This is great! I am not familiar with the codebase, so I cannot judge the diff. However, from a user perspective, the clone_block would be greatly appreciated. Many users work around that they cannot use clippy/rustfmt with closures inside clone! and some like @sdroege don't use clone! at all. clone_block would solve that.

@jf2048
Copy link
Member Author

jf2048 commented Feb 14, 2023

Working on benchmark here https://github.com/jf2048/glib-macro-bench

As I expected enum_derive is a bit faster now but the wrapper is slower...

@jf2048 jf2048 force-pushed the macro-deluxe-port branch 2 times, most recently from 287aebf to 0743c00 Compare February 15, 2023 17:12
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.

macros: Error out on unknown/invalid attributes
4 participants