From fecf383d1cb6e08b7b4142482a5a6175a910d2cd Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Sat, 3 Aug 2024 11:53:11 -0700 Subject: [PATCH] Include file/line context for config file errors --- include/ghostty.h | 2 +- src/cli/args.zig | 214 ++++++++++++++++++++++++++++++++++-------- src/config/CAPI.zig | 14 +-- src/config/Config.zig | 10 +- src/config/Wasm.zig | 14 +-- src/config/theme.zig | 13 ++- 6 files changed, 206 insertions(+), 61 deletions(-) diff --git a/include/ghostty.h b/include/ghostty.h index b413dec41..101ba5f13 100644 --- a/include/ghostty.h +++ b/include/ghostty.h @@ -506,7 +506,7 @@ ghostty_info_s ghostty_info(void); ghostty_config_t ghostty_config_new(); void ghostty_config_free(ghostty_config_t); void ghostty_config_load_cli_args(ghostty_config_t); -void ghostty_config_load_string(ghostty_config_t, const char*, uintptr_t); +void ghostty_config_load_string(ghostty_config_t, const char*, uintptr_t, const char*, uintptr_t); void ghostty_config_load_default_files(ghostty_config_t); void ghostty_config_load_recursive_files(ghostty_config_t); void ghostty_config_finalize(ghostty_config_t); diff --git a/src/cli/args.zig b/src/cli/args.zig index e46fe09dc..25053c64c 100644 --- a/src/cli/args.zig +++ b/src/cli/args.zig @@ -102,43 +102,111 @@ pub fn parse(comptime T: type, comptime Iter: type, alloc: Allocator, dst: *T, i // 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}; + var message: [:0]u8 = undefined; switch (@as(ErrSet, @errorCast(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, - "{s}: unknown field", - .{key}, - ), - }), + error.InvalidField => { + if (@hasDecl(Iter, "lineContext")) { + const line_context = iter.lineContext(); + message = try std.fmt.allocPrintZ( + arena_alloc, + "{s}:{}:{}: unknown field {s}", + .{ + iter.filepath, + line_context.line_number, + line_context.column_number_key, + key, + }, + ); + } else { + message = try std.fmt.allocPrintZ( + arena_alloc, + "{s}: unknown field", + .{key}, + ); + } + }, - error.ValueRequired => try dst._errors.add(arena_alloc, .{ - .message = try std.fmt.allocPrintZ( - arena_alloc, - "{s}: value required", - .{key}, - ), - }), + error.ValueRequired => { + if (@hasDecl(Iter, "lineContext")) { + const line_context = iter.lineContext(); + message = try std.fmt.allocPrintZ( + arena_alloc, + "{s}:{}:{}: value required for {s}", + .{ + iter.filepath, + line_context.line_number, + if (line_context.column_number_value) |column_number_value| + column_number_value + else + line_context.column_number_key, + key, + }, + ); + } else { + 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}, - ), - }), + error.InvalidValue => { + // The value must exist if it was invalid. + assert(value != null); + if (@hasDecl(Iter, "lineContext")) { + const line_context = iter.lineContext(); + message = try std.fmt.allocPrintZ( + arena_alloc, + "{s}:{}:{}: invalid value {s} for {s}", + .{ + iter.filepath, + line_context.line_number, + line_context.column_number_value.?, + value.?, + key, + }, + ); + } else { + message = try std.fmt.allocPrintZ( + arena_alloc, + "{s}: invalid value {s}", + .{ key, value.? }, + ); + } + }, - else => try dst._errors.add(arena_alloc, .{ - .message = try std.fmt.allocPrintZ( - arena_alloc, - "{s}: unknown error {}", - .{ key, err }, - ), - }), + else => { + // Any other errors will likely be an issue with the value since we've + // already handled errors for invalid keys and missing values above. + assert(value != null); + if (@hasDecl(Iter, "lineContext")) { + const line_context = iter.lineContext(); + message = try std.fmt.allocPrintZ( + arena_alloc, + "{s}:{}:{}: {} for value {s}", + .{ + iter.filepath, + line_context.line_number, + line_context.column_number_value.?, + err, + value.?, + }, + ); + } else { + message = try std.fmt.allocPrintZ( + arena_alloc, + "{s}: {} for value {s}", + .{ key, err, value.? }, + ); + } + }, } + try dst._errors.add(arena_alloc, .{ .message = message }); }; } } @@ -748,6 +816,12 @@ test "parseIntoField: struct with parse func with unsupported error tracking" { /// configuration files. pub fn LineIterator(comptime ReaderType: type) type { return struct { + const LineContext = struct { + line_number: usize, + column_number_key: usize, + column_number_value: ?usize, + }; + const Self = @This(); /// The maximum size a single line can be. We don't expect any @@ -756,7 +830,13 @@ pub fn LineIterator(comptime ReaderType: type) type { pub const MAX_LINE_SIZE = 4096; r: ReaderType, + filepath: []const u8, entry: [MAX_LINE_SIZE]u8 = [_]u8{ '-', '-' } ++ ([_]u8{0} ** (MAX_LINE_SIZE - 2)), + line_context: LineContext = LineContext{ + .line_number = 0, + .column_number_key = 0, + .column_number_value = null, + }, pub fn next(self: *Self) ?[]const u8 { // TODO: detect "--" prefixed lines and give a friendlier error @@ -768,6 +848,33 @@ pub fn LineIterator(comptime ReaderType: type) type { unreachable; } orelse return null; + self.line_context.line_number += 1; + if (mem.indexOfNone(u8, entry, whitespace)) |index_key| { + self.line_context.column_number_key = index_key + 1; + if (mem.indexOf(u8, entry, "=")) |index_separator| { + const entry_after_separator = entry[index_separator + 1 ..]; + if (mem.indexOfNone( + u8, + entry_after_separator, + whitespace ++ "\"", + )) |index_value| { + // We add two because each index is 0-based and we need to represent + // them as 1-based. + self.line_context.column_number_value = index_separator + index_value + 2; + } else { + // No value found after a separator, point to it instead in case + // this is an error for a missing value. + self.line_context.column_number_value = index_separator + 1; + } + } else { + self.line_context.column_number_value = null; + } + } else { + // Empty line, we'll skip it below. + self.line_context.column_number_key = 0; + self.line_context.column_number_value = null; + } + // Trim any whitespace (including CR) around it const trim = std.mem.trim(u8, entry, whitespace ++ "\r"); if (trim.len != entry.len) { @@ -815,12 +922,16 @@ pub fn LineIterator(comptime ReaderType: type) type { // as CLI args. return self.entry[0 .. buf.len + 2]; } + + pub fn lineContext(self: *Self) LineContext { + return self.line_context; + } }; } // Constructs a LineIterator (see docs for that). -pub fn lineIterator(reader: anytype) LineIterator(@TypeOf(reader)) { - return .{ .r = reader }; +pub fn lineIterator(reader: anytype, filepath: []const u8) LineIterator(@TypeOf(reader)) { + return .{ .r = reader, .filepath = filepath }; } /// An iterator valid for arg parsing from a slice. @@ -853,28 +964,57 @@ test "LineIterator" { \\D \\ \\ # An indented comment - \\ E + \\ E = value \\ \\# A quoted string with whitespace \\F= "value " + \\ ); - var iter = lineIterator(fbs.reader()); + var iter = lineIterator(fbs.reader(), ""); try testing.expectEqualStrings("--A", iter.next().?); + try testing.expectEqual(1, iter.lineContext().line_number); + try testing.expectEqual(1, iter.lineContext().column_number_key); + try testing.expectEqual(null, iter.lineContext().column_number_value); + try testing.expectEqualStrings("--B=42", iter.next().?); + try testing.expectEqual(2, iter.lineContext().line_number); + try testing.expectEqual(1, iter.lineContext().column_number_key); + try testing.expectEqual(3, iter.lineContext().column_number_value.?); + try testing.expectEqualStrings("--C", iter.next().?); + try testing.expectEqual(3, iter.lineContext().line_number); + try testing.expectEqual(1, iter.lineContext().column_number_key); + try testing.expectEqual(null, iter.lineContext().column_number_value); + try testing.expectEqualStrings("--D", iter.next().?); - try testing.expectEqualStrings("--E", iter.next().?); + try testing.expectEqual(6, iter.lineContext().line_number); + try testing.expectEqual(1, iter.lineContext().column_number_key); + try testing.expectEqual(null, iter.lineContext().column_number_value); + + try testing.expectEqualStrings("--E=value", iter.next().?); + try testing.expectEqual(9, iter.lineContext().line_number); + try testing.expectEqual(3, iter.lineContext().column_number_key); + try testing.expectEqual(7, iter.lineContext().column_number_value.?); + try testing.expectEqualStrings("--F=value ", iter.next().?); + try testing.expectEqual(12, iter.lineContext().line_number); + try testing.expectEqual(1, iter.lineContext().column_number_key); + try testing.expectEqual(6, iter.lineContext().column_number_value.?); + + // No lines left, line context remains the same. try testing.expectEqual(@as(?[]const u8, null), iter.next()); try testing.expectEqual(@as(?[]const u8, null), iter.next()); + try testing.expectEqual(12, iter.lineContext().line_number); + try testing.expectEqual(1, iter.lineContext().column_number_key); + try testing.expectEqual(6, iter.lineContext().column_number_value); } test "LineIterator end in newline" { const testing = std.testing; var fbs = std.io.fixedBufferStream("A\n\n"); - var iter = lineIterator(fbs.reader()); + var iter = lineIterator(fbs.reader(), ""); try testing.expectEqualStrings("--A", iter.next().?); try testing.expectEqual(@as(?[]const u8, null), iter.next()); try testing.expectEqual(@as(?[]const u8, null), iter.next()); @@ -884,7 +1024,7 @@ test "LineIterator spaces around '='" { const testing = std.testing; var fbs = std.io.fixedBufferStream("A = B\n\n"); - var iter = lineIterator(fbs.reader()); + var iter = lineIterator(fbs.reader(), ""); try testing.expectEqualStrings("--A=B", iter.next().?); try testing.expectEqual(@as(?[]const u8, null), iter.next()); try testing.expectEqual(@as(?[]const u8, null), iter.next()); @@ -894,7 +1034,7 @@ test "LineIterator no value" { const testing = std.testing; var fbs = std.io.fixedBufferStream("A = \n\n"); - var iter = lineIterator(fbs.reader()); + var iter = lineIterator(fbs.reader(), ""); try testing.expectEqualStrings("--A=", iter.next().?); try testing.expectEqual(@as(?[]const u8, null), iter.next()); } @@ -903,7 +1043,7 @@ test "LineIterator with CRLF line endings" { const testing = std.testing; var fbs = std.io.fixedBufferStream("A\r\nB = C\r\n"); - var iter = lineIterator(fbs.reader()); + var iter = lineIterator(fbs.reader(), ""); try testing.expectEqualStrings("--A", iter.next().?); try testing.expectEqualStrings("--B=C", iter.next().?); try testing.expectEqual(@as(?[]const u8, null), iter.next()); diff --git a/src/config/CAPI.zig b/src/config/CAPI.zig index 6e62a0891..125cd0d93 100644 --- a/src/config/CAPI.zig +++ b/src/config/CAPI.zig @@ -43,17 +43,19 @@ export fn ghostty_config_load_cli_args(self: *Config) void { /// the file-based syntax for the desktop version of the terminal. export fn ghostty_config_load_string( self: *Config, - str: [*]const u8, - len: usize, + filepath: [*]const u8, + filepath_len: usize, + contents: [*]const u8, + contents_len: usize, ) void { - config_load_string_(self, str[0..len]) catch |err| { + config_load_string_(self, filepath[0..filepath_len], contents[0..contents_len]) catch |err| { log.err("error loading config err={}", .{err}); }; } -fn config_load_string_(self: *Config, str: []const u8) !void { - var fbs = std.io.fixedBufferStream(str); - var iter = cli.args.lineIterator(fbs.reader()); +fn config_load_string_(self: *Config, filepath: []const u8, contents: []const u8) !void { + var fbs = std.io.fixedBufferStream(contents); + var iter = cli.args.lineIterator(fbs.reader(), filepath); try cli.args.parse(Config, @TypeOf(iter), global.alloc, self, &iter); } diff --git a/src/config/Config.zig b/src/config/Config.zig index 5f5d8cd7e..425aaa47c 100644 --- a/src/config/Config.zig +++ b/src/config/Config.zig @@ -2077,7 +2077,7 @@ pub fn loadFile(self: *Config, alloc: Allocator, path: []const u8) !void { std.log.info("reading configuration file path={s}", .{path}); var buf_reader = std.io.bufferedReader(file.reader()); - var iter = cli.args.lineIterator(buf_reader.reader()); + var iter = cli.args.lineIterator(buf_reader.reader(), path); try self.loadIter(@TypeOf(iter), alloc, &iter); try self.expandPaths(std.fs.path.dirname(path).?); } @@ -2271,7 +2271,7 @@ pub fn loadRecursiveFiles(self: *Config, alloc_gpa: Allocator) !void { log.info("loading config-file path={s}", .{path}); var buf_reader = std.io.bufferedReader(file.reader()); - var iter = cli.args.lineIterator(buf_reader.reader()); + var iter = cli.args.lineIterator(buf_reader.reader(), path); try self.loadIter(@TypeOf(iter), alloc_gpa, &iter); try self.expandPaths(std.fs.path.dirname(path).?); } @@ -2302,11 +2302,13 @@ fn expandPaths(self: *Config, base: []const u8) !void { fn loadTheme(self: *Config, theme: []const u8) !void { // Find our theme file and open it. See the open function for details. - const file: std.fs.File = (try themepkg.open( + const file_with_path = (try themepkg.open( self._arena.?.allocator(), theme, &self._errors, )) orelse return; + const path = file_with_path.path; + const file = file_with_path.file; defer file.close(); // From this point onwards, we load the theme and do a bit of a dance @@ -2332,7 +2334,7 @@ fn loadTheme(self: *Config, theme: []const u8) !void { // Load our theme var buf_reader = std.io.bufferedReader(file.reader()); - var iter = cli.args.lineIterator(buf_reader.reader()); + var iter = cli.args.lineIterator(buf_reader.reader(), path); try new_config.loadIter(@TypeOf(iter), alloc_gpa, &iter); // Replay our previous inputs so that we can override values diff --git a/src/config/Wasm.zig b/src/config/Wasm.zig index d8db3c13a..d9fcba95b 100644 --- a/src/config/Wasm.zig +++ b/src/config/Wasm.zig @@ -33,17 +33,19 @@ export fn config_free(ptr: ?*Config) void { /// the file-based syntax for the desktop version of the terminal. export fn config_load_string( self: *Config, - str: [*]const u8, - len: usize, + filepath: [*]const u8, + filepath_len: usize, + contents: [*]const u8, + contents_len: usize, ) void { - config_load_string_(self, str[0..len]) catch |err| { + config_load_string_(self, filepath[0..filepath_len], contents[0..contents_len]) catch |err| { log.err("error loading config err={}", .{err}); }; } -fn config_load_string_(self: *Config, str: []const u8) !void { - var fbs = std.io.fixedBufferStream(str); - var iter = cli.args.lineIterator(fbs.reader()); +fn config_load_string_(self: *Config, filepath: []const u8, contents: []const u8) !void { + var fbs = std.io.fixedBufferStream(contents); + var iter = cli.args.lineIterator(fbs.reader(), filepath); try cli.args.parse(Config, @TypeOf(iter), alloc, self, &iter); } diff --git a/src/config/theme.zig b/src/config/theme.zig index fdb5dd08a..b2180e09b 100644 --- a/src/config/theme.zig +++ b/src/config/theme.zig @@ -108,14 +108,13 @@ pub fn open( arena_alloc: Allocator, theme: []const u8, errors: *ErrorList, -) error{OutOfMemory}!?std.fs.File { +) error{OutOfMemory}!?struct { path: []const u8, file: std.fs.File } { // Absolute themes are loaded a different path. - if (std.fs.path.isAbsolute(theme)) return try openAbsolute( - arena_alloc, - theme, - errors, - ); + if (std.fs.path.isAbsolute(theme)) { + const file = try openAbsolute(arena_alloc, theme, errors) orelse return null; + return .{ .path = theme, .file = file }; + } const basename = std.fs.path.basename(theme); if (!std.mem.eql(u8, theme, basename)) { @@ -136,7 +135,7 @@ pub fn open( while (try it.next()) |loc| { const path = try std.fs.path.join(arena_alloc, &.{ loc.dir, theme }); if (cwd.openFile(path, .{})) |file| { - return file; + return .{ .path = path, .file = file }; } else |err| switch (err) { // Not an error, just continue to the next location. error.FileNotFound => {},