Skip to content
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

Breaks under reexport #28

Open
blueforesticarus opened this issue Mar 20, 2023 · 6 comments
Open

Breaks under reexport #28

blueforesticarus opened this issue Mar 20, 2023 · 6 comments

Comments

@blueforesticarus
Copy link

blueforesticarus commented Mar 20, 2023

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
Copy link
Owner

frozenlib commented Mar 20, 2023

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

Regarding the method of making attributes unnecessary:

  • There may be issues when using multiple versions of parse_display.
  • 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.

@blueforesticarus
Copy link
Author

blueforesticarus commented Mar 20, 2023

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
Copy link
Author

blueforesticarus commented Mar 20, 2023

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
Copy link
Owner

  • 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
Copy link
Author

blueforesticarus commented Mar 21, 2023

To be honest I don't really understand def_site.

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

@frozenlib
Copy link
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants