Improve error reporting around atomics

Author: silversquirl

lib/std/atomic.zig

@@ -493,10 +493,31 @@ pub const Op = union(enum) {
     }
     /// Check if the operation is supported on the given type, on a specified CPU.
     pub fn supportedOnCpu(op: Op, comptime T: type, cpu: std.Target.Cpu) bool {
-        if (!op.isValidAtomicType(T)) return false;
+        const valid_types = op.supportedTypes();
+        const is_valid_type = switch (@typeInfo(T)) {
+            .bool => valid_types.bool,
+            .int => valid_types.integer,
+            .float => valid_types.float,
+            .@"enum" => valid_types.@"enum",
+            .error_set => valid_types.error_set,
+            .@"struct" => |s| s.layout == .@"packed" and valid_types.packed_struct,
 
-        if (!std.math.isPowerOfTwo(@sizeOf(T))) return false;
+            .optional => |opt| switch (@typeInfo(opt.child)) {
+                .pointer => |ptr| switch (ptr.size) {
+                    .slice, .c => false,
+                    .one, .many => !ptr.is_allowzero and valid_types.pointer,
+                },
+            },
+            .pointer => |ptr| switch (ptr.size) {
+                .slice => false,
+                .one, .many, .c => valid_types.pointer,
+            },
 
+            else => false,
+        };
+        if (!is_valid_type) return false;
+
+        if (!std.math.isPowerOfTwo(@sizeOf(T))) return false;
         const required_features = op.supportedSizes(cpu.arch).get(@sizeOf(T)) orelse {
             return false;
         };
@@ -682,55 +703,58 @@ pub const Op = union(enum) {
         }
     }
 
-    /// Returns true if the type may be usable with this atomic operation.
-    /// This does not check that the type actually fits within the target's atomic size constriants.
-    /// This function must be kept in sync with the compiler implementation.
-    fn isValidAtomicType(op: Op, comptime T: type) bool {
-        const supports_floats = switch (op) {
-            .load, .store => true,
+    /// Returns a description of the kinds of type supported by this operation.
+    pub fn supportedTypes(op: Op) Types {
+        return switch (op) {
+            .load, .store => .{},
             .rmw => |rmw| switch (rmw) {
-                .Xchg, .Add, .Sub, .Min, .Max => true,
-                .And, .Nand, .Or, .Xor => false,
-            },
-            // floats are not supported for cmpxchg because float equality differs from bitwise equality
-            .cmpxchg => false,
-        };
-
-        return switch (@typeInfo(T)) {
-            .bool, .int, .@"enum", .error_set => true,
-            .float => supports_floats,
-            .@"struct" => |s| s.layout == .@"packed",
-
-            .optional => |opt| switch (@typeInfo(opt.child)) {
-                .pointer => |ptr| switch (ptr.size) {
-                    .slice, .c => false,
-                    .one, .many => !ptr.is_allowzero,
+                .Xchg => .{},
+                .Add, .Sub, .Min, .Max => .{
+                    .bool = false,
+                    .@"enum" = false,
+                    .error_set = false,
+                },
+                .And, .Nand, .Or, .Xor => .{
+                    .float = false,
+                    .bool = false,
+                    .@"enum" = false,
+                    .error_set = false,
                 },
             },
-            .pointer => |ptr| switch (ptr.size) {
-                .slice => false,
-                .one, .many, .c => true,
+            .cmpxchg => .{
+                // floats are not supported for cmpxchg because float equality differs from bitwise equality
+                .float = false,
             },
-
-            else => false,
         };
     }
-
-    test isValidAtomicType {
-        try testing.expect(isValidAtomicType(.load, u8));
-        try testing.expect(isValidAtomicType(.load, f32));
-        try testing.expect(isValidAtomicType(.load, bool));
-        try testing.expect(isValidAtomicType(.load, enum { a, b, c }));
-        try testing.expect(isValidAtomicType(.load, packed struct { a: u8, b: u8 }));
-        try testing.expect(isValidAtomicType(.load, error{OutOfMemory}));
-        try testing.expect(isValidAtomicType(.load, u200)); // doesn't check size
-
-        try testing.expect(!isValidAtomicType(.load, struct { a: u8 }));
-        try testing.expect(!isValidAtomicType(.load, union { a: u8 }));
-
-        // cmpxchg doesn't support floats
-        try testing.expect(!isValidAtomicType(.{ .cmpxchg = .weak }, f32));
-    }
+    pub const Types = packed struct {
+        bool: bool = true,
+        integer: bool = true,
+        float: bool = true,
+        @"enum": bool = true,
+        error_set: bool = true,
+        packed_struct: bool = true,
+        pointer: bool = true,
+
+        pub fn format(types: Types, writer: *std.io.Writer) !void {
+            const bits: @typeInfo(Types).@"struct".backing_integer.? = @bitCast(types);
+            var count = @popCount(bits);
+            inline for (@typeInfo(Types).@"struct".fields) |field| {
+                if (@field(types, field.name)) {
+                    var name = field.name[0..].*;
+                    std.mem.replaceScalar(u8, &name, '_', ' ');
+                    try writer.writeAll(&name);
+
+                    count -= 1;
+                    switch (count) {
+                        0 => {},
+                        1 => try writer.writeAll(", or "),
+                        else => try writer.writeAll(", "),
+                    }
+                }
+            }
+        }
+    };
 
     test supportedOnCpu {
         const x86 = std.Target.x86;

src/Sema.zig

@@ -23592,39 +23592,17 @@ fn checkAtomicPtrOperand(
     const pt = sema.pt;
     const zcu = pt.zcu;
 
-    // This logic must be kept in sync with `std.atomic.Op.isValidAtomicType`.
-    const supports_floats = switch (atomic_op) {
-        .load, .store => true,
-        .cmpxchg => false,
-        .rmw => |rmw| switch (rmw) {
-            .Xchg, .Add, .Sub, .Min, .Max => true,
-            .And, .Nand, .Or, .Xor => false,
-        },
-    };
-    const is_valid_atomic_type =
-        elem_ty.toIntern() == .bool_type or
-        elem_ty.isAbiInt(zcu) or
-        elem_ty.isPtrAtRuntime(zcu) or
-        (elem_ty.isRuntimeFloat() and supports_floats);
-    if (!is_valid_atomic_type) {
+    if (sema.isInvalidAtomicType(atomic_op, elem_ty)) |why| {
         const msg = msg: {
             const msg = try sema.errMsg(
                 elem_ty_src,
-                "expected bool, integer,{s} enum, error set, packed struct, or pointer type; found '{f}'",
-                .{
-                    if (atomic_op == .cmpxchg) "" else " float,",
-                    elem_ty.fmt(pt),
-                },
+                "expected {f} type; found '{f}'",
+                .{ atomic_op.supportedTypes(), elem_ty.fmt(pt) },
             );
             errdefer msg.destroy(sema.gpa);
 
-            if (elem_ty.isRuntimeFloat() and atomic_op == .cmpxchg) {
-                try sema.errNote(
-                    elem_ty_src,
-                    msg,
-                    "floats are not supported for cmpxchg because float equality differs from bitwise equality",
-                    .{},
-                );
+            if (why.len != 0) {
+                try sema.errNote(elem_ty_src, msg, "{s}", .{why});
             }
 
             try sema.addDeclaredHereNote(msg, elem_ty);
@@ -23701,6 +23679,52 @@ fn checkAtomicPtrOperand(
     return casted_ptr;
 }
 
+/// If the type is invalid for this atomic operation, returns an error note explaining why.
+fn isInvalidAtomicType(sema: *Sema, op: std.atomic.Op, elem_ty: Type) ?[]const u8 {
+    const zcu = sema.pt.zcu;
+    const valid_types = op.supportedTypes();
+    if (elem_ty.isRuntimeFloat()) {
+        if (!valid_types.float) {
+            switch (op) {
+                .load, .store => unreachable,
+                .rmw => return "@atomicRmw with float only allowed with .Xchg, .Add, .Sub, .Max, and .Min",
+                .cmpxchg => return "floats are not supported for cmpxchg because float equality differs from bitwise equality",
+            }
+        }
+    } else if (elem_ty.isPtrAtRuntime(zcu)) {
+        // TODO: pointers are currently supported for things like rmw add, but maybe this shouldn't be the case?
+        std.debug.assert(valid_types.pointer);
+    } else switch (elem_ty.zigTypeTag(zcu)) {
+        .bool => if (!valid_types.bool) {
+            switch (op) {
+                .load, .store, .cmpxchg => unreachable,
+                .rmw => return "@atomicRmw with bool only allowed with .Xchg",
+            }
+        },
+        .int => std.debug.assert(valid_types.integer),
+        .@"enum" => if (!valid_types.@"enum") {
+            switch (op) {
+                .load, .store, .cmpxchg => unreachable,
+                .rmw => return "@atomicRmw with enum only allowed with .Xchg",
+            }
+        },
+        .error_set => if (!valid_types.error_set) {
+            switch (op) {
+                .load, .store, .cmpxchg => unreachable,
+                .rmw => return "@atomicRmw with error set only allowed with .Xchg",
+            }
+        },
+        .@"struct" => if (elem_ty.containerLayout(zcu) != .@"packed") {
+            return ""; // No notes
+        } else {
+            std.debug.assert(valid_types.packed_struct);
+        },
+        else => return "", // No notes
+    }
+
+    return null; // All ok!
+}
+
 fn checkPtrIsNotComptimeMutable(
     sema: *Sema,
     block: *Block,
@@ -24549,20 +24573,6 @@ fn zirAtomicRmw(sema: *Sema, block: *Block, inst: Zir.Inst.Index) CompileError!A
     const uncasted_ptr = try sema.resolveInst(extra.ptr);
     const op = try sema.resolveAtomicRmwOp(block, op_src, extra.operation);
     const ptr = try sema.checkAtomicPtrOperand(block, .{ .rmw = op }, src, elem_ty, elem_ty_src, uncasted_ptr, ptr_src, false);
-
-    switch (elem_ty.zigTypeTag(zcu)) {
-        .@"enum" => if (op != .Xchg) {
-            return sema.fail(block, op_src, "@atomicRmw with enum only allowed with .Xchg", .{});
-        },
-        .bool => if (op != .Xchg) {
-            return sema.fail(block, op_src, "@atomicRmw with bool only allowed with .Xchg", .{});
-        },
-        .float => switch (op) {
-            .Xchg, .Add, .Sub, .Max, .Min => {},
-            else => return sema.fail(block, op_src, "@atomicRmw with float only allowed with .Xchg, .Add, .Sub, .Max, and .Min", .{}),
-        },
-        else => {},
-    }
     const order = try sema.resolveAtomicOrder(block, order_src, extra.ordering, .{ .simple = .atomic_order });
 
     if (order == .unordered) {

test/cases/compile_errors/atomicrmw_with_bool_op_not_.Xchg.zig

@@ -4,7 +4,6 @@ export fn entry() void {
 }
 
 // error
-// backend=stage2
-// target=native
 //
-// :3:31: error: @atomicRmw with bool only allowed with .Xchg
+// :3:20: error: expected integer, float, packed struct, or pointer type; found 'bool'
+// :3:20: note: @atomicRmw with bool only allowed with .Xchg

test/cases/compile_errors/atomicrmw_with_enum_op_not_.Xchg.zig

@@ -1,16 +1,16 @@
+const E = enum(u8) {
+    a,
+    b,
+    c,
+    d,
+};
 export fn entry() void {
-    const E = enum(u8) {
-        a,
-        b,
-        c,
-        d,
-    };
     var x: E = .a;
     _ = @atomicRmw(E, &x, .Add, .b, .seq_cst);
 }
 
 // error
-// backend=stage2
-// target=native
 //
-// :9:28: error: @atomicRmw with enum only allowed with .Xchg
+// :9:20: error: expected integer, float, packed struct, or pointer type; found 'tmp.E'
+// :9:20: note: @atomicRmw with enum only allowed with .Xchg
+// :1:11: note: enum declared here

test/cases/compile_errors/atomicrmw_with_float_op_not_.Xchg_.Add_.Sub_.Max_or_.Min.zig

@@ -4,7 +4,6 @@ export fn entry() void {
 }
 
 // error
-// backend=stage2
-// target=native
 //
-// :3:30: error: @atomicRmw with float only allowed with .Xchg, .Add, .Sub, .Max, and .Min
+// :3:20: error: expected integer, packed struct, or pointer type; found 'f32'
+// :3:20: note: @atomicRmw with float only allowed with .Xchg, .Add, .Sub, .Max, and .Min