Allow trait bounds to be manually specified

[michaelsproul]

While working with some generic types I found that the T: Arbitrary bounds added by arbitrary_derive were too restrictive.

Consider a trait that requires Arbitrary, like this:

pub trait WrapperTrait: for<'a> Arbitrary<'a> {}

And a struct that uses it:

#[derive(Arbitrary)]
struct GenericSingleBound<T: WrapperTrait> {
    t: T,
}

The derive macro currently generates an impl like:

impl<'arbitrary, T: WrapperTrait + Arbitrary<'arbitrary>> Arbitrary<'arbitrary> for GenericSingleBound<T> {
    // ...
}

Even though WrapperTrait already implies Arbitrary<'arbitrary> via the universal lifetime quantifier, the compiler doesn't seem to be smart enough to work out that the bound is redundant. We get an error like this:

error[E0283]: type annotations needed: cannot satisfy `T: Arbitrary<'arbitrary>`
  --> tests/bound.rs:16:10
   |
16 | #[derive(Arbitrary)]
   |          ^^^^^^^^^
   |
   = note: cannot satisfy `T: Arbitrary<'arbitrary>`
   = note: this error originates in the derive macro `Arbitrary` (in Nightly builds, run with -Z macro-backtrace for more info)

Taking a leaf out of serde's book, this PR adds a bound = ".." container attribute which allows manually overriding the inferred bound. In our example arbitrary(bound = "T: WrapperTrait") works as expected. See the test case in ./tests/bound.rs.

Implementation notes

• I've added a dependency on darling as I find it much easier to write proc macro code with it. I could switch to vanilla syn (or another macro crate) if you would prefer not to depend on darling.
• If a subset of types have bounds declared with bound then the omitted types will have their bounds from the type declaration dropped. This mimics serde.
• Bounds for irrelevant types will result in an error being raised.
• I'm a little unsure of how to handle the attributes field of TypeParam. I figured that replacing the full TypeParam gives the most flexibility (even if it means duplicating attributes from the type declaration).

[Manishearth]

Not sure if I want to support this feature: but yeah I'd rather not have a bound on darling here; it would be nice to keep this crate small.

[fitzgen]

• +1 for avoiding additional dependencies (darling)
• I'm also not convinced that this feature is worth supporting from a complexity/maintenance point of view. It sort of feels to me that this is the sort of edge case where users can stop using the derive and write the implementation by hand instead.

[michaelsproul]

I just pushed a commit to remove darling and refine the behaviour, specifically for irrelevant bounds which now result in an error. I also added some compile_fail tests covering this and a few other cases.

If you think the updated implementation looks OK I would still love to merge this. For large codebases implementing dozens of Arbitrary instances by hand is a real pain, and seems well-suited to automation with derive. I imagine users making use of serde's bound or derivative would benefit from this feature in arbitrary too.

[fitzgen]

Implementation LGTM, thanks.

I think I've been convinced that it is reasonable to support, if only because of the prior art of serde supporting this. @Manishearth do you feel strongly that this feature shouldn't be supported? Otherwise, I'm inclined to merge.

[michaelsproul]

Thanks for the review and the consideration @fitzgen 😊

I just rebased on main so this is ready to merge as soon as it gets the OK

[michaelsproul]

If there are no objections from @Manishearth, perhaps we can merge soon @fitzgen? 🙏

[Manishearth]

Yeah I'm +1

[michaelsproul]

Comments addressed, ready for re-review. Thanks!

[fitzgen]

Sorry I've been on vacation. LGTM, modulo the nitpicks about returning errors instead of panicking I just noticed below.

[michaelsproul]

Thanks @fitzgen! I've updated all of those panics to errors. For the error on lib.rs:120 about too many bounds I used Span::call_site() for lack of anything better. Let me know if you think that's OK.

[fitzgen]

LGTM, thanks! And thanks for your patience with this one.

[fitzgen]

Tests are failing in CI:

https://github.com/rust-fuzz/arbitrary/actions/runs/4369258606/jobs/7659042814#step:5:93

test src/container_attributes.rs - container_attributes::ContainerAttributes::bounds (line 11) ... FAILED
test src/container_attributes.rs - container_attributes::ContainerAttributes::bounds (line 17) ... FAILED

failures:

---- src/container_attributes.rs - container_attributes::ContainerAttributes::bounds (line 11) stdout ----
error: expected statement after outer attribute
 --> src/container_attributes.rs:12:1
  |
error: doctest failed, to rerun pass `-p derive_arbitrary --doc`
3 | #[arbitrary(bound = "T: Default, U: Debug")]
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to previous error

Couldn't compile the test.
---- src/container_attributes.rs - container_attributes::ContainerAttributes::bounds (line 17) stdout ----
error: expected statement after outer attribute
 --> src/container_attributes.rs:19:1
  |
4 | #[arbitrary(bound = "U: Default")]
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to previous error

Couldn't compile the test.

failures:
    src/container_attributes.rs - container_attributes::ContainerAttributes::bounds (line 11)
    src/container_attributes.rs - container_attributes::ContainerAttributes::bounds (line 17)

test result: FAILED. 0 passed; 2 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.03s

[michaelsproul]

Tests passing locally now, should be good to go 🤞

[michaelsproul]

Omg feels great to merge it, thank you for your patience Nick and Manish 😌

[michaelsproul]

Could we do a v1.2.4 release of derive_arbitrary for this change? 🙏

[fitzgen]

Published new release!