From cc13f0fe4954e50673cf6f1532864d2b49c34f6c Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Mon, 11 Sep 2023 08:31:42 -0700 Subject: [PATCH 1/9] config: cannot set underscore-prefixed fields --- src/cli_args.zig | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/src/cli_args.zig b/src/cli_args.zig index b89c51a0a..76bcbe245 100644 --- a/src/cli_args.zig +++ b/src/cli_args.zig @@ -83,7 +83,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. @@ -167,7 +167,7 @@ fn parseIntoField( } } - return error.InvalidFlag; + return error.InvalidField; } fn parseBool(v: []const u8) !bool { @@ -240,6 +240,23 @@ test "parse: quoted value" { try testing.expectEqualStrings("hello!", data.b); } +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); From 58a43f1980b881f9912139b72e3fd8781d4c8d2a Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Mon, 11 Sep 2023 08:54:37 -0700 Subject: [PATCH 2/9] config: store some basic errors on parse --- src/cli_args.zig | 73 ++++++++++++++++++++++++++++++++++++++++++- src/config/Config.zig | 6 ++++ src/input/Binding.zig | 2 +- 3 files changed, 79 insertions(+), 2 deletions(-) diff --git a/src/cli_args.zig b/src/cli_args.zig index 76bcbe245..55fc2136c 100644 --- a/src/cli_args.zig +++ b/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,45 @@ 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; + switch (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 => return 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 @@ -240,6 +288,29 @@ 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); diff --git a/src/config/Config.zig b/src/config/Config.zig index c37939d22..c2d2f4f8d 100644 --- a/src/config/Config.zig +++ b/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; diff --git a/src/input/Binding.zig b/src/input/Binding.zig index 9cbd2f1c4..81d22327b 100644 --- a/src/input/Binding.zig +++ b/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); From 75e8d8f0daf23f9a7e67069351f9f50d365bee95 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Mon, 11 Sep 2023 09:08:33 -0700 Subject: [PATCH 3/9] config: arg parser supports custom types with error tracking --- src/cli_args.zig | 79 +++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 72 insertions(+), 7 deletions(-) diff --git a/src/cli_args.zig b/src/cli_args.zig index 55fc2136c..7559ee162 100644 --- a/src/cli_args.zig +++ b/src/cli_args.zig @@ -155,6 +155,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"), } @@ -191,22 +200,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, }; @@ -229,7 +238,7 @@ fn parseBool(v: []const u8) !bool { if (mem.eql(u8, v, str)) return false; } - return error.InvalidBooleanValue; + return error.InvalidValue; } test "parse: simple" { @@ -469,6 +478,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. From a359641d073ae329028de50eaaa406458fd1d113 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Mon, 11 Sep 2023 09:14:27 -0700 Subject: [PATCH 4/9] config: store unknown errors in list too --- src/cli_args.zig | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/src/cli_args.zig b/src/cli_args.zig index 7559ee162..fc056f43c 100644 --- a/src/cli_args.zig +++ b/src/cli_args.zig @@ -78,7 +78,15 @@ pub fn parse(comptime T: type, alloc: Allocator, dst: *T, iter: anytype) !void { parseIntoField(T, arena_alloc, dst, key, value) catch |err| { if (comptime !canTrackErrors(T)) return err; - switch (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, @@ -103,7 +111,13 @@ pub fn parse(comptime T: type, alloc: Allocator, dst: *T, iter: anytype) !void { ), }), - else => return err, + else => try dst._errors.add(arena_alloc, .{ + .message = try std.fmt.allocPrintZ( + arena_alloc, + "{s}: unknown error {}", + .{ key, err }, + ), + }), } }; } From 4ee9531ce3c0acd0d221419dc36567d4df67c727 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Mon, 11 Sep 2023 09:16:56 -0700 Subject: [PATCH 5/9] apprt/glfw: log configuration errors --- src/apprt/glfw.zig | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/apprt/glfw.zig b/src/apprt/glfw.zig index 8d96c99ef..c3c07af07 100644 --- a/src/apprt/glfw.zig +++ b/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 = .{}, From e5123330ce012b5578b6d0804407fa5f5766d712 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Mon, 11 Sep 2023 09:17:29 -0700 Subject: [PATCH 6/9] config: add ErrorList file --- src/config/ErrorList.zig | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) create mode 100644 src/config/ErrorList.zig diff --git a/src/config/ErrorList.zig b/src/config/ErrorList.zig new file mode 100644 index 000000000..cb98fa5f8 --- /dev/null +++ b/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); +} From 9be46fa80a763deb3681a764a1edf5a3f475d7f3 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Mon, 11 Sep 2023 09:18:03 -0700 Subject: [PATCH 7/9] apprt/gtk: log configuration errors --- src/apprt/gtk.zig | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/apprt/gtk.zig b/src/apprt/gtk.zig index f54d2cfda..b0f845ced 100644 --- a/src/apprt/gtk.zig +++ b/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. From f0ee2fb454a1e231f8a64e24f78c4e89e497ef8b Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Mon, 11 Sep 2023 09:39:58 -0700 Subject: [PATCH 8/9] macos: log configuration errors --- include/ghostty.h | 6 ++++++ macos/Sources/Ghostty/AppState.swift | 14 ++++++++++++++ src/config/CAPI.zig | 15 +++++++++++++++ 3 files changed, 35 insertions(+) diff --git a/include/ghostty.h b/include/ghostty.h index 94b69b829..ed66824ed 100644 --- a/include/ghostty.h +++ b/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); diff --git a/macos/Sources/Ghostty/AppState.swift b/macos/Sources/Ghostty/AppState.swift index 12ece198b..40225b2b1 100644 --- a/macos/Sources/Ghostty/AppState.swift +++ b/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..= 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 = "", +}; From 22f3fea98f24a1fc6402d4d1e399f2ec087d82c0 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Mon, 11 Sep 2023 09:46:27 -0700 Subject: [PATCH 9/9] config: turn invalid config-file values into errors in the list --- src/config/Config.zig | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/src/config/Config.zig b/src/config/Config.zig index c2d2f4f8d..5ca6b27cd 100644 --- a/src/config/Config.zig +++ b/src/config/Config.zig @@ -764,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()); @@ -783,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}, + ), + }); + } } }