Skip to content

Add no_std Support #216

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Conversation

bushrat011899
Copy link

@bushrat011899 bushrat011899 commented Apr 8, 2025

Objective

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.

@Manishearth
Copy link
Member

  • 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.

@bushrat011899
Copy link
Author

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.

@Manishearth Manishearth requested review from fitzgen and nagisa April 8, 2025 16:06
@Manishearth
Copy link
Member

I'd also wantthe other maintainers to look at this

Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

@fitzgen
Copy link
Member

fitzgen commented Apr 8, 2025

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.)

@bushrat011899
Copy link
Author

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.

@fitzgen
Copy link
Member

fitzgen commented Apr 8, 2025

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

Filed #217 for this discussion.

@fitzgen
Copy link
Member

fitzgen commented Apr 8, 2025

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.

@Manishearth
Copy link
Member

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.

@nagisa
Copy link
Member

nagisa commented Apr 9, 2025

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.

@bushrat011899
Copy link
Author

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

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.

@fitzgen
Copy link
Member

fitzgen commented Apr 9, 2025

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.

@Manishearth
Copy link
Member

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)

@fitzgen
Copy link
Member

fitzgen commented Apr 9, 2025

(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?

@Manishearth
Copy link
Member

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.

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

Successfully merging this pull request may close these issues.

Failure to build 1.4.1 on existing projects that were using 1.3.2 #![no_std] Support
4 participants