Skip to content

Zigify the Zig code#217

Merged
emil-e merged 4 commits into
dakra:mainfrom
emil-e:emil-e/zigify
May 2, 2026
Merged

Zigify the Zig code#217
emil-e merged 4 commits into
dakra:mainfrom
emil-e:emil-e/zigify

Conversation

@emil-e
Copy link
Copy Markdown
Collaborator

@emil-e emil-e commented May 1, 2026

This PR adds some light wrappers around the libghostty-vt C API to make it play better with Zig:

  • Convert out-pointer APIs to return value APIs
  • Convert return error codes to Zig error sets
  • Distinguish between optional values and errors

This also has the effect that errors are actually propagated all the way and signaled to Emacs so they don't go unnoticed. When built in debug mode, there will also be a stack trace printed to Warnings.

@emil-e
Copy link
Copy Markdown
Collaborator Author

emil-e commented May 1, 2026

Also added some agent instructions to improve things for the future.

@dakra
Copy link
Copy Markdown
Owner

dakra commented May 1, 2026

Great. To me it looks very nice and we can merge it when you want.
Not sure if it's helpful, but here are some minor issues from claude /review:


Issues

1. callFmt returns uninitialized memory on NoSpaceLeft (correctness)

emacs.zig:280-285:

fn callFmt(self: Env, func: anytype, comptime fmt: []const u8, args: anytype) void {
    var buf: [1024]u8 = undefined;
    const msg = std.fmt.bufPrint(&buf, fmt, args) catch |err| switch (err) {
        error.NoSpaceLeft => buf[0..],
    };
    @call(.auto, func, .{ self, msg });
}

When bufPrint returns NoSpaceLeft, it does not return a slice telling you how much was written — so buf[0..] contains the partially-written prefix followed by uninitialized stack memory, which is then sent to Emacs as a string and shown to the user. This can leak whatever was on the stack.

Easy fix: replace the fallback with a fixed sentinel ("<message truncated>"), or switch to std.io.Writer.fixed like logStackTrace does and slice via writer.end.

2. getMulti swallows partial-write into Error.Unknown (debuggability)

ghostty.zig:256-269:

return if (num_written == keys_types.len) values else Error.Unknown;

If libghostty returns SUCCESS but writes fewer pointers than requested, you get Error.Unknown, which is ambiguous with the catch-all in toError. Consider a distinct error (PartialMulti / IncompleteRead) so the caller can tell "unmapped C return code" apart from "succeeded but wrote N of M".

3. Asymmetric error-handling for fg/bg in readCellStyle (suspect)

render.zig:84-92:

style.fg = gt.rs_row_cells.get(gt.ColorRgb, cells, gt.RS_CELLS_DATA_FG_COLOR) catch |err| switch (err) {
    gt.Error.InvalidValue => null, ...
};
style.bg = gt.rs_row_cells.getOpt(gt.ColorRgb, cells, gt.RS_CELLS_DATA_BG_COLOR) catch |err| switch (err) {
    gt.Error.InvalidValue => null, ...
};

fg uses .get + catches InvalidValue; bg uses .getOpt (which already maps NoValue→null) and also catches InvalidValue. Either both keys can return both NoValue and InvalidValue (then fg is missing the NoValue case), or only one can return NoValue (then bg's getOpt is unnecessary). Please verify against the libghostty header and make these symmetric, or comment why they differ.

4. Lost fallbacks change failure semantics (behavioral)

A couple of pre-existing fallbacks vanished:

  • terminal.zig getTotalRows/getScrollbackRows previously returned self.size.rows on libghostty failure. Now they error.
  • render.zig getDefaultColors previously fell back to hardcoded (0,0,0) / (204,204,204). Now any failure aborts the entire render.

The new behavior is arguably more correct, but the old defaults existed presumably because libghostty can return non-SUCCESS on these in some states. If you've verified that's never the case for the keys involved, great — otherwise users may now see error popups where they previously saw a slightly-wrong color or row count. Worth a smoke test against a long-running session and the make test-native suite paying particular attention to emitPlacements and redraw.

5. Minor

  • ghostty.zig:9pub const Self = @This(); appears unused in the file.
  • emacs.zig:282-287message/messagef are added but never called; per CLAUDE.md "no features beyond what was asked," consider deferring until the first use site.
  • module.zig:1024(term.getScrollbar() catch ...).offset reads OK but the parenthesized catch ... return env.nil() inside a field-access expression is a little dense. A two-line const sb = ... ; const saved_offset = sb.offset; is more grep-friendly. (Repeats in fnDebugState/fnDebugFeed/fnCursorPosition.)
  • getMulti uses tuple destructuring + @Type-built anonymous struct — slick, but the // zig fmt: off/on fences around two-line literals at getDefaultColors and renderCursor suggest the formatter dislikes the const fg, const bg = pattern. Worth checking if a small helper struct with named fields would read better than the tuple.

@emil-e
Copy link
Copy Markdown
Collaborator Author

emil-e commented May 1, 2026

It is helpful, will take care of them tomorrow. I will just merge after the fixes are applied and CI passes.

@dakra
Copy link
Copy Markdown
Owner

dakra commented May 1, 2026

It is helpful, will take care of them tomorrow. I will just merge after the fixes are applied and CI passes.

Great. btw, Github actions had problems all day and the CI is red for the last few commits already.
It's a github actions issue and normally retrying fixes it but apparently not today 🤷
So don't be bothered if it doesn't turn green when it's not red because of your changes.

emil-e added 4 commits May 2, 2026 08:37
…rgonomics

These methods:
- Adapt the API to be return value oriented rather that out-pointer
oriented
- Report errors the Zig way
Meaning:
- Avoid out-pointers, prefer return values
- Distinguish between optional and errors
- Propagate and log/handle errors
- Use try/catch and error unions, no C error values
@emil-e
Copy link
Copy Markdown
Collaborator Author

emil-e commented May 2, 2026

Merging this although the melpazoid job is still failing. I'm assuming it's due to the issues that you described, which makes sense because the failing command is the package installation, not any test or build.

@emil-e emil-e merged commit 370c139 into dakra:main May 2, 2026
20 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants