Does not work with cfg_attr

novacrazy commented 2 weeks ago

Using version 2.0.0

#[cfg_attr(feature = "bon", bon::builder)]
struct Test {
    #[cfg_attr(feature = "bon", builder(into))]
    field: Option<String>,
}

results in:

error: cannot find attribute `builder` in this scope 
   --> src/main.rs:386:33
    |
386 |     #[cfg_attr(feature = "bon", builder(into))]
    |                                 ^^^^^^^        
    |
help: consider importing this attribute macro
    |
382 + use bon::builder;

doing that results in this error:

error: expected non-macro attribute, found attribute macro `builder`
   --> src/main.rs:388:33
    |
388 |     #[cfg_attr(feature = "bon", builder(into))]
    |                                 ^^^^^^^ not a non-macro attribute

ignore the line numbers, it's just from a large test file.

Perhaps a #[derive(bon::Builder)] would work better 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.

Veetaha commented 2 weeks ago

Hi, thank you for creating the issue! Unfortunately, conditional compilation (#[cfg(...)]) of members is currently not possible with bon as described in the limitations due to the lack of rust-lang/rust/#115590.

This is because the finish_fn (i.e. build() or call()) needs to have a conditional where bound for the members where it allows you to transition to the terminal builder state only when all members are set.

However, maybe things are different with cfg_attr 🤔 .

Maybe there is a way to work around it, I just haven't found it. I'm not sure derive() syntax would change anything, but I'll try to check. If it fixes the situation, then bon::builder could internally use a derive(), but this won't solve the problem for function syntax, because one can't place #[derive()] on top of functions.

Veetaha commented 2 weeks ago

Ah, looks like the bug label attracts malware bot spammers 🫤

Veetaha commented 2 weeks ago

I've checked the docs a bit, and I'm almost sure a derive() is what's going to fix this for struct syntax. I can use a derive() internally in the code expanded by #[builder] to fix this for structs (which is going to require a good amount of rewrite to be fair), but I still need to find a way to fix it for function syntax.

Maybe the way to do this for function syntax is to somehow hack it on top of a derive as well. Need to think about this more.

novacrazy commented 2 weeks ago

Having the #[bon::builder] macro simply apply a new #[derive(bon::Builder)] (note the capitalization) seems like a reasonable backwards-compatible change. For this kind of functionality it's very common in the Rust ecosystem to use a derive-trait/pseudo-trait instead of an attribute macro anyway, so #[derive(bon::Builder)] feels more natural.

For functions the regular attribute macro makes more sense.

Veetaha commented 2 weeks ago

Yep, derive()'s more common, but I wanted to have a reusable single macro for both functions and structs. There is basically a single macro attribute parsing impl internally between structs and fns in bon thanks to this right now. Also, it's a bit more convenient to write

#[builder(some_config_attrs_here)]
struct Foo { }

rather than

#[derive(bon::Builder)]
#[builder(some_config_attrs_here)]
struct Foo { }

Another reason's that regular proc-macro-attributes are a bit more powerful, since they allow mutating the struct/enum they are placed on. bon doesn't do that right now, but this potential is nice to have. Therefore, I landed on a single #[builder] API.

I'm not to strictly say that we can't have derive(bon::Builder) exposed as well, but yeah, it's possible to just use it in the expanded code internally (given it's marked with #[doc(hidden)]).

Veetaha commented 2 weeks ago

Note that for the code mentioned in the issue description, you could use the following workaround, while this problem exists in bon. If you need to configure only into at fields level, you can configure it for all fields of type String at once this way using only the macro only at the top-level like this, which should avoid the compile error:

#[cfg_attr(feature = "bon", builder(on(String, into)))]
struct Test {
    field: Option<String>,
}

The type pattern specified in on() matches the inner type of Option<T> fields. It's worded in the docs as:

For optional members the underlying type is matched ignoring the Option wrapper.

The main idea is to avoid using member-level #[cfg_attr(..., builder())] if possible. This is what's triggering the problem. Using #[cfg_attr(..., builder)] at the top level works fine.\

Some other ideas (mainly notes to myself) on how to solve this: use this StackOverflow approach of delivering info about the resolved values of cfg/cfg_attr expressions to the macro by using some recursion

Veetaha commented 2 weeks ago

I found this post from @recatek on Rust forum, that suggests a particular pattern of "macro induction chain" to resolve the CFG attributes from proc macros. The solution looks rather obvious to me. ~~However, here is a question to @recatek (🙏), I wonder if it was implemented in some public repository, that could be used as a reference to speed up this implementation in bon.~~

UPD: I found the implementation here: https://github.com/recatek/gecs/blob/e55af6f882905204cb7cbecd7ca27383029972b6/macros/src/generate/cfg.rs#L6-L51

In general, though, I think it could even be a separate crate e.g. expand-cfg to assist in expanding the cfg/cfg_attr attributes for proc macros. Although, I'm not sure I want to extract this into a separate crate at least initially.

recatek commented 2 weeks ago

Hello! Yes, a generalized crate for this technique would be neat. It certainly isn't the cleanest thing in the world, and probably isn't great for compile times, but it does work and does give your proc macro an awareness of what cfg flags are available in your invoking environment.

Veetaha commented 2 weeks ago

Great! Thank you for sharing it! :heart: I'll keep your name engraved on the implementation :cat:

Veetaha commented 1 week ago

Some updates on this issue. I started working on this in https://github.com/elastio/bon/pull/99. I improved the approach by using a single macro (not defining new macro_rules! in the expansion) that just internally uses recursion. The PR is still WIP and doesn't compile yet, but I'm positive it'll be merged and released this week.

Veetaha commented 1 week ago

So.. I've basically reinvented the cfg/cfg_attr myself. And it works, but.. the IDE experience isn't the best. The syntax highlighting doesn't make it clear that the code is disabled. I also figured out that Rust Rover in particular sucks at support for proc macro attributes (#104), and it caused people to stop using bon.

Therefore I'm considering adding the derive variant of the API for structs specifically. Most IDEs support derive macros much better, and it's anyway a more common pattern of usage for developers.

So my next steps will be:

Add bon::Builder derive macro as a new public API
Change the #[builder] macro to expand to #[derive(bon::Builder)] itnernally, or just share code between both of them.
Usages of #[builder] on structs will generate a deprecation warning (pending the next major release to stop supporting this, which will happen... hopefully never, but never say never).

It won't be a rewrite of bon. Derive macros are very similar to proc macro attributes. It'll be a medium sized internal code change (I was too pessimistic on my initial estimate) that I can probably manage to do and release this week.

I also wonder what @EdJoPaTo thinks about this? I think you also said you'd prefer a derive variant.

Note that it doesn't change the fact that #[builder] support still stays on functions and functions inside of impl blocks as the single way to do that.

I'll use my custom implementation of cfg/cfg_attr expansion logic there to support conditional compilation. I think that's fine to use it for fn syntax because it's very rare that people would want to conditionally compile function arguments, although time will tell. Anyway conditional compilation with function syntax will be supported but with a hack that I already developed (manual cfg/cfg_attr evaluation)

Btw, using the derive syntax should slightly improve the performance of macro expansion. I bet rustc is more optimized for derive syntax because with derive syntax the struct the macro is placed on is guaranteed to stay the same, so no re-parsing of the struct should happen.

Veetaha commented 1 week ago

I've implemented a #[derive(bon::Builder)] version in my PR: https://github.com/elastio/bon/pull/99, which was an embarrasingly small amount of work to do.

I even added a CLI that assists in migration, it uses rust-analyzer's API to parse all rust files it can find and replaces the occurences of #[builder] on structs with a #[derive(Builder)] in your code.

In case if you want to test it from my branch, here is how you can do this:

Add this WIP version of bon into your Cargo.toml

[dependencies]
bon = { git = "https://github.com/elastio/bon", branch = "feat/support-cfg-attrs" }

Install and run the migration script:

cargo install --git https://github.com/elastio/bon --branch feat/support-cfg-attrs bon-cli
bon migrate

# Prettify just in case
cargo fmt

There is still a bunch of chores left to before the release:

[ ] Update all the docs, maybe add new docs
[x] Add more regression tests for #[builder] syntax on struct (it's still supported although deprecated)
[ ] Make a blog post for the new update with explanations

EdJoPaTo commented 1 week ago

I also wonder what @EdJoPaTo thinks about this? I think you also said you'd prefer a derive variant.

Especially when using #[builder(into) on the elements on the struct directly the derive is way more similar to known, big crates like for example serde. So I would prefer it even when the cfg_attr stuff isn't needed.

Personally I wouldnt start using a cli to migrate. Guess frankenstein is quite a big example, but for other projects it would be just a few lines to change so quite easy to do manually. The bigger question this change raises for me: Is the deprecation worth it? Is a breaking release just simpler and getting rid of workarounds in the code to keep the old logic working?

Veetaha commented 1 week ago

Is a breaking release just simpler and getting rid of workarounds in the code to keep the old logic working?

I'd always go for a compatible release if possible, and minimize the number of major versions. This way, for example, existing crates that use the #[builder] syntax still compile with the new version of bon, and cargo doesn't need to keep two versions of bon in your dependency tree if one of your dependencies uses the older major version of bon, and you use the latest version of bon directly for your needs as well.

The workaround isn't that big of a deal. It's just this one simple if: https://github.com/elastio/bon/blob/e394a0f52469fdd677c9236478d8c07f9ab2c2f6/bon-macros/src/builder/mod.rs#L43-L51

So as I see the way forward is first the deprecation (usages of #[builder] on struct produce a warning), and then the removal of the ability to place #[builder] on structs, which will happen once we have a really good reason to do a new major release. I don't think the deprecation of #[builder] on a struct is that good reason yet. For example the removal of automatic into was such a good reason because it couldn't be done in a compatible way. So I propose to do a deprecation cycle to allow people to adapt to the new changes without big disruption and wait for another chance to do a major release to remove this completely.

Personally I wouldnt start using a cli to migrate.

Sure, the automated migration with the CLI is just an optional convenience. I bet some people will use it to just save some time of mechanically moving from one syntax to the other. If zero people use that CLI and manually update instead, that's also fine with me. That CLI is just a technical byproduct of bon's development lifecycle, that I'd rather never have, but it may come in handy for any other future changes.

Veetaha commented 6 days 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 supports cfg/cfg_attr natively. Also, if you want to do conditional compilation with impl/fn syntax with #[builder] that is supported too. There are some long-to-explain macro hacks that make it work for the #[builder] proc-macro attribute.

Note that I decided not to emit a deprecation warning for the usages of #[builder] macro on top of the structs just yet. I'm planning to give anyone who will read my Reddit post some time to upgrade without any disruption and then I'll enable the deprecation later (in a subsequent minor release 2.3 in a week or two).

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 5 days ago

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

elastio / bon

Does not work with cfg_attr #68

A note for the community from the maintainers