Skip to content

Commit

Permalink
few small things based on review:
Browse files Browse the repository at this point in the history 
* make Type.eql for errors not reach unreachable
* use global_error_set instead of getErrorValue
* use value in analyzeInstCmp instead of type and use the u16 error
  value field instead of the name so that we don't need mem.eql
* clean up code by factoring code from Module.zig -> type.zig so that it can be reused in peer type resolution and coercion
* update after rebasing and fix dumb things that I saw when reviewing my own code
  • Loading branch information
@g-w1
g-w1 committed Jan 2, 2021
1 parent 2441b8e commit e5fcdfa
Show file tree
Hide file tree
Showing 5 changed files with 77 additions and 86 deletions.
63 changes: 4 additions & 59 deletions src/Module.zig
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2753,6 +2753,7 @@ pub fn resolvePeerTypes(self: *Module, scope: *Scope, instructions: []*Inst) !Ty
chosen = candidate;
continue;
}

if (chosen.ty.isInt() and
candidate.ty.isInt() and
chosen.ty.isSignedInt() == candidate.ty.isSignedInt())
Expand Down Expand Up @@ -2865,65 +2866,9 @@ pub fn coerce(self: *Module, scope: *Scope, dest_type: Type, inst: *Inst) !*Inst

// error set widening
if (inst.ty.zigTypeTag() == .ErrorSet and dest_type.zigTypeTag() == .ErrorSet) {
const gotten_err_set = inst.ty.getErrs();
switch (dest_type.tag()) {
.error_set => {
switch (gotten_err_set) {
.multiple => |fields| {
const dt_e = dest_type.getErrs().multiple;
// we can do > because if they were equal then Type.eql wouldn't have let control flow get this far
if (dt_e.size > fields.size) blk: {
var it = fields.iterator();
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);
}
}
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);
return inst;
},
else => unreachable,
}
},
.error_set_single => {}, // we do nothing, because if they were equal Type.eql would have caught and if not, then not coercible
.anyerror => {
switch (gotten_err_set) {
.multiple => |fields| {
inst.ty = Type.initTag(.anyerror);
return inst;
},
.err_single => |_| {
inst.ty = Type.initTag(.anyerror);
return inst;
},
else => unreachable,
}
},
else => unreachable,
if (dest_type.errorSetFitsInAnother(inst.ty)) {
inst.ty = dest_type;
return inst;
}
}

Expand Down
56 changes: 51 additions & 5 deletions src/type.zig
View file
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,8 @@ pub const Type = extern union {
return true
else
return false;
} else if (b.tag() == .anyerror) {
return false;
}

if (a.tag() == .error_set_single and b.tag() == .error_set_single) {
Expand Down Expand Up @@ -301,9 +303,9 @@ pub const Type = extern union {
unreachable;
},
.ErrorUnion => {
const a_casted = a.cast(Payload.ErrorUnion).?;
const b_casted = b.cast(Payload.ErrorUnion).?;
return a_casted.error_set.eql(b_casted.error_set) and a_casted.payload.eql(b_casted.payload);
const a_data = a.castTag(.error_union).?.data;
const b_data = b.castTag(.error_union).?.data;
return a_data.error_set.eql(b_data.error_set) and a_data.payload.eql(b_data.payload);
},
.Float,
.Struct,
Expand Down Expand Up @@ -1705,12 +1707,56 @@ pub const Type = extern union {
.const_slice_u8,
.single_const_pointer_to_comptime_int,
.pointer,
.inferred_alloc_mut,
.inferred_alloc_const,
=> unreachable,
.anyerror => return .{ .anyerror = {} },
.error_set_single => return .{ .err_single = self.cast(Payload.ErrorSetSingle).?.name },
.error_set => return .{ .multiple = &self.cast(Payload.ErrorSet).?.decl.typed_value.most_recent.typed_value.val.cast(Value.Payload.ErrorSet).?.fields },
.error_set_single => return .{ .err_single = self.castTag(.error_set_single).?.data },
.error_set => return .{ .multiple = &self.castTag(.error_set).?.data.typed_value.most_recent.typed_value.val.castTag(.error_set).?.data.fields },
};
}
/// Asserts the type is error_set or error_set_single or anyerror and that if it is error_set, it's decl has been analyzed.
/// Returns true if fitee can fit into fitter, and false if not.
pub fn errorSetFitsInAnother(fitter: Type, fitee: Type) bool {
if (fitter.eql(fitee)) return true;
switch (fitee.getErrs()) {
.multiple => |fitee_set| {
switch (fitter.getErrs()) {
.multiple => |fitter_set| {
// we can do < because if they were equal then Type.eql wouldn't have let control flow get this far
if (fitee_set.size < fitter_set.size) {
var it = fitter_set.iterator();
while (it.next()) |entry| {
// the smaller set has a key not in the larger set
if (fitee_set.get(entry.key) == null) return false;
}
return true;
} else return false;
},
// we return false, because if they were equal Type.eql would have caught and if not, then not coercible
.err_single => return false,
// any set can fit into anyerror
.anyerror => return true,
}
},
.err_single => |fitee_name| {
switch (fitter.getErrs()) {
.multiple => |fitter_set| {
// the smaller set has a key not in the larger set
if (fitter_set.get(fitee_name) == null) return false;
return true;
},
// we return false, because if they were equal Type.eql would have caught and if not, then not coercible
.err_single => return false,
// any set can fit into anyerror
.anyerror => return true,
}
},

// if they were both anyerror, then Type.eql would have caught it, and if not, then nothing else can fit into anyerorr
.anyerror => return false,
}
}
/// Asserts the type is a pointer or array type.
pub fn elemType(self: Type) Type {
return switch (self.tag()) {
Expand Down
2 changes: 1 addition & 1 deletion src/value.zig
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1381,7 +1381,7 @@ pub const Value = extern union {
const b_name = b.castTag(.enum_literal).?.data;
return std.mem.eql(u8, a_name, b_name);
} else if (a.tag() == .@"error" and b.tag() == .@"error") {
return a.cast(Payload.Error).?.value == b.cast(Payload.Error).?.value;
return a.castTag(.@"error").?.data.value == b.castTag(.@"error").?.data.value;
}
}
if (a.isType() and b.isType()) {
Expand Down
38 changes: 19 additions & 19 deletions src/zir_sema.zig
View file
Original file line number Diff line number Diff line change
Expand Up @@ -944,43 +944,46 @@ fn analyzeInstMergeErrorSets(mod: *Module, scope: *Scope, inst: *zir.Inst.BinOp)

const payload = try scope.arena().create(Value.Payload.ErrorSet);
payload.* = .{
.fields = .{},
.decl = undefined, // populated below
.base = .{ .tag = .error_set },
.data = .{
.fields = .{},
.decl = undefined, // populated below
},
};
try payload.fields.ensureCapacity(&new_decl_arena.allocator, @intCast(u32, switch (rhs_fields) {
try payload.data.fields.ensureCapacity(&new_decl_arena.allocator, @intCast(u32, switch (rhs_fields) {
.err_single => 1,
.multiple => |mul| mul.size,
else => unreachable,
} + switch (rhs_fields) {
} + switch (lhs_fields) {
.err_single => 1,
.multiple => |mul| mul.size,
else => unreachable,
}));

switch (lhs_fields) {
.err_single => |name| {
const entry = mod.global_error_set.get(name).?;
payload.fields.putAssumeCapacity(name, entry);
const num = mod.global_error_set.get(name).?;
payload.data.fields.putAssumeCapacity(name, num);
},
.multiple => |multiple| {
var it = multiple.iterator();
while (it.next()) |entry| {
payload.fields.putAssumeCapacity(entry.key, entry.value);
payload.data.fields.putAssumeCapacity(entry.key, entry.value);
}
},
else => unreachable,
}

switch (rhs_fields) {
.err_single => |name| {
const entry = try mod.getErrorValue(name);
payload.fields.putAssumeCapacity(entry.key, entry.value);
const num = mod.global_error_set.get(name).?;
payload.data.fields.putAssumeCapacity(name, num);
},
.multiple => |multiple| {
var it = multiple.iterator();
while (it.next()) |name| {
const entry = try mod.getErrorValue(name.key);
payload.fields.putAssumeCapacity(entry.key, entry.value);
payload.data.fields.putAssumeCapacity(entry.key, entry.value);
}
},
else => unreachable,
Expand All @@ -989,7 +992,7 @@ fn analyzeInstMergeErrorSets(mod: *Module, scope: *Scope, inst: *zir.Inst.BinOp)
.ty = Type.initTag(.type),
.val = Value.initPayload(&payload.base),
});
payload.decl = new_decl;
payload.data.decl = new_decl;

return mod.constInst(scope, inst.base.src, .{
.ty = Type.initTag(.type),
Expand Down Expand Up @@ -1463,7 +1466,7 @@ fn validateSwitch(mod: *Module, scope: *Scope, target: *Inst, inst: *zir.Inst.Sw
const resolved = try resolveInst(mod, scope, item);
const casted = try mod.coerce(scope, target.ty, resolved);
const val = try mod.resolveConstValue(scope, casted);
const err_name = val.cast(Value.Payload.Error).?.name;
const err_name = val.castTag(.@"error").?.data.name;
if (try seen_values.fetchPut(val, item.src)) |prev| {
return mod.fail(scope, item.src, "duplicate switch value", .{});
}
Expand Down Expand Up @@ -1876,13 +1879,10 @@ fn analyzeInstCmp(
if (!is_equality_cmp) {
return mod.fail(scope, inst.base.src, "{} operator not allowed for errors", .{@tagName(op)});
}
const lhs_comptime = lhs.ty.cast(Type.Payload.ErrorSetSingle);
const rhs_comptime = rhs.ty.cast(Type.Payload.ErrorSetSingle);
if (lhs_comptime != null and rhs_comptime != null) {
if (op == .eq)
return mod.constBool(scope, inst.base.src, !std.mem.eql(u8, lhs_comptime.?.name, rhs_comptime.?.name))
else
return mod.constBool(scope, inst.base.src, std.mem.eql(u8, lhs_comptime.?.name, rhs_comptime.?.name));
const lhs_val = lhs.value();
const rhs_val = rhs.value();
if (lhs_val != null and rhs_val != null) {
return mod.constBool(scope, inst.base.src, (lhs_val.?.castTag(.@"error").?.data.value == rhs_val.?.castTag(.@"error").?.data.value) == (op == .eq));
}
return mod.fail(scope, inst.base.src, "TODO runtime error comparison", .{});
} else if (lhs.ty.isNumeric() and rhs.ty.isNumeric()) {
Expand Down
4 changes: 2 additions & 2 deletions test/stage2/test.zig
View file
Original file line number Diff line number Diff line change
Expand Up @@ -362,7 +362,7 @@ pub fn addCases(ctx: *TestContext) !void {
\\ }
\\ unreachable; // because it will give error above
\\}
, &[_][]const u8{":6:14: error: expected _start__anon_13, found error{Z}"});
, &[_][]const u8{":6:14: error: expected _start__anon_12, found error{Z}"});
case.addError(
\\export fn _start() noreturn {
\\ const T = error{ A, B, C, D };
Expand Down Expand Up @@ -501,7 +501,7 @@ pub fn addCases(ctx: *TestContext) !void {
var case = ctx.exe("comptime error equality", linux_x64);
case.addCompareOutput(
\\export fn _start() noreturn {
\\ if (error.V == error.T) {
\\ if (error.T == error.T) {
\\ condPrint();
\\ }
\\
Expand Down

0 comments on commit e5fcdfa

Please sign in to comment.