gtk-rs-core icon indicating copy to clipboard operation
gtk-rs-core copied to clipboard

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

Open jf2048 opened this issue 2 years ago • 12 comments

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

jf2048 avatar Feb 07 '23 23:02 jf2048

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

GuillaumeGomez avatar Feb 08 '23 09:02 GuillaumeGomez

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.

jf2048 avatar Feb 08 '23 14:02 jf2048

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

GuillaumeGomez avatar Feb 08 '23 14:02 GuillaumeGomez

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

jf2048 avatar Feb 08 '23 18:02 jf2048

@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 avatar Feb 14 '23 16:02 jf2048

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 avatar Feb 14 '23 16:02 jf2048

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

jf2048 avatar Feb 14 '23 16:02 jf2048

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.

ranfdev avatar Feb 14 '23 17:02 ranfdev

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 avatar Feb 14 '23 18:02 jf2048

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

jf2048 avatar Feb 14 '23 18:02 jf2048

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.

Hofer-Julian avatar Feb 14 '23 19:02 Hofer-Julian

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 avatar Feb 14 '23 21:02 jf2048