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: improve behavior of noreturn #12462

Merged
merged 4 commits into from
Aug 18, 2022
Merged

Conversation

Vexu
Copy link
Member

@Vexu Vexu commented Aug 17, 2022

Closes #12459
Closes #7913
Closes #3461 for stage2
Closes #3257

@Vexu
Copy link
Member Author

Vexu commented Aug 17, 2022

This should now implement #3257 in stage2 thought I'm not sure whether tagged unions were also supposed to be allowed to be empty.

@ifreund
Copy link
Member

ifreund commented Aug 17, 2022

This should now implement #3257 in stage2 thought I'm not sure whether tagged unions were also supposed to be allowed to be empty.

I believe this comment on that issue describes the desired semantics for empty tagged unions and enums: #3257 (comment)

@Vexu Vexu force-pushed the stage2-noreturn branch 2 times, most recently from 936fab8 to 58ac632 Compare August 17, 2022 18:24
Comment on lines +22 to +28
const U = union(enum) {
a: noreturn,
b: void,
};
var e = @typeInfo(U).Union.tag_type.?.a;
var u: U = undefined;
u = e;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this is an interesting one. I think it would make more sense for the enum tag type of unions to exclude noreturn fields. In this case @typeInfo(U).Union.tag_type would be effectively equivalent to enum { b }.

This would move the compile error in this example to the var e = @typeInfo(U).Union.tag_type.?.a; instead giving an unknown field error.

The major difference would be that this example would compile if var e = @typeInfo(U).Union.tag_type.?.b; was used instead.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That does sound better and more like what was described in the issue. I'll see if it works without too much issues.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excluding the noreturn fields from the enum changes the errors like so:

-// :19:10: error: cannot initialize 'noreturn' field of union
-// :16:9: note: field 'a' declared here
-// :15:15: note: union declared here
-// :28:9: error: runtime coercion from enum '@typeInfo(tmp.entry3.U).Union.tag_type.?' to unio
n 'tmp.entry3.U' which has a 'noreturn' field
-// :23:9: note: 'noreturn' field here
-// :22:15: note: union declared here
+// :19:10: error: no field named 'a' in enum '@typeInfo(tmp.entry2.U).Union.tag_type.?'
+// :15:15: note: enum declared here
+// :26:43: error: enum '@typeInfo(tmp.entry3.U).Union.tag_type.?' has no member named 'a'
+// :22:15: note: enum declared here

... but it also completely breaks the test added to union.zig and doesn't apply when the union is passed an already existing enum type. Would it instead be better to add a runtime safety check for attempting to initiate noreturn field of a union?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also turn the other errors about runtime coercions of enums to unions into runtime safety checks allowing code such as:

test {
    const E = enum { a, b };
    const U = union(E) {
        a: void,
        b: u32,
    };
    var u: U = undefined;
    var e: E = .a;
    u = e;
}

and

test {
    const E = enum(u8) { a, b, _ };
    const U = union(E) {
        a: void,
        b: void,
    };
    var u: U = undefined;
    var e: E = .a;
    u = e;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, I see now why that idea doesn't really work. I suspect the primary use-case for this feature will be exclusion of tagged union fields based on a comptime known value (e.g. a build option), a use-case that requires being able to write switch statements the way you have demonstrated in the new union.zig tests.

Would it instead be better to add a runtime safety check for attempting to initiate noreturn field of a union?

Hmm, this should be able to stay a compile error in all cases where the field being initialized is comptime-known. If I understand correctly, this example which coerces a runtime known enum value to a tagged union value is the only case in which the field being initialized isn't comptime known. It seems reasonable to me to make this a runtime safety check instead.

I don't think I've ever seen this feature of coercing a runtime known enum value to a tagged union used in practice and I question its usefulness. It seems to only be allowed for tagged unions with only zero-bit types as fields. Perhaps there is some metaprogramming related use-case I'm missing, but I personally question why this feature exists in the first place.

Copy link
Member

@ifreund ifreund Aug 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also turn the other errors about runtime coercions of enums to unions into runtime safety checks allowing code such as ...

Good examples, I personally think that the runtime safety check in these cases would be too implicit and that we should prefer an explicit switch instead. For example:

test {
    const E = enum { a, b };
    const U = union(E) {
        a: void,
        b: u32,
    };
    var u: U = undefined;
    var e: E = .a;
    u = switch (e) {
        .a => .a,
        .b => unreachable,
    };
}

This keeps the language a bit simpler and IMO makes the resulting code more readable and the intent easier to see.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given these examples, I'm starting to think you were right to make this kind of runtime coercion from enum to tagged union a hard compile error for tagged unions with noreturn fields in the first place.

I still question why we allow this coercion at all, but I don't think that decision needs to block this PR.

@Vexu Vexu merged commit b038dba into ziglang:master Aug 18, 2022
@Vexu Vexu deleted the stage2-noreturn branch August 18, 2022 14:19
TUSF pushed a commit to TUSF/zig that referenced this pull request May 9, 2024
Stage2: improve behavior of noreturn
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants