Add Arbitrary Trait To RecordBuilder

[cukie]

This is an optional dependency, enabled with cargo flag --feature arbitrary,std that allows for fuzzing of loggers with much less boilerplate than was previously required.

An example of usage would be

use log::RecordBuilder;
use arbitrary::Arbitrary;
use libfuzzer::
use libfuzzer_sys::fuzz_target;

#[derive(Arbitrary, Debug)]
struct FuzzerInput {
  builder: RecordBuilder,
  message: String,
}

fuzz_target!(|input: &FuzzerInput| {
  let logger = MyLogger();
  logger.log(&input.builder.args(format_args!("{}", message)).build())
});

Note that if there is ever progress on a more flexible format_args! as described in rust-lang/rust#92698 (reference) then this could be simplified even further and we could provide an Arbitrary implementation of Record itself.

[Thomasdezeeuw]

Personally I don't think the log crate should add any dependencies, even optional ones. The implementation seems pretty small, so it can be easily implemented outside of the crate.

[cukie]

Personally I don't think the log crate should add any dependencies, even optional ones. The implementation seems pretty small, so it can be easily implemented outside of the crate.

Thanks @Thomasdezeeuw for the feedback. Implementing outside of the crate is certainly possible. I was hoping to avoid the extra boilerplate stemming from the orphan rules. If I split into a separate crate I can't derive Arbitrary and instead need to define parallel enums and structs and keep them in sync with the log crate.

[mgeisler]

Hi @Thomasdezeeuw

Personally I don't think the log crate should add any dependencies, even optional ones.

I'm another engineer in Android who reviewed Gil's patch to Android. I suggested trying to upstream it, so I'm curious to hear the reasoning for not taking the patch. Would you care to elaborate?

As I'm sure you know, it would be similar to how many crates have a serde feature which conditionally enables #[derive(Serialize, Deserialize)]. Example of libraries doing this include petgraph, rand_isaac, rusoto_s3, and others.

You're right that the code in question is small, the log specific parts are about 20 lines in Android. I don't know if anybody apart from Android is using log for fuzzing, but if they are, I believe they'll end up redoing these From implementations (unless someome publishes are log-fuzzing crate). So would it not be nice to do this once in a place where people can find it?

[Thomasdezeeuw]

Hi @Thomasdezeeuw

Personally I don't think the log crate should add any dependencies, even optional ones.

I'm another engineer in Android who reviewed Gil's patch to Android. I suggested trying to upstream it, so I'm curious to hear the reasoning for not taking the patch. Would you care to elaborate?

As I'm sure you know, it would be similar to how many crates have a serde feature which conditionally enables #[derive(Serialize, Deserialize)]. Example of libraries doing this include petgraph, rand_isaac, rusoto_s3, and others.

The reason is that the log crate is fairly low in the dependency graph, a lot of crates depend on it and adding a single dependency means adding it to a lot of crates. As far as I can tell, I could be wrong here, Arbitrary is mostly relevant for testing/fuzzing. So I'm weighing 20 lines of test code (per your comment) vs. adding a dependency, which in my opinion doesn't outweigh the downside of adding the dependency.

You're right that the code in question is small, the log specific parts are about 20 lines in Android. I don't know if anybody apart from Android is using log for fuzzing, but if they are, I believe they'll end up redoing these From implementations (unless someome publishes are log-fuzzing crate). So would it not be nice to do this once in a place where people can find it?

We could add the code as an example.

[mgeisler]

The reason is that the log crate is fairly low in the dependency graph, a lot of crates depend on it and adding a single dependency means adding it to a lot of crates.

I agree that lots of crates depend on log, but they won't be affected by an optional dependency, right? I think that's a crucial difference since it makes the patch a no-op for everybody who don't explicitly opt-in.