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

Allow specific enum variants to be avoided. #24

Merged
merged 4 commits into from Feb 18, 2022
Merged

Allow specific enum variants to be avoided. #24

merged 4 commits into from Feb 18, 2022

Conversation

teymour-aldridge
Copy link
Collaborator

@teymour-aldridge teymour-aldridge commented Dec 26, 2021

This adds an attribute called #[ignore_variant], usable on enums
which instructs Fuzzcheck to not construct that variant of the
enumeration.

I haven't yet solved this for generic types.

This adds an attribute called `#[ignore_variant]`, usable on enums
which instructs Fuzzcheck to not construct that variant of the
enumeration.
@loiclec
Copy link
Owner

loiclec commented Dec 28, 2021

Amazing, thank you! I can't review it properly yet, but I think it looks good. A couple of quick comments/questions:

  • is there a reason that the item attribute #[ignore_variant] impacts FieldMutatorKind? I imagined that instead of a Vec<Vec<FieldMutatorKind>> where the FieldMutatorKind has a Ignore variant, we would have something like Vec<Option<Vec<FieldMutatorKind>>>, because I think it is impossible to ignore just one of the item’s fields. It's not a big deal, so it is okay if the answer is that it was easier to write that way.

  • I think it would be good to have a test using derive(DefaultMutator) (I expect that it doesn't work at the moment, as you need to register the attribute in the declaration of the derive macro), as well as tests with slightly more complicated enums, such as:

enum A<T, U> {
     #[ignore_variant]
     X, // empty variant ignored
     #[ignore_variant]
     Y(u8, u16) // variant with multiple fields ignored
     #[ignore_variant]
     Z { t: T, u: U } // variant with named parameters, variant with fields using generic type parameters
     // etc. ?
     // + maybe see what happens when all variants are ignored? not sure what the correct behaviour should be in that case
}

Regarding the edge cases with generic types, I really think it's ok if it is not solved yet. I already have some problems with the generic bounds and it may need a more thorough refactor rather than a quick fix.

Anyway, I am happy to merge it whenever you want, just let me know if you're satisfied with it as it is, if you'd like to continue working on it, or if you'd prefer me to continue doing some work on it :)

- also fix a bug where the macro was emitting too many commas
@teymour-aldridge
Copy link
Collaborator Author

teymour-aldridge commented Jan 15, 2022

Sorry for the delay! Thank you for the review :)

is there a reason that the item attribute #[ignore_variant] impacts FieldMutatorKind? I imagined that instead of a Vec<Vec> where the FieldMutatorKind has a Ignore variant, we would have something like Vec<Option<Vec>>, because I think it is impossible to ignore just one of the item’s fields. It's not a big deal, so it is okay if the answer is that it was easier to write that way.

Yes, it was just because I thought it was easier to write that way.

Regarding the edge cases with generic types, I really think it's ok if it is not solved yet. I already have some problems with the generic bounds and it may need a more thorough refactor rather than a quick fix.

Yeah I feel like the generic bounds may be a bit of work.

I think it would be good to have a test using derive(DefaultMutator) (I expect that it doesn't work at the moment, as you need to register the attribute in the declaration of the derive macro),

I tried registering the attributes, but for some reason it didn't work. I'll try to investigate further.

as well as tests with slightly more complicated enums, such as:

working on that (I seem to have broken a few things)

@loiclec
Copy link
Owner

loiclec commented Feb 10, 2022

Hello again! I'm just letting you know that I plan to release an update to fuzzcheck in a couple weeks or so and plan to merge this pull request by then :) You do not need to do anything.

@loiclec loiclec merged commit a220bc2 into loiclec:main Feb 18, 2022
@loiclec
Copy link
Owner

loiclec commented Feb 18, 2022

It's merged!

I also fixed a couple of minor issues (attribute registration with derived macro, misnamed argument in new() method).

Thanks again :) I am really happy about it 😄

@teymour-aldridge
Copy link
Collaborator Author

Thank you too :)

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