-
Notifications
You must be signed in to change notification settings - Fork 76
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
Conversation
There was a problem hiding this 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.
@@ -231,3 +231,42 @@ fn recursive_and_empty_input() { | |||
|
|||
let _ = Nat5::arbitrary(&mut Unstructured::new(&[])); | |||
} | |||
|
|||
#[test] | |||
fn test_field_attributes() { |
There was a problem hiding this comment.
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;
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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:
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.
There was a problem hiding this comment.
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.
78a64c7
to
eb4ebc6
Compare
eb4ebc6
to
727ab88
Compare
The remaining part is using |
@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. |
32eb8f1
to
4fdf16f
Compare
There was a problem hiding this 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.
tests/ui/multiple_arbitrary.stderr
Outdated
5 | / #[arbitrary(value = 2)] | ||
6 | | #[arbitrary(value = 3)] | ||
7 | | x: i32, | ||
| |__________^ | ||
5 | #[arbitrary(value = 2)] | ||
| ^ |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!!
@fitzgen @Manishearth Thank you guys for accepting the PR. Would you mind publishing a new version? |
Yeah I can cut a release in a little bit |
published |
Thanks again! |
@fitzgen Thank you! |
This PR addresses #33 and enables the following syntax for derive:
Note
In case of custom function the lower bound for
hint_size
is defined throughcore::mem::size_of::<T>()
which is correct for the types allocated on stack, but not for the heap allocated types.