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

[proptest-macro] add the ability to specify custom strategies #523

Merged

Conversation

matthew-russo
Copy link
Member

replaces #478

Meta::NameValue(_) => {
if first_strategy_seen {
error.extend(quote_spanned! {
attr.span() => compile_error!("duplicate `#[strategy = ...]` definition");
Copy link
Contributor

Choose a reason for hiding this comment

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

may be worth saying the name of the parameter here for added clarity

Copy link
Member

@cameron1024 cameron1024 Dec 16, 2024

Choose a reason for hiding this comment

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

Worth mentioning that, since we're using quote_spanned, the compile error will appear in IDEs to be "coming from" the second attribute (i.e. that's where the red squiggly line will appear).

We also don't have an amazing way of getting the "name" of the parameter, since parameters can be patterns like (a, b): (i32, i32). We can get an equivalent representation, but it's not guaranteed to be the same. Probably not the end of the world for diagnostics, but still a minor papercut potentially

snapshot_test!(custom_strategy {
fn foo(
#[strategy = 123] x: i32,
#[strategy = a + more()("complex") - expression!()] y: String,
Copy link
Contributor

Choose a reason for hiding this comment

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

this is really cool that one can just assign a literal if needed. i'm curious about y here though, like what happens if an operator that cannot be used on string types is used? I guess that question may reduce down to, what happens if the strategies type does not match the type of the parameter, will the type error be relatively obvious for the user?

Copy link
Contributor

@rexmas rexmas Sep 22, 2024

Choose a reason for hiding this comment

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

also do we expect argument types to ever use lifetimes or generics? what happens if someone tries to and should we test for that?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry I've had these comments stuck in "pending" 🤦

  • If there's an error typechecking the expression itself (e.g. 1 + "hello", we'll get a nice rustc error:
error[E0277]: cannot add `&str` to `{integer}`
  --> src/main.rs:29:27
   |
29 |     fn foo(#[strategy = 1 + "hello"] x: bool) {
   |                           ^ no implementation for `{integer} + &str`
   |
  • if the expression has a type where the Strategy impl has the wrong Value type (or it doesn't implement Strategy at all) we also get a nice-ish error:
rror[E0599]: the method `prop_map` exists for tuple `(NotAStrategy,)`, but its trait bounds were not satisfied
  --> src/main.rs:30:5
   |
22 | struct NotAStrategy;
   | ------------------- doesn't satisfy `NotAStrategy: proptest::strategy::Strategy`
...
30 |     #[proptest::property_test]
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^ method cannot be called on `(NotAStrategy,)` due to unsatisfied trait bounds
   |
   = note: the following trait bounds were not satisfied:
           `NotAStrategy: proptest::strategy::Strategy`
           which is required by `(NotAStrategy,): proptest::strategy::Strategy`
           `(NotAStrategy,): proptest::strategy::Strategy`
           which is required by `&(NotAStrategy,): proptest::strategy::Strategy`
           `(NotAStrategy,): proptest::strategy::Strategy`
           which is required by `&mut (NotAStrategy,): proptest::strategy::Strategy`
  • we can have generics, but they have to be used with a concrete instantiation (i.e. you can have Foo<i32> but not Foo<T>. not sure how you'd do lifetimes, but I'm not 100% sure what that would even mean, since test functions can't have lifetimes. But the following works:
struct Foo<T>(PhantomData<T>);

impl Strategy for Foo<i32> {
  type Value = i32;
  // ...
}

#[property_test]
fn foo(#[strategy = Foo<i32>] x: i32) {}

@rexmas rexmas closed this Sep 22, 2024
@rexmas rexmas reopened this Sep 22, 2024
Copy link
Contributor

@rexmas rexmas left a comment

Choose a reason for hiding this comment

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

had a few questions but overall this looks great to me and happy to move forward with it! (assuming ci is 💚 after running)

also just to weigh in a comment from the original pr

I think for now, disallowing other attributes is safer from a backwards compatibility point of view - we don't lock ourselves into supporting something that is difficult later

i think i agree with taking a conservative/iterative stance here

@rexmas
Copy link
Contributor

rexmas commented Dec 15, 2024

cc @cameron1024

@cameron1024 cameron1024 merged commit b844446 into proptest-rs:main Dec 16, 2024
5 checks passed
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.

3 participants