Lokathor / bytemuck

A crate for mucking around with piles of bytes
https://docs.rs/bytemuck
Apache License 2.0
697 stars 77 forks source link

Can't use derive macros when bytemuck is re-exported from another crate. #159

Open GlennFolker opened 1 year ago

GlennFolker commented 1 year ago
error[E0433]: failed to resolve: could not find `bytemuck` in the list of imported crates
  --> crates\avocado_winit\src\render\graph.rs:65:31
   |
65 |         #[derive(Copy, Clone, Pod, Zeroable)]
   |                               ^^^ could not find `bytemuck` in the list of imported crates
   |
   = note: this error originates in the derive macro `Pod` (in Nightly builds, run with -Z macro-backtrace for more info)

This is probably because bytemuck's proc macro crate uses ::bytemuck::* instead of bytemuck::* here.

Lokathor commented 1 year ago

Using the :: prefix is the standard way to write proc-macros that impl unsafe traits. I asked on the discord and others agreed that it should stay.

If an alternative prefix is desired to be supported we'd have to expand the derive to accept a dummy attribute letting you specify the prefix, or something like that.

In general, proc-macros just aren't the best with re-exports.

elast0ny commented 1 year ago

I've been running into this issue also 👍

Trying to reduce the number of duplicated dependencies in a project becomes much harder if bytemuck doesn't support being re-exported. I like your idea of adding another derive attribute so users could provide the path to bytemuck if they want.

leod commented 1 year ago

I need this in order to use #[derive(Pod)] on structs that are generated in a derive crate of mine, without the user of that derive crate having to add bytemuck as a dependency themselves. I'll admit that this is a rare situation as far as usages of bytemuck go.

I have a branch of bytemuck that seems to do the trick: https://github.com/Lokathor/bytemuck/compare/main...leod:bytemuck:specify_crate_in_derive. It follows the approach suggested by @Lokathor. If folks agree that this is worth upstreaming, I'd be happy to clean it up (and maybe rename the attribute?) and create a PR.

Example usage:

mod custom_mod {
  pub use ::bytemuck;
}

#[derive(Copy, Clone, Pod, Zeroable)]
#[repr(C)]
#[bytemuck_crate(custom_mod::bytemuck)]
struct TestWithCrateAttribute {
  a: u16,
  b: u16,
}

FWIW, as @Lokathor pointed out, this is a general issue with derive macros. I wonder if there already is a pattern recognized by the Rust community that we could follow here.

richardpringle commented 1 month ago

This is solved for Pod and Zeroable. Seems like it's still an issue for NoUninit

richardpringle commented 1 month ago

This is solved for Pod and Zeroable. Seems like it's still an issue for NoUninit

PR here https://github.com/Lokathor/bytemuck/pull/259

Can this issue be closed though? Or should it stay open until the other macros can do the same thing? In the above PR (at least at the time of writing), I added the bytemuck attribute to NoUninit. I'm pretty sure it could be added to the others and it should just work everywhere.

Not sure if that's desired or not.

richardpringle commented 1 month ago

@Lokathor, any chance we could get a patch release of bytemuck_derive? And just curious, is there a reason bytemuck is lagging behind on its version of bytemuck_derive?

Lokathor commented 1 month ago

1) yes, I'll try to remember to do this later today. Unfortunately I haven't set things up on my new phone to be able to do rust on my phone, so it'll have to be later today.

2) regarding the version, my understanding has been that, since cargo will use the latest semver compatible version of all dependencies, bytemuck doesn't need to track the actual latest version of bytemuck_derive itself with every new bytemuck_derive release. Simply depending on the minimum version that's compatible is enough, and cargo will still select the newest version of bytemuck_derive when doing builds.

Lokathor commented 1 month ago

Released bytemuck_derive-v1.7.1