Merge pull request #428 from mitchellh/config-errs

Track and log configuration errors, non-fatal

include/ghostty.h

@@ -250,6 +250,10 @@ typedef struct {
 // Fully defined types. This MUST be kept in sync with equivalent Zig
 // structs. To find the Zig struct, grep for this type name. The documentation
 // for all of these types is available in the Zig source.
+typedef struct {
+    const char *message;
+} ghostty_error_s;
+
 typedef struct {
     void *userdata;
     void *nsview;
@@ -303,6 +307,8 @@ void ghostty_config_load_recursive_files(ghostty_config_t);
 void ghostty_config_finalize(ghostty_config_t);
 bool ghostty_config_get(ghostty_config_t, void *, const char *, uintptr_t);
 ghostty_input_trigger_s ghostty_config_trigger(ghostty_config_t, const char *, uintptr_t);
+uint32_t ghostty_config_errors_count(ghostty_config_t);
+ghostty_error_s ghostty_config_get_error(ghostty_config_t, uint32_t);
 
 ghostty_app_t ghostty_app_new(const ghostty_runtime_config_s *, ghostty_config_t);
 void ghostty_app_free(ghostty_app_t);

macos/Sources/Ghostty/AppState.swift

@@ -128,6 +128,20 @@ extension Ghostty {
             // Finalize will make our defaults available.
             ghostty_config_finalize(cfg)
             
+            // Log any configuration errors. These will be automatically shown in a
+            // pop-up window too.
+            let errCount = ghostty_config_errors_count(cfg)
+            if errCount > 0 {
+                AppDelegate.logger.warning("config error: \(errCount) configuration errors on reload")
+                var errors: [String] = [];
+                for i in 0..<errCount {
+                    let err = ghostty_config_get_error(cfg, UInt32(i))
+                    let message = String(cString: err.message)
+                    errors.append(message)
+                    AppDelegate.logger.warning("config error: \(message)")
+                }
+            }
+            
             return cfg
         }
         

src/apprt/glfw.zig

@@ -67,6 +67,13 @@ pub const App = struct {
         var config = try Config.load(core_app.alloc);
         errdefer config.deinit();
 
+        // If we had configuration errors, then log them.
+        if (!config._errors.empty()) {
+            for (config._errors.list.items) |err| {
+                log.warn("configuration error: {s}", .{err.message});
+            }
+        }
+
         // Queue a single new window that starts on launch
         _ = core_app.mailbox.push(.{
             .new_window = .{},

src/apprt/gtk.zig

@@ -63,6 +63,13 @@ pub const App = struct {
         var config = try Config.load(core_app.alloc);
         errdefer config.deinit();
 
+        // If we had configuration errors, then log them.
+        if (!config._errors.empty()) {
+            for (config._errors.list.items) |err| {
+                log.warn("configuration error: {s}", .{err.message});
+            }
+        }
+
         // Our uniqueness ID is based on whether we're in a debug mode or not.
         // In debug mode we want to be separate so we can develop Ghostty in
         // Ghostty.

src/cli_args.zig

@@ -4,10 +4,20 @@ const assert = std.debug.assert;
 const Allocator = mem.Allocator;
 const ArenaAllocator = std.heap.ArenaAllocator;
 
+const ErrorList = @import("config/ErrorList.zig");
+
 // TODO:
 //   - Only `--long=value` format is accepted. Do we want to allow
 //     `--long value`? Not currently allowed.
 
+/// The base errors for arg parsing. Additional errors can be returned due
+/// to type-specific parsing but these are always possible.
+pub const Error = error{
+    ValueRequired,
+    InvalidField,
+    InvalidValue,
+};
+
 /// Parse the command line arguments from iter into dst.
 ///
 /// dst must be a struct. The fields and their types will be used to determine
@@ -19,6 +29,10 @@ const ArenaAllocator = std.heap.ArenaAllocator;
 /// an arena allocator will be created (or reused if set already) for any
 /// allocations. Allocations are necessary for certain types, like `[]const u8`.
 ///
+/// If the destination type has a field "_errors" of type "ErrorList" then
+/// errors will be added to that list. In this case, the only error returned by
+/// parse are allocation errors.
+///
 /// Note: If the arena is already non-null, then it will be used. In this
 /// case, in the case of an error some memory might be leaked into the arena.
 pub fn parse(comptime T: type, alloc: Allocator, dst: *T, iter: anytype) !void {
@@ -62,11 +76,59 @@ pub fn parse(comptime T: type, alloc: Allocator, dst: *T, iter: anytype) !void {
                 break :value null;
             };
 
-            try parseIntoField(T, arena_alloc, dst, key, value);
+            parseIntoField(T, arena_alloc, dst, key, value) catch |err| {
+                if (comptime !canTrackErrors(T)) return err;
+
+                // The error set is dependent on comptime T, so we always add
+                // an extra error so we can have the "else" below.
+                const ErrSet = @TypeOf(err) || error{Unknown};
+                switch (@as(ErrSet, @errSetCast(err))) {
+                    // OOM is not recoverable since we need to allocate to
+                    // track more error messages.
+                    error.OutOfMemory => return err,
+
+                    error.InvalidField => try dst._errors.add(arena_alloc, .{
+                        .message = try std.fmt.allocPrintZ(
+                            arena_alloc,
+                            "unknown field: {s}",
+                            .{key},
+                        ),
+                    }),
+
+                    error.ValueRequired => try dst._errors.add(arena_alloc, .{
+                        .message = try std.fmt.allocPrintZ(
+                            arena_alloc,
+                            "{s}: value required",
+                            .{key},
+                        ),
+                    }),
+
+                    error.InvalidValue => try dst._errors.add(arena_alloc, .{
+                        .message = try std.fmt.allocPrintZ(
+                            arena_alloc,
+                            "{s}: invalid value",
+                            .{key},
+                        ),
+                    }),
+
+                    else => try dst._errors.add(arena_alloc, .{
+                        .message = try std.fmt.allocPrintZ(
+                            arena_alloc,
+                            "{s}: unknown error {}",
+                            .{ key, err },
+                        ),
+                    }),
+                }
+            };
         }
     }
 }
 
+/// Returns true if this type can track errors.
+fn canTrackErrors(comptime T: type) bool {
+    return @hasField(T, "_errors");
+}
+
 /// Parse a single key/value pair into the destination type T.
 ///
 /// This may result in allocations. The allocations can only be freed by freeing
@@ -83,7 +145,7 @@ fn parseIntoField(
     assert(info == .Struct);
 
     inline for (info.Struct.fields) |field| {
-        if (mem.eql(u8, field.name, key)) {
+        if (field.name[0] != '_' and mem.eql(u8, field.name, key)) {
             // For optional fields, we just treat it as the child type.
             // This lets optional fields default to null but get set by
             // the CLI.
@@ -107,6 +169,15 @@ fn parseIntoField(
                         // 3 arg = (self, alloc, input) => void
                         3 => try @field(dst, field.name).parseCLI(alloc, value),
 
+                        // 4 arg = (self, alloc, errors, input) => void
+                        4 => if (comptime canTrackErrors(T)) {
+                            try @field(dst, field.name).parseCLI(alloc, &dst._errors, value);
+                        } else {
+                            var list: ErrorList = .{};
+                            try @field(dst, field.name).parseCLI(alloc, &list, value);
+                            if (!list.empty()) return error.InvalidValue;
+                        },
+
                         else => @compileError("parseCLI invalid argument count"),
                     }
 
@@ -143,22 +214,22 @@ fn parseIntoField(
 
                 bool => try parseBool(value orelse "t"),
 
-                u8 => try std.fmt.parseInt(
+                u8 => std.fmt.parseInt(
                     u8,
                     value orelse return error.ValueRequired,
                     0,
-                ),
+                ) catch return error.InvalidValue,
 
-                u32 => try std.fmt.parseInt(
+                u32 => std.fmt.parseInt(
                     u32,
                     value orelse return error.ValueRequired,
                     0,
-                ),
+                ) catch return error.InvalidValue,
 
-                f64 => try std.fmt.parseFloat(
+                f64 => std.fmt.parseFloat(
                     f64,
                     value orelse return error.ValueRequired,
-                ),
+                ) catch return error.InvalidValue,
 
                 else => unreachable,
             };
@@ -167,7 +238,7 @@ fn parseIntoField(
         }
     }
 
-    return error.InvalidFlag;
+    return error.InvalidField;
 }
 
 fn parseBool(v: []const u8) !bool {
@@ -181,7 +252,7 @@ fn parseBool(v: []const u8) !bool {
         if (mem.eql(u8, v, str)) return false;
     }
 
-    return error.InvalidBooleanValue;
+    return error.InvalidValue;
 }
 
 test "parse: simple" {
@@ -240,6 +311,46 @@ test "parse: quoted value" {
     try testing.expectEqualStrings("hello!", data.b);
 }
 
+test "parse: error tracking" {
+    const testing = std.testing;
+
+    var data: struct {
+        a: []const u8 = "",
+        b: enum { one } = .one,
+
+        _arena: ?ArenaAllocator = null,
+        _errors: ErrorList = .{},
+    } = .{};
+    defer if (data._arena) |arena| arena.deinit();
+
+    var iter = try std.process.ArgIteratorGeneral(.{}).init(
+        testing.allocator,
+        "--what --a=42",
+    );
+    defer iter.deinit();
+    try parse(@TypeOf(data), testing.allocator, &data, &iter);
+    try testing.expect(data._arena != null);
+    try testing.expectEqualStrings("42", data.a);
+    try testing.expect(!data._errors.empty());
+}
+
+test "parseIntoField: ignore underscore-prefixed fields" {
+    const testing = std.testing;
+    var arena = ArenaAllocator.init(testing.allocator);
+    defer arena.deinit();
+    const alloc = arena.allocator();
+
+    var data: struct {
+        _a: []const u8 = "12",
+    } = .{};
+
+    try testing.expectError(
+        error.InvalidField,
+        parseIntoField(@TypeOf(data), alloc, &data, "_a", "42"),
+    );
+    try testing.expectEqualStrings("12", data._a);
+}
+
 test "parseIntoField: string" {
     const testing = std.testing;
     var arena = ArenaAllocator.init(testing.allocator);
@@ -381,6 +492,62 @@ test "parseIntoField: struct with parse func" {
     try testing.expectEqual(@as([]const u8, "HELLO!"), data.a.v);
 }
 
+test "parseIntoField: struct with parse func with error tracking" {
+    const testing = std.testing;
+    var arena = ArenaAllocator.init(testing.allocator);
+    defer arena.deinit();
+    const alloc = arena.allocator();
+
+    var data: struct {
+        a: struct {
+            const Self = @This();
+
+            pub fn parseCLI(
+                _: Self,
+                parse_alloc: Allocator,
+                errors: *ErrorList,
+                value: ?[]const u8,
+            ) !void {
+                _ = value;
+                try errors.add(parse_alloc, .{ .message = "OH NO!" });
+            }
+        } = .{},
+
+        _errors: ErrorList = .{},
+    } = .{};
+
+    try parseIntoField(@TypeOf(data), alloc, &data, "a", "42");
+    try testing.expect(!data._errors.empty());
+}
+
+test "parseIntoField: struct with parse func with unsupported error tracking" {
+    const testing = std.testing;
+    var arena = ArenaAllocator.init(testing.allocator);
+    defer arena.deinit();
+    const alloc = arena.allocator();
+
+    var data: struct {
+        a: struct {
+            const Self = @This();
+
+            pub fn parseCLI(
+                _: Self,
+                parse_alloc: Allocator,
+                errors: *ErrorList,
+                value: ?[]const u8,
+            ) !void {
+                _ = value;
+                try errors.add(parse_alloc, .{ .message = "OH NO!" });
+            }
+        } = .{},
+    } = .{};
+
+    try testing.expectError(
+        error.InvalidValue,
+        parseIntoField(@TypeOf(data), alloc, &data, "a", "42"),
+    );
+}
+
 /// Returns an iterator (implements "next") that reads CLI args by line.
 /// Each CLI arg is expected to be a single line. This is used to implement
 /// configuration files.

src/config/CAPI.zig

@@ -108,3 +108,18 @@ fn config_trigger_(
     const action = try inputpkg.Binding.Action.parse(str);
     return self.keybind.set.getTrigger(action) orelse .{};
 }
+
+export fn ghostty_config_errors_count(self: *Config) u32 {
+    return @intCast(self._errors.list.items.len);
+}
+
+export fn ghostty_config_get_error(self: *Config, idx: u32) Error {
+    if (idx >= self._errors.list.items.len) return .{};
+    const err = self._errors.list.items[idx];
+    return .{ .message = err.message.ptr };
+}
+
+/// Sync with ghostty_error_s
+const Error = extern struct {
+    message: [*:0]const u8 = "",
+};

src/config/Config.zig

@@ -14,6 +14,7 @@ const cli_args = @import("../cli_args.zig");
 
 const Key = @import("key.zig").Key;
 const KeyValue = @import("key.zig").Value;
+const ErrorList = @import("ErrorList.zig");
 
 const log = std.log.scoped(.config);
 
@@ -341,6 +342,11 @@ keybind: Keybinds = .{},
 /// This is set by the CLI parser for deinit.
 _arena: ?ArenaAllocator = null,
 
+/// List of errors that occurred while loading. This can be accessed directly
+/// by callers. It is only underscore-prefixed so it can't be set by the
+/// configuration file.
+_errors: ErrorList = .{},
+
 pub fn deinit(self: *Config) void {
     if (self._arena) |arena| arena.deinit();
     self.* = undefined;
@@ -758,16 +764,25 @@ pub fn loadCliArgs(self: *Config, alloc_gpa: Allocator) !void {
 
 /// Load and parse the config files that were added in the "config-file" key.
 pub fn loadRecursiveFiles(self: *Config, alloc: Allocator) !void {
-    // TODO(mitchellh): we should parse the files form the homedir first
     // TODO(mitchellh): support nesting (config-file in a config file)
     // TODO(mitchellh): detect cycles when nesting
 
     if (self.@"config-file".list.items.len == 0) return;
 
+    const arena_alloc = self._arena.?.allocator();
     const cwd = std.fs.cwd();
     const len = self.@"config-file".list.items.len;
     for (self.@"config-file".list.items) |path| {
-        var file = try cwd.openFile(path, .{});
+        var file = cwd.openFile(path, .{}) catch |err| {
+            try self._errors.add(arena_alloc, .{
+                .message = try std.fmt.allocPrintZ(
+                    arena_alloc,
+                    "error opening config-file {s}: {}",
+                    .{ path, err },
+                ),
+            });
+            continue;
+        };
         defer file.close();
 
         var buf_reader = std.io.bufferedReader(file.reader());
@@ -777,8 +792,15 @@ pub fn loadRecursiveFiles(self: *Config, alloc: Allocator) !void {
         // We don't currently support adding more config files to load
         // from within a loaded config file. This can be supported
         // later.
-        if (self.@"config-file".list.items.len > len)
-            return error.ConfigFileInConfigFile;
+        if (self.@"config-file".list.items.len > len) {
+            try self._errors.add(arena_alloc, .{
+                .message = try std.fmt.allocPrintZ(
+                    arena_alloc,
+                    "config-file cannot be used in a config-file. Found in {s}",
+                    .{path},
+                ),
+            });
+        }
     }
 }
 

src/config/ErrorList.zig

@@ -0,0 +1,23 @@
+const ErrorList = @This();
+
+const std = @import("std");
+const Allocator = std.mem.Allocator;
+
+pub const Error = struct {
+    message: [:0]const u8,
+};
+
+/// The list of errors. This will use the arena allocator associated
+/// with the config structure (or whatever allocated used to call ErrorList
+/// functions).
+list: std.ArrayListUnmanaged(Error) = .{},
+
+/// True if there are no errors.
+pub fn empty(self: ErrorList) bool {
+    return self.list.items.len == 0;
+}
+
+/// Add a new error to the list.
+pub fn add(self: *ErrorList, alloc: Allocator, err: Error) !void {
+    try self.list.append(alloc, err);
+}

src/input/Binding.zig

@@ -393,7 +393,7 @@ pub const Set = struct {
         alloc: Allocator,
         t: Trigger,
         action: Action,
-    ) !void {
+    ) Allocator.Error!void {
         // unbind should never go into the set, it should be handled prior
         assert(action != .unbind);