WIP attr macro

[cameron1024]

WIP attribute macro for proptest

Roughly speaking:

#[property_test]
fn foo(x: i32) {
  assert_eq!(x, x);
}

is equivalent to:

proptest! {
  #[test]
  fn foo(x in any::<i32>()) {
    assert_eq!(x, x);
  }
}

Config can be set with #[property_test(config = ...)]

Known issues:

• trybuild tests
• what to do about the trybuild fork
• better test coverage
• docs

Opening this now to get some initial feedback

[rex-remind101]

is there an equivalent of a snapshot test of the printed output? this tree i find tough to interpret directly -- though this will for sure be helpful as things change, to have a basis to compare to

[cameron1024]

I think the Display snapshot probably makes a lot more sense

[cameron1024]

Decided to run it through a formatter first - much more readable now

[rex-remind101]

This looks really good to me, looking forward to non-wip status. besides the code and behavior i tried to make sure what i saw was hygienic based on these rules: https://gist.github.com/Kestrer/8c05ebd4e0e9347eb05f265dfb7252e1

sharing in case this is helpful to you or other reviewers. I'm not sure if this is common knowledge, it was helpful in building my understanding of proc macros though.

[matthew-russo]

lgtm. changes are fairly straight forward. i'll try to pull this locally and give it a whirl as well

[rklaehn]

I think this would be nice. Compile errors within a big proptest! block are always a bit hard to track down.

There is also the test-strategy crate that seems to do something very similar.

[cameron1024]

I've picked this back up again after a long hiatus. I'll respond to feedback, and push those changes today.

The main issue I'd like to address at some point is the lack of compile-fail tests via trybuild (it doesn't build #[cfg(test)] code, which means the tests are pretty much only testing the parsing of the attribute, which is better than nothing I guess). I'll migrate them to use compiletest_rs in a future PR, unless people feel strongly that this should be included in this PR.

The only other thing I'm not sure about is how strongly we feel like we should mark it as "experimental" - I'm feeling pretty confident about the API. It leaves room to add more features, and it doesn't really lock us into much. The only exception is the name of the macro itself. A library can't simultaneously export both a proc macro and a macro_rules macro with the same name. Some alternatives are:

• ptest - IMO looks too much like the regular test
• property- not clear its a test
• we could also just re-export the proptest-macro crate, so you could use proptest::proptest_macro::proptest, though I'm not sure if this would be more confusing. Perhaps if we rename the crate, we could get something like proptest::proptest_attr::proptest, but it's still not ideal

Personally, I feel like property_test is the way to go, but keen to hear other people's thoughts

[cameron1024]

This looks really good to me, looking forward to non-wip status. besides the code and behavior i tried to make sure what i saw was hygienic based on these rules: gist.github.com/Kestrer/8c05ebd4e0e9347eb05f265dfb7252e1

sharing in case this is helpful to you or other reviewers. I'm not sure if this is common knowledge, it was helpful in building my understanding of proc macros though.

This looks really good - I'll have a read through and see if I can break anything

[rexmas]

the test says trailing_comma but I don't see where the trailing comma is being tested? is that meant to be on Default::default()?

[rexmas]

might be nice to have a snapshot test with errors?

[rexmas]

I wanted to give this a full review again before approving, thanks for your patience :)

There's 1 test I am a little confused about that I commented on. If we can clear that up we're good to go. also I added a suggestion for another test but i won't block on that.

Personally, I feel like property_test is the way to go, but keen to hear other people's thoughts

I like that name 👍

I'll migrate them to use compiletest_rs in a future PR, unless people feel strongly that this should be included in this PR.

totally reasonable

The only other thing I'm not sure about is how strongly we feel like we should mark it as "experimental" - I'm feeling pretty confident about the API.

I feel fairly similar about the api, though I usually lean on the side of letting things bake. If we merge i can play around with it a bit more and maybe make a decision then?

[matthew-russo]

Looks good to me -- I didn't have any specific comments about the code itself. I think the main area that could use some work in a follow up would be documentation and examples -- both in our crates and in the book. What does the shape of the macro look like? what attributes does it support? what attributes doesn't it support?

Doesn't need to fall on your plate.

Also how are we thinking about how this sits w/ the decl macro? When do we suggest one or the other, what does one support that the other doesn't?

Personally, I feel like property_test is the way to go, but keen to hear other people's thoughts

agreed

I'll migrate them to use compiletest_rs in a future PR, unless people feel strongly that this should be included in this PR.

sounds good

The only other thing I'm not sure about is how strongly we feel like we should mark it as "experimental" - I'm feeling pretty confident about the API.

i like that its behind a feature flag for now. I think there are a number of things we'll want to change going forward (async support, explicit test cases, ability to customize strategies inline, etc) and having some flexibility to iterate will be good.

[rexmas]

👍