Skip to content

Union field ordered differently error should report the position the field should be in #18854

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

Open
euclidianAce opened this issue Feb 7, 2024 · 2 comments
Labels
error message This issue points out an error message that is unhelpful and should be improved.
Milestone

Comments

@euclidianAce
Copy link

Zig Version

0.12.0-dev.2636+476ba0475

Steps to Reproduce and Observed Output

Use an out of order tagged union:

pub fn main() void {
    _ = U.a;
}

const E = enum { a, b };
const U = union(E) { b, a };
$ zig run enum.zig
enum.zig:6:22: error: union field 'b' ordered differently than corresponding enum field
const U = union(E) { b, a };
                     ^
enum.zig:5:21: note: enum field here
const E = enum { a, b };
                    ^
referenced by:
    U: enum.zig:6:11
    main: enum.zig:2:9
    remaining reference traces hidden; use '-freference-trace' to see all reference traces

Expected Output

The error should say something along the lines of Expected union field 'b' to be listed 1st (as it is in enum E).

While the error does provide the location of the defined enum, this is not as helpful when the enum is generated via comptime code. I'd argue that maybe the error should only show up for types not defined trough @Type, but I don't have enough knowledge of the compiler to know if that info is tracked or feasible to track.

I attempted to make a patch, but my computer isn't good enough to build the compiler, but here is a simple diff:

--- a/src/Sema.zig
+++ b/src/Sema.zig
@@ -37227,8 +37227,16 @@ fn semaUnionFields(mod: *Module, arena: Allocator, union_type: InternPool.Key.Un
                         .range = .name,
                     }).lazy;
                     const enum_field_src = mod.fieldSrcLoc(tag_info.decl, .{ .index = enum_index }).lazy;
-                    const msg = try sema.errMsg(&block_scope, ty_src, "union field '{}' ordered differently than corresponding enum field", .{
+                    const one_index = enum_index + 1;
+                    const msg = try sema.errMsg(&block_scope, ty_src, "union field '{}' should be listed {}{s}, as it is in the corresponding enum field", .{
                         field_name.fmt(ip),
+                        one_index,
+                        switch (one_index % 10) {
+                            1 => if (one_index == 11) "th" else "st",
+                            2 => if (one_index == 12) "th" else "nd",
+                            3 => if (one_index == 13) "th" else "rd",
+                            else => "th",
+                        },
                     });
                     errdefer msg.destroy(sema.gpa);
                     const decl_ptr = mod.declPtr(tag_info.decl);

Not sure if there is a style guide for error messages or any prior art for the ordinal indicator there, but should be a nice jump start if anyone else wants to take a stab at it.

@euclidianAce euclidianAce added the error message This issue points out an error message that is unhelpful and should be improved. label Feb 7, 2024
@rohlem
Copy link
Contributor

rohlem commented Feb 8, 2024

As for the ordinal, I don't really like the idea of reading the error message and then having to count up to the n-th field in my code by hand to know where to place it.
I think it would be preferable if we told the user after which correctly-ordered field to put the pointed-out field.
So go from first to last field, and then formulate the error message as field 'foo' should be ordered after field 'bar',
or field 'foo' should be the first field if there's no field in front of it.

(I guess it wouldn't really matter whether we use the enum's field order or the union's field order for this.
The user either has to move from arbitrary position to linear order, or from linear order to arbitrary position.)

@schmee
Copy link
Contributor

schmee commented Feb 8, 2024

If we want to improve this right now, I suggest printing out the entire union with the correct field order so the user can copy-paste it straight from the error message. Long term, this might be better solved as part of #17584.

@Vexu Vexu added this to the 0.15.0 milestone Mar 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
error message This issue points out an error message that is unhelpful and should be improved.
Projects
None yet
Development

No branches or pull requests

4 participants