Proposal: Recommend cfg(fuzzing) and avoid cfg(feature = arbitrary).

[hoxxep]

Context

The Structure Aware Fuzzing section of the guide recommends importing the arbitrary crate as optional, and then using #[cfg(feature = "arbitrary)] throughout the code.

This seems juxtaposed to the recommendation in the #[cfg(fuzzing)] section in the Guide, which specifically hints at the benefit being:

... without the need for an externally visible Cargo feature that must be maintained throughout every dependency.

This issue proposes including a recommendation in the guide that crates should consistently stick to one approach or the other.

Example Issue

We are building a library crate example, and fuzzing it. The example crate includes quinn as a dependency, and so the simplified dependency tree on the example/fuzz crate is:

fuzz
└── example
    └── quinn
        └── quinn-proto

The quinn-proto sub-dependency is using a mix of #[cfg(feature = "arbitrary)] and #[cfg(fuzzing)]. As of version 0.11.0 of the crate, compiling example's fuzz crate under cfg(fuzzing) but not enabling the arbitrary feature on the sub-dependency will fail to compile the sub-dependency. This prevents our example crate from compiling, with a vague error use of undeclared crate or module 'arbitrary', which requires digging into the internals of a sub-dependency to understand and fix.

The workaround in this case is to explicitly enable the "arbitrary" feature on quinn-proto in the example crate's fuzz/Cargo.toml, even though it's not directly accessed and is a sub-sub-dependency of this crate:

quinn-proto = { features = ["arbitrary"] }

This is awkward because the user only finds out this is required with a compile time error. The error itself is unclear as it simply complains about a missing "arbitrary" crate. To fix this required digging into the sub-dependency and understanding how quinn-proto mixes the feature and the fuzzing cfg. One could argue it's an issue with quinn-proto for mixing them in incompatible ways, but I thought it could be helpful to reach a safer normalised approach in the guide.

Proposal

Libraries that implement fuzzing should only make use of #[cfg(fuzzing)] to enable/disable code paths and arbitrary derives, and avoid any extra features related to fuzzing.

Libraries should then include the arbitrary dependency via the target."cfg(fuzzing)" syntax as follows:

[target."cfg(fuzzing)".dependencies]
arbitrary = "*"

Everything else fuzzing/arbitrary related can consistently be gated by #[cfg(fuzzing)]:

#[cfg(fuzzing)]
use arbitrary::Arbitrary;

#[cfg(fuzzing)]
pub mod fuzzing {
    // ...
}

#[cfg_attr(fuzzing, derive(Arbitrary))]
struct Example {
    // ...
}

Some tweaks to the Structure Aware Fuzzing page should hopefully be enough.

Alternatives

One alternative could be for library crates to only make use of #[cfg(feature = "arbitrary")] and avoid mixing flags. Perhaps the feature approach is helpful if the arbitrary crate is used in ways other than fuzzing; or if importers would like to keep the ability to enable production codepaths in sub-dependencies even when the primary crate is being compiled for fuzzing.

Equivalent examples using a feature-only approach:

[dependencies]
arbitrary = { version = "*", optional = true}

[features]
fuzzing = ["arbitrary"]

#[cfg(feature = "fuzzing")]
use arbitrary::Arbitrary;

#[cfg(feature = "fuzzing")]
pub mod fuzzing {
    // ...
}

#[cfg_attr(feature = "fuzzing", derive(Arbitrary))]
struct Example {
    // ...
}

[fitzgen]

I don't really have an opinion on whether crates should do

#[cfg_attr(fuzzing, derive(Arbitrary))]

or

#[cfg_attr(feature = "arbitrary", derive(Arbitrary))]

but I do think it makes sense for crates to have arbitrary as an optional dependency so that they don't have to build crates that won't be used if they aren't fuzzing.

FWIW, sometimes people use arbitrary to generate a corpus of inputs for a benchmark or something as well, and aren't necessarily running under cargo fuzz.

So I guess I am soft leaning towards feature = "arbitrary".

[hoxxep]

Thanks for the swift response! Yeah I would be happy with either, as long as the guide makes it clear that we should be consistent, and possibly present both options on the same page.