upgrade to latest srf and zig 0.16

.gitignore

@@ -1,2 +1,4 @@
 .zig-cache/
 zig-out/
+# Zig 0.16 materialises fetched dependencies here.
+zig-pkg/

.mise.toml

@@ -1,5 +1,5 @@
 [tools]
 prek = "0.3.1"
 "ubi:DonIsaac/zlint" = "0.7.9"
-zig = "0.15.2"
-zls = "0.15.1"
+zig = "0.16.0"
+zls = "0.16.0"

build.zig.zon

@@ -2,11 +2,11 @@
     .name = .srf_lsp,
     .version = "0.0.0",
     .fingerprint = 0x96c04d564e5a0f32,
-    .minimum_zig_version = "0.15.2",
+    .minimum_zig_version = "0.16.0",
     .dependencies = .{
         .srf = .{
-            .url = "git+https://git.lerch.org/lobo/srf.git#1a42735f11c15ce2e125aaad99699159925b3fdc",
-            .hash = "srf-0.0.0-qZj578XCAQDWNP6V8NxyyDCkrBbY7NWR8tP6wAigl8it",
+            .url = "git+https://git.lerch.org/lobo/srf#7692d85745a90144e358bac9f73d1090ddc125fe",
+            .hash = "srf-0.0.0-qZj5760tAgDN_2Y-nsXzkW_qbntB05Iio___ziFdi937",
         },
     },
     .paths = .{

src/Server.zig

@@ -39,7 +39,9 @@ const Phase = enum {
 };
 
 allocator: std.mem.Allocator,
-out: std.fs.File,
+/// Where protocol traffic goes. Buffered, so `rpc.writeMessage` flushes after
+/// every message.
+out: *std.Io.Writer,
 documents: std.StringHashMapUnmanaged([]const u8) = .empty,
 phase: Phase = .uninitialized,
 position_encoding: analysis.PositionEncoding = .@"utf-16",
@@ -50,7 +52,7 @@ exit_requested: bool = false,
 /// `shutdown`, 1 without one.
 exit_code: u8 = 1,
 
-pub fn init(allocator: std.mem.Allocator, out: std.fs.File) Server {
+pub fn init(allocator: std.mem.Allocator, out: *std.Io.Writer) Server {
     return .{ .allocator = allocator, .out = out };
 }
 
@@ -347,49 +349,43 @@ fn publishDiagnostics(self: *Server, uri: []const u8, text: []const u8) !void {
 
 const testing = std.testing;
 
-/// Drives a server against a scratch file so tests can read back exactly what
-/// went over the wire.
+/// A server writing into an in-memory buffer, so tests can assert on the exact
+/// bytes that went over the wire without touching the filesystem.
+///
+/// Pinned: `server.out` points at `self.out.writer`, so this must be initialised
+/// in place and never copied or moved. Declare it as
+/// `var h: Harness = undefined; h.setup(allocator);`.
 const Harness = struct {
-    dir: std.testing.TmpDir,
-    file: std.fs.File,
+    out: std.Io.Writer.Allocating,
     server: Server,
 
-    fn init(allocator: std.mem.Allocator) !Harness {
-        var dir = std.testing.tmpDir(.{});
-        const file = try dir.dir.createFile("out.jsonl", .{ .read = true });
-        return .{ .dir = dir, .file = file, .server = Server.init(allocator, file) };
+    fn setup(self: *Harness, allocator: std.mem.Allocator) void {
+        self.out = .init(allocator);
+        self.server = Server.init(allocator, &self.out.writer);
     }
 
     fn deinit(self: *Harness) void {
         self.server.deinit();
-        self.file.close();
-        self.dir.cleanup();
+        self.out.deinit();
     }
 
-    fn sent(self: *Harness, allocator: std.mem.Allocator) ![]u8 {
-        const size = try self.file.getEndPos();
-        const buf = try allocator.alloc(u8, size);
-        errdefer allocator.free(buf);
-        _ = try self.file.preadAll(buf, 0);
-        return buf;
+    /// Everything the server has sent so far.
+    fn transcript(self: *Harness) []const u8 {
+        return self.out.written();
     }
 
     /// Asserts the transcript contains `needle`, which keeps the tests readable
     /// without reimplementing a JSON-RPC client here.
     fn expectSent(self: *Harness, needle: []const u8) !void {
-        const transcript = try self.sent(testing.allocator);
-        defer testing.allocator.free(transcript);
-        if (std.mem.indexOf(u8, transcript, needle) == null) {
-            std.debug.print("\nexpected to find: {s}\nin transcript:\n{s}\n", .{ needle, transcript });
+        if (std.mem.indexOf(u8, self.transcript(), needle) == null) {
+            std.debug.print("\nexpected to find: {s}\nin transcript:\n{s}\n", .{ needle, self.transcript() });
             return error.NotSent;
         }
     }
 
     fn expectNotSent(self: *Harness, needle: []const u8) !void {
-        const transcript = try self.sent(testing.allocator);
-        defer testing.allocator.free(transcript);
-        if (std.mem.indexOf(u8, transcript, needle) != null) {
-            std.debug.print("\nexpected NOT to find: {s}\nin transcript:\n{s}\n", .{ needle, transcript });
+        if (std.mem.indexOf(u8, self.transcript(), needle) != null) {
+            std.debug.print("\nexpected NOT to find: {s}\nin transcript:\n{s}\n", .{ needle, self.transcript() });
             return error.UnexpectedlySent;
         }
     }
@@ -402,7 +398,8 @@ const Harness = struct {
 };
 
 test "initialize advertises capabilities and answers the request" {
-    var h = try Harness.init(testing.allocator);
+    var h: Harness = undefined;
+    h.setup(testing.allocator);
     defer h.deinit();
 
     try h.initialize();
@@ -413,7 +410,8 @@ test "initialize advertises capabilities and answers the request" {
 }
 
 test "requests before initialize are refused, not ignored" {
-    var h = try Harness.init(testing.allocator);
+    var h: Harness = undefined;
+    h.setup(testing.allocator);
     defer h.deinit();
 
     try h.server.handleMessage(
@@ -423,7 +421,8 @@ test "requests before initialize are refused, not ignored" {
 }
 
 test "an unimplemented request gets method_not_found rather than silence" {
-    var h = try Harness.init(testing.allocator);
+    var h: Harness = undefined;
+    h.setup(testing.allocator);
     defer h.deinit();
     try h.initialize();
 
@@ -435,24 +434,23 @@ test "an unimplemented request gets method_not_found rather than silence" {
 }
 
 test "an unimplemented notification is silently dropped" {
-    var h = try Harness.init(testing.allocator);
+    var h: Harness = undefined;
+    h.setup(testing.allocator);
     defer h.deinit();
     try h.initialize();
 
-    const before = try h.sent(testing.allocator);
-    defer testing.allocator.free(before);
+    const before = h.transcript().len;
 
     try h.server.handleMessage(
         \\{"jsonrpc":"2.0","method":"$/setTrace","params":{"value":"verbose"}}
     );
 
-    const after = try h.sent(testing.allocator);
-    defer testing.allocator.free(after);
-    try testing.expectEqual(before.len, after.len);
+    try testing.expectEqual(before, h.transcript().len);
 }
 
 test "string request ids are echoed back verbatim" {
-    var h = try Harness.init(testing.allocator);
+    var h: Harness = undefined;
+    h.setup(testing.allocator);
     defer h.deinit();
     try h.initialize();
 
@@ -463,7 +461,8 @@ test "string request ids are echoed back verbatim" {
 }
 
 test "didOpen on a broken document publishes diagnostics" {
-    var h = try Harness.init(testing.allocator);
+    var h: Harness = undefined;
+    h.setup(testing.allocator);
     defer h.deinit();
     try h.initialize();
 
@@ -475,7 +474,8 @@ test "didOpen on a broken document publishes diagnostics" {
 }
 
 test "didOpen on a valid document publishes an empty diagnostic list" {
-    var h = try Harness.init(testing.allocator);
+    var h: Harness = undefined;
+    h.setup(testing.allocator);
     defer h.deinit();
     try h.initialize();
 
@@ -486,7 +486,8 @@ test "didOpen on a valid document publishes an empty diagnostic list" {
 }
 
 test "didChange replaces the document and republishes" {
-    var h = try Harness.init(testing.allocator);
+    var h: Harness = undefined;
+    h.setup(testing.allocator);
     defer h.deinit();
     try h.initialize();
 
@@ -502,7 +503,8 @@ test "didChange replaces the document and republishes" {
 }
 
 test "didChange for an unopened document still tracks it" {
-    var h = try Harness.init(testing.allocator);
+    var h: Harness = undefined;
+    h.setup(testing.allocator);
     defer h.deinit();
     try h.initialize();
 
@@ -513,7 +515,8 @@ test "didChange for an unopened document still tracks it" {
 }
 
 test "an incremental change is refused instead of corrupting the document" {
-    var h = try Harness.init(testing.allocator);
+    var h: Harness = undefined;
+    h.setup(testing.allocator);
     defer h.deinit();
     try h.initialize();
 
@@ -527,7 +530,8 @@ test "an incremental change is refused instead of corrupting the document" {
 }
 
 test "didClose forgets the document and clears its diagnostics" {
-    var h = try Harness.init(testing.allocator);
+    var h: Harness = undefined;
+    h.setup(testing.allocator);
     defer h.deinit();
     try h.initialize();
 
@@ -542,7 +546,8 @@ test "didClose forgets the document and clears its diagnostics" {
 }
 
 test "reopening a document does not leak the previous text" {
-    var h = try Harness.init(testing.allocator);
+    var h: Harness = undefined;
+    h.setup(testing.allocator);
     defer h.deinit();
     try h.initialize();
 
@@ -555,7 +560,8 @@ test "reopening a document does not leak the previous text" {
 }
 
 test "shutdown then exit gives exit code 0" {
-    var h = try Harness.init(testing.allocator);
+    var h: Harness = undefined;
+    h.setup(testing.allocator);
     defer h.deinit();
     try h.initialize();
 
@@ -571,7 +577,8 @@ test "shutdown then exit gives exit code 0" {
 }
 
 test "exit without shutdown gives exit code 1" {
-    var h = try Harness.init(testing.allocator);
+    var h: Harness = undefined;
+    h.setup(testing.allocator);
     defer h.deinit();
     try h.initialize();
 
@@ -583,7 +590,8 @@ test "exit without shutdown gives exit code 1" {
 }
 
 test "requests after shutdown are refused" {
-    var h = try Harness.init(testing.allocator);
+    var h: Harness = undefined;
+    h.setup(testing.allocator);
     defer h.deinit();
     try h.initialize();
 
@@ -597,7 +605,8 @@ test "requests after shutdown are refused" {
 }
 
 test "malformed json does not take the server down" {
-    var h = try Harness.init(testing.allocator);
+    var h: Harness = undefined;
+    h.setup(testing.allocator);
     defer h.deinit();
     try h.initialize();
 
@@ -610,7 +619,8 @@ test "malformed json does not take the server down" {
 }
 
 test "wrongly typed fields are survivable" {
-    var h = try Harness.init(testing.allocator);
+    var h: Harness = undefined;
+    h.setup(testing.allocator);
     defer h.deinit();
     try h.initialize();
 
@@ -635,7 +645,8 @@ test "wrongly typed fields are survivable" {
 }
 
 test "a message with no method but an id is answered" {
-    var h = try Harness.init(testing.allocator);
+    var h: Harness = undefined;
+    h.setup(testing.allocator);
     defer h.deinit();
     try h.initialize();
 
@@ -646,7 +657,8 @@ test "a message with no method but an id is answered" {
 }
 
 test "exit is honoured before initialize" {
-    var h = try Harness.init(testing.allocator);
+    var h: Harness = undefined;
+    h.setup(testing.allocator);
     defer h.deinit();
 
     try h.server.handleMessage(
@@ -656,7 +668,8 @@ test "exit is honoured before initialize" {
 }
 
 test "client offering utf-8 gets utf-8 and byte columns" {
-    var h = try Harness.init(testing.allocator);
+    var h: Harness = undefined;
+    h.setup(testing.allocator);
     defer h.deinit();
 
     try h.server.handleMessage(
@@ -667,7 +680,8 @@ test "client offering utf-8 gets utf-8 and byte columns" {
 }
 
 test "client offering only utf-16 gets utf-16" {
-    var h = try Harness.init(testing.allocator);
+    var h: Harness = undefined;
+    h.setup(testing.allocator);
     defer h.deinit();
 
     try h.server.handleMessage(
@@ -677,14 +691,16 @@ test "client offering only utf-16 gets utf-16" {
 }
 
 test "client offering nothing falls back to utf-16" {
-    var h = try Harness.init(testing.allocator);
+    var h: Harness = undefined;
+    h.setup(testing.allocator);
     defer h.deinit();
     try h.initialize();
     try testing.expectEqual(analysis.PositionEncoding.@"utf-16", h.server.position_encoding);
 }
 
 test "an unknown encoding is skipped in favour of one we support" {
-    var h = try Harness.init(testing.allocator);
+    var h: Harness = undefined;
+    h.setup(testing.allocator);
     defer h.deinit();
 
     try h.server.handleMessage(
@@ -694,7 +710,8 @@ test "an unknown encoding is skipped in favour of one we support" {
 }
 
 test "didSave is accepted without complaint" {
-    var h = try Harness.init(testing.allocator);
+    var h: Harness = undefined;
+    h.setup(testing.allocator);
     defer h.deinit();
     try h.initialize();
 
@@ -705,7 +722,8 @@ test "didSave is accepted without complaint" {
 }
 
 test "we do not advertise capabilities we cannot serve" {
-    var h = try Harness.init(testing.allocator);
+    var h: Harness = undefined;
+    h.setup(testing.allocator);
     defer h.deinit();
     try h.initialize();
 

src/analysis.zig

@@ -1,16 +1,12 @@
 //! Turns SRF text into LSP diagnostics.
 //!
-//! The srf library reports parse errors through its `Diagnostics` interface, so
-//! we implement that interface directly instead of using the library's
-//! `BoundedDiagnostics` helper. Three reasons:
-//!
-//!   1. `BoundedDiagnostics.errors()` takes `self` by value and returns a slice
-//!      into that copy, so the slice dangles the moment the call returns.
-//!      Reading it segfaults (see the `errors() is unusable` test below).
-//!   2. It caps out at a comptime error count, and hitting the cap turns into
-//!      `error.ParseFailed`, losing the errors we already had.
-//!   3. It would mean allocating each message twice: once by the parser, once
-//!      when we copy it into a diagnostic. We take ownership instead.
+//! The srf library reports parse errors through its `Diagnostics` interface. We
+//! implement that interface directly rather than using the library's
+//! `BoundedDiagnostics` helper, because the helper caps out at a comptime error
+//! count and hitting the cap returns `error.ParseFailed`, which aborts the parse.
+//! A document being actively edited can easily hold more errors than any fixed
+//! cap, and an editor wants all of them. Implementing the interface also avoids
+//! the helper's `max_errors * 256` byte message buffer living on our stack.
 
 const std = @import("std");
 const srf = @import("srf");
@@ -93,27 +89,28 @@ const Sink = struct {
         return .{ .ptr = self, .addErrorFn = addError };
     }
 
-    /// `err.message` was allocated by the parser with `allocator` and ownership
-    /// passes to us, so on any failure path we have to free it ourselves.
-    fn addError(
-        ptr: *anyopaque,
-        allocator: std.mem.Allocator,
-        err: srf.ParseLineError,
-    ) srf.ParseError!void {
+    /// The parser hands us a *borrowed* message. Every one is a string literal
+    /// today, but the interface promises nothing past the call, so we copy it.
+    /// Note this is the opposite of the older srf API, which allocated the
+    /// message and passed ownership.
+    fn addError(ptr: *anyopaque, err: srf.ParseLineError) srf.ParseError!void {
         const self: *Sink = @ptrCast(@alignCast(ptr));
 
-        const diagnostic: Diagnostic = .{
-            .range = self.rangeFor(err),
-            .severity = severityFor(err.level),
-            .message = err.message,
+        const message = self.allocator.dupe(u8, err.message) catch {
+            self.dropped = true;
+            // Swallowed on purpose: returning an error here aborts the parse and
+            // costs us the diagnostics we already collected, which is worse than
+            // reporting a short list plus the note added by `analyze`.
+            return;
         };
 
-        self.items.append(self.allocator, diagnostic) catch {
-            allocator.free(err.message);
+        self.items.append(self.allocator, .{
+            .range = self.rangeFor(err),
+            .severity = severityFor(err.level),
+            .message = message,
+        }) catch {
+            self.allocator.free(message);
             self.dropped = true;
-            // Swallowed on purpose: returning an error here aborts the parse
-            // and costs us the diagnostics we already collected, which is worse
-            // than reporting a short list plus the note added by `analyze`.
             return;
         };
     }
@@ -437,24 +434,24 @@ test "position encoding round trips through its wire name" {
     try testing.expectEqualStrings("utf-16", PositionEncoding.@"utf-16".wireName());
 }
 
-test "srf BoundedDiagnostics.errors() is unusable, which is why Sink exists" {
-    // Regression guard for the bug that made the server segfault. `errors()`
-    // takes `self` by value, so the returned slice points into a copy that dies
-    // with the call. If this ever starts passing, upstream fixed it and this
-    // test should be deleted along with the comment at the top of the file.
+test "srf BoundedDiagnostics.errors() returns a stable view" {
+    // This used to be the bug that made the server segfault: `errors()` took
+    // `self` by value, so the returned slice pointed into a copy that died with
+    // the call. Fixed upstream in srf 7692d85 (`self: *const Self`). Kept as a
+    // guard because a regression here would be silent until it crashed.
     var bounded = srf.BoundedDiagnostics(4).empty;
-    var diagnostics = bounded.diagnostics();
-    var reader = std.Io.Reader.fixed("name::alice\n");
-    if (srf.iterator(&reader, testing.allocator, .{ .diagnostics = &diagnostics })) |it| {
-        var mut = it;
-        mut.deinit();
-    } else |_| {}
-    defer bounded.deinit(testing.allocator);
+    try bounded.addError(.{
+        .message = "error parsing numeric value",
+        .level = .err,
+        .line = 3,
+        .column = 15,
+    });
 
-    try testing.expect(bounded.error_count > 0);
-    const from_fn = bounded.errors();
-    const from_field = bounded.buffer[0..bounded.error_count];
-    try testing.expect(from_fn.ptr != from_field.ptr);
+    try testing.expectEqual(
+        @intFromPtr(&bounded.buffer),
+        @intFromPtr(bounded.errors().ptr),
+    );
+    try testing.expectEqualStrings("error parsing numeric value", bounded.errors()[0].message);
 }
 
 test "severity serializes as the protocol's integer, not its tag name" {

src/main.zig

@@ -8,51 +8,36 @@ const std = @import("std");
 const rpc = @import("rpc.zig");
 const Server = @import("Server.zig");
 
+/// `std.log.defaultLog` writes to stderr, which is exactly what we need: stdout
+/// is the protocol stream and a stray byte on it desynchronises the client. So
+/// there is no custom `logFn` here on purpose.
 pub const std_options: std.Options = .{
     .log_level = .info,
-    .logFn = logToStderr,
 };
 
-fn logToStderr(
-    comptime level: std.log.Level,
-    comptime scope: @Type(.enum_literal),
-    comptime format: []const u8,
-    args: anytype,
-) void {
-    const scope_prefix = if (scope != .default) "(" ++ @tagName(scope) ++ ") " else "";
-    const prefix = "[" ++ comptime level.asText() ++ "] " ++ scope_prefix;
-
-    var buf: [4096]u8 = undefined;
-    var writer = std.fs.File.stderr().writer(&buf);
-    const stderr = &writer.interface;
-
-    // Logging is best effort: a failed write to stderr must not break the
-    // protocol conversation on stdout.
-    nosuspend {
-        stderr.print(prefix ++ format ++ "\n", args) catch return;
-        stderr.flush() catch return;
-    }
-}
-
 const log = std.log.scoped(.srf_lsp);
 
-pub fn main() !u8 {
-    var debug_allocator: std.heap.DebugAllocator(.{}) = .init;
-    defer _ = debug_allocator.deinit();
-    const allocator = debug_allocator.allocator();
+/// Returns the process exit code: 0 for an orderly shutdown, 1 otherwise, which
+/// is what the protocol asks of an `exit` notification.
+pub fn main(init: std.process.Init) !u8 {
+    const io = init.io;
+    const gpa = init.gpa;
 
     log.info("srf-lsp starting", .{});
 
-    // Large enough that a header line always fits; message bodies are read into
+    // Large enough that a header line always fits. Message bodies are read into
     // their own exact-sized allocations, so this is not a message size limit.
     var stdin_buf: [64 * 1024]u8 = undefined;
-    var stdin_reader = std.fs.File.stdin().reader(&stdin_buf);
+    var stdin_reader = std.Io.File.stdin().reader(io, &stdin_buf);
 
-    var server = Server.init(allocator, std.fs.File.stdout());
+    var stdout_buf: [64 * 1024]u8 = undefined;
+    var stdout_writer = std.Io.File.stdout().writer(io, &stdout_buf);
+
+    var server = Server.init(gpa, &stdout_writer.interface);
     defer server.deinit();
 
     while (true) {
-        const body = rpc.readMessage(allocator, &stdin_reader.interface) catch |err| {
+        const body = rpc.readMessage(gpa, &stdin_reader.interface) catch |err| {
             // A framing failure means the stream is no longer trustworthy: we
             // cannot know where the next message starts, so resynchronising
             // would be guesswork. Report and stop.
@@ -64,7 +49,7 @@ pub fn main() !u8 {
             log.info("input closed, exiting", .{});
             return 0;
         };
-        defer allocator.free(body);
+        defer gpa.free(body);
 
         try server.handleMessage(body);
 

src/rpc.zig

@@ -77,15 +77,12 @@ pub fn readMessage(allocator: std.mem.Allocator, reader: *std.Io.Reader) ReadErr
     return body;
 }
 
-/// Writes one framed message. Unbuffered on purpose: a client blocks waiting on
-/// our replies, so every message has to hit the pipe before we return.
-pub fn writeMessage(out: std.fs.File, body: []const u8) !void {
-    var header_buf: [64]u8 = undefined;
-    // Cannot overflow in practice (a 20-digit length still fits), but propagating
-    // costs nothing and avoids relying on `unreachable`.
-    const header = try std.fmt.bufPrint(&header_buf, "Content-Length: {d}\r\n\r\n", .{body.len});
-    try out.writeAll(header);
+/// Writes one framed message and flushes it. The flush is the point: a client
+/// blocks waiting on our replies, so a message sitting in a buffer is a hang.
+pub fn writeMessage(out: *std.Io.Writer, body: []const u8) std.Io.Writer.Error!void {
+    try out.print("Content-Length: {d}\r\n\r\n", .{body.len});
     try out.writeAll(body);
+    try out.flush();
 }
 
 const testing = std.testing;
@@ -157,3 +154,36 @@ test "end of stream mid header block is truncation" {
 test "zero length body is a valid message" {
     try expectMessages("Content-Length: 0\r\n\r\n", &.{""});
 }
+
+test "writeMessage frames the body with its byte length" {
+    var out: std.Io.Writer.Allocating = .init(testing.allocator);
+    defer out.deinit();
+
+    try writeMessage(&out.writer, "{\"jsonrpc\":\"2.0\"}");
+    try testing.expectEqualStrings(
+        "Content-Length: 17\r\n\r\n{\"jsonrpc\":\"2.0\"}",
+        out.written(),
+    );
+}
+
+test "writeMessage counts bytes, not characters" {
+    var out: std.Io.Writer.Allocating = .init(testing.allocator);
+    defer out.deinit();
+
+    // "¥" is two bytes; a character count here would desynchronise the stream.
+    try writeMessage(&out.writer, "\"¥\"");
+    try testing.expectEqualStrings("Content-Length: 4\r\n\r\n\"¥\"", out.written());
+}
+
+test "a framed message round trips through readMessage" {
+    var out: std.Io.Writer.Allocating = .init(testing.allocator);
+    defer out.deinit();
+
+    const body = "{\"method\":\"initialize\",\"id\":1}";
+    try writeMessage(&out.writer, body);
+
+    var reader = std.Io.Reader.fixed(out.written());
+    const got = (try readMessage(testing.allocator, &reader)) orelse return error.UnexpectedNull;
+    defer testing.allocator.free(got);
+    try testing.expectEqualStrings(body, got);
+}