From 16bc78773801a77e36034c6cdf89d82767693b17 Mon Sep 17 00:00:00 2001 From: Emil Lerch Date: Tue, 1 Sep 2026 16:22:53 -0700 Subject: [PATCH] nix flake + ci --- .forgejo/workflows/zig-build.yaml | 34 ++++ README.md | 40 +++- build.zig | 9 + build.zig.zon | 2 +- flake.nix | 46 +++++ nix/package.nix | 55 ++++++ src/Server.zig | 4 +- src/analysis.zig | 303 ++++++++++++++++++++++++++++++ src/document.zig | 105 +++++++++++ src/hover.zig | 128 ++++++++++++- 10 files changed, 721 insertions(+), 5 deletions(-) create mode 100644 .forgejo/workflows/zig-build.yaml create mode 100644 flake.nix create mode 100644 nix/package.nix diff --git a/.forgejo/workflows/zig-build.yaml b/.forgejo/workflows/zig-build.yaml new file mode 100644 index 0000000..cf37353 --- /dev/null +++ b/.forgejo/workflows/zig-build.yaml @@ -0,0 +1,34 @@ +name: Generic zig build +on: + workflow_dispatch: + push: + branches: + - '*' +jobs: + build: + runs-on: ubuntu-latest + steps: + - name: Check out repository code + uses: actions/checkout@v4 + - name: Setup Zig + # No version given on purpose: setup-zig resolves it from + # minimum_zig_version in build.zig.zon, so the toolchain cannot drift + # from what the package declares. + uses: https://codeberg.org/mlugg/setup-zig@v2.2.1 + - name: Check formatting + # Cheap and needs nothing fetched, so it runs before the build. There is + # a pre-commit hook for this too; this catches a commit that skipped it. + run: zig fmt --check . + - name: Build project + run: zig build --summary all + - name: Run tests + run: zig build test --summary all + - name: Notify + uses: https://git.lerch.org/lobo/action-notify-ntfy@v2 + if: always() && env.GITEA_ACTIONS == 'true' + with: + host: ${{ secrets.NTFY_HOST }} + topic: ${{ secrets.NTFY_TOPIC }} + status: ${{ job.status }} + user: ${{ secrets.NTFY_USER }} + password: ${{ secrets.NTFY_PASSWORD }} diff --git a/README.md b/README.md index 87be3ee..1d830e7 100644 --- a/README.md +++ b/README.md @@ -10,6 +10,14 @@ Provides real-time parse error diagnostics using the SRF library's parser direct directly: missing or duplicate `#!srfv1`, bad type hints, unparseable values, data after `#!eof`, and so on. - All errors from a parse are reported, not just the first. +- A type-consistency warning when one key holds different value kinds in + different places in the same document, for example `close_price:num:100.00` in + most records and `close_price::200.00` in one. `k::32` is valid SRF and often + deliberate, so the check is not "this value looks numeric" but "this key is + typed differently elsewhere in this file", which the document itself evidences. + Kinds are compared by the `srf.Value` variant a hint produces, so an empty + hint, `string` and a byte count are all one kind. A uniformly mistyped document + has no minority and so gets no warning. - Length-prefix checking for the cases srf does not cover. A numeric type hint declares an exact byte count. srf reports a wrong one in compact format, but in long format it keeps the declared bytes and discards the rest of the line @@ -23,6 +31,8 @@ Provides real-time parse error diagnostics using the SRF library's parser direct counts for strings, and for `binary` values the decoded content when it is text (`decoding to 5 bytes: "hello"`) or a note when it is not. - Type hints are explained, including a numeric hint's length-prefix meaning. + - A string value that also parses as a number gets a one-line note that a + consumer coercing it to a numeric field needs `strings_to_numbers`. - Position encoding is negotiated during `initialize` (`utf-8`, `utf-16` or `utf-32`), so columns line up even on lines containing multi-byte characters. - Full-document sync, which matches SRF's single-pass parser. @@ -57,7 +67,35 @@ The binary is at `zig-out/bin/srf-lsp`. ## Install -The Neovim config below expects `srf-lsp` on `PATH`: +The Neovim config below expects `srf-lsp` on `PATH`. + +### With nix + +This repository is a flake. To add it to a `buildEnv`-style home profile: + +```nix +inputs = { + srf-lsp.url = "git+https://git.lerch.org/lobo/srf-lsp.git"; + # Optional: reuse your own nixpkgs instead of instantiating another one. + srf-lsp.inputs.nixpkgs.follows = "nixpkgs"; +}; +``` + +then add `srf-lsp.packages.${system}.default` to your package list. Or run it +directly: + +```sh +nix run git+https://git.lerch.org/lobo/srf-lsp.git +``` + +`nix flake update srf-lsp` picks up a new version. The build runs the full test +suite in the sandbox, so a release that fails its tests will not install. + +Bumping the `srf` dependency changes `build.zig.zon`, which invalidates the +`zigDeps` hash in `nix/package.nix`. To refresh it: set the hash to +`lib.fakeHash`, build, and paste the hash nix reports. + +### Without nix ```sh zig build && install -m755 zig-out/bin/srf-lsp ~/.local/bin/srf-lsp diff --git a/build.zig b/build.zig index daedd8b..8a71888 100644 --- a/build.zig +++ b/build.zig @@ -1,5 +1,10 @@ const std = @import("std"); +/// Single source of truth for the version. `serverInfo.version` in the LSP +/// handshake reads it through the generated `build_options` module below, so the +/// number cannot drift from the package metadata. +const build_zon = @import("build.zig.zon"); + pub fn build(b: *std.Build) void { const target = b.standardTargetOptions(.{}); const optimize = b.standardOptimizeOption(.{}); @@ -8,6 +13,9 @@ pub fn build(b: *std.Build) void { .target = target, }); + const options = b.addOptions(); + options.addOption([]const u8, "version", build_zon.version); + const exe = b.addExecutable(.{ .name = "srf-lsp", .root_module = b.createModule(.{ @@ -16,6 +24,7 @@ pub fn build(b: *std.Build) void { .optimize = optimize, .imports = &.{ .{ .name = "srf", .module = srf_dep.module("srf") }, + .{ .name = "build_options", .module = options.createModule() }, }, }), }); diff --git a/build.zig.zon b/build.zig.zon index 3c46ffe..f905035 100644 --- a/build.zig.zon +++ b/build.zig.zon @@ -1,6 +1,6 @@ .{ .name = .srf_lsp, - .version = "0.0.0", + .version = "0.0.1", .fingerprint = 0x96c04d564e5a0f32, .minimum_zig_version = "0.16.0", .dependencies = .{ diff --git a/flake.nix b/flake.nix new file mode 100644 index 0000000..ad283c3 --- /dev/null +++ b/flake.nix @@ -0,0 +1,46 @@ +{ + description = "Language Server Protocol implementation for SRF (Simple Record Format)"; + + inputs.nixpkgs.url = "nixpkgs/master"; + + outputs = { self, nixpkgs }: + let + supportedSystems = [ "x86_64-linux" "aarch64-linux" "x86_64-darwin" "aarch64-darwin" ]; + + # Helper to generate an attrset '{ x86_64-linux = f "x86_64-linux"; ... }'. + forAllSystems = nixpkgs.lib.genAttrs supportedSystems; + in + { + packages = forAllSystems (system: + let + pkgs = nixpkgs.legacyPackages.${system}; + srf-lsp = pkgs.callPackage ./nix/package.nix { + # Pinned deliberately rather than tracking `pkgs.zig`: build.zig.zon + # declares 0.16.0 as the minimum, and 0.16 was a large standard + # library refactor, so a newer default would not build unchanged. + zig = pkgs.zig_0_16; + # The git-tracked tree, which keeps .zig-cache, zig-out and zig-pkg + # out of the derivation. + src = self; + }; + in + { + inherit srf-lsp; + default = srf-lsp; + }); + + devShells = forAllSystems (system: + let pkgs = nixpkgs.legacyPackages.${system}; + in + { + # `nix develop` for anyone without mise. The versions here are the same + # ones .mise.toml pins. + default = pkgs.mkShell { + packages = [ + pkgs.zig_0_16 + pkgs.zls + ]; + }; + }); + }; +} diff --git a/nix/package.nix b/nix/package.nix new file mode 100644 index 0000000..bc120cf --- /dev/null +++ b/nix/package.nix @@ -0,0 +1,55 @@ +{ lib, stdenv, zig, src }: + +stdenv.mkDerivation (finalAttrs: { + pname = "srf-lsp"; + # Kept in step with build.zig.zon, which is also what the server reports in its + # `initialize` response (see build.zig). + version = "0.0.1"; + + inherit src; + + __structuredAttrs = true; + strictDeps = true; + + # Zig resolves build.zig.zon dependencies over the network, which the build + # sandbox forbids. `fetchDeps` runs `zig build --fetch` inside a fixed-output + # derivation, so the fetch happens once against a known hash and the real build + # stays offline. + # + # This hash changes whenever build.zig.zon changes, which in practice means + # whenever the srf dependency is bumped. To update it: set `lib.fakeHash`, run + # the build, and paste the hash nix reports. + zigDeps = zig.fetchDeps { + inherit (finalAttrs) pname version src; + fetchAll = true; + hash = "sha256-hS69otO9YY1acs+Vn8IdFj+mV8QjoeDVtxtsCrXD3RY="; + }; + + postConfigure = '' + # A writable copy rather than a symlink into the store: Zig writes cache + # metadata alongside the fetched packages while it verifies them. + cp -rLT ${finalAttrs.zigDeps} "$ZIG_GLOBAL_CACHE_DIR/p" + chmod -R u+w "$ZIG_GLOBAL_CACHE_DIR/p" + ''; + + nativeBuildInputs = [ zig ]; + + # The zig setup hook's check phase runs `zig build test`, which is the whole + # suite. Nothing in it touches the network or the filesystem outside the build + # directory, so it runs happily in the sandbox: every rebuild is a test run. + doCheck = true; + + meta = { + description = "Language Server Protocol implementation for SRF (Simple Record Format)"; + longDescription = '' + Diagnostics and hover for SRF documents, using the srf library's own parser + so the editor cannot disagree with the reference implementation. Reports + parse errors, length-prefix declarations that do not match the bytes + present, and keys whose type varies across records in one document. + ''; + homepage = "https://git.lerch.org/lobo/srf-lsp"; + license = lib.licenses.mit; + mainProgram = "srf-lsp"; + platforms = lib.platforms.unix; + }; +}) diff --git a/src/Server.zig b/src/Server.zig index 13b9b4a..1ae2301 100644 --- a/src/Server.zig +++ b/src/Server.zig @@ -12,6 +12,7 @@ //! take the session down. const std = @import("std"); +const build_options = @import("build_options"); const analysis = @import("analysis.zig"); const hover = @import("hover.zig"); const positions = @import("positions.zig"); @@ -263,7 +264,8 @@ fn handleInitialize(self: *Server, root: std.json.ObjectMap, id: ?std.json.Value // and a client that took us up on pull would just collect // MethodNotFound. }, - .serverInfo = .{ .name = "srf-lsp", .version = "0.0.1" }, + // Version comes from build.zig.zon via build.zig, so it cannot drift. + .serverInfo = .{ .name = "srf-lsp", .version = build_options.version }, }); } diff --git a/src/analysis.zig b/src/analysis.zig index 706c528..a0f1116 100644 --- a/src/analysis.zig +++ b/src/analysis.zig @@ -166,6 +166,7 @@ pub fn analyze( // it detects a length-prefixed value that misses its delimiter, logs it, and // returns a bare `ParseFailed` with no diagnostic at all. try appendLengthProblems(allocator, &sink, text, encoding); + try appendTypeInconsistencies(allocator, &sink, text, encoding); if (parse_error) |err| { // Only if nothing at all was found. srf can fail without saying why: @@ -269,6 +270,123 @@ fn appendLengthProblems( } } +/// Warns when one key holds different value kinds in different places in the same +/// document. +/// +/// This is the only diagnostic here that is not about validity. `k::32` is +/// perfectly legal SRF, and whether it is a mistake depends entirely on the +/// consumer: srf's own tests contain both `close_price::200.00` ("the shape that +/// took down a downstream consumer") and `foo::1` consumed deliberately as +/// `[]const u8`. The text cannot tell them apart, because what separates them is +/// not in the file. +/// +/// What *is* in the file is how the author typed the same key elsewhere. So the +/// rule is not "this value looks numeric" (unanswerable, and noisy) but "this key +/// is `num` in thirty other places and a string here" (self-evidencing). A +/// document with one odd record out of forty is the case that sits in a cache +/// until it breaks something; a uniformly mistyped document has no minority and +/// gets no warning, which is the accepted limitation. +fn appendTypeInconsistencies( + allocator: std.mem.Allocator, + sink: *Sink, + text: []const u8, + encoding: PositionEncoding, +) error{OutOfMemory}!void { + const Stats = struct { + /// Occurrences per kind, indexed by `@intFromEnum`. + counts: [std.enums.values(document.ValueKind).len]u32 = @splat(0), + /// Breaks ties: with no majority, the first spelling in the document is + /// taken as the intended one. + first: document.ValueKind, + + fn total(self: @This()) u32 { + var sum: u32 = 0; + for (self.counts) |c| sum += c; + return sum; + } + + fn majority(self: @This()) document.ValueKind { + var best = self.first; + var best_count = self.counts[@intFromEnum(self.first)]; + for (std.enums.values(document.ValueKind)) |kind| { + const count = self.counts[@intFromEnum(kind)]; + if (count > best_count) { + best = kind; + best_count = count; + } + } + return best; + } + + fn distinctKinds(self: @This()) u32 { + var seen: u32 = 0; + for (self.counts) |c| if (c > 0) { + seen += 1; + }; + return seen; + } + }; + + // Keys are slices into `text`, which outlives this function, so there is + // nothing to duplicate. Byte-exact comparison on purpose: srf does not trim + // keys either, so `k ` and `k` really are different keys to it. + var stats: std.StringHashMapUnmanaged(Stats) = .empty; + defer stats.deinit(allocator); + + var counting = document.items(text); + while (counting.next()) |item| { + const located = switch (item) { + .field => |f| f, + else => continue, + }; + const kind = located.field.valueKind() orelse continue; + + const entry = try stats.getOrPut(allocator, located.field.key); + if (!entry.found_existing) entry.value_ptr.* = .{ .first = kind }; + entry.value_ptr.counts[@intFromEnum(kind)] += 1; + } + + var emitting = document.items(text); + while (emitting.next()) |item| { + const located = switch (item) { + .field => |f| f, + else => continue, + }; + const kind = located.field.valueKind() orelse continue; + + const entry = stats.get(located.field.key) orelse continue; + // A key seen once proves nothing, and a consistent key is fine. + if (entry.total() < 2 or entry.distinctKinds() < 2) continue; + + const expected = entry.majority(); + if (kind == expected) continue; + + const others = entry.counts[@intFromEnum(expected)]; + const message = try std.fmt.allocPrint( + allocator, + "`{s}` is {s} in {d} other field(s) in this file, but {s} here", + .{ located.field.key, expected.describe(), others, kind.describe() }, + ); + errdefer allocator.free(message); + + sink.items.append(allocator, .{ + // Point at the hint, which is the thing that disagrees. An untyped + // field has no hint to point at, so fall back to the key. + .range = if (located.hint_span) |hs| + positions.rangeFromSpan(text, hs.start, hs.end, encoding) + else + positions.rangeFromSpan(text, located.key_span.start, located.key_span.end, encoding), + // Not a validity problem: the document parses. It is a warning that + // a consumer coercing this key will fail on this record. + .severity = .warning, + .message = message, + }) catch { + allocator.free(message); + return error.OutOfMemory; + }; + } +} + /// Records a parse failure that the parser gave us no location for. Keeps the /// underlying error name in the message: it is the only information we have /// about why the parse stopped, so hiding it would leave the user guessing. @@ -593,3 +711,188 @@ test "the unlocated fallback still fires when nothing else explains the failure" try testing.expectEqual(@as(usize, 1), diags.len); try testing.expect(std.mem.indexOf(u8, diags[0].message, "empty") != null); } + +/// Only the type-consistency warnings, without srf's own diagnostics. +fn inconsistenciesForTest(text: []const u8) ![]Diagnostic { + var sink: Sink = .{ + .allocator = testing.allocator, + .text = text, + .encoding = .@"utf-8", + .items = .empty, + }; + errdefer { + for (sink.items.items) |d| d.deinit(testing.allocator); + sink.items.deinit(testing.allocator); + } + try appendTypeInconsistencies(testing.allocator, &sink, text, .@"utf-8"); + return sink.items.toOwnedSlice(testing.allocator); +} + +test "a key typed num everywhere but one place warns on the odd one" { + const text = + "#!srfv1\n#!long\n" ++ + "close_price:num:100.00\n\n" ++ + "close_price:num:150.00\n\n" ++ + "close_price:num:175.00\n\n" ++ + "close_price::200.00\n"; + const diags = try inconsistenciesForTest(text); + defer freeDiagnostics(testing.allocator, diags); + + try testing.expectEqual(@as(usize, 1), diags.len); + try testing.expectEqual(Severity.warning, diags[0].severity); + try testing.expectEqualStrings( + "`close_price` is `num` in 3 other field(s) in this file, but a string here", + diags[0].message, + ); + // The untyped field is on the last line, and with no hint to point at the + // range covers the key. + try testing.expectEqual(@as(u32, 8), diags[0].range.start.line); + try testing.expectEqual(@as(u32, 0), diags[0].range.start.character); + try testing.expectEqual(@as(u32, 11), diags[0].range.end.character); +} + +test "the warning points at the hint when there is one" { + const text = "#!srfv1\n#!long\nk::abc\n\nk::def\n\nk:num:5\n"; + const diags = try inconsistenciesForTest(text); + defer freeDiagnostics(testing.allocator, diags); + try testing.expectEqual(@as(usize, 1), diags.len); + // "k:" is two bytes, so the `num` hint runs from character 2 to 5. + try testing.expectEqual(@as(u32, 6), diags[0].range.start.line); + try testing.expectEqual(@as(u32, 2), diags[0].range.start.character); + try testing.expectEqual(@as(u32, 5), diags[0].range.end.character); +} + +test "srf's own foo::1 fixture stays silent" { + // From srf.zig's "iterator with blank" test, where `foo` is deliberately a + // []const u8 holding digits. Consistent, so there is no signal. + const text = + "#!srfv1\n" ++ + "foo::1,bar:num:42\n\n" ++ + "foo::2,bar:num:24\n"; + const diags = try inconsistenciesForTest(text); + defer freeDiagnostics(testing.allocator, diags); + try testing.expectEqual(@as(usize, 0), diags.len); +} + +test "the README long-format example stays silent" { + // The same key appears as `:7:` and `::` here. Both are strings, so comparing + // hint text rather than value kind would flag the reference documentation. + const text = + \\#!srfv1 + \\#!long + \\key::string value, with any data except a \n + \\this is a number:num: 5 + \\null value:null: + \\array::array's don't exist. Use json or toml or something + \\data with newlines must have a length:7:foo + \\bar + \\boolean value:bool:false + \\ + \\key::this is the second record + \\this is a number:num:42 + \\null value:null: + \\array::array's still don't exist + \\data with newlines must have a length::single line + \\ + ; + const diags = try inconsistenciesForTest(text); + defer freeDiagnostics(testing.allocator, diags); + if (diags.len != 0) { + for (diags) |d| std.debug.print("\nunexpected: {s}\n", .{d.message}); + } + try testing.expectEqual(@as(usize, 0), diags.len); +} + +test "a key seen once proves nothing" { + const diags = try inconsistenciesForTest("#!srfv1\n#!long\nk::32\nother:num:5\n"); + defer freeDiagnostics(testing.allocator, diags); + try testing.expectEqual(@as(usize, 0), diags.len); +} + +test "a null value mixes with any type without warning" { + const text = "#!srfv1\n#!long\nk:num:5\n\nk:null:\n\nk:num:7\n"; + const diags = try inconsistenciesForTest(text); + defer freeDiagnostics(testing.allocator, diags); + try testing.expectEqual(@as(usize, 0), diags.len); +} + +test "an empty value mixes with any type without warning" { + const text = "#!srfv1\n#!long\nk:num:5\n\nk:num:\n\nk:num:7\n"; + const diags = try inconsistenciesForTest(text); + defer freeDiagnostics(testing.allocator, diags); + try testing.expectEqual(@as(usize, 0), diags.len); +} + +test "a one against one tie flags the later occurrence" { + // No majority exists, but the inconsistency is still the signal. The first + // spelling wins so the result is deterministic. + const text = "#!srfv1\n#!long\nk:num:5\n\nk::abc\n"; + const diags = try inconsistenciesForTest(text); + defer freeDiagnostics(testing.allocator, diags); + try testing.expectEqual(@as(usize, 1), diags.len); + try testing.expectEqual(@as(u32, 4), diags[0].range.start.line); + try testing.expect(std.mem.indexOf(u8, diags[0].message, "is `num` in 1 other") != null); +} + +test "the three string spellings are one kind, not three" { + const text = "#!srfv1\n#!long\nk::abc\n\nk:string:def\n\nk:3:ghi\n"; + const diags = try inconsistenciesForTest(text); + defer freeDiagnostics(testing.allocator, diags); + try testing.expectEqual(@as(usize, 0), diags.len); +} + +test "a repeated key within one compact record is checked too" { + // `to()` takes only the first occurrence, so a disagreeing duplicate matters. + const diags = try inconsistenciesForTest("#!srfv1\nk:num:1,k::2\n"); + defer freeDiagnostics(testing.allocator, diags); + try testing.expectEqual(@as(usize, 1), diags.len); +} + +test "bool and binary inconsistencies are reported too" { + const bools = try inconsistenciesForTest("#!srfv1\n#!long\nk:bool:true\n\nk:bool:false\n\nk::true\n"); + defer freeDiagnostics(testing.allocator, bools); + try testing.expectEqual(@as(usize, 1), bools.len); + try testing.expect(std.mem.indexOf(u8, bools[0].message, "`bool`") != null); + + const bins = try inconsistenciesForTest("#!srfv1\n#!long\nk:binary:aGk=\n\nk:binary:aGk=\n\nk::aGk=\n"); + defer freeDiagnostics(testing.allocator, bins); + try testing.expectEqual(@as(usize, 1), bins.len); + try testing.expect(std.mem.indexOf(u8, bins[0].message, "`binary`") != null); +} + +test "keys are compared byte exactly, matching srf" { + // srf does not trim keys, so `k ` and `k` are different keys to it. + const diags = try inconsistenciesForTest("#!srfv1\n#!long\nk:num:5\n\nk ::abc\n"); + defer freeDiagnostics(testing.allocator, diags); + try testing.expectEqual(@as(usize, 0), diags.len); +} + +test "an unrecognised hint is left to srf, not counted as a type" { + const diags = try inconsistenciesForTest("#!srfv1\n#!long\nk:num:5\n\nk:nmu:6\n"); + defer freeDiagnostics(testing.allocator, diags); + try testing.expectEqual(@as(usize, 0), diags.len); +} + +test "type warnings coexist with length warnings without duplication" { + const text = "#!srfv1\n#!long\nk:num:5\n\nk::abc\n\nbio:3:hello\n"; + const diags = try analyzeForTest(text); + defer freeDiagnostics(testing.allocator, diags); + var type_warnings: usize = 0; + var length_warnings: usize = 0; + for (diags) |d| { + if (std.mem.indexOf(u8, d.message, "other field(s) in this file") != null) type_warnings += 1; + if (std.mem.indexOf(u8, d.message, "length prefix") != null) length_warnings += 1; + } + try testing.expectEqual(@as(usize, 1), type_warnings); + try testing.expectEqual(@as(usize, 1), length_warnings); +} + +test "a consistent document produces no warnings at all" { + const text = + "#!srfv1\n" ++ + "id:num:1,name::alice,active:bool:true\n" ++ + "id:num:2,name::bob,active:bool:false\n"; + const diags = try analyzeForTest(text); + defer freeDiagnostics(testing.allocator, diags); + try testing.expectEqual(@as(usize, 0), diags.len); +} diff --git a/src/document.zig b/src/document.zig index dd9bb78..35b335f 100644 --- a/src/document.zig +++ b/src/document.zig @@ -46,6 +46,54 @@ pub const Field = struct { for (hint) |c| if (c < '0' or c > '9') return null; return std.fmt.parseInt(usize, hint, 10) catch null; } + + /// Which `srf.Value` variant this field's hint produces. + /// + /// Coercion cares about the variant, not the hint text: an empty hint, + /// `string`, and a byte count all yield `.string` and coerce identically, so + /// treating them as different types would flag correct documents. srf's own + /// README uses `key:7:...` and `key::...` for the same key. + /// + /// Null when the field carries no type evidence: + /// + /// * a `null` hint, which is an absent value and mixes legitimately with + /// any other type (`coerce` handles it through `.optional`); + /// * an empty value, which says nothing about the intended type; + /// * an unrecognised hint, which srf already reports as "unrecognized + /// metadata for key", so treating it as a type would double-report. + /// + /// The order mirrors `Value.parse`'s dispatch in srf.zig. + pub fn valueKind(self: Field) ?ValueKind { + if (self.value.len == 0) return null; + + const hint = self.hint orelse return .string; + if (hint.len == 0 or std.mem.eql(u8, hint, "string")) return .string; + if (std.mem.eql(u8, hint, "binary")) return .bytes; + if (std.mem.eql(u8, hint, "num")) return .number; + if (std.mem.eql(u8, hint, "bool")) return .boolean; + if (std.mem.eql(u8, hint, "null")) return null; + // A byte count is a length-prefixed string; anything else is a bad hint. + if (self.declaredLength() != null) return .string; + return null; + } +}; + +/// The kinds of value an SRF field can hold, as `srf.Value` sees them. +pub const ValueKind = enum { + string, + number, + boolean, + bytes, + + /// How to name this kind in a message aimed at someone editing the file. + pub fn describe(self: ValueKind) []const u8 { + return switch (self) { + .string => "a string", + .number => "`num`", + .boolean => "`bool`", + .bytes => "`binary`", + }; + } }; pub const Directive = struct { @@ -765,3 +813,60 @@ test "the surplus of a multi-line overrun is measured on its own line" { text[problem.overrun.surplus_start..][0..problem.overrun.surplus], ); } + +/// The value kind of the field containing the midpoint of `needle`. +fn kindAt(text: []const u8, needle: []const u8) ??ValueKind { + const offset = midpointOf(text, needle); + var it = items(text); + while (it.next()) |item| switch (item) { + .field => |located| { + if (located.key_span.contains(offset) or located.value_span.contains(offset) or + (located.hint_span != null and located.hint_span.?.contains(offset))) + { + return located.field.valueKind(); + } + }, + else => {}, + }; + return null; +} + +test "an untyped value is a string" { + try testing.expectEqual(ValueKind.string, (kindAt("#!srfv1\nk::32\n", "k::").?).?); +} + +test "an explicit string hint is a string" { + try testing.expectEqual(ValueKind.string, (kindAt("#!srfv1\nk:string:32\n", "string").?).?); +} + +test "a length prefix is a string, the same as the other two forms" { + // This is what keeps srf's own README quiet: it uses `key:7:...` and + // `key::...` for the same key in one document. + try testing.expectEqual(ValueKind.string, (kindAt("#!srfv1\n#!long\nk:2:32\n", "k:2:").?).?); +} + +test "num, bool and binary map to their own kinds" { + try testing.expectEqual(ValueKind.number, (kindAt("#!srfv1\nk:num:32\n", "num").?).?); + try testing.expectEqual(ValueKind.boolean, (kindAt("#!srfv1\nk:bool:true\n", "bool").?).?); + try testing.expectEqual(ValueKind.bytes, (kindAt("#!srfv1\nk:binary:aGk=\n", "binary").?).?); +} + +test "a null hint carries no type evidence" { + try testing.expectEqual(@as(?ValueKind, null), kindAt("#!srfv1\nk:null:x\n", "null").?); +} + +test "an empty value carries no type evidence" { + try testing.expectEqual(@as(?ValueKind, null), kindAt("#!srfv1\n#!long\nk:num:\n", "k:num:").?); + try testing.expectEqual(@as(?ValueKind, null), kindAt("#!srfv1\n#!long\nk::\n", "k::").?); +} + +test "an unrecognised hint carries no type evidence, since srf reports it" { + try testing.expectEqual(@as(?ValueKind, null), kindAt("#!srfv1\nk:nmu:32\n", "nmu").?); +} + +test "value kinds describe themselves for a message" { + try testing.expectEqualStrings("a string", ValueKind.string.describe()); + try testing.expectEqualStrings("`num`", ValueKind.number.describe()); + try testing.expectEqualStrings("`bool`", ValueKind.boolean.describe()); + try testing.expectEqualStrings("`binary`", ValueKind.bytes.describe()); +} diff --git a/src/hover.zig b/src/hover.zig index c4c4f2b..9488c79 100644 --- a/src/hover.zig +++ b/src/hover.zig @@ -242,9 +242,60 @@ fn describeValue( } try describeParsed(allocator, w, f); + try writeCoercionCaveat(allocator, w, f); try w.print("```\n{s}\n```", .{f.value}); } +/// Notes that a string value which happens to parse as a number will be refused by +/// a consumer coercing it to a numeric field. +/// +/// `k::32` is valid SRF and often deliberate, so this cannot be a diagnostic +/// without crying wolf: srf's own tests hold both `close_price::200.00` (the shape +/// that broke a downstream consumer) and `foo::1` read on purpose as +/// `[]const u8`. Hover is the right home for it. Nobody is interrupted, and the +/// note only appears when someone points at the field. +fn writeCoercionCaveat( + allocator: std.mem.Allocator, + w: *std.Io.Writer, + f: document.Field, +) std.Io.Writer.Error!void { + if (f.valueKind() != .string) return; + if (!parsesAsNumber(allocator, f)) return; + + if (f.hint == null) { + try w.writeAll( + "Untyped, so this is a string: a consumer coercing it to a number needs `strings_to_numbers`.\n\n", + ); + } else { + try w.writeAll( + "Declared as a string: a consumer coercing it to a number needs `strings_to_numbers`.\n\n", + ); + } +} + +/// Whether srf would accept this value under a `num` hint. +/// +/// Answered by re-parsing with the hint swapped rather than calling +/// `std.fmt.parseFloat` here. srf's number rules depend on +/// `strict_number_parsing`, and its lenient `parseFloat` is private, so deferring +/// to the parser is the only way this cannot drift from it. +fn parsesAsNumber(allocator: std.mem.Allocator, f: document.Field) bool { + var probe = f; + probe.hint = "num"; + + const synthetic = buildSyntheticDocument(allocator, probe) catch return false; + defer allocator.free(synthetic); + + var reader = std.Io.Reader.fixed(synthetic); + var it = srf.iterator(&reader, allocator, .{}) catch return false; + defer it.deinit(); + + const fields = (it.next() catch return false) orelse return false; + const field = (fields.next() catch return false) orelse return false; + const value = field.value orelse return false; + return value == .number; +} + /// A wrong length prefix is the easiest way to corrupt an SRF file by hand, and /// the byte count is invisible in the text, so say exactly what is wrong. fn writeLengthProblem( @@ -391,9 +442,12 @@ const testing = std.testing; /// Hovers at the midpoint of `needle` and returns the markdown. fn hoverOn(text: []const u8, needle: []const u8) ![]const u8 { - const at = std.mem.indexOf(u8, text, needle).?; + const at = std.mem.indexOf(u8, text, needle) orelse return error.NeedleNotInText; const position = positions.positionFromByte(text, at + needle.len / 2, .@"utf-8"); - const hover = (try hoverAt(testing.allocator, text, position, .@"utf-8")).?; + // An error beats a panic here: hovering punctuation legitimately returns + // null, and a test that lands there should say so rather than abort. + const hover = (try hoverAt(testing.allocator, text, position, .@"utf-8")) orelse + return error.NothingToHoverThere; return hover.contents.value; } @@ -680,3 +734,73 @@ test "a correct length prefix with commas in compact format reports no mismatch" try testing.expect(std.mem.indexOf(u8, markdown, "Length mismatch") == null); try testing.expect(std.mem.indexOf(u8, markdown, "a,b,c") != null); } + +test "an untyped numeric-looking value carries the coercion caveat" { + try expectHoverContains("#!srfv1\nk::32\n", "k::", "Untyped, so this is a string"); + try expectHoverContains("#!srfv1\nk::32\n", "k::", "strings_to_numbers"); +} + +test "the caveat fires on the shape that broke a real consumer" { + try expectHoverContains("#!srfv1\nclose_price::200.00\n", "200.00", "strings_to_numbers"); +} + +test "a non-numeric string carries no caveat" { + const markdown = try hoverOn("#!srfv1\nname::alice\n", "alice"); + defer testing.allocator.free(markdown); + try testing.expect(std.mem.indexOf(u8, markdown, "strings_to_numbers") == null); +} + +test "an explicitly typed number carries no caveat" { + const markdown = try hoverOn("#!srfv1\nage:num:30\n", "30"); + defer testing.allocator.free(markdown); + try testing.expect(std.mem.indexOf(u8, markdown, "strings_to_numbers") == null); +} + +test "an explicit string hint gets the declared wording" { + try expectHoverContains("#!srfv1\nk:string:32\n", "k:string:", "Declared as a string"); +} + +test "a length-prefixed numeric value gets the caveat too" { + // Same coercion problem: a length prefix is still a string. + try expectHoverContains("#!srfv1\n#!long\nk:2:32\n", "k:2:", "Declared as a string"); +} + +test "a bool value carries no caveat" { + const markdown = try hoverOn("#!srfv1\nk:bool:true\n", "true"); + defer testing.allocator.free(markdown); + try testing.expect(std.mem.indexOf(u8, markdown, "strings_to_numbers") == null); +} + +test "the caveat covers negative and floating point values" { + try expectHoverContains("#!srfv1\nk::-5\n", "k::", "strings_to_numbers"); + try expectHoverContains("#!srfv1\nk::1.5e3\n", "k::", "strings_to_numbers"); +} + +test "a multi-line length-prefixed value carries no caveat" { + const markdown = try hoverOn("#!srfv1\n#!long\nbio:7:foo\nbar\n", "foo"); + defer testing.allocator.free(markdown); + try testing.expect(std.mem.indexOf(u8, markdown, "strings_to_numbers") == null); +} + +test "the caveat appears on the key as well as the value" { + // Hovering either half of the field should tell the same story. + const text = "#!srfv1\nprice::32\n"; + try expectHoverContains(text, "price", "strings_to_numbers"); + try expectHoverContains(text, "32", "strings_to_numbers"); +} + +test "hovering the punctuation between key and value gives nothing" { + // The colons are not addressable, which is why the helper above reports it + // rather than dereferencing a null. + try testing.expectError(error.NothingToHoverThere, hoverOn("#!srfv1\nk::32\n", "::")); +} + +test "the caveat is one line, not a paragraph" { + const markdown = try hoverOn("#!srfv1\nk::32\n", "k::"); + defer testing.allocator.free(markdown); + const at = std.mem.indexOf(u8, markdown, "Untyped, so this").?; + const rest = markdown[at..]; + const line_end = std.mem.indexOfScalar(u8, rest, '\n').?; + // A single sentence on a single line. + try testing.expect(line_end < 120); +}