refactor derive - oliver #59

ghost · 2021-02-08T18:07:04Z

Currently this approach fails with the following compiler error:

   Compiling gfp-derive v0.1.0 (/media/oliver/WD/generic-field-projection/derive)
error[E0308]: mismatched types
   --> derive/src/lib.rs:197:5
    |
196 |   ) -> proc_macro::TokenStream {
    |        ----------------------- expected `proc_macro::TokenStream` because of return type
197 | /     item!(
198 | |         unsafe impl #generic_header ::gfp_core::Field for #ident<super::#input_ident #generic> {
199 | |             type Parent = super::#input_ident #generic;
200 | |             type Type = #ty;
...   |
211 | |         }
212 | |     )
    | |_____^ expected struct `proc_macro::TokenStream`, found enum `syn::Item`
    |
    = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0308]: mismatched types
   --> derive/src/lib.rs:298:23
    |
298 |           contents.push(gen_header(
    |  _______________________^
299 | |             generic_header,
300 | |             ident,
301 | |             input_ident,
302 | |             generic,
303 | |             ty,
304 | |         ));
    | |_________^ expected enum `syn::Item`, found struct `proc_macro::TokenStream`

error[E0308]: mismatched types
   --> derive/src/lib.rs:419:23
    |
419 |           contents.push(gen_header(
    |  _______________________^
420 | |             generic_header,
421 | |             ident,
422 | |             input_ident,
423 | |             generic,
424 | |             ty,
425 | |         ));
    | |_________^ expected enum `syn::Item`, found struct `proc_macro::TokenStream`

error: aborting due to 3 previous errors

For more information about this error, try `rustc --explain E0308`.
error: could not compile `gfp-derive`

…n into refactor-derive

derive/src/lib.rs

RustyYato · 2021-02-08T18:17:10Z

This looks good! If you find something else that's duplicated like this can you de-duplicate it?

ghost · 2021-02-08T18:21:59Z

Yes I spotted several other clear opportunities to simplify the module constructions.

RustyYato

I think I should have been more clear, if you find any pieces of code that are mostly duplicated across multiple places with only a small amount of variation, then extract that out to a function.

For example gen_header takes some code that was duplicated twice, and extracts it out. However, gen_header_unnamed just shifts some code around.

One other place that could benefit from extraction:

generic-field-projection/derive/src/lib.rs

Lines 387 to 393 in e46e228

    
           for field in fields.named { 
        
               let ident = field.ident.unwrap(); 
        
               contents.push(item!( 
        
                   #[allow(non_camel_case_types)] 
        
                   pub struct #ident<T>(::gfp_core::derive::Invariant<T>); 
        
               ));

generic-field-projection/derive/src/lib.rs

Lines 490 to 497 in e46e228

    
           for (i, field) in fields.unnamed.iter().enumerate() { 
        
               use syn::spanned::Spanned; 
        
               let ident = quote::format_ident!("_{}", i, span = field.span()); 
        
               contents.push(item!( 
        
                   #[allow(non_camel_case_types)] 
        
                   pub struct #ident<T>(::gfp_core::derive::Invariant<T>); 
        
               ));

generic-field-projection/derive/src/lib.rs

Lines 598 to 604 in e46e228

    
           for field in fields.named { 
        
               let ident = field.ident.unwrap(); 
        
               contents.push(item!( 
        
                   #[allow(non_camel_case_types)] 
        
                   pub struct #ident<T>(::gfp_core::derive::Invariant<T>); 
        
               ));

These for loops are very similar, with only a few minor differences between them. If you can find a way to unify them, that would be incredible.

RustyYato · 2021-02-08T20:48:57Z

.rustfmt.toml

-merge_imports = true
+imports_granularity = "Crate"


What's the difference between these two? I'm not too familiar with rustfmt config

this was the suggested resolution to a cli warning

RustyYato · 2021-02-08T20:50:49Z

derive/src/lib.rs

+fn gen_header_unnamed(
+    generic_header: &syn::ImplGenerics,
+    ident: &syn::Ident,
+    input_ident: &syn::Ident,
+    generic: &syn::TypeGenerics<'_>,
+    ty: &&syn::Type,
+    index: &syn::Member,
+) -> syn::Item {


If a function is only called once, then it doesn't pull its weight.

RustyYato · 2021-02-08T20:51:15Z

derive/src/lib.rs

+}
+
+// `TokenStream` construction for `derive_struct()` and `derive_named()`
+fn token_fields(


same as above

RustyYato · 2021-02-08T20:51:26Z

derive/src/lib.rs

+}
+
+// `TokenStream` construction for `derive_unnamed()`
+fn token_fields_unnamed(


same as above

RustyYato · 2021-02-08T20:51:39Z

derive/src/lib.rs

+}
+
+// `TokenStream` construction for `derive_union()`
+fn token_fields_union(


same as above

ghost · 2021-02-08T21:06:57Z

I think the only marginal benefit to functions holding macro code are the type signatures for all of the elements.

It's interesting to learn it should be possible to harmonize all of these only slightly differing macros contents, so I'll look into doing that and refactor these properly.

Thanks for the review!

RustyYato · 2021-02-08T21:19:01Z

To me, that's not enough of a benefit to justify extracting it to a function. However, if you think it makes the code easier to follow, hopefully, it will make it easier to get new contributors like yourself!

ghost · 2021-02-08T21:32:59Z

I agree with you about it, though yes I think there are some benefits to code readability having the types shown. If I can get the refactor done properly it shouldn't be an issue.

RustyYato · 2021-02-25T14:26:26Z

Hey @kw-fn, are you still working on this PR? If you're busy that's OK, I just want an update.

ghost · 2021-02-25T23:00:43Z

I have just been distracted with other projects, I hope to make progress this weekend

RustyYato · 2021-02-26T02:54:52Z

Ok, thanks for the update!

ghost · 2021-03-06T11:44:39Z

Looking at this right now! 🍰

oliver added 2 commits February 8, 2021 14:05

refactor derive - oliver

cf9fa42

Merge branch 'master' of github.com:RustyYato/generic-field-projectio…

2b7550e

…n into refactor-derive

RustyYato reviewed Feb 8, 2021

View reviewed changes

derive/src/lib.rs Outdated Show resolved Hide resolved

refactor derive/src/lib.rs -oliver

e46e228

ghost requested a review from RustyYato February 8, 2021 19:58

ghost mentioned this pull request Feb 8, 2021

Refactor derive #19

Open

RustyYato linked an issue Feb 8, 2021 that may be closed by this pull request

Refactor derive #19

Open

RustyYato added the enhancement New feature or request label Feb 8, 2021

RustyYato requested changes Feb 8, 2021

View reviewed changes

ghost closed this by deleting the head repository Mar 5, 2023

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor derive - oliver #59

refactor derive - oliver #59

ghost commented Feb 8, 2021

RustyYato commented Feb 8, 2021

ghost commented Feb 8, 2021

RustyYato left a comment

RustyYato Feb 8, 2021

ghost Mar 6, 2021

RustyYato Feb 8, 2021

RustyYato Feb 8, 2021

RustyYato Feb 8, 2021

RustyYato Feb 8, 2021

ghost commented Feb 8, 2021

RustyYato commented Feb 8, 2021

ghost commented Feb 8, 2021

RustyYato commented Feb 25, 2021

ghost commented Feb 25, 2021

RustyYato commented Feb 26, 2021

ghost commented Mar 6, 2021

	for field in fields.named {
	let ident = field.ident.unwrap();

	contents.push(item!(
	#[allow(non_camel_case_types)]
	pub struct #ident<T>(::gfp_core::derive::Invariant<T>);
	));

	for (i, field) in fields.unnamed.iter().enumerate() {
	use syn::spanned::Spanned;
	let ident = quote::format_ident!("_{}", i, span = field.span());

	contents.push(item!(
	#[allow(non_camel_case_types)]
	pub struct #ident<T>(::gfp_core::derive::Invariant<T>);
	));

		merge_imports = true
		imports_granularity = "Crate"

refactor derive - oliver #59

refactor derive - oliver #59

Conversation

ghost commented Feb 8, 2021

RustyYato commented Feb 8, 2021

ghost commented Feb 8, 2021

RustyYato left a comment

Choose a reason for hiding this comment

RustyYato Feb 8, 2021

Choose a reason for hiding this comment

ghost Mar 6, 2021

Choose a reason for hiding this comment

RustyYato Feb 8, 2021

Choose a reason for hiding this comment

RustyYato Feb 8, 2021

Choose a reason for hiding this comment

RustyYato Feb 8, 2021

Choose a reason for hiding this comment

RustyYato Feb 8, 2021

Choose a reason for hiding this comment

ghost commented Feb 8, 2021

RustyYato commented Feb 8, 2021

ghost commented Feb 8, 2021

RustyYato commented Feb 25, 2021

ghost commented Feb 25, 2021

RustyYato commented Feb 26, 2021

ghost commented Mar 6, 2021