arbitrary icon indicating copy to clipboard operation
arbitrary copied to clipboard

Add `no_std` Support

Open bushrat011899 opened this issue 8 months ago • 15 comments

Objective

  • Fixes #208
  • Fixes #38
  • Supersedes #74
  • Supersedes #177

Solution

  • Added #![no_std] unconditionally (to ensure a consistent implicit prelude)
  • Added std and alloc features to conditionally enable access to the std and alloc crates respectively.
  • Added a new CI task check_no_std which checks for no_std compatibility using a target which does not have access to std (x86_64-unknown-none is chosen arbitrarily, any appropriate target could be used instead).
  • Added alloc_instead_of_core, std_instead_of_alloc, and std_instead_of_core Clippy lints to help maintain no_std compatibility.
  • Updated the derive macro to include a private extern crate std; statement, allowing compilation within no_std libraries on std-compatible targets.
  • Added a recursive_count feature to the main and derive crates, which allows using derive on all platforms.
  • Switched to alloc and core over std where possible.
  • Added appropriate feature gates based on the new features.
  • Gated imports of Arc behind target_has_atomic = "ptr", allowing compilation on atomically challenged platforms (typical for embedded).
  • Increased MSRV to 1.81 for better no_std support.

Migration Guide

MSRV Increased

The MSRV has increased from 1.63 to 1.81. If you are unable to upgrade your Rust compiler, consider pinning to an earlier version of arbitrary.

Existing Functionality Gated

If you have disabled default features but rely on functionality now gated behind the std and/or recursive_count features, enable them.

# Before
arbitrary = { version = "1.4.1", default-features = "false" }

# After
arbitrary = { version = "1.4.1", default-features = "false", features = ["std", "recursive_count"] }

Notes

I've been working on no_std support for the Bevy game engine and WGPU. arbitrary is used by WGPU and may become an issue for no_std efforts in the future. In general, I believe it's good practice to remove reliance on std when possible, even if it isn't strictly necessary (it has been noted that fuzzing requires substantially more effort without the standard library, so there is a reluctance to go to the trouble of maintaining this support). With the added lints, it is my experience that maintaining no_std support is of minimal concern even for maintainers without previous no_std exposure.

bushrat011899 avatar Apr 08 '25 04:04 bushrat011899

  • Updated the derive macro to include a private extern crate std; statement, allowing compilation within no_std libraries on std-compatible targets.

I'd rather this be explicit; either just have users get an error when they use the derive macro without std, or potentially have a default std feature that turns on the recursion counter.

Manishearth avatar Apr 08 '25 05:04 Manishearth

I'd rather this be explicit; either just have users get an error when they use the derive macro without std, or potentially have a default std feature that turns on the recursion counter.

I've added an explicitly named recursive_count feature to the derive crate, and then exposed it in the main. It is enabled by default in both crates. This allows using the derive macro on no_std targets, provided they aren't recursive. I had considered doing this originally but I wasn't sure how popular it would be.

bushrat011899 avatar Apr 08 '25 08:04 bushrat011899

I'd also wantthe other maintainers to look at this

Manishearth avatar Apr 08 '25 16:04 Manishearth

We should also bundle any other breaking changes we might want at the same time.

Notably, this seems like an opportunity to clean up the size hint methods.

(This PR is, ofc, not expected to do that, I'm just musing out loud.)

fitzgen avatar Apr 08 '25 22:04 fitzgen

LGTM but if we are going to do a breaking change, then we should just bump the MSRV and avoid the cargo features for core::error::Error and all that, IMO.

Perfectly happy to update this PR to just bump the MSRV. Just wasn't sure how controversial that would be. Would an MSRV of 1.81 be challenging for any of the major consumers of this crate? I know WGPU is now on 1.82 with blessing from Mozilla, but not sure about the others.

bushrat011899 avatar Apr 08 '25 22:04 bushrat011899

We should also bundle any other breaking changes we might want at the same time.

Filed https://github.com/rust-fuzz/arbitrary/issues/217 for this discussion.

fitzgen avatar Apr 08 '25 22:04 fitzgen

Perfectly happy to update this PR to just bump the MSRV.

We can't do it often, so if we are doing a breaking change anyways, we should just take it all the way to the latest stable.

fitzgen avatar Apr 08 '25 22:04 fitzgen

LGTM but if we are going to do a breaking change

n.b. splitting features whilst keeping them in default features is often not considered breaking since no-default-features users are not common and the ones that break can be fixed from afar. we may wish to consider doing that.

though as a dev tool there's not really a huge downside to a new major release. and we can have cargo fuzz print a warning asking people to upgrade arbitrary if we really want.

Manishearth avatar Apr 09 '25 00:04 Manishearth

IMO it would be fine to go all the way to the most recent version(s) too. If anybody has lower MSRVs they can hold off on updating arbitrary for a little while.

The more significant issue with a breaking change is that arbitrary is similar in principle to a foundational crate, meaning that the ecosystem will have to migrate quickly & at a similar time in order to make upgrading not painful (#[derive(Arbitrary)] is not going to work if your ADT contains a field that implements Arbitrary from the wrong version of the crate.) At the same time we could explore using the semver hack.

nagisa avatar Apr 09 '25 08:04 nagisa

Since arbitrary states it reserves the right to increase MSRV in minor releases, would it make sense to have this PR just go to the MSRV required to avoid new features (1.81)? If there's some functionality between 1.81 and 1.86 that other maintainers would like to use it might make more sense to update the MSRV further in a PR that actually needs it? Obviously a PR after this one but before the actual release could bump MSRV even further to stable if that's still desirable.

bushrat011899 avatar Apr 09 '25 10:04 bushrat011899

Interestingly, cargo-semver-checks considers this PR to not violate semantic versioning, despite increasing MSRV and gating existing functionality behind new features. I wouldn't read too much into that though, since I would consider increasing MSRV and such feature gating to be breaking changes regardless of what such a tool says.

bushrat011899 avatar Apr 09 '25 11:04 bushrat011899

The more significant issue with a breaking change is that arbitrary is similar in principle to a foundational crate, meaning that the ecosystem will have to migrate quickly & at a similar time in order to make upgrading not painful (#[derive(Arbitrary)] is not going to work if your ADT contains a field that implements Arbitrary from the wrong version of the crate.)

Agreed. Our ecosystem is obviously not as big as serde's but it is definitely intended that people can build libraries on top of arbitrary and stitch together multiple Arbitrary-implementing types from different libraries. We do this with wasm-smith and in Wasmtime, for example.

So I wouldn't say this is "just" a dev tool. (Where as libfuzzer-sys really is only used at the top level, and therefore really is just a dev tool, that we can be a little looser with breaking changes.)

That said, I don't think it is unreasonable to consider a breaking release since this PR is valuable, we have a few other things we'd like to do at the same time, and we aren't doing breaking changes frequently enough that we have used up all of our users patience and goodwill.

At the same time we could explore using the semver hack.

I personally would not do this work, but if someone else wanted to I'd be up for reviewing and merging it.

fitzgen avatar Apr 09 '25 15:04 fitzgen

since I would consider increasing MSRV and such feature gating to be breaking changes regardless of what such a tool says.

As highlighted, these are not things the ecosystem has broad agreement on as being breaking, and with the latest cargo resolver the MSRV thing is basically non-breaking.

(My position is that neither are breaking in a way that requires a major release)

Manishearth avatar Apr 09 '25 15:04 Manishearth

(My position is that neither are breaking in a way that requires a major release)

I think it is fuzzy and I don't necessarily disagree, but I think that since it already is a little fuzzy, and we have some API warts that we haven't otherwise had a chance to clean up, we might as well roll them all up together and do a breaking release.

Are you against doing that? Or would you prefer landing this as a non-breaking change first, and then doing the definitely-breaking changes in a new breaking release afterwards?

fitzgen avatar Apr 09 '25 15:04 fitzgen

I am not against doing a breaking release, I just wanted us to fully consider our options. If we have a bunch of breaking changes to bundle, let's do that.

Manishearth avatar Apr 09 '25 15:04 Manishearth