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

stage2: astgen unionDecl fixes #9925

Merged
merged 2 commits into from
Oct 10, 2021
Merged

Conversation

mattbork
Copy link
Contributor

When unionDeclInner builds the trailing data for its UnionDecl payload, the bit to indicate that a field has a type is set if member.ast.type_expr != 0. However, the Ref to the type is only actually included in the field data if additionally node_tags[member.ast.type_expr] != .@"anytype". Anything walking over the trailing field data for a union with an anytype field will start reading garbage after this point. For example, running ast-check -t on

const U = union(enum) {
	a: anytype,
	b: u32,
	c: f32,
};

panics with an index of out bounds on my machine.

These changes revise #9235, which fixed an unreachable statement being hit in astgen's expr. The problem there was that an .anytype field type node was not handled specially and was being passed to typeExpr, which calls expr, which explicitly says a tag of .anytype is unreachable. Instead, anytype union fields should, like anytype struct fields, write an explicit .none for field_type in the UnionDecl payload. Omitting the field type is already used with tagged unions to indicate an inferred void type. This distinction is also missing from semaUnionFields, which currently treats a missing type and a .none type as both indicating a void field type, so I followed the example of semaStructFields and set the Type of an anytype field to .noreturn.

The other fixes for unionDeclInner are just error reporting. First, when error is reported for a field assignment in a union without an explicit integer tag type, this is done by checking whether arg_node is unset. This only catches the cases of union and union(enum) and not the case of a union with an explicit tag type. So, for example,

const E = enum { a, b };
const U = union(E) {
	a: u32 = 1,
	b: f32 = 2,
};

passes ast-check. Second, untagged unions cannot omit field types, and astgen can easily check this but isn't currently. I also added tests for these errors to test/stage2/cbe.zig. That seems to be where tests for errors that can be caught this early in compilation are being put, but I'm not sure.

@andrewrk
Copy link
Member

That seems to be where tests for errors that can be caught this early in compilation are being put, but I'm not sure.

Totally fine- easy to move these tests around as we find better ways to organize the tests.

Thanks for this fix!

@andrewrk andrewrk merged commit f42725c into ziglang:master Oct 10, 2021
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.

2 participants