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

Fix #[derive(TypeGenerator)] #126

Merged
merged 2 commits into from
Mar 17, 2023

Conversation

adpaco-aws
Copy link
Collaborator

@adpaco-aws adpaco-aws commented Mar 17, 2023

This PR fixes the macro for #[derive(TypeGenerator)] and resolves #121

In particular, these are the changes made:

  1. Avoid adding trait bounds for non-generic types (this is what caused problems in #derive(TypeGenerator) for recursive enums #121).
  2. Refactor code into a more functional style. The top-level function adds trait bounds, retrieves the generate and mutate methods, and emits the generated impl TypeGenerator for ... at the end.
  3. Add enum tests for the macro (with recursive variants, type parameters)
  4. Add comments for most important functions and steps in the generation.

Regarding (1), it wasn't clear to me why non-generic types were added in where clauses. Removing the apply_constraint function (which did this) doesn't cause a testing regression. Trait bounds are now added to each type parameter in #impl_generics.

Using the example in #121, this is what was produced before:

impl bolero_generator::TypeGenerator for Expr
where
    i32: bolero_generator::TypeGenerator,
    Box<Expr>: bolero_generator::TypeGenerator,
    Box<Expr>: bolero_generator::TypeGenerator,
    Box<Expr>: bolero_generator::TypeGenerator,
{ ...

Now we produce this instead:

#[automatically_derived]
impl bolero::generator::bolero_generator::TypeGenerator for Expr {

If the enum contains type parameters, as in the example added in this PR

pub enum GenericTypes<T1, T2> {
    T1Value(T1),
    T2Value(T2),
    NoValue,
}

then we add trait bounds to T1 and T2

#[automatically_derived]
impl<
    T1: bolero::generator::bolero_generator::TypeGenerator,
    T2: bolero::generator::bolero_generator::TypeGenerator,
> bolero::generator::bolero_generator::TypeGenerator for GenericTypes<T1, T2> {

@camshaft
Copy link
Owner

One potentially breaking change is this now forces all of the generic types to implement TypeGenerator. Let's consider this example:

#[derive(TypeGenerator)]
pub struct MyStruct<T> {
    v: PhantomData<T>,
}

The implementation for PhantomData doesn't require that the inner type implement TypeGenerator. The way the the derive macro is now allows this to go through, since it's just checking that each of the fields' type implements TypeGenerator. After this change, T will need to implement it. I'm don't think it's the end of the world; but it may break existing usage.

But it looks like all of the common derives have the same behavior, so it's probably worth switching to: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=2d4f5e556ce315023c1f695dcffb8e47

@camshaft camshaft merged commit 75bf577 into camshaft:master Mar 17, 2023
@adpaco-aws
Copy link
Collaborator Author

Thanks, I didn't consider that case. But yes, most derives have the same behavior and I think it's desirable to have the trait requirements in place (compilation errors are easier to understand and correct).

Also, it's likely we'll revisit this in the future because of lifetimes. The goal with this PR was to allow deriving for simpler enums (i.e., one in #121), we can adjust it to handle more complex types later.

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.

#derive(TypeGenerator) for recursive enums
2 participants