From 84432a7beb01ffdc87812a3f3ddde2dc74475010 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sat, 28 Jun 2025 13:06:36 -0700 Subject: [PATCH] config: more general purpose backwards compatibility handlers Fixes #7706 We previously had a very specific backwards compatibility handler for handling renamed fields. We always knew that wouldn't scale but I wanted to wait for a real case. Well, #7706 is a real case, so here we are. This commit makes our backwards compatibility handler more general purpose, and makes a special-case handler for renamed fields built on top of this same general purpose system. The new system lets us do a lot more with regards to backwards compatibility. To start, this addresses #7706 by allowing us to handle a removed single enum value of a still-existing field. --- src/cli.zig | 2 + src/cli/args.zig | 190 ++++++++++++++++++++++++++++++------------ src/config/Config.zig | 37 +++++++- 3 files changed, 174 insertions(+), 55 deletions(-) diff --git a/src/cli.zig b/src/cli.zig index 4336501a8..151e6e648 100644 --- a/src/cli.zig +++ b/src/cli.zig @@ -2,6 +2,8 @@ const diags = @import("cli/diagnostics.zig"); pub const args = @import("cli/args.zig"); pub const Action = @import("cli/action.zig").Action; +pub const CompatibilityHandler = args.CompatibilityHandler; +pub const compatibilityRenamed = args.compatibilityRenamed; pub const DiagnosticList = diags.DiagnosticList; pub const Diagnostic = diags.Diagnostic; pub const Location = diags.Location; diff --git a/src/cli/args.zig b/src/cli/args.zig index 3c34e17fe..65e72636e 100644 --- a/src/cli/args.zig +++ b/src/cli/args.zig @@ -40,11 +40,14 @@ pub const Error = error{ /// "DiagnosticList" and any diagnostic messages will be added to that list. /// When diagnostics are present, only allocation errors will be returned. /// -/// If the destination type has a decl "renamed", it must be of type -/// std.StaticStringMap([]const u8) and contains a mapping from the old -/// field name to the new field name. This is used to allow renaming fields -/// while still supporting the old name. If a renamed field is set, parsing -/// will automatically set the new field name. +/// If the destination type has a decl "compatibility", it must be of type +/// std.StaticStringMap(CompatibilityHandler(T)), and it will be used to +/// handle backwards compatibility for fields with the given name. The +/// field name doesn't need to exist (so you can setup compatibility for +/// removed fields). The value is a function that will be called when +/// all other parsing fails for that field. If a field changes such that +/// the old values would NOT error, then the caller should handle that +/// downstream after parsing is done, not through this method. /// /// 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. @@ -57,24 +60,6 @@ pub fn parse( const info = @typeInfo(T); assert(info == .@"struct"); - comptime { - // Verify all renamed fields are valid (source does not exist, - // destination does exist). - if (@hasDecl(T, "renamed")) { - for (T.renamed.keys(), T.renamed.values()) |key, value| { - if (@hasField(T, key)) { - @compileLog(key); - @compileError("renamed field source exists"); - } - - if (!@hasField(T, value)) { - @compileLog(value); - @compileError("renamed field destination does not exist"); - } - } - } - } - // Make an arena for all our allocations if we support it. Otherwise, // use an allocator that always fails. If the arena is already set on // the config, then we reuse that. See memory note in parse docs. @@ -148,6 +133,16 @@ pub fn parse( }; parseIntoField(T, arena_alloc, dst, key, value) catch |err| { + // If we get an error parsing a field, then we try to fall + // back to compatibility handlers if able. + if (@hasDecl(T, "compatibility")) { + // If we have a compatibility handler for this key, then + // we call it and see if it handles the error. + if (T.compatibility.get(key)) |handler| { + if (handler(dst, arena_alloc, key, value)) return; + } + } + if (comptime !canTrackDiags(T)) return err; // The error set is dependent on comptime T, so we always add @@ -177,6 +172,58 @@ pub fn parse( } } +/// The function type for a compatibility handler. The compatibility +/// handler is documented in the `parse` function documentation. +/// +/// The function type should return bool if the compatibility was +/// handled, and false otherwise. If false is returned then the +/// naturally occurring error will continue to be processed as if +/// this compatibility handler was not present. +/// +/// Compatibility handlers aren't allowed to return errors because +/// they're generally only called in error cases, so we already have +/// an error message to show users. If there is an error in handling +/// the compatibility, then the handler should return false. +pub fn CompatibilityHandler(comptime T: type) type { + return *const fn ( + dst: *T, + alloc: Allocator, + key: []const u8, + value: ?[]const u8, + ) bool; +} + +/// Convenience function to create a compatibility handler that +/// renames a field from `from` to `to`. +pub fn compatibilityRenamed( + comptime T: type, + comptime to: []const u8, +) CompatibilityHandler(T) { + comptime assert(@hasField(T, to)); + + return (struct { + fn compat( + dst: *T, + alloc: Allocator, + key: []const u8, + value: ?[]const u8, + ) bool { + _ = key; + + parseIntoField(T, alloc, dst, to, value) catch |err| { + log.warn("error parsing renamed field {s}: {}", .{ + to, + err, + }); + + return false; + }; + + return true; + } + }).compat; +} + fn formatValueRequired( comptime T: type, arena_alloc: std.mem.Allocator, @@ -401,16 +448,6 @@ pub fn parseIntoField( } } - // Unknown field, is the field renamed? - if (@hasDecl(T, "renamed")) { - for (T.renamed.keys(), T.renamed.values()) |old, new| { - if (mem.eql(u8, old, key)) { - try parseIntoField(T, alloc, dst, new, value); - return; - } - } - } - return error.InvalidField; } @@ -752,6 +789,75 @@ test "parse: diagnostic location" { } } +test "parse: compatibility handler" { + const testing = std.testing; + + var data: struct { + a: bool = false, + _arena: ?ArenaAllocator = null, + + pub const compatibility: std.StaticStringMap( + CompatibilityHandler(@This()), + ) = .initComptime(&.{ + .{ "a", compat }, + }); + + fn compat( + self: *@This(), + alloc: Allocator, + key: []const u8, + value: ?[]const u8, + ) bool { + _ = alloc; + if (std.mem.eql(u8, key, "a")) { + if (value) |v| { + if (mem.eql(u8, v, "yuh")) { + self.a = true; + return true; + } + } + } + + return false; + } + } = .{}; + defer if (data._arena) |arena| arena.deinit(); + + var iter = try std.process.ArgIteratorGeneral(.{}).init( + testing.allocator, + "--a=yuh", + ); + defer iter.deinit(); + try parse(@TypeOf(data), testing.allocator, &data, &iter); + try testing.expect(data._arena != null); + try testing.expect(data.a); +} + +test "parse: compatibility renamed" { + const testing = std.testing; + + var data: struct { + a: bool = false, + _arena: ?ArenaAllocator = null, + + pub const compatibility: std.StaticStringMap( + CompatibilityHandler(@This()), + ) = .initComptime(&.{ + .{ "old", compatibilityRenamed(@This(), "a") }, + }); + } = .{}; + defer if (data._arena) |arena| arena.deinit(); + + var iter = try std.process.ArgIteratorGeneral(.{}).init( + testing.allocator, + "--old=true", + ); + defer iter.deinit(); + try parse(@TypeOf(data), testing.allocator, &data, &iter); + try testing.expect(data._arena != null); + try testing.expect(data.a); +} + test "parseIntoField: ignore underscore-prefixed fields" { const testing = std.testing; var arena = ArenaAllocator.init(testing.allocator); @@ -1176,24 +1282,6 @@ test "parseIntoField: tagged union missing tag" { ); } -test "parseIntoField: renamed field" { - const testing = std.testing; - var arena = ArenaAllocator.init(testing.allocator); - defer arena.deinit(); - const alloc = arena.allocator(); - - var data: struct { - a: []const u8, - - const renamed = std.StaticStringMap([]const u8).initComptime(&.{ - .{ "old", "a" }, - }); - } = undefined; - - try parseIntoField(@TypeOf(data), alloc, &data, "old", "42"); - try testing.expectEqualStrings("42", data.a); -} - /// An iterator that considers its location to be CLI args. It /// iterates through an underlying iterator and increments a counter /// to track the current CLI arg index. diff --git a/src/config/Config.zig b/src/config/Config.zig index 44089fb57..3cb61fc76 100644 --- a/src/config/Config.zig +++ b/src/config/Config.zig @@ -46,14 +46,22 @@ const c = @cImport({ @cInclude("unistd.h"); }); -/// Renamed fields, used by cli.parse -pub const renamed = std.StaticStringMap([]const u8).initComptime(&.{ +pub const compatibility = std.StaticStringMap( + cli.CompatibilityHandler(Config), +).initComptime(&.{ // Ghostty 1.1 introduced background-blur support for Linux which // doesn't support a specific radius value. The renaming is to let // one field be used for both platforms (macOS retained the ability // to set a radius). - .{ "background-blur-radius", "background-blur" }, - .{ "adw-toolbar-style", "gtk-toolbar-style" }, + .{ "background-blur-radius", cli.compatibilityRenamed(Config, "background-blur") }, + + // Ghostty 1.2 renamed all our adw options to gtk because we now have + // a hard dependency on libadwaita. + .{ "adw-toolbar-style", cli.compatibilityRenamed(Config, "gtk-toolbar-style") }, + + // Ghostty 1.2 removed the `hidden` value from `gtk-tabs-location` and + // moved it to `window-show-tab-bar`. + .{ "gtk-tabs-location", compatGtkTabsLocation }, }); /// The font families to use. @@ -3792,6 +3800,27 @@ pub fn parseManuallyHook( return true; } +/// parseFieldManuallyFallback is a fallback called only when +/// parsing the field directly failed. It can be used to implement +/// backward compatibility. Since this is only called when parsing +/// fails, it doesn't impact happy-path performance. +fn compatGtkTabsLocation( + self: *Config, + alloc: Allocator, + key: []const u8, + value: ?[]const u8, +) bool { + _ = alloc; + assert(std.mem.eql(u8, key, "gtk-tabs-location")); + + if (std.mem.eql(u8, value orelse "", "hidden")) { + self.@"window-show-tab-bar" = .never; + return true; + } + + return false; +} + /// Create a shallow copy of this config. This will share all the memory /// allocated with the previous config but will have a new arena for /// any changes or new allocations. The config should have `deinit`