diff --git a/.gitignore b/.gitignore index 3389c86..abaacc3 100644 --- a/.gitignore +++ b/.gitignore @@ -1,2 +1,4 @@ .zig-cache/ zig-out/ +# Zig 0.16 materialises fetched dependencies here. +zig-pkg/ diff --git a/.mise.toml b/.mise.toml index 9927c37..affb4cf 100644 --- a/.mise.toml +++ b/.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" diff --git a/build.zig.zon b/build.zig.zon index e6b61dc..189d129 100644 --- a/build.zig.zon +++ b/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 = .{ diff --git a/src/Server.zig b/src/Server.zig index 25b2569..3f83158 100644 --- a/src/Server.zig +++ b/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(); diff --git a/src/analysis.zig b/src/analysis.zig index d426f00..bacbdcc 100644 --- a/src/analysis.zig +++ b/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" { diff --git a/src/main.zig b/src/main.zig index 6f4c9b4..81b949a 100644 --- a/src/main.zig +++ b/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); diff --git a/src/rpc.zig b/src/rpc.zig index d373d9f..730e5e5 100644 --- a/src/rpc.zig +++ b/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); +}