Don't require Arbitrary::Parameters to implement Default

[mirandaconrado]

I have found myself having to implement Default for some types that don't make sense (i.e. they don't have a default value) with a panic just to satisfy the fact that I want to use such types as parameters for my Arbitrary implementation. This essentially means that the types now implement Default that just causes a runtime error, but it would be much cleaner to remove the implementation that won't be used.

I'm not familiar with why Default is added as a requirement for Arbitrary::Parameters. I'd guess that is required when you actually have to create a strategy for the value that will appear in the test, as at that moment we have to call Arbitrary::arbitrary() since we don't have any value for the arguments to pass around.

I don't have a good suggestion here. My gut feeling is that removing this constraint will require a breaking change since Arbitrary::arbitrary() wouldn't be able to provide a default implementation anymore (and might not exist). I played around with having a wrapper trait ArbitraryWithDefaultParameters but I wanted to float this idea to folks before spending more time.

I'm happy to contribute this change once we have a recommended path forward, if any.

[cameron1024]

I agree this sounds annoying. Creating panicking Default impls definitely feels like a code smell, so we shouldn't be forcing users into doing that. As a temporary improvement, I guess you could conditionally implement them in tests with #[cfg_attr(test, derive(Default))], though that may not work for your use case

In theory, we could:

• remove the Default bound on Parameters
• add a Default bound to fn arbitrary() and some others (any() comes to mind)

If you know you're always going to call arbitrary_with rather than arbitrary, the Default::default function is never even called

I'm not actually 100% certain about whether this would be a breaking change or not. Going into proptest and just making the change does give a bunch of errors, but a trait in a crate that depends on it is fine. It seems like you can have:

trait Arbitrary {
  type Parameters;
  fn arbitrary() where Self::Parameters: Default;
}

struct Foo;

impl Arbitrary for Foo {
  type Parameters = ();
  fn arbitrary() {
    println!("look mom, no trait bound");
  }
}

The proptest crate relies heavily on macro_rules, and these may be the source of some of the compile issues, but I'm worried about the generic case. I'd want to test a bunch of large repos that use proptest before releasing this.

If we conclude that this change requires a breaking change, I'd definitely put this in our "2.0 wishlist"

[mirandaconrado]

Oh, I didn't think of putting the binding on the "arbitrary" method itself. That's a smart idea.

I went with conditional compilation but ended up calling it by mistake in some places. Not a massive deal, just something that is slightly annoying and surprising.

I think the breakage situation is where someone is receiving an "Arbitrary" type (e.g. generic "T: Arbitrary") and calling "Arbitrary::Parameters::default()". That case would break because you can no longer guarantee that parameters will always have default. I don't know what use case someone would have, but it's possible for such code to exist. It's also not a big fix on the user side (add another "T::Parameters: Default" to the clause), but it's a breaking change and hence not a great experience.

I'm 100% ok delaying this. Just wanted to raise as a thing we could consider fixing, but not urgent.

[cameron1024]

Yeah I think you're right, shame it's not possible. I'll leave the issue open but put the "2.0 wishlist" label on it. Thanks for bringing it up 🙏

[matthew-russo]

I couldn't get the example pseudocode to compile:
https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=b859dbcf057e99795e17d2db74d2ec75

we'd need some theoretical form of specialization / conditional compilation.

i'm curious what the percentage of proptest users are that use parameters in their generation and what the ergonomic impact would be to split this to two traits.