elastio / bon

Generate builders for everything!
https://elastio.github.io/bon/
Apache License 2.0
995 stars 16 forks source link

Poor dev experience in Rust Rover #104

Closed Veetaha closed 1 week ago

Veetaha commented 1 week ago

I've got some feedback in https://github.com/ansg191/pandascore/pull/13#issuecomment-2327385592 about poor dev experience in Rust rover.

The syntax highlighting is off and go-to-definition seems to break. This needs to be researched. If nothing helps, we may consider having a derive version of the #[builder] specifically for structs.

A note for the community from the maintainers

Please vote on this issue by adding a 👍 reaction to help the maintainers with prioritizing it. You may add a comment describing your real use case related to this issue for us to better understand the problem domain.

ansg191 commented 1 week ago

So, I did some experimentation by reordering the position of the struct:

diff --git a/bon-macros/src/builder/item_struct.rs b/bon-macros/src/builder/item_struct.rs
index 8a5b98a..4211ee2 100644
--- a/bon-macros/src/builder/item_struct.rs
+++ b/bon-macros/src/builder/item_struct.rs
@@ -23,6 +23,7 @@ pub(crate) fn generate(
     let struct_ident = &adapted_struct.ident;

     Ok(quote! {
+        #adapted_struct
         #[automatically_derived]
         impl #generics_decl #struct_ident #generic_args
             #where_clause
@@ -31,6 +32,5 @@ pub(crate) fn generate(
         }

         #other_items
-        #adapted_struct
     })
 }

It seemed to make some difference.

Before:

image

After:

image

But it isn't perfect. The field attribute isn't highlighted and the usages hint is missing:

image

Though it seems that the usages hint is missing for pin_project as well:

image image
Veetaha commented 1 week ago

I see, the field attribute not highlighted is a tough problem because the macro just removes that attribute from the output, and it means these tokens just disappear for IDE (it doesn't know what they map to). I think some sort of highlighting is only possible with derive macros. There can be hacks like generating a dummy struct with a derive and mapping the input attributes there, but it's a hack.

Given that #[builder] supports impls and function items where there are also some helper attributes, that's one more problem to solve. Maybe just a hack is what this needs anyway.

Veetaha commented 1 week ago

I think at this point I've had enough evidence and headache with #68 to convince myself that derive API for structs is better than a #[builder], even though it breaks my ❤️ consistency within the bon API, it sticks to the consistency of using derive syntax for structs across the ecosystem. So now I'm inclined to expose a derive API and deprecate the usage of #[builder] on structs (it will still be supported, but it'll generate a deprecation warning): https://github.com/elastio/bon/issues/68#issuecomment-2327679151.

Veetaha commented 1 week ago

@ansg191 FYI, I have a working version of #[derive(bon::Builder)] on my branch. If you want to test it before the release, then see this comment: https://github.com/elastio/bon/issues/68#issuecomment-2330353117

ansg191 commented 1 week ago

The PR seems to work really well in my quick testing:

image

The struct fields and field attributes are highlighted, the usages hint shows and is correct, and the cfg_attr-disabled attributes are dimmed as well.

Veetaha commented 1 week ago

This issue has been closed by https://github.com/elastio/bon/pull/99. There I've introduced a new derive(bon::Builder) syntax that Rust Rover works much better with.

Right now the change is in master, and it's pending the 2.2 release of bon. I'll do that release tomorrow in the afternoon together with the blog post on Reddit which describes this change in detail.

Btw. I also created a Discord server for bon where you can leave any other feedback or questions you might have. Feel free to join!

Veetaha commented 1 week ago

This feature is now available in bon 2.2. Here is the release blog post on Reddit