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

Use cargo-semver-checks to make sure derive feature doesn't change API surface #422

Open
6 tasks
joshlf opened this issue Sep 27, 2023 · 1 comment
Open
6 tasks
Labels
experience-medium This issue is of medium difficulty, and requires some experience help wanted Extra attention is needed

Comments

@joshlf
Copy link
Member

joshlf commented Sep 27, 2023

Zerocopy comprises two crates: zerocopy and zerocopy-derive. Zerocopy has a disabled-by-default feature called derive. When it's enabled, zerocopy depends on zerocopy-derive and re-exports its derives so that users can just depend on zerocopy and don't have to also depend directly on zerocopy-derive.

Some of zerocopy's types implement the same traits that we provide derives for (currently, FromZeroes, FromBytes, AsBytes, and Unaligned). When the derive feature is enabled, we use the derives to derive implementations of those traits for our own types. When the derive feature is disabled, we implement those traits manually.

This creates a potential footgun: How do we know that the impls we emit when derive is enabled are identical to the impls that we emit when derive is disabled? Do they have the same bounds?

This issue tracks using the cargo-semver-checks crate to detect such mismatches for us. We already use cargo-semver-checks in CI:

# Check semver compatibility with the most recently-published version on
# crates.io. We do this in the matrix rather than in its own job so that it
# gets run on different targets. Some of our API is target-specific (e.g.,
# SIMD type impls), and so we need to run on each target.
#
# TODO(https://github.com/obi1kenobi/cargo-semver-checks-action/issues/54):
# Currently we don't actually do anything with `matrix.target`, so we're
# just duplicating work by running this job multiple times, each time
# targetting the host platform.
- name: Check semver compatibility
uses: obi1kenobi/cargo-semver-checks-action@v2
with:
# Don't semver check zerocopy-derive; as a proc macro, it doesn't have
# an API that cargo-semver-checks can understand.
package: zerocopy
feature-group: all-features
# TODO: Set this to the specific nightly we have pinned in CI. Not a big
# deal since this isn't affected by the trybuild stderr files, which is
# the reason we need to pin to a specific nightly.
rust-toolchain: nightly
if: matrix.crate == 'zerocopy' && matrix.features == '--all-features' && matrix.toolchain == 'nightly'

Specifically, this issue tracks using cargo-semver-checks to make sure that:

  • The derive feature is semver backwards-compatible with the absence of the derive feature
  • The absence of the derive feature is semver backwards-compatible with the derive feature

Combined, these two conditions should have the effect of ensuring that the APIs with and without derive are equivalent.

This issue tracks the following concrete tasks:

  • Using the technique described in this comment, update ci.yml's cargo-semver-checks use to check that:
    • The crate with the derive feature is semver backwards-compatible with the crate without the derive feature
    • The crate without the derive feature is semver backwards-compatible with the crate with the derive feature. Note that this case is extra tricky! The derive feature re-exports the derives, which means that the APIs are expected to differ! Specifically, we expect that the crate without derive will fail to be backwards-compatible because it has "removed" these exports compared to the crate with derive. You will need to do one of the following:
      • Figure out how to tell cargo-semver-checks to expect this difference, and allow it
      • Decide that there's no way to support this in cargo-semver-checks, and leave a comment in ci.yml explaining why we don't currently support this check
  • Intentionally push two PRs which include this technique and which update zerocopy's source code to violate compatibility:
    • One which makes a change which makes derive semver-incompatible with no-derive
    • One which makes a change which makes no-derive semver-incompatible with derive
@joshlf joshlf added help wanted Extra attention is needed experience-medium This issue is of medium difficulty, and requires some experience Hacktoberfest labels Sep 27, 2023
@Aditya-PS-05
Copy link
Contributor

Can you assign me this issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
experience-medium This issue is of medium difficulty, and requires some experience help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants