Open mirandaconrado opened 1 month ago
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:
Default
bound on Parameters
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"
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.
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 :pray:
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.
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 myArbitrary
implementation. This essentially means that the types now implementDefault
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 forArbitrary::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 callArbitrary::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 traitArbitraryWithDefaultParameters
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.