frozenlib / parse-display

Procedural macro to implement Display and FromStr using common settings.
Apache License 2.0
182 stars 14 forks source link

Breaks under reexport #28

Open blueforesticarus opened 1 year ago

blueforesticarus commented 1 year ago

I'd like to use this crate via a re-export in my workspace's common crate. The code generated here refers to ::parse-display, which will only exist if the crate has parse-display added to its Cargo.toml.

In other crates like serde and strum it is possible to get around this with a #[serde(crate = "self::serde")]. It should also be possible to eliminate the need for said attribute using a method like this, though I don't fully understand it yet. Relevant code snippet

How strum implements it

frozenlib commented 1 year ago

I added support for the same method as strum and serde. (d4161dc17e499075051684e9fe62b10e23e18fef)

Regarding the method of making attributes unnecessary:

blueforesticarus commented 1 year ago

Thanks for responding so quickly.

There may be issues when using multiple versions of parse_display. I agree this is a concern. Specifically where

  1. crate is uses a reexport deps::parse-display::Display
  2. crate has parse-display in the Cargo.toml
  3. the version of parse-display in deps and the crate differ In that case crate_name() will see parse-display in the Cargo.toml and generated code ends up using the "wrong" version.

Could we put this behind a feature flag "reexport" with documentation pointing out this issue.

There is a issue in that it is necessary to write something like use deps::parse_display at the root of a crate that uses the re-exported #[derive(Display)], which makes the usage less simple.

I think this is not quite right. The linked example uses self::bytemuck, which should resolve to the current module scope not crate scope. It is debatable which is better. crate:: would allow writing use deps::* once at the crate root, but it may be less intuitive. self:: is requires a use deps::parse-display in every module where its used, but that is likely the case anyway. I think self:: is better.

blueforesticarus commented 1 year ago

If I understand right this will be the right way to do this once stabilized https://docs.rs/proc-macro2/latest/proc_macro2/struct.Span.html#method.def_site

EDIT: could this be used? https://docs.rs/proc-macro2/latest/proc_macro2/struct.Span.html#method.mixed_site

frozenlib commented 1 year ago
  • crate is uses a reexport deps::parse-display::Display
  • crate has parse-display in the Cargo.toml
  • the version of parse-display in deps and the crate differ In that case crate_name() will see parse-display in the Cargo.toml and generated code ends up using the "wrong" version.

Thanks for the explanation.

I was worried that changing the behavior depending on the future flag would cause problems when the flag was turned on by other crates, but this seems to work fine.

I think this is not quite right. The linked example uses self::bytemuck, which should resolve to the current module scope not crate scope. It is debatable which is better. crate:: would allow writing use deps::* once at the crate root, but it may be less intuitive. self:: is requires a use deps::parse-display in every module where its used, but that is likely the case anyway. I think self:: is better.

Indeed it is. I too find it more intuitive to use self:: instead of crate root.

Another option would be to give up on exporting the macro directly with pub use parse_display::Display, and create a new derive macro that generates code like #[derive(Display)] #[display(crate = ::exported_crate_name, ...)], and export that. This way, when you use the exported macros, you don't need use deps::*.

If I understand right this will be the right way to do this once stabilized https://docs.rs/proc-macro2/latest/proc_macro2/struct.Span.html#method.def_site

It would be best if we could do something like $crate in macro_rules using def_site().

I tried, but I could not use the crates listed in [dependeicies] of the crate that defines the macro from the code generated using def_site().

// sandbox_rust_macros/src/lib.rs
use proc_macro2::Span;
use quote::quote_spanned;

#[proc_macro]
pub fn ts_new(input: proc_macro::TokenStream) -> proc_macro::TokenStream {
    quote_spanned!(Span::def_site() => ::proc_macro2::TokenStream::new()).into()
}
// main.rs
fn main() {
    let x = sandbox_rust_macros::ts_new!();
}
error[E0433]: failed to resolve: could not find `proc_macro2` in the list of imported crates
 --> src\main.rs:2:13
  |
2 |     let x = sandbox_rust_macros::ts_new!();
  |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ could not find `proc_macro2` in the list of imported crates
  |

Is there a way to use def_site() to do something like $crate in macro_rules ?

blueforesticarus commented 1 year ago

To be honest I don't really understand def_site.

I was told I should keep an eye on this instead https://github.com/rust-lang/rust/issues/54363, sadly It doesn't look like its happening any time soon.

frozenlib commented 1 year ago

It seems to me that if the features suggested in the linked issue are available, the problem will be solved.

I am also disappointed that it is not scheduled to be available soon, but I would like to use it as soon as it becomes available.