pacak / bpaf

Command line parser with applicative interface
337 stars 17 forks source link

bpaf_derive: allow specification of a custom crate path to resolve proc_macro hygiene related issues that might develop in some situations #332

Closed bzm3r closed 6 months ago

bzm3r commented 7 months ago

Suppose that we have a crate called indirector.

indirector publicly re-exports bpaf:

pub use bpaf;
pub use bpaf::Bpaf;

Then, we have a crate called user, which imports indirector, and uses bpaf through it:

use indirector::bpaf::{Parser, Bpaf};

This will cause a hygiene related issue, because currently bpaf_derive assumes that bpaf is available globally (via ::bpaf) in line with the official recommendations.

There was a discussion on how to address this sort of issue, which lead me to experiment with the two solutions listed there:

So I implemented a simple version of it. I tried to keep the changes as small as possible, but was forced to increase how large a change was made due to the realities of how use some::path is handled in syn.

I have included one simple test, and have also tested it out in the use case where I first ran into this issue.

pacak commented 7 months ago

I did consider this scenario a while ago and decided not to do anything about it back then. Reexports make sense if you have a library and and you want your users to derive some implementations using some specific version of a third crate, usually because that's what your library expects to consume. With bpaf, at least when used the intended way, consumer is usually a crate with a binary and authors of that library tend to control all the places where instances can be derived. That said this MR adds some flexibility to how it can be used and doesn't add any external dependencies so I don't mind merging this.

Any ideas how much compilation time this adds?

bzm3r commented 7 months ago

I did consider this scenario a while ago and decided not to do anything about it back then. Reexports make sense if you have a library and and you want your users to derive some implementations using some specific version of a third crate, usually because that's what your library expects to consume. With bpaf, at least when used the intended way, consumer is usually a crate with a binary and authors of that library tend to control all the places where instances can be derived. That said this MR adds some flexibility to how it can be used and doesn't add any external dependencies so I don't mind merging this.

Any ideas how much compilation time this adds?

Sorry for taking a while to respond to this. It was not because I was ignoring it, but rather because I fell into a rabbit hole of trying to be accurate.

Long story short, I couldn't detect significant differences between compilation time, beyond what seems to be natural variation between build runs. I figured it might be due to my compilation setup (sccache + mold), but it seemed to be insensitive to that. Then I fell into the rabbit hole of performing many build runs to create a statistical comparison (that work is about 80% done), but then I fell into a side-rabbit-hole related to Nix/NixOS and how I'm setting up the Rust dev environments...

:facepalm:

So it's a work in progress, and a task that is taking longer than intended. Perhaps there might be some future benefit to it, but likely I just got distracted...there's probably already a good statistical tool for measuring build differences, now that I think about it...probably some variant of criterion...so my rabbit hole is also likely ultimately pointless. I guess I did learn about cargo build --timings....

Sigh, but for now, I guess, the short answer is: I couldn't detect any differences, even after being careful with cleaning previous build artifacts.

pacak commented 7 months ago

:) Well, good to hear that. To get this merged we need to rebase, make rustfmt happy and briefly mention it somewhere in the documentation, maybe even add an example info /examples folder:

// this tells the derive macro that bpaf library  is available at a custom path, here it's
// the same as default, but you can specify your own if you are importing the crate 
// under some other name or reexporting it from elsewhere, for example
// `::bpaf_0_8` or `::indirector::bpaf` could also be valid paths
#[bpaf(options, bpaf_path = ::bpaf)]
...
pacak commented 7 months ago

rustfmt still fails - running cargo fmt and committing the updated code should do the trick.

pacak commented 6 months ago

I merged the change but replaced #[bpaf(..., bpaf_path = xxx)] with #[bpaf(..., path(xxx))] mostly to keep things uniform with other annotations. Will try to release 0.9.10 soon-ish after cleaning up the documentation and a few other things.