generate enum variant accessors

[GoldsteinE]

(imagine it’s a tracking issue for what was proposed in this comment now)

• parsing
• testing
• docs

variant classes:

• match
• match_ref
• match_mut
• check
• constructor from fields
• field access

enum class:

• match
• match_ref
• match_mut
• constructors from variants

original text

hi! currently, to use rust enums in cpp you have to manually write and bind functions that extract data from enum variants. this patch adds automatic generation of .unwrap_Variant() methods for the trivial case: tuple variants that only have a single field

this restriction avoids the question of how the return type should look for more complex cases: I think for tuple variants with multiple fields we should return std::tuple, and for struct variants we should codegen a helper struct, but in these cases there’re multiple options, and a single-field tuple variant case is straightforward. at the same time, this has a lot of utility, as this case is really common, and all the other cases can be emulated by putting a struct inside the single field

(unfortunately, this also generates an unwrap method for single-field tuple structs, as zng format does not distinguish between structs and enums. I could gate the generation on having multiple constructors though, let me know what you think)

obviously, this will also need tests and documentation, which I’m happy to write if you accept the idea in general

[GoldsteinE]

out of interest, I tried to also allow multi-field tuple variants, but doing that via std::tuple requires modifications to cpp function template: there’s no good way to create an std::tuple on rust side, so cpp wrapper needs to pass separate pointer for every field. I’ll think about it some more, but I’m not yet sure what the best way to achieve this.

[hkalbasi]

Hi! Thanks for the PR. I think you are addressing a real problem, I thought about this problem a little, let's explore the problem space.

The problem is that there is Rust's match (and it's syntax sugars if let, let else, ...) which C++/Zngur doesn't have, so using of ADT types is limited to Rust defined methods.

The match syntax does these things:

1. Switch casing over variants
2. Checking if a variant is match, i.e. matches!(value, Variant { .. })
3. Moving out of fields of variants
4. Getting &/&mut reference of variant fields from &/&mut value

If I want to design a solution addressing all of these on the fly, I would add a class member for each variant into the enum, and each variant class has these static functions on it:

1. match(Enum) -> Variant
2. match_ref(Ref<Enum>) -> Ref<Variant>
3. match_mut(RefMut<Enum>) -> RefMut<Variant>
4. check(Ref<Enum>) -> bool
5. Constructor from fields

And enum will get these static function members:

1. match(Enum) -> std::variant<Variant1, Variant2, Variant3>
2. Constructor from all variants

These variant classes can get field access with similar trick that we use for structs. Inside them, there is only an Enum data member which is the Enum that we know is in the variant state. So your unwrap_Variant would become something like this:

Field f = e.unwrap_Variant();
// Becomes
Field f = Enum::Variant::match(e).f0;

What do you think about this? I generated this on the fly, so probably there is something I missed. And names are definitely subject of bikeshed.

I would like to determine the general solution first, then if you want to implement only a part of it (e.g. for single field variants) that's fine as long as it's compatible with the big picture plan.

[GoldsteinE]

hi! thanks for you thoughts.

do I understand correctly that Enum::Variant would hold the Enum-sized byte array, just like the Enum itself does? (and then match_ref and match_mut are just check + pointer cast). I think I can try implementing this, although I’ll need to research how you do fields (I hope I can reuse some of the code generating infrastructure, since generating a variant class should be really similar to generating a struct).

this syntax is nice and general, although it might look a bit more verbose, e.g.

if (val.matches_Variant()) {
    auto val = val.unwrap_Variant();
}

turns into

if (Enum::Variant::check(val)) {
    auto val = enum::Variant::match(val).f0;
}

but I don’t think it’s a big deal. either way, I’m happy to try implementing (some subset of) your design (probably starting with check() and match(), since that’s what I need right now lol, but I’ll try to get more methods done)

[GoldsteinE]

also would probably need .match_ref() and .match_mut() on enum, corresponding to match &en and match &mut en?

[GoldsteinE]

I want to also share my thoughts on implementation so you can verify if that’s reasonable.

1. I want to add a new variant to RustType representing individual enum variants. based on that I’ll generate a faux ZngurType to reuse the field & constructor generating infrastructure. this faux type inherits layout from the parent. this covers field access and constructor from fields.
2. this does not cover checking and matching methods, which need to be generated separately. all the match{_ref,_mut,} methods on variant types seem relatively straightforward and basically compile into check + transmute. I’ll add new methods to RustFile for that, and corresponding code to cpp_header.sptl. (if I undestand correctly, basically all the C++ code generation happens via askama, as opposed to Rust code generation, which mostly uses write!())
3. all the static functions on enum need to be written manually, again in cpp_header.sptl. I think no new code needs to go into C++ source file.

does that look right? if so, I’ll start implementing that.

[hkalbasi]

do I understand correctly that Enum::Variant would hold the Enum-sized byte array

It can hold the Enum itself, saving some code generation about move constructor and similar things.

this syntax is nice and general, although it might look a bit more verbose

What about also providing match_opt, match_opt_ref and match_opt_mut returning std::optional? Then your example becomes:

if (auto val = Enum::Variant::match_opt(val)) {
    auto val = val->f0;
}

also would probably need .match_ref() and .match_mut() on enum, corresponding to match &en and match &mut en?

Makes sense.

does that look right? if so, I’ll start implementing that.

I think you need to change the zng file as well, for annotating fields. A candidate syntax:

type Option<i32> {
    variant Some {
        constructor(i32);
        field 0 (offset = 4, type = i32);
    }
    variant None {
         constructor;
    }
}

Other than that, I think it is good for start.

[GoldsteinE]

What about also providing match_opt, match_opt_ref and match_opt_mut returning std::optional?

maybe, yeah, although the ->f0 part of verbosity is still there. I think it’s also possible to generate std::get_if overloads if we want it to feel more C++-ish.

I think you need to change the zng file as well, for annotating fields. A candidate syntax:

it is a bit unfortunate that this would require mentioning every field twice. maybe variants could have implicit constructors based on their fields?

[GoldsteinE]

one other interesting moment: if not every variant is declared (do we want to support this?), Enum::match() needs something to do in the default case. should it just panic? should this be a compile error? maybe we could have a separate Default pseudo-variant as a catch-all.

[hkalbasi]

it is a bit unfortunate that this would require mentioning every field twice. maybe variants could have implicit constructors based on their fields?

This also exists for struct. We could have a constructor auto or something like that to tell the Zngur generate a constructor based on available fields. But anything we do should be consistent with structs. The idea for structs was that you may want the constructor, but couldn't afford offsets for some fields.

one other interesting moment

I think we have these options:

1. Give up on match on enum, at least for now
2. Runtime panic/exception when this happens
3. Force all variants to be available by a compile error
4. Have a Default variant unconditionally
5. Add some annotation, controlling whether match functions need to be generated, and if so, controlling whether it is exhaustive and generate Default variant only if it is needed

I don't like 2 and 4, and going with 1 makes 3 in future a breaking change (breaking change isn't a hard limit, but I prefer to avoid it).

[GoldsteinE]

the difference with structs is that enum variants are guaranteed to have all fields public. could you explain what do you mean by “couldn’t afford offsets”? I thought offsets only incur runtime cost if they’re used, and otherwise are just a single usize somewhere in the binary.

going with 3 for now is the most conservative choice probably? also while 1 makes 3 a breaking change, it still leaves 5 open (although that’s more boilerplate forever).

[hkalbasi]

I thought offsets only incur runtime cost if they’re used, and otherwise are just a single usize somewhere in the binary.

Offsets were mandatory at the time, we then got runtime offsets later. So we may want to revisit this (but I still want consistency between structs and variants).

the difference with structs is that enum variants are guaranteed to have all fields public.

If a field is not public for a struct, the constructor is not usable.

going with 3 for now is the most conservative choice probably? also while 1 makes 3 a breaking change, it still leaves 5 open (although that’s more boilerplate forever).

👍 . It is not forever (yet), it just creates some barrier for adopting 1. I'm not afraid of doing breaking changes (this PR is going to be a breaking change, breaking constructor Some(i32)) I just want to not do it for no reason.

[GoldsteinE]

If a field is not public for a struct, the constructor is not usable.

yeah, and that is not a concern for enum variants, because all the fields are always public, so you can always specify every field. on the other hand, you may not be able to specify some struct field, in which case we can’t auto-generate a constructor.

[hkalbasi]

yeah, and that is not a concern for enum variants, because all the fields are always public, so you can always specify every field. on the other hand, you may not be able to specify some struct field, in which case we can’t auto-generate a constructor.

This is fine for an explicit constructor auto since user will just get a compile error and remove that line, but if we want to implicitly generate constructors for all enum variants, then we need to diverge. If you want to go that path and only have field annotations for variants, then it's fine to not making it consistent with structs.

[GoldsteinE]

I think implicitly generating constructors for enum variants makes sense, but if you want full consistency, I can try implementing constructor auto

[hkalbasi]

I'm fine with implicit constructor for variants, and not touching structs in this PR.

[GoldsteinE]

status update: making Variant a ZngurType turned out to be kind of annoying, since ZngurType has a lot of stuff that’s totally irrelevant for variants, including layout. maybe it would be easier to diverge somewhere down the line, and make variants a separate struct.

side note: is there any way to improve cargo check times on zngur-parser? it takes ~3.5 minutes on my laptop (apparently spent in borrowck), and it turns out I was quite used to interactive on-the-fly diagnostics and fast cargo test loop lol

[GoldsteinE]

this PR is going to be a breaking change, breaking constructor Some(i32))

do I understand correctly that I should remove enum constructors completely, replacing them with variant declarations (rather than letting the two coexist)?

[GoldsteinE]

status update update: made a checklist tracker in the original post.

I need to do field support now, and I’m a bit intimidated by all the machinery. I definitely don’t want to duplicate all this code, so ideally it would be generic for type fields and enum variant fields. I think basically the only thing that would change between type fields and variant fields is the base pointer, right? could you point me to where this base pointer is created/passed?

[hkalbasi]

is there any way to improve cargo check times on zngur-parser? it takes ~3.5 minutes on my laptop (apparently spent in borrowck), and it turns out I was quite used to interactive on-the-fly diagnostics and fast cargo test loop lol

You can download a faster laptop with more ram. The problem is the chumsky crate we use for parser. The chumksy has some advice on this topic that you can try.

do I understand correctly that I should remove enum constructors completely, replacing them with variant declarations (rather than letting the two coexist)?

I think so.

could you point me to where this base pointer is created/passed?

The black magic trick is that you create an annonymous union of the data and all fields. So the field would get the same address of the struct/variant data. So you can cast this to Variant*/Enum* and expect it to work.

[GoldsteinE]

okay, thanks, I’ll try adapting all this magic so it hopefully also works with variant classes without separate implementation

The black magic trick is that you create an annonymous union of the data and all fields. So the field would get the same address of the struct/variant data.

this is cool, but isn’t it kinda union punning between field types, which should be UB in C++?.. or is the fact that no actual memory access happens saves us here?

[hkalbasi]

is the fact that no actual memory access happens saves us here

I think so. The act of taking pointer to inactive members is not UB, but dereferencing that pointer is UB (which we don't).

[GoldsteinE]

okay, so the approach used for structs unfortunately won’t work for enums, because offset_of!(), required both for assertions with static offsets and for auto offsets does not work on enums. barring absolutely ridiculous shenanigans like abusing the optimizer and then extracting this info by passing an uninit value I think we straight up can’t get access to these offsets without having an enum value.

there’re two worse versions that I see, but maybe there’s something better I’m missing:

1. we could calculate them lazily on first access. this makes first field access unpredictably longer and turns a regular load into atomic relaxed load + (very predictable) branch for subsequent accesses. dirty, but probably not that awful?

2. we could instead generate projection functions mapping a reference to the variant to a pointer to the field. I’m not sure if it’s more or less overhead, since it’ll be an FFI call for every field every time, but no atomics and no conditionals. also all the accesses take the same time in this case.

in either case, we could probably reuse most of the logic still, as basically only offset calculation changes. also, in either case there’s seemingly no point in supporting non-auto offsets, as we have no real way of checking them in compile time (well, I guess we could if the enum implements Default or something, but this feels really weird).

of these two options, I think option 2 is more sensible, as it doesn’t introduce mutable state.

[GoldsteinE]

I pushed a version that covers all the API surface you described. the tests will fail rn, since I yeeted named constructors in favour of variants, but otherwise it should be ready. this is a semi-large patch, so I pushed the implementation while I work on tests and docs.

[GoldsteinE]

status update: porting rustyline example kinda forces a decision on exhaustiveness. fully describing its Error type is way too messy.

interestingly, there’re two kinds of nonexhaustiveness:

• zng could skip describing some variants, in which case we can’t generate Enum::match(), but can generate basically everything else
• or it could skip describing some variant fields, in which case we can generate Enum::match(), but can’t auto-generate a constructor

I propose a new non_exhaustive annotation (mirroring rust’s attribute) that covers both cases: when applied on type-level, it disables Enum::match() generation, and when applied on variant-level it disables variant constructor generation

(alternatively, it could add a Default option into variant, as you proposed)

[hkalbasi]

I like omitting the match function more than Default. People can always use if Var1::match else if Var2::match.

[GoldsteinE]

status update: implemented non_exhaustive, fixed every (hopefully) example, fixed a bunch of bugs. would be nice to have a CI run now, because I didn’t actually test tutorial-wasm32 (I have a very-much non-rustup setup).

I will do book changes next, although I’m not sure it would be very soon, since I’ll be away for some time.

[GoldsteinE]

another note: migrating examples revealed some usability papercuts. I think the most annoying by far is that when you do stuff like

auto meow = Enum::Variant(42);

meow has type Enum::Variant, not type Enum, and so has none of its methods. you need to write out

Enum meow = Enum::Variant(42);

which is annoyingly repetitive (and it’s not intuitive that this is what you need to do from the error). I’m not sure what to do with this. one band-aid would be to provide some kind of upcast method, like Enum::Variant(42).bikeshed() or smth, but that’s still not pretty. the same problem goes for lambdas, where returning different variants from different branches leads to inference failures.

some other issues:

• x.matches_Variant() is much shorter than Enum::Variant::check(x), we might want to restore that? same with Enum::Variant::match(x), a method notation might be desirable.
• in template functions Enum::Variant is a dependent name, requiring typename Enum::Variant (but at least the compiler tells you what to do)

[GoldsteinE]

as a wild idea, variants might inherit from the enum class instead of containing it, but that would need a different solution for fields, since we can’t put a superclass into a union. might have other problems, C++ inheritance is not something I’m very familiar with

[GoldsteinE]

thanks! I somehow missed a test I guess, I’ll re-check

[hkalbasi]

migrating examples revealed some usability papercuts

I thought about this problem. The inheritance is dead end (It breaks the union trick, won't fix the lambda problem, make upcasting ambiguous, adds a vtable member to every enum, ...) but I had thought of two more solutions you didn't mention:

• Repeat all methods for every variant as well. Still won't fix the lambda problem, and makes a n*m code generation problem (which is bad due compile times).
• Return Enum directly. So we could not use the constructor and need a build static function.

I concluded that Enum(Enum::Variant(1)) was not that bad, so I didn't bring it up. I still think Enum::Variant(1).bikeshed() is not much better than Enum(Enum::Variant(1)) and given that the latter already works, do we need the bikeshed method? I don't like my solutions either, and would say the status quo is good enough, but don't have a strong opinion here so brought them up so that .

x.matches_Variant() is much shorter than Enum::Variant::check(x), we might want to restore that? same with Enum::Variant::match(x), a method notation might be desirable.

I prefer to not have methods on types, to prevent shadowing original methods. Enum::Variant::check(x) only has the additional Enum:: which could be using ed away if it becomes repeated.

in template functions Enum::Variant is a dependent name, requiring typename Enum::Variant (but at least the compiler tells you what to do)

I hate C++. Probably we can't do much here.

I will do book changes next, although I’m not sure it would be very soon, since I’ll be away for some time.

That's fine, and thanks for the great work on this PR!

[GoldsteinE]

I still think Enum::Variant(1).bikeshed() is not much better than Enum(Enum::Variant(1)) and given that the latter already works, do we need the bikeshed method?

I think my example here hid the problem. consider:

Result<Unit, ReadlineError>(Result<Unit, ReadlineError>::Err(err))

this is really annoying to write, doubly so because irl it would be in some kind of namespace, requiring either a lot of using or even more typing.

the same goes for all the other free functions.

to prevent shadowing original methods

I understand this concern, but I feel like a weird enough naming convention (like the original check_Variant, which mixed snake and camel case) may be enough to avoid this problem in practice.

[GoldsteinE]

noooo, not MSVC т_т

[GoldsteinE]

that is really fun! I love microsoft visual studio c++ compiler!

https://devblogs.microsoft.com/cppblog/msvc-now-correctly-reports-__cplusplus/

hold on a sec, I’ll fix all the nmakefiles

[GoldsteinE]

oh, right, this was a thing too. sorry to make you re-approve it so many times, I just don’t have MSVC locally

[hkalbasi]

oh, right, this was a thing too. sorry to make you re-approve it so many times, I just don’t have MSVC locally

No worries. It's such a dumb feature of github.

[hkalbasi]

It's fine when we know the output is u32. Also, we can use a simple pointer instead of a pointer to a pointer. The out pointer and pointer to pointer is for when we want to be generic over everything.

[hkalbasi]

Let's just add a || !defined(__cplusplus) to our guards, to not requiring a new flag for windows people, and supporting other broken compilers.

[GoldsteinE]

it’s defined, it’s just set to 1997. we could check for _MSVC_LANG, which should work regardless of flags, although it’s really annoying that we have to do this.

[hkalbasi]

It doesn't make any sense. So windows people deserve this misery I guess and we shouldn't help them then.

[hkalbasi]

The name is copy paste.

[hkalbasi]

Is this even used?

[hkalbasi]

So what happens when we call match_ref currently? Does zngur panics?

[GoldsteinE]

nah, this is just unreachable I think, .match_ref() doesn’t use this func. (but maybe it should, I’ll check if that’s possible)

[hkalbasi]

We control this and nothing bad could call it, right? If so, do a unsafe { unreachable_unchecked() } which helps the optimizer optimize this function to a single constant.

[GoldsteinE]

yeah, I’m just reluctant to introduce UB like that, so I did an abort for now. I could do unreachable_unchecked if you prefer that, we do control this and this should be statically prevented from happening.

[hkalbasi]

We will get the UB anyway when we move to offsetof!, so I think the risk of UB here is justified.

[hkalbasi]

Yes I meant a normal method. match_ref and match_mut are not keywords, but they aren't there?

[hkalbasi]

I'm fine with those being a normal method as well. My requirements are a bit fuzzy :). Also, using static methods won't help in avoiding name conflict since C++ is garbage static methods are also available as obj.method syntax. Function overloading helps us here, but relying on function overloading for preventing conflicts makes things confusing.

[hkalbasi]

You mean calling both functions match by function overloading? Then we would lose Enum e; e.match_mut() but the conflict prevention is nice.

[GoldsteinE]

it’s not really function overloading I guess, because the methods are defined on distinct types, but yeah, that does lose this, and it might be to big of an ergonomic hit

[hkalbasi]

So let's keep them as is (I think Enum e; e.match_mut() already doesn't work in this PR, please add it and use it in tests).