stage2: boolean type equality checking, compile time error comparison, validate switch errors, error set merging

[g-w1]

The theme of this pr is on errors and types.

• boolean type equality checking (const b: bool = error {Thing1, Thing2} == error {Thing2, Thing1}):
  • add error union and error set case to Type.eql()
• compile time error comparison (error.B == error.A)
• validate switch for errors
• merge error sets (var b: bool = error { Oof } || error { Other } == error { Oof, Other })
• tests

[Vexu]

coerce should only check that inst.ty is a subset of dest_type. Error set widening should be happening in resolvePeerTypes.

[g-w1]

I am a little confused. If one can be coerced into another, I shouldn't do it and just return inst? Or should I make a new inst instead of modifying the other inst in place? If making a new inst, should I keep that in coerce, or still move it to peer type?

[Vexu]

If inst.ty can coerce into dest_type then you wouldn't need to modify anything or return a new inst since inst.yt would be a subset of dest_type and all of its errors can already be found in dest_type.

[g-w1]

wouldn't need to modify anything or return a new inst

I think I am understanding you wrong. With a diff of this: (just removing the code that changes the inst)

diff --git a/src/Module.zig b/src/Module.zig
index ca2282060..e6f601738 100644
--- a/src/Module.zig
+++ b/src/Module.zig
@@ -2841,33 +2841,33 @@ pub fn coerce(self: *Module, scope: *Scope, dest_type: Type, inst: *Inst) !*Inst
                             while (it.next()) |entry| {
                                 if (dt_e.get(entry.key) == null) break :blk; // the smaller set has a key not in the larger set
                             }
-                            var it_dest = dt_e.iterator();
-                            while (it_dest.next()) |entry| {
-                                try fields.put(self.gpa, entry.key, entry.value);
-                            }
+                            // var it_dest = dt_e.iterator();
+                            // while (it_dest.next()) |entry| {
+                            //     try fields.put(self.gpa, entry.key, entry.value);
+                            // }
                         }
                         return inst;
                     },
                     .err_single => |name| blk: { // dont have to deinit anything because it is a slice
                         const dt_e = dest_type.getErrs().multiple;
                         if (dt_e.get(name) == null) break :blk; // the smaller set has a key not in the larger set
-                        var new_decl_arena = std.heap.ArenaAllocator.init(self.gpa);
-                        errdefer new_decl_arena.deinit();
-                        const payload_val = try scope.arena().create(Value.Payload.ErrorSet);
-                        payload_val.* = .{
-                            .fields = .{},
-                            .decl = undefined, // populated below
-                        };
-                        payload_val.fields = try dt_e.clone(&new_decl_arena.allocator);
-                        // TODO create name in format "error:line:column"
-                        const new_decl = try self.createAnonymousDecl(scope, &new_decl_arena, .{
-                            .ty = Type.initTag(.type),
-                            .val = Value.initPayload(&payload_val.base),
-                        });
-                        payload_val.decl = new_decl;
-                        const payload_ty = try scope.arena().create(Type.Payload.ErrorSet);
-                        payload_ty.* = .{ .decl = new_decl };
-                        inst.ty = Type.initPayload(&payload_ty.base);
+                        // var new_decl_arena = std.heap.ArenaAllocator.init(self.gpa);
+                        // errdefer new_decl_arena.deinit();
+                        // const payload_val = try scope.arena().create(Value.Payload.ErrorSet);
+                        // payload_val.* = .{
+                        //     .fields = .{},
+                        //     .decl = undefined, // populated below
+                        // };
+                        // payload_val.fields = try dt_e.clone(&new_decl_arena.allocator);
+                        // // TODO create name in format "error:line:column"
+                        // const new_decl = try self.createAnonymousDecl(scope, &new_decl_arena, .{
+                        //     .ty = Type.initTag(.type),
+                        //     .val = Value.initPayload(&payload_val.base),
+                        // });
+                        // payload_val.decl = new_decl;
+                        // const payload_ty = try scope.arena().create(Type.Payload.ErrorSet);
+                        // payload_ty.* = .{ .decl = new_decl };
+                        // inst.ty = Type.initPayload(&payload_ty.base);
                         return inst;
                     },
                     else => unreachable,
@@ -2877,11 +2877,11 @@ pub fn coerce(self: *Module, scope: *Scope, dest_type: Type, inst: *Inst) !*Inst
             .anyerror => {
                 switch (gotten_err_set) {
                     .multiple => |fields| {
-                        inst.ty = Type.initTag(.anyerror);
+                        // inst.ty = Type.initTag(.anyerror);
                         return inst;
                     },
                     .err_single => |_| {
-                        inst.ty = Type.initTag(.anyerror);
+                        // inst.ty = Type.initTag(.anyerror);
                         return inst;
                     },
                     else => unreachable,

I get errors like this when running the test

❯ zig build test-stage2 -Denable-qemu
Test [1/30] test "self-hosted"... tests [1/49] referencing decls which appear later in the file [1/1] up
tests [33/49] switch and coerce error sets [2/4] update [3/3] switch and coerce error sets
Unexpected error:
================
:6:14: error: expected _start__anon_12, found error{Z}
================
Test failed.
error: the following test command failed with exit code 1:
/home/jacob/dev/zig/zig-cache/o/410e73f2855175dd2c3126a64c2793dc/test
The following command exited with error code 1:
/home/jacob/.local/zig/0.7.1+8fc765387/files/zig test /home/jacob/dev/zig/src/test.zig --pkg-begin build
_options /home/jacob/dev/zig/zig-cache/test_build_options.zig --pkg-end --cache-dir /home/jacob/dev/zig/
zig-cache --global-cache-dir /home/jacob/.cache/zig --name test --pkg-begin stage2_tests /home/jacob/dev
/zig/test/stage2/test.zig --pkg-end
error: the following build command failed with exit code 1:
/home/jacob/dev/zig/zig-cache/o/ff28c7d5a2605f406d44b22dd03068c7/build /home/jacob/.local/zig/0.7.1+8fc7
65387/files/zig /home/jacob/dev/zig /home/jacob/dev/zig/zig-cache /home/jacob/.cache/zig test-stage2 -De
nable-qemu

which is expected because it is not coercing anything. I can understand that it is bad to modify the type of the inst, but I am confused where to go from here.

Also, when I do the widening in resolvePeerTypes, should I modify the errorset in place to widen it, or make a new one? Thanks!

[g-w1]

Does 0b6e935 fix this comment? I think I will add peer type resolution as a separate pr with coercing error sets to error unions, and values to error unions.

[g-w1]

Sorry for the 2 merge conflicts at the end, I know they clutter stuff up. I tried to rebase, but it was asking me to resolve conflicts multiple times, so I thought a merge would be easier, but the downside is that it creates commits.

[andrewrk]

I think this is probably not the right direction for the codebase to go, and any further work on error sets needs to incorporate inferred error sets.

There is too much unrelated stuff in this PR across too many commits. Can you try to break this into smaller, more mergeable chunks?

[andrewrk]

I don't think putting this in type.zig is viable - we need the concept of "resolving" an error set which is the part where it figures out what all errors are in the set. Resolving an error set could cause compile errors, which will need to be attached to a Module. So I think this code does not belong here.

[andrewrk]

This logic is not correct, because (error{Foo} || anyerror) == anyerror is true. An inferred error set could be the LHS too. And again the concept of "resolving" an error set applies here. I think error sets are one of the trickier things to get right here and I don't want to go down a path that leads us towards bugs. Probably even the first implementation of error sets should include inferred ones.

[g-w1]

Yes, I think it is a good idea to split this into multiple prs. Thanks for the feedback. PRS incoming!