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

Make derive macros hygienic #11

Open
1 of 7 tasks
joshlf opened this issue Sep 19, 2022 · 6 comments
Open
1 of 7 tasks

Make derive macros hygienic #11

joshlf opened this issue Sep 19, 2022 · 6 comments
Labels
compatibility-nonbreaking Changes that are (likely to be) non-breaking experience-hard This issue is hard, and requires a lot of experience google-20%-project Potential 20% project for a Google employee help wanted Extra attention is needed

Comments

@joshlf
Copy link
Member

joshlf commented Sep 19, 2022

Status

  • core items are referenced as ::core::xxx rather than core::xxx
  • zerocopy-derive supports a #[zerocopy(crate = "...")] annotation
  • zerocopy-derive supports a #[zerocopy_rename] annotation
  • zerocopy-derive supports a #[zerocopy(crate = "...", derive-version = "...")] annotation

Crate name annotation

Sometimes, our derives will be used in a context in which zerocopy isn't called zerocopy. For example, this might happen if zerocopy itself is re-exported from another crate, or if a crate wishes to support deriving zerocopy traits from multiple zerocopy versions (see e.g. #557).

We can take inspiration from Serde, which defines the crate attribute option:

#[serde(crate = "...")]

In other words, we can do:

#[zerocopy(crate = "...")]

However, this isn't enough. For users who wish to invoke derives from multiple versions of zerocopy-derive at once, we need a way of disambiguating which attributes are meant to be consumed by which version of zerocopy-derive. We could provide a disambiguation option like so:

#[zerocopy(crate = "...", derive-version = "...")]

This would let us write code like the following (from #557):

#[cfg_attr(feature = "zerocopy_0_7", derive(zerocopy_0_7::FromBytes))]
#[cfg_attr(feature = "zerocopy_0_8", derive(zerocopy_0_8::FromBytes))]
#[zerocopy(crate = "zerocopy_0_7", derive-version = "0.7")]
#[zerocopy(crate = "zerocopy_0_8", derive-version = "0.8")]
struct Foo {
    ...
}

In this example, each zerocopy-derive would use derive-version to filter out attributes not meant for that version.

Simpler alternative

The above-described design is a bit complex, and requires users to add a new attribute for each zerocopy version. This is especially painful for crates like libc who will have hundreds of uses.

An alternative is to not permit the user to specify the name of the crate, but instead only support a single naming scheme:

#[cfg_attr(feature = "zerocopy_0_7", derive(zerocopy_0_7::FromBytes))]
#[cfg_attr(feature = "zerocopy_0_8", derive(zerocopy_0_8::FromBytes))]
#[zerocopy_rename]
struct Foo {
    ...
}

This has the same semantics as the preceding example, but it assumes that for zerocopy-derive 0.X.Y, the zerocopy crate is imported as zerocopy_X_Y, and for zerocopy-derive X.Y.Z, that zerocopy is imported as zerocopy_X.

Core re-export

We can't rely on core being in scope (or referring to the "real" core crate). However, we can rely on zerocopy being in scope (possibly renamed, as described above). If we re-export core from zerocopy, then we can emit code that doesn't refer to ::core, but instead refers to ::zerocopy::core_reexport.

Mentoring instructions

Interested in contributing? See our contributing guide.

This issue can be tackled in a number of discrete steps. Progress on any one of these would be fantastic, so don't feel like you need to tackle the whole thing!

  • Add support for a #[zerocopy(crate = "...")] annotation similar to serde's
  • Add support for a #[zerocopy_rename] annotation as described above
  • Add support for a #[zerocopy(crate = "...", derive-version = "...")] annotation as described above
joshlf added a commit that referenced this issue Aug 5, 2023
TODO:
- What does doing `::core` buy us over `core`?
- Doesn't seem like we can do `::zerocopy` since zerocopy itself
  defines a `zerocopy` module at the top level which is meant to
  mimic the `zerocopy` that the emitted code expects; however, are
  we losing anything by not doing `::zerocopy`?

Part of #11
@joshlf
Copy link
Member Author

joshlf commented Aug 8, 2023

@Kestrer, do you know if it's sufficient to instead emit ::zerocopy? My assumption is that, even if zerocopy is reexported by another crate, it still has to be available to rustc at compile time. I assume there's an issue with this approach since it's not the approach that Serde takes, but I'd be curious to know what the issue is.

Another approach that occurred to me - which I suspect also won't work for some reason - is to emit extern crate zerocopy in the generated code.

@joshlf
Copy link
Member Author

joshlf commented Aug 8, 2023

@djkoloski Flagging this in case you'd be interested in it - specifically the #[zerocopy(crate = "...")] annotation.

joshlf added a commit that referenced this issue Aug 8, 2023
joshlf added a commit that referenced this issue Aug 8, 2023
joshlf added a commit that referenced this issue Aug 8, 2023
@Kestrer
Copy link

Kestrer commented Aug 11, 2023

do you know if it's sufficient to instead emit ::zerocopy?

This should be the default option, since in most cases it works. However, it fails in the following cases:

  1. The user renames zerocopy to something else, e.g. if they do foobar = { package = "zerocopy", version = "0.6.3" } in their Cargo.toml
  2. The user does not depend on zerocopy itself, but rather imports it through some other crate — e.g. foobar has pub use zerocopy; in its crate root, and the user’s crate tries #[derive(foobar::zerocopy::AsBytes)].

Supporting these cases is the motivation for adding a #[zerocopy(crate = path::to::zerocopy)] annotation.

Another approach that occurred to me - which I suspect also won't work for some reason - is to emit extern crate zerocopy in the generated code.

I believe that also fails in the two cases listed above.

@joshlf joshlf added the compatibility-nonbreaking Changes that are (likely to be) non-breaking label Aug 12, 2023
This was referenced Aug 20, 2023
@joshlf joshlf added help wanted Extra attention is needed experience-hard This issue is hard, and requires a lot of experience labels Aug 28, 2023
ozaner added a commit to project-pku/zorua that referenced this issue Dec 10, 2023
Turns out `rkyv` is more for defining a new binary encoding scheme
rather than reusing an existing one (which is the main purpose of
zorua). So, replacing it with the `zerocopy` crate.
- Reexported items from the zerocopy crate for to/from bytes traits.
- Also reexported endian-aware int/float types via the byteorder
feature of zerocopy.
- Unfortunately, until google/zerocopy#11 is resolved, downstream must also import the zerocopy crate (ideally of the same minor version).
ozaner added a commit to project-pku/zorua that referenced this issue Dec 11, 2023
Turns out `rkyv` is more for defining a new binary encoding scheme
rather than reusing an existing one (which is the main purpose of
zorua). So, replacing it with the `zerocopy` crate.
- Reexported items from the zerocopy crate for to/from bytes traits.
- Also reexported endian-aware int/float types via the byteorder
feature of zerocopy.
- Unfortunately, until google/zerocopy#11 is resolved, downstream must also import the zerocopy crate (ideally of the same minor version).
ozaner added a commit to project-pku/zorua that referenced this issue Dec 11, 2023
Turns out `rkyv` is more for defining a new binary encoding scheme
rather than reusing an existing one (which is the main purpose of
zorua). So, replacing it with the `zerocopy` crate.
- Reexported items from the zerocopy crate for to/from bytes traits.
- Also reexported endian-aware int/float types via the byteorder
feature of zerocopy.
- Unfortunately, until google/zerocopy#11 is resolved, downstream must also import the zerocopy crate (ideally of the same minor version).
ozaner added a commit to project-pku/zorua that referenced this issue Jan 19, 2024
Turns out `rkyv` is more for defining a new binary encoding scheme
rather than reusing an existing one (which is the main purpose of
zorua). So, replacing it with the `zerocopy` crate.
- Reexported items from the zerocopy crate for to/from bytes traits.
- Also reexported endian-aware int/float types via the byteorder
feature of zerocopy.
- Unfortunately, until google/zerocopy#11 is resolved, downstream must also import the zerocopy crate (ideally of the same minor version).
joshlf added a commit that referenced this issue Feb 16, 2024
This has the effect of ensuring that derive-emitted code will fail to
compile if it spuriously relies on certain identifiers being in scope -
namely, identifiers which are part of the prelude.

Disabling the prelude surfaced a few bugs which are also fixed in this
commit.

Makes progress on #11
joshlf added a commit that referenced this issue Feb 16, 2024
This has the effect of ensuring that derive-emitted code will fail to
compile if it spuriously relies on certain identifiers being in scope -
namely, identifiers which are part of the prelude.

Disabling the prelude surfaced a few bugs which are also fixed in this
commit.

Makes progress on #11
joshlf added a commit that referenced this issue Feb 16, 2024
This has the effect of ensuring that derive-emitted code will fail to
compile if it spuriously relies on certain identifiers being in scope -
namely, identifiers which are part of the prelude.

Disabling the prelude surfaced a few bugs which are also fixed in this
commit.

Makes progress on #11
jswrenn pushed a commit that referenced this issue Feb 16, 2024
This has the effect of ensuring that derive-emitted code will fail to
compile if it spuriously relies on certain identifiers being in scope -
namely, identifiers which are part of the prelude.

Disabling the prelude surfaced a few bugs which are also fixed in this
commit.

Makes progress on #11
jswrenn pushed a commit that referenced this issue Feb 16, 2024
This has the effect of ensuring that derive-emitted code will fail to
compile if it spuriously relies on certain identifiers being in scope -
namely, identifiers which are part of the prelude.

Disabling the prelude surfaced a few bugs which are also fixed in this
commit.

Makes progress on #11
jswrenn pushed a commit that referenced this issue Feb 16, 2024
This has the effect of ensuring that derive-emitted code will fail to
compile if it spuriously relies on certain identifiers being in scope -
namely, identifiers which are part of the prelude.

Disabling the prelude surfaced a few bugs which are also fixed in this
commit.

Makes progress on #11
github-merge-queue bot pushed a commit that referenced this issue Feb 16, 2024
This has the effect of ensuring that derive-emitted code will fail to
compile if it spuriously relies on certain identifiers being in scope -
namely, identifiers which are part of the prelude.

Disabling the prelude surfaced a few bugs which are also fixed in this
commit.

Makes progress on #11
dorryspears pushed a commit to dorryspears/zerocopy that referenced this issue Feb 20, 2024
This has the effect of ensuring that derive-emitted code will fail to
compile if it spuriously relies on certain identifiers being in scope -
namely, identifiers which are part of the prelude.

Disabling the prelude surfaced a few bugs which are also fixed in this
commit.

Makes progress on google#11
ozaner added a commit to project-pku/zorua that referenced this issue Apr 24, 2024
Turns out `rkyv` is more for defining a new binary encoding scheme
rather than reusing an existing one (which is the main purpose of
zorua). So, replacing it with the `zerocopy` crate.
- Reexported items from the zerocopy crate for to/from bytes traits.
- Also reexported endian-aware int/float types via the byteorder
feature of zerocopy.
- Unfortunately, until google/zerocopy#11 is resolved, downstream must
also import the zerocopy crate (ideally of the same minor version).
ozaner pushed a commit to project-pku/zorua that referenced this issue Apr 24, 2024
Turns out `rkyv` is more for defining a new binary encoding scheme
rather than reusing an existing one (which is the main purpose of
zorua). So, replacing it with the `zerocopy` crate.
- Reexported items from the zerocopy crate for to/from bytes traits.
- Also reexported endian-aware int/float types via the byteorder
feature of zerocopy.
- Unfortunately, until google/zerocopy#11 is resolved, downstream must
also import the zerocopy crate (ideally of the same minor version).
@joshlf joshlf added the google-20%-project Potential 20% project for a Google employee label May 7, 2024
@ljtpetersen
Copy link

do you know if it's sufficient to instead emit ::zerocopy?

This should be the default option, since in most cases it works. However, it fails in the following cases:

1. The user renames `zerocopy` to something else, e.g. if they do `foobar = { package = "zerocopy", version = "0.6.3" }` in their `Cargo.toml`

2. The user does not depend on `zerocopy` itself, but rather imports it through some other crate — e.g. `foobar` has `pub use zerocopy;` in its crate root, and the user’s crate tries `#[derive(foobar::zerocopy::AsBytes)]`.

Supporting these cases is the motivation for adding a #[zerocopy(crate = path::to::zerocopy)] annotation.

Another approach that occurred to me - which I suspect also won't work for some reason - is to emit extern crate zerocopy in the generated code.

I believe that also fails in the two cases listed above.

I am not sure what constraints you have on dependencies, but there is a crate, proc-macro-crate that pulls the name of the crate by parsing Cargo.toml as found in CARGO_MANIFEST_DIR. Would this solution work, or a re-implementation of it?

@djkoloski
Copy link
Member

rkyv also has a crate = .. attribute for these reasons. I think the less clever the better, especially because zerocopy is used by Fuchsia which does not use cargo to build.

@joshlf
Copy link
Member Author

joshlf commented Oct 10, 2024

Merging #619 (authored by @jswrenn) into this


There are a number of high-profile crates in the Rust ecosystem that define types whose memory representation is part of their documented contract. By implementing zerocopy's traits for these types, their users could benefit from zerocopy's compiler-time checks that operations depending on these contracts are sound. But: Where should these trait implementations go?

It is risky for zerocopy to provide trait implementations on foreign types, since zerocopy cannot check the soundness of its implementations on foreign types. If zerocopy's trait implementations implementations on foreign types fell out of sync with the definitions of those types, unsoundness would ensue.

It is strictly safer for third-party types to provide their own implementations of zerocopy's traits, using zerocopy's derive feature. However, zerocopy is still a crate in active development, with occasional breaking changes. For greater interoperability in the ecosystem, we would like our public customers to track zerocopy's latest version. To do so, customers today must either:

  1. document their zerocopy support as unstable
  2. be willing to release new breaking changes of their crate, in step with zerocopy

Both of these options adds friction to the adoption of zerocopy in the Rust ecosystem.

Proposed Solution

There exists a third not-yet-quite-possible-today option: Provided multi-versioned support for zerocopy.

A customer taking this approach would specify a new optional dependency for each version of zerocopy they support; e.g., they would start with:

[dependencies]
zerocopy_0_7 = {  package = "zerocopy", version = "0.7", optional = true, features = ["derive"] }

...and provide feature-gated implementations for that version:

#[cfg_attr(
    feature = "zerocopy_0_7",
    derive(
        zerocopy_0_7::FromZeroes,
        zerocopy_0_7::FromBytes,
        zerocopy_0_7::AsBytes
    ),
    zerocopy_0_7(root = ::zerocopy_0_7),
)]
#[repr(C)]
pub struct sembuf {
    pub sem_num: c_ushort,
    pub sem_op: c_short,
    pub sem_flg: c_short,
}

Upon a new major release of zerocopy, the customer could append a new version to their Cargo.toml:

  [dependencies]
  zerocopy_0_7 = {  package = "zerocopy", version = "0.7", optional = true, features = ["derive"] }
+ zerocopy_0_8 = {  package = "zerocopy", version = "0.8", optional = true, features = ["derive"] }

...and add new, corresponding feature-gated derives:

  #[cfg_attr(
      feature = "zerocopy_0_7",
      derive(
          zerocopy_0_7::FromZeroes,
          zerocopy_0_7::FromBytes,
          zerocopy_0_7::AsBytes
      ),
  )]
+ #[cfg_attr(
+     feature = "zerocopy_0_8",
+     derive(
+         zerocopy_0_8::KnownLayout,
+         zerocopy_0_8::TryFromBytes,
+         zerocopy_0_8::FromZeroes,
+         zerocopy_0_8::FromBytes,
+         zerocopy_0_8::AsBytes
+     ),
+ )]
  #[repr(C)]
  pub struct sembuf {
      pub sem_num: c_ushort,
      pub sem_op: c_short,
      pub sem_flg: c_short,
  }

Technical Details

Presently, zerocopy's derives unconditionally expand to code referencing the path ::zerocopy. This makes depending on multiple versions of zerocopy presently impossible, since they cannot all share that path. To support this approach, zerocopy can make one of two technical changes:

1. Root-setting helper attribute.

Zerocopy could provide a helper attribute for setting its crate root; e.g.:

#[cfg_attr(
    feature = "zerocopy_0_7",
    derive(
        zerocopy_0_7::FromZeroes,
        zerocopy_0_7::FromBytes,
        zerocopy_0_7::AsBytes
    ),
    zerocopy_0_7(crate = ::zerocopy_0_7), // <--- this
)]

The attribute would instruct zerocopy's derives to expand to paths beginning with ::zerocopy_0_7, instead of ::zerocopy.

The drawback of this approach is that this attribute adds line noise to critical derives.

2. Re-export old versions

Alternatively, zerocopy could optionally depend on older versions of itself:

[dependencies]
# ...
v_0_6 = {  package = "zerocopy", version = "0.6", optional = true }
# ...

...and provide versioned re-exports of itself at its root:

#[doc(hidden)]
mod version {
    pub use super:: as v0_7;

    #[cfg(feature = "v_0_6")]
    pub use ::v_0_6;
}

We would then modify zerocopy's derives to always expand to paths beginning with ::zerocopy::version::v_XXX.

Unresolved question: In this approach, how would we populate the zerocopy path on the consumer side?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compatibility-nonbreaking Changes that are (likely to be) non-breaking experience-hard This issue is hard, and requires a lot of experience google-20%-project Potential 20% project for a Google employee help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

4 participants