Add an "impl_arbitrary" feature to enable implementing Arbitrary.

[sunfishcode]

Fixes #248.

[konsumlamm]

We might want to reexport arbitrary to avoid for example mod arbitrary {} breaking stuff (see also #247). This would require making arbitrary an optional dependency, adding pub extern crate arbitrary as _arbitrary (with a #[cfg]) and then implementing $crate::_arbitrary::Arbitrary<'a>. Then we should also add a test to tests/compile-pass/redefinition/.

[sunfishcode]

We might want to reexport arbitrary to avoid for example mod arbitrary {} breaking stuff (see also #247). This would require making arbitrary an optional dependency, adding pub extern crate arbitrary as _arbitrary (with a #[cfg]) and then implementing $crate::_arbitrary::Arbitrary<'a>. Then we should also add a test to tests/compile-pass/redefinition/.

I've now implemented this.

[konsumlamm]

I found an issue with the compile-pass test: trybuild apparently doesn't enable the features passed to cargo test, so the redefinition test doesn't get compiled. For some reason, the test doesn't seem to pass though, when extracting it to a separate file... I'm probably doing something wrong here.

EDIT: The test seems to work when adding extern crate arbitrary as a and use a::Arbitrary (this requires using a::Unstructured and putting #![cfg(feature = "arbitrary")] at the top level). So as a workaround, I'd suggest moving the test outside of compile-pass and adding the things I mentioned.

[sunfishcode]

I've now moved the test and made the changes you suggested.

[KodrAus]

Thanks for working on this @sunfishcode!

It looks like we're failing in CI on our no-std and MSRV targets, which pull in the example_generated feature. Based on rust-fuzz/arbitrary#74 it looks like no-std support for arbitrary isn't considered really desirable, but that should be ok. Any no-std consumers can conditionally enable arbitrary in bitflags when fuzzing.

Maybe what we could do is avoid adding arbitrary to the example_generated feature and instead include it in our docs.rs enabled features. That way we don't try build it in no-std or on targets without const generics but it'll still show up in published docs. Other targets will still test arbitrary in CI.

[sunfishcode]

Maybe what we could do is avoid adding arbitrary to the example_generated feature and instead include it in our docs.rs enabled features. That way we don't try build it in no-std or on targets without const generics but it'll still show up in published docs. Other targets will still test arbitrary in CI.

Sounds good; I've now implemented this.

[konsumlamm]

Sounds good; I've now implemented this.

Then we need to enable the arbitrary feature again in the CI.

[sunfishcode]

Enabled.

[sunfishcode]

Are there any other changes that would be helpful to make here?

[KodrAus]

Thanks for working on this @sunfishcode! 🙇 Sorry to shift the goalposts at the very end like this, but just before we merge this in, I'd like to make sure we've explored all the space and understand any kind of precedent we're setting. It might be nice to get some thoughts from @Manishearth or @fitzgen too.

In bitflags we should generally avoid adding foreign implementations automatically because they could conflict with any added manually in the calling crate. By doing this we're effectively saying never add a manual Arbitrary impl to a bitflags! type. As a counter-point, we don't automatically add Serialize and Deserialize through a serde feature. I do think that's a bit different though, because you may want a different serialized representation of your flags, or you may want to not support any serialization or deserialization at all.

For arbitrary, it's an interchange trait that services a specific niche, so I don't think there's a reason not to implement it, except if you're not fuzzing, in which case it doesn't matter if it's there anyway. There also isn't an obvious alternative useful implementation. So the alternative approach I can see to the one in this PR is if it were possible to do something like this with its #[derive(Arbitrary)]:

bitflags! {
    #[derive(Arbitrary)]
    // Would `IncorrectFormat` be a reasonable `with_optional_error` default so we could elide it?
    #[arbitrary(arbitrary_optional = "Flags::from_bits", arbitrary_optional_error = "arbitrary::Error::IncorrectFormat")]
    struct Flags: u32 {
        const A = 1;
        const B = 2;
        const C = 4;
        const D = 8;
    }
}

where arbitrary_optional accepts any:

fn arbitrary_optional<'a, A: Arbitrary<'a>, T>(input: A) -> Option<T>;

to make the signature of from_bits line up:

fn from_bits(bits: u32) -> Option<Flags>;

That's shifting the design questions from bitflags onto arbitrary. If that's something that's considered desirable I'd be happy to look at exploring it properly and implementing it in arbitrary. If it's not desirable then I think we can forge ahead with the approach in this PR, because it's at least consistent with how consumers will enable arbitrary support for other data-type libraries.

All that's to say I don't think we're blocked on merging this, but want to make sure we're totally comfortable with the approach (and with not accepting it as blanket precedent for an indefinite list of other traits that could be considered).

[Manishearth]

Unsure, that attribute doesn't really make sense to have in arbitrary either IMO: having an arbitrary_with to override fields is certainly useful, but for the whole type it feels like from arbitrary's POV it should just be "do a manual impl". I'm not totally against adding that to arbitrary, but it feels ... weird.

[fitzgen]

Yeah, having the Arbitrary impl derived automatically when the feature is enabled precludes manual implementation, and even worse can break manual implementations for crates that don't enable the feature when another crate in the dep graph turns it on.

Better to have it as something you opt into in the macro invocation, not just via cargo feature.

One reason I could imagine wanting to manually implement Arbitrary even if the cargo feature is enabled: maybe some bit flags are mutually exclusive with each other and so arbitrary bit patterns aren't valid instances of the type.

[KodrAus]

Hmm, bitflags doesn't currently have a concept of mutually exlusive flags. If you can poke at the flags type then you can freely combine any flags it exposes. That sounds like an assumption that fuzzing would do a great job of catching if it snuck in actually.

It might be best to hold off on this and look at other avenues. We could try poking into any #[derive] attributes that appear on the bitflags type, but that kind of thing gets tricky with macro_rules pretty fast. Maybe something like a #[bitflags] attribute would be best. So you could then opt-in to other useful traits that would otherwise conflict with manual implementations:

bitflags! {
    #[derive(Debug)] // standard derive
    #[bitflags::derive(TryFrom, Arbitrary)] // traits that can be derived knowing the target is a flags type
    pub struct MyFlags: i32 { .. }
}

We could even do that as an independent crate for a start.

[KodrAus]

Thanks again for working on this @sunfishcode! Now that #220 is merged we could look at a generic approach that could support building flags from any T: BitFlags where T::Bits: Arbitrary. This could either take the form of a bitflags_arbitrary crate, or as a custom derive target in a bitflags_derive crate.

We'll still have the papertrail from the original issue back to the implementation here for anybody that wanted to follow along.