Change grammar to require or disallow types on fields for different containers

[mattbork]

The grammar should be more specific about what kind of fields a container allows. struct and untagged union containers should mandate types for fields, whereas tagged union containers may omit the type. Further, enum containers should not allow types for fields at all. Examples:

const T = struct { a: u32 }; // always require types on fields in structs
const E = enum { a, b }; // don't allow types to be specified here
const U1 = union { a: u32 }; // always require types on fields in untagged unions
const U2 = union(E) { a: u32, b } // types may be present or omitted in tagged unions

This requires separating ContainerMembers from the new EnumMembers and TaggedUnionMembers nonterminals. To reduce duplication, container declarations are first pulled out into a new nonterminal, ContainerDeclarations. This also allows making the mixing of declarations in between container fields a parse error: all containers have a possibly empty prefix of declarations, followed by a possibly empty list of fields, followed by a possibly empty suffix of declarations.

All tests (expecting the invalid_ ones) pass except for async_fn.zig, which was already failing before these changes.

[andrewrk]

@SpexGuy how comfortable are you with reviewing grammar changes? I feel this is one of my weak spots, I tend to make a lot of grammar mistakes. Never mind, I think @ifreund has it under control :)

[ifreund]

The first commit here should definitely be merged as it brings the grammar back in sync with the implementation.

I'm not yet sure whether splitting the current ContainerField/ContainerMembers by container type is the right approach to take though. What we need to consider here is whether the overall complexity of the compiler implementation will be reduced or increased by this change. Right now, I'm pretty sure it would be increased as what is currently a single code path in the parser would be split into 4. Also, none of the errors that would be caught during parsing with this grammar change would make it past the new zig ast-check command, so I don't think catching them earlier is beneficial enough to justify this change. Furthermore, error messages generated during astgen are usually better as they have more context available.

[mattbork]

For the sake of seeing all the required changes at once, I've now accounted for the differences between bare unions (must have types), unions with implicit tags (can have tag assignments and omit types), and unions with explicit tags (can omit type but must not have tag assignments). There are now five different *Members / *Field / *FieldList sets of nonterminals, which is indeed a substantial complication of the grammar. One benefit, perhaps, is that now it is clear that Root is the same as the inside of a struct, matching how files are treated by @import.

[ifreund]

Having thought about this a bit more the past few days, I don't think that differentiating between container types in the grammar makes sense for the zig compiler implementation. Compared to the status quo it would cause a significant increase in code duplication in the parser and zig fmt. The behavioral impact would be that some compile errors would be moved from astgen to parsing, but no new compile error messages would be possible and the quality of the error messages would be, if anything, reduced. Due to the fact that we now have a zig ast-check command to check for these errors without performing a full compilation, I don't think this would be worth it.

On the other hand, we definitely do want the change to disallow declarations between fields in the grammar merged. It's up to you @mattbork how to extract that from the rest of these changes, closing this PR and opening a new one might be simplest.

[mattbork]

I'll be happy to close this and open a new one for just the field and declaration split. My one concern, having looked at parse.zig more, is how this affects recovery on parsing errors. Currently, after an error in a parseContainerMembers, findNextContainerMember is used to skip over tokens to find what is likely the start of another member. Because fields and declarations are mixed, recovery is more likely. Splitting fields and declarations will mean separating findNextContainerDeclaration, which will be almost the same as for members, and findNextContainerField, which will only be able to stop on .keyword_comptime or .identifier. Further, when declarations occur between fields, parseContainerMembers goes into an error state but continues parsing members. I assumed this is to allow later parsing errors in a struct or at top level to be found and reported at the same time as the error for declarations between fields, though as far as I can tell the compiler stops after reporting only the latter.

[ifreund]

I'm not sure I understand your concern, splitting findNextContainerMember() shouldn't be necessary as we don't care if a field or decl is found. After the first error all we want to do is find as many further errors as possible before exiting.

Recovery for the decl_between_fields error works for me in this case at least:

const Foo = struct {
    a: u32,
    const x = 42;
    b: u32,

    const y = 42..;
};

/tmp/foo.zig:3:5: error: declarations are not allowed between container fields
    const x = 42;
    ~~~~~
/tmp/foo.zig:6:17: error: expected ';', found '..'
    const y = 42..;
                ~~

[mattbork]

You're right, there's no need to split findNextContainerMember. My thinking was, assuming parse.zig will be changed to match the grammar with new parseContainerDeclarations and parseContainerFields functions, then the new parseContainerMembers would look schematically like

parseContainerDeclarations
while parseContainerField
    expect(.comma)
if !parseContainerField
    parseContainerDeclarations

Because parseContainerDeclarations has to assume a valid call of parseContainerField may follow, it will have to return if the start of a field is found. If this happens in the second parseContainerDeclarations, it'll leave some members unread before the end of the container. So the end of the new parseContainerMembers will have to peek at the next token. If it isn't .r_brace or .eof, then it will have to eat everything until a plausible end of container is found. I suppose I thought somehow this would mean there would be no opportunity to also parse those leftover members, but it should only take a loop of calling parseContainerField and parseContainerDeclarations.

Thanks for all the feedback. After I make a PR for the grammar changes here, I'll give the parse.zig changes a try.

I am confused though why I can't get multiple errors after a declaration between fields, on my Windows machine or on godbolt. The only thing I can think of is that zig nightly and the trunk zig on godbolt are compiled without logging enabled?

[ifreund]

We don't necessarily need to make any changes to the code in parse.zig since it already adheres to the new grammar. There's no need to make the functions there correspond one-to-one with the grammar rules, especially if this causes complexity with regards to recovery or other issues. See for example parseTypeExpr() which handles several sub-rules at once. I think the only change to parse.zig we'd really need to make is to update the comment on parseContainerMembers() to match the corrected grammar.

I am confused though why I can't get multiple errors after a declaration between fields, on my Windows machine or on godbolt. The only thing I can think of is that zig nightly and the trunk zig on godbolt are compiled without logging enabled?

Hmm, not sure what's going on here, does the test case I used above not work for you? I can add as many copies of the const y decl as I want and get an error for each one.

[mattbork]

Okay, that makes sense. The error recovery is definitely clearer when declarations and fields are handled in one function.

Hmm, not sure what's going on here, does the test case I used above not work for you?

It doesn't. In fact, no errors anywhere in the file are reported after the first. I would have assumed this was a problem on my end, but again Compiler Explorer has the same behavior and on 0.7.1 and older as well on trunk.

[ifreund]

Oh I'm pretty sure I know whats happening here, I'm testing the self hosted parse.zig only by using zig fmt, where as if you compile normally you will be using the stage1 parser written in C++ which doesn't do any recovery.

[mattbork]

Ok, good to know there's nothing wrong. I haven't looked at the stage1 parser much and assumed it did the same recovery. Thanks!