-
-
Notifications
You must be signed in to change notification settings - Fork 22
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
Does not work with cfg_attr #68
Comments
Hi, thank you for creating the issue! Unfortunately, conditional compilation ( This is because the However, maybe things are different with Maybe there is a way to work around it, I just haven't found it. I'm not sure |
This comment was marked as off-topic.
This comment was marked as off-topic.
I've checked the docs a bit, and I'm almost sure a Maybe the way to do this for function syntax is to somehow hack it on top of a |
Having the For functions the regular attribute macro makes more sense. |
Yep, #[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. I'm not to strictly say that we can't have |
Note that for the code mentioned in the issue description, you could use the following workaround, while this problem exists in #[cfg_attr(feature = "bon", builder(on(String, into)))]
struct Test {
field: Option<String>,
} The type pattern specified in
The main idea is to avoid using member-level Some other ideas (mainly notes to myself) on how to solve this: use this StackOverflow approach of delivering info about the resolved values of |
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. 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. |
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. |
Great! Thank you for sharing it! ❤️ I'll keep your name engraved on the implementation 🐱 |
Some updates on this issue. I started working on this in #99. I improved the approach by using a single macro (not defining new |
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 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:
It won't be a rewrite of 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 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. |
I've implemented a 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 In case if you want to test it from my branch, here is how you can do this: Add this WIP version of [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:
|
Especially when using Personally I wouldnt start using a cli to migrate. Guess |
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 The workaround isn't that big of a deal. It's just this one simple bon/bon-macros/src/builder/mod.rs Lines 43 to 51 in e394a0f
So as I see the way forward is first the deprecation (usages of
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 |
This issue has been closed by #99. There I've introduced a new Note that I decided not to emit a deprecation warning for the usages of Right now the change is in master, and it's pending the 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! |
This feature is now available in |
Using version 2.0.0
results in:
doing that results in this error:
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.
The text was updated successfully, but these errors were encountered: