Allow trait bounds to be manually specified by michaelsproul · Pull Request #138 · rust-fuzz/arbitrary

michaelsproul · 2023-01-10T02:32:45Z

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 · 2023-01-10T05:30:45Z

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 · 2023-01-10T16:55:54Z

+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 · 2023-01-11T06:48:21Z

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.

tests/bound.rs

src/lib.rs

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 · 2023-01-30T01:03:46Z

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 · 2023-02-14T00:03:32Z

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

Manishearth · 2023-02-14T00:34:52Z

Yeah I'm +1

derive/src/container_attributes.rs

derive/src/lib.rs

michaelsproul · 2023-02-22T22:28:52Z

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.

derive/src/container_attributes.rs

derive/src/lib.rs

michaelsproul · 2023-03-08T22:55:35Z

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 · 2023-03-09T16:38:31Z

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 · 2023-03-09T23:05:45Z

Tests passing locally now, should be good to go 🤞

michaelsproul · 2023-03-10T21:55:17Z

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

michaelsproul · 2023-03-13T00:38:32Z

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

fitzgen · 2023-03-13T19:21:22Z

Published new release!

michaelsproul mentioned this pull request Jan 10, 2023

Fix Arbitrary implementations sigp/lighthouse#3867

Merged

fitzgen reviewed Jan 17, 2023

View reviewed changes

tests/bound.rs Outdated Show resolved Hide resolved

src/lib.rs Show resolved Hide resolved

fitzgen approved these changes Jan 23, 2023

View reviewed changes

Allow trait bounds to be manually specified

1c6d77a

michaelsproul force-pushed the specify-bounds branch from 2e0ee67 to 1c6d77a Compare January 30, 2023 01:02

Manishearth requested changes Feb 14, 2023

View reviewed changes

derive/src/container_attributes.rs Show resolved Hide resolved

derive/src/lib.rs Outdated Show resolved Hide resolved

Doc and comment clarifications

04798c4

fitzgen reviewed Mar 7, 2023

View reviewed changes

derive/src/container_attributes.rs Outdated Show resolved Hide resolved

derive/src/container_attributes.rs Outdated Show resolved Hide resolved

derive/src/lib.rs Outdated Show resolved Hide resolved

Don't panic

6c689bb

fitzgen approved these changes Mar 9, 2023

View reviewed changes

Ignore container attributes examples

a5a9527

fitzgen merged commit c397cc2 into rust-fuzz:main Mar 10, 2023

michaelsproul deleted the specify-bounds branch March 13, 2023 00:37

michaelsproul mentioned this pull request Mar 14, 2023

Use upstream arbitrary sigp/lighthouse#4079

Closed

Conversation

michaelsproul commented Jan 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Implementation notes

Uh oh!

Manishearth commented Jan 10, 2023

Uh oh!

fitzgen commented Jan 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michaelsproul commented Jan 11, 2023

Uh oh!

Uh oh!

Uh oh!

fitzgen left a comment

Choose a reason for hiding this comment

Uh oh!

michaelsproul commented Jan 30, 2023

Uh oh!

michaelsproul commented Feb 14, 2023

Uh oh!

Manishearth commented Feb 14, 2023

Uh oh!

Uh oh!

Uh oh!

michaelsproul commented Feb 22, 2023

Uh oh!

fitzgen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

michaelsproul commented Mar 8, 2023

Uh oh!

fitzgen left a comment

Choose a reason for hiding this comment

Uh oh!

fitzgen commented Mar 9, 2023

Uh oh!

michaelsproul commented Mar 9, 2023

Uh oh!

michaelsproul commented Mar 10, 2023

Uh oh!

michaelsproul commented Mar 13, 2023

Uh oh!

fitzgen commented Mar 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

michaelsproul commented Jan 10, 2023 •

edited

Loading

fitzgen commented Jan 10, 2023 •

edited

Loading