Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support customization of fields on derive #129

Merged
merged 13 commits into from Oct 20, 2022

Conversation

greyblake
Copy link
Contributor

@greyblake greyblake commented Oct 19, 2022

This PR addresses #33 and enables the following syntax for derive:

#[derive(Arbitrary)]
pub struct Rgb {
    // set `r` to Default::default()
    #[arbitrary(default)]
    pub r: u8,

    // set `g` to 255
    #[arbitrary(value = 255)]
    pub g: u8,

    // generate `b` with a custom function
    #[arbitrary(with = arbitrary_b)]
    pub b: u8,
}

fn arbitrary_b(u: &mut Unstructured) -> arbitrary::Result<u8> {
    u.int_in_range(64..=128)
}

Note

In case of custom function the lower bound for hint_size is defined through core::mem::size_of::<T>() which is correct for the types allocated on stack, but not for the heap allocated types.

Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! A few suggestions below before we merge this in.

README.md Outdated Show resolved Hide resolved
@@ -231,3 +231,42 @@ fn recursive_and_empty_input() {

let _ = Nat5::arbitrary(&mut Unstructured::new(&[]));
}

#[test]
fn test_field_attributes() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also add some doc tests that use compile_fail to check that we return compiler errors when

  • #[arbitrary] is used multiple times on a single field
  • #[arbitrary(unknown_attr)] and #[arbitrary(unknown_attr = unknown_val)] are used
  • #[arbitrary(value)] and #[arbitrary(with)] are used without RHS values

Should be able to just do something like the following in this file:

/// Can only use `#[arbitrary]` once per field:
///
/// ```compile_fail
/// use arbitrary::*;
/// #[derive(Arbitrary)]
/// struct Foo {
///     #[arbitrary(with = foo)]
///     #[arbitrary(with = bar)]
///     field: u32,
/// }
/// fn foo(_: &mut Unstructured) -> u32 { todo!() }
/// fn bar(_: &mut Unstructured) -> u32 { todo!() }
/// ```
///
/// Etc...
pub struct CompileFailTests;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 8913ec0

I was puzzled where to put such doc tests (I don't think users really want to see them in the docs).
Eventually I look into serde again, they're using trybuild to test compile failures.
So I've decided to introduce trybuild to this project too, I think it's a nice tool for this purpose.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was doing doc tests on a pub struct CompileFailTests in tests/derive.rs not getting picked up by rustdoc?

If not, then we could maybe avoid a new dependency by doing

// src/lib.rs

#[cfg(all(test, feature = "derive"))]
/// ...
pub struct CompileFailTests {}

So that it never shows up in the docs and only is built when we are running tests (with the derive enabled).

Copy link
Contributor Author

@greyblake greyblake Oct 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fitzgen I just tried doc tests within derive/src/lib.rs, they are not picked up.

If not, then we could maybe avoid a new dependency by doing

It's dev-dependency, shouldn't it be fine? I definitely like trybuild, it generates really helpful error messages and allows to structure tests in more easy to understand / maintainable way.

For example, this is what a reported failure looks like:
image

This way we actually test, the compiler fails in the way we expect it to fail. Doc tests may give false positives if something in the code gets mistyped.

I would see the cost of using trybuild is rather potential failures if rustc changes the way it prints errors.
But still I think pros of trybuild outweights its cons.

Please let me know, if you still insist on using doc tests.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if the test is in src/lib.rs rather than derive/src/lib.rs it should work? I'd really want to avoid failing tests on nightly but not stable unless as an absolute last resort.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
derive/src/field_attributes.rs Outdated Show resolved Hide resolved
tests/derive.rs Outdated Show resolved Hide resolved
@greyblake
Copy link
Contributor Author

The remaining part is using syn::Result instead of panic. I will finish it today later.

@greyblake
Copy link
Contributor Author

@fitzgen , @Manishearth I've addressed your comments guys. Thanks to you even learned a few new things today.

The easiest way to review the changes will be to review them commit by commit.

If you merge this PR, I would very very appreciate releasing a new version of the crate, so I can benefit from the changes in my main project.
Thanks.

Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Looking great.

So trybuild is kinda nice because it lets you check the error messages, but if we can't have the tests passing on both stable and nightly at the same time, then I'd prefer exploring the doc tests work around instead.

Comment on lines 4 to 5
5 | / #[arbitrary(value = 2)]
6 | | #[arbitrary(value = 3)]
7 | | x: i32,
| |__________^
5 | #[arbitrary(value = 2)]
| ^
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean that the tests won't pass on both nightly and stable at the same time right now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's correct. I got one tests broken on CI, because I was running nightly locally, but CI was running stable.

OK, i'll change it to doc tests then.

@greyblake
Copy link
Contributor Author

@fitzgen Ok, here are the doc tests: 755b8e0

Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!!

@fitzgen fitzgen merged commit 33b855f into rust-fuzz:master Oct 20, 2022
@greyblake
Copy link
Contributor Author

@fitzgen @Manishearth Thank you guys for accepting the PR. Would you mind publishing a new version?

@fitzgen
Copy link
Member

fitzgen commented Oct 20, 2022

Yeah I can cut a release in a little bit

@fitzgen
Copy link
Member

fitzgen commented Oct 20, 2022

published

@fitzgen
Copy link
Member

fitzgen commented Oct 20, 2022

Thanks again!

@greyblake
Copy link
Contributor Author

@fitzgen Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants