Bug: non-overridden choice in all-choices rule creates struct and not enum

[ethindp]

Idk if this is a bug or I'm doing something wrong, but it definitely looks like a bug. When I do the following:

@leftrec
Name = @:DirectName | @:ExplicitDereference
| @:IndexedComponent | @:Slice
| @:SelectedComponent | AttributeReference:*AttributeReference
| @:TypeConversion | @:FunctionCall
| @:CharacterLiteral | @:QualifiedExpression
| @:GeneralizedReference | @:GeneralizedIndexing
| @:TargetName;

The resulting parser generates the wrong code:

#[derive(Debug, Clone, Serialize, Deserialize)]
pub struct Name {
    pub _override: Option<Name__override>,
    pub AttributeReference: Option<Box<AttributeReference>>,
}
#[allow(non_camel_case_types)]
#[derive(Debug, Clone, Serialize, Deserialize)]
pub enum Name__override {
    CharacterLiteral(CharacterLiteral),
    DirectName(DirectName),
    ExplicitDereference(ExplicitDereference),
    FunctionCall(FunctionCall),
    GeneralizedIndexing(GeneralizedIndexing),
    GeneralizedReference(GeneralizedReference),
    IndexedComponent(IndexedComponent),
    QualifiedExpression(QualifiedExpression),
    SelectedComponent(SelectedComponent),
    Slice(Slice),
    TargetName(TargetName),
    TypeConversion(TypeConversion),
}

I think that, in general, an all-choices rule should always generate an enum -- it should never generate a struct, since that creates error-prone code.

[ethindp]

Alternatively, overrides could be made to allow boxing.

[badicsalex]

Yeah, there's no reason to disallow boxing in override enums.

Also this should have resulted in an error, and definitely not a struct with an _override field.

[ethindp]

@badicsalex Per the documentation, override fields cannot be boxed, and doing @:* anywhere did indeed generate an error. In general I (don't think) it should be an error to box an override field.

[badicsalex]

yeah I deleted my first comment because it was plain wrong.

[badicsalex]

I think there was an issue with overrides being typedefs, but I don't remember. Some new tests will have to be written for sure before enabling boxing.

[ethindp]

Ah okay, I missed your original comment entirely. I have discovered more bugs though, but I can open those as separate issues. I'll offer at least a brief summary:

• The peginator parser currently allows duplicate rules. (I ran into this twice, and it took me a bit to figure out since Rust was the one complaining, not Peginator.)
• The parser uses the name "state" liberally in it's auto-generated code, but allows you to (also) declare a name "state" as a rule field. This results in duplicate name use errors on the Rust side of things.
• The parser allows the use of Rust keywords as rule names. Though this isn't necessarily a "bug", it does create some.... Interesting challenges.
• When a character escape sequence is parsed, it is evaluated at the parse site. I ran into this yesterday; my Unicode rules contained some 5-and 6-byte Unicode character escapes via \u{...}, and after I looked at the generated code it turned out that the escape sequences were being evaluated by peginator and then those evaluated characters were directly inserted into character literals. I'm not sure if these were necessarily "valid" sequences (e.g. one of them was '\u{E01F0}') but they resulted in character literals that had two characters, not one (at least the one I saw, I don't remember which one it was though).

As noted, I can open these as separate issues if wanted, and I'm pretty sure all of these are pretty easy to fix (I think?).

[badicsalex]

For the first three, I always considered the "doesn't error out in peginator but does during rust compilation" kind of errors low priority, because you get the error and it's obvious what's wrong, it's just not pretty.

Please confirm that the unicode thing actually generates wrong code. Supporting actually invalid UTF-8 (i.e. illegal code sequences) is out of scope, since those would be problematic during actual parsing too.

[badicsalex]

A quick precheck on the rules for some of these common issues before actual code generation is certainly possible though.