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

custom strategies in attribute macro #478

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Conversation

cameron1024
Copy link
Member

This adds the ability to add custom strategies to parameters of a #[property_test] function:

#[property_test]
fn test_something(x: i32, #[strategy = "[0-9]*"] s: String) {
  // test body
}

This adds a second code path to codegen:

  • if there are no custom strategies, we use the old code path which internally uses an Arbitrary impl with fully spelled out types (i.e. no boxing)
  • if there is one or more custom strategy, we no longer have enough information to spell out the full type of the Strategy, since macros run before typechecking. So we just fallback to BoxedStrategy<Self>

I've tried to retain span info, so error messages should appear at the correct place if you do things like add 2 strategies to a field, or use invalid syntax. For other stuff (e.g. passing non-strategies) we rely on rustc

fn strip_strategy(mut pat_ty: PatType) -> Argument {
let (strategies, others) = pat_ty.attrs.into_iter().partition(is_strategy);

pat_ty.attrs = others;
Copy link
Member

Choose a reason for hiding this comment

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

attrs must always be empty here because we validate in validate_parameter_attrs that only strategy = <expr> attrs exist right?

  • if we handle the extra attrs here, why do we disallow other attrs?
  • does disallowing them mean #[property_test] doesn't compose well with other proc macros?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll double check. I'd like to compose with other macros as much as is feasible, so other attributes should be left alone. I think validate is being overly aggressive here

Copy link
Member Author

Choose a reason for hiding this comment

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

But the more I think about it, I'm not sure what we could meaningfully do with the attributes, since they're going into our own struct - the output from proptest is a zero-arg function, so maybe it doesn't make sense here?

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


mod arbitrary;
Copy link
Member

Choose a reason for hiding this comment

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

i dont think this file has been committed yet -- which also exposes gaps in our CI which likely isn't building this new crate

Copy link
Member Author

Choose a reason for hiding this comment

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

Good spot - let's get this in CI

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

2 participants