Skip to content

Improve derived generator ergonomics#154

Open
Liam-DeVoe wants to merge 2 commits intomainfrom
derive-syntax
Open

Improve derived generator ergonomics#154
Liam-DeVoe wants to merge 2 commits intomainfrom
derive-syntax

Conversation

@Liam-DeVoe
Copy link
Copy Markdown
Member

I'll just restate the release notes here:

RELEASE_TYPE: minor

This release makes several changes to `#[derive(DefaultGenerator)]` ([#149](https://github.com/hegeldev/hegel-rust/issues/149)):

- Tuple variants now take field generators directly as positional arguments
- Named variants use a closure that receives the default variant generator:
- Generated method names are converted to snake_case instead of preserving PascalCase
  - If this would produce a name collision, we keep the original casing for both method names.

```rust
enum Op {
    Reset,
    ReadWrite(usize, usize),
    Configure { retries: u32, timeout: u64 },
}

// before
let g = Op::default_generator();
Op::default_generator()
    .ReadWrite(g.default_ReadWrite().value_0(gs::just(42)).value_1(gs::just(43)))
    .Configure(g.default_Configure().retries(gs::just(44)))

// after
Op::default_generator()
    .read_write(gs::just(42), gs::just(43))
    .configure(|g| g.retries(gs::just(44)))
```

Thanks to Rain for providing feedback on enum ergonomics!

I also cleaned up a bunch of the enum_gen.rs code in our macro, which was splitting out different code paths for no good reason.

Closes #149. See also #148.

@DRMacIver
Copy link
Copy Markdown
Member

Hmm. I think this change is directionally correct and I appreciate it overall, but I'm like... weak -1 on some of the details.

Specifically:

  1. I don't think I like the rewriting to snake case. I know that having the pascal cased method names is extremely unidiomatic, but I think in this case it's basically the right thing to do. If people who have much stronger rust aesthetic opinions than me really hate the camel case I'm prepared to be overridden, but I think given that you cannot easily see the generated code from the macro I'd like to keep the mapping of names very straightforward. In particular I think the fact that we have to have collision detection logic is evidence that there are some very confusing ways for this to go wrong.
  2. I don't like the inconsistency between the tuple and named variants. I'd rather both use the closure approach and add support for tuple index updates to the variant generator.

@DRMacIver
Copy link
Copy Markdown
Member

DRMacIver commented Mar 30, 2026

I think I'd withdraw my objection to snake casing if a clash were a compile error instead of a fallback. Because this is a type we entirely create ourselves, the only way you can trigger that (I think) is to have both FooBar and foo_bar as variants names, and frankly fuck you you're on your own if you do that.

@Liam-DeVoe
Copy link
Copy Markdown
Member Author

don't think I like the rewriting to snake case. I know that having the pascal cased method names is extremely unidiomatic, but I think in this case it's basically the right thing to do.

I think the fact that cargo ships a default lint rule that method names be snake_case implies rust as an ecosystem follows this idiom more strongly than other languages. I don't want users to have to suppress warnings for normal hegel usage.; that'll annoy them.

Because this is a type we entirely create ourselves, the only way you can trigger that (I think) is to have both FooBar and foo_bar as variants names, and frankly fuck you you're on your own if you do that.

If a user has chosen to do this, they have good reason to and have already suppressed a builtin cargo lint warning to do so. I want the way in which they're on their own to be "we did the best we could to provide you reasonably-named methods", not "we refused to generate any methods".

I don't like the inconsistency between the tuple and named variants. I'd rather both use the closure approach and add support for tuple index updates to the variant generator.

Agree the inconsistency is bad. But:

  • .single_field(|g| g._0(gs::just(42)) feels worse than .single_field(gs::just(42)), because you're only configuring one field;
  • and .double_field(|g| g._0(gs::just(42)._1(gs::just(42)) feels worse than .double_field(gs::just(42), gs::just(42)), because ._0 and ._01 differ from the standard .0 and .1 tuple accessors due to syntax restrictions on method names.

@DRMacIver
Copy link
Copy Markdown
Member

I am still not wild about the clash case, but I basically think it doesn't matter in practice, and will defer to your judgement.

Still lightly -1 on the tuple/named inconsistency but you've thought about it more than I have and I agree there's an annoying tradeoff here and that this isn't obviously the wrong point in tradeoff space, so OK. I think being able to pass gs::default() is going to be covering up a lot of the API awkwardness in the tuples case, but we do at least have gs::default() so that at least bounds the awkwardness.

@Liam-DeVoe
Copy link
Copy Markdown
Member Author

I have strong feelings about the clash case and weak feelings about the tuple case. An additional negative mark against my tuple semantics: you can't easily configure fields where the value of one depends on the other. With the |g| syntax, you can do full imperative code and then configure the fields. With .field(gen1, gen2), you'd do a composite + destructuring externally, which is annoying.

@DRMacIver
Copy link
Copy Markdown
Member

An additional negative mark against my tuple semantics: you can't easily configure fields where the value of one depends on the other

Hmmm actually yeah I think that convinces me I want to be able to use the closure method here. I would like to be able to override these generators entirely.

I'd be sortof OK with having both if you had strong feelings about wanting the (admittedly more convenient for the common case!) version with direct arguments.

@Liam-DeVoe
Copy link
Copy Markdown
Member Author

Liam-DeVoe commented Mar 30, 2026

I played with this for a while because I do think the |g| syntax for tuples is bad, for the two reasons I listed above:

  • more characters to type when configuring one field
  • needing to look up what name we give to the autogenerated tuple configuration fields, as we can't use the standard .0 names

I would really like the ability to write the following:

enum_gen.single_field(gen1)
enum_gen.double_field(gen1, gen2)
enum_gen.double_field(compose!(|tc| { let n = tc.draw(...); (gs::just(n), gs::just(n)) })

Of course this is not possible with rust's type system.

I also realized the limitation I pointed out above of dependent generation in unnamed tuple variants not being possible is also true for named tuple variants, because you can't write compose! inline there, and don't always have access to an outer tc to bind to (and even if you did the implicit bind is awkward).

I do feel strongly enough here that I'd at least like to fully explore this design space. Here's a crazy idea: what if we always pass tc, for both named and unnamed variants?

enum_gen.single_field(|tc| { (gen) })
enum_gen.double_field(|tc| { (gen1, gen2) })
enum_gen.named_field(|tc, g| { g.x(gs::just(42)})

Yes, |tc| { (gen) } is sad and verbose when you don't need to configure the generator. But this totally unifies derived enum configuration.

I actually think in some sense this might be the only option available to us after applying restrictions from both the rust language and our desired usage forms (setting aside multiple methods).

@DRMacIver
Copy link
Copy Markdown
Member

DRMacIver commented Mar 30, 2026

you can't write compose! inline there

Huh? Why not? Doesn't enum_gen.named_field(|g| compose!(|tc| ...)) work?

setting aside multiple methods

I don't think we should necessarily set aside multiple methods fwiw.

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.

Better syntax for configuring #[derive(DefaultGenerator)] generators on enum tuple variants

2 participants