better algorithm for determining the most likely "what the user meant" for length-prefixed strings in compact form

.mise.toml

@@ -2,4 +2,4 @@
 prek = "0.3.1"
 "ubi:DonIsaac/zlint" = "0.7.9"
 zig = "0.16.0"
-zls = "0.15.1"
+zls = "0.16.0"

src/srf.zig

@@ -29,6 +29,12 @@ pub const ParseLineError = struct {
 };
 pub const Diagnostics = struct {
     ptr: *anyopaque,
+
+    /// adds an error to the diagnostics implementation. Note that this carries
+    /// no allocator and the ParseLineError message field may be stack allocated.
+    /// It is up to the implementation to manage diagnostics message lifetime
+    /// BoundedDiagnostics struct keeps an internal 256 byte/message buffer
+    /// for diagnostics messages
     addErrorFn: *const fn (*anyopaque, ParseLineError) ParseError!void,
     has_errors: bool = false,
 
@@ -37,6 +43,9 @@ pub const Diagnostics = struct {
         self.has_errors = true;
     }
 };
+
+/// Implements the Diagnostics interface, maintaining a 256 byte/message
+/// stack allocated buffer for error messages
 pub fn BoundedDiagnostics(comptime max_errors: usize) type {
     return struct {
         buffer: [max_errors]ParseLineError,
@@ -143,7 +152,7 @@ pub const Value = union(enum) {
     ///
     /// This function is intended to be used by the SRF parser
     pub fn parse(str: []const u8, state: *RecordIterator.State, delimiter: u8) ParseError!ValueWithMetaData {
-        const type_val_sep_raw = std.mem.indexOfScalar(u8, str, ':');
+        const type_val_sep_raw = std.mem.findScalar(u8, str, ':');
         if (type_val_sep_raw == null) {
             try parseError("no type data or value after key", state);
             return ParseError.ParseFailed;
@@ -309,7 +318,7 @@ pub const Value = union(enum) {
         try state.reader.readSliceAll(buf[rest_of_data.len + 1 ..]);
         // Because we've now advanced the line, we need to reset everything
         state.line += std.mem.count(u8, buf, "\n");
-        state.column = buf.len - std.mem.lastIndexOf(u8, buf, "\n").?;
+        state.column = buf.len - std.mem.find(u8, buf, "\n").?;
         state.partial_line_column = state.column;
         // However, we want to be past the end of the *next* newline too (in long
         // format mode)
@@ -354,7 +363,7 @@ pub const Value = union(enum) {
             }
         } else if (past_val[0] != state.field_delimiter) {
             // compact form
-            const extra_bytes = std.mem.indexOfScalar(u8, past_val, state.field_delimiter) orelse past_val.len;
+            const extra_bytes = likelyExtraBytes(past_val, state.field_delimiter);
             const msg = std.fmt.bufPrint(
                 &buf,
                 "field value has {} additional bytes. Perhaps length should be restated as {}?",
@@ -362,10 +371,27 @@ pub const Value = union(enum) {
             ) catch "field value has additional bytes. Perhaps length should be restated";
             // we can try to advance the reader to the next field, but we might be cooked
             try parseError(msg, state);
-            return std.mem.indexOfScalar(u8, past_val, state.field_delimiter) orelse past_val.len;
+            return extra_bytes;
         }
         return 0;
     }
+
+    /// Bytes between the declared end of the value and where the field most likely
+    /// actually ends.
+    ///
+    /// A comma is ambiguous in compact format: field separator, or content inside
+    /// a length-prefixed value. It could even be a comma inside a key name. The
+    /// byte count normally tells them apart, so with the count wrong the next ':'
+    /// is the best anchor left, being where the following field's key ends. The
+    /// comma just before it is therefore the separator. Falling back to the first
+    /// comma covers a value that contains a colon itself.
+    fn likelyExtraBytes(past_val: []const u8, delimiter: u8) usize {
+        if (std.mem.findScalar(u8, past_val, ':')) |colon|
+            if (std.mem.findScalarLast(u8, past_val[0..colon], delimiter)) |sep|
+                return sep;
+        return std.mem.findScalar(u8, past_val, delimiter) orelse past_val.len;
+    }
+
     inline fn fallbackAllocatorFor(state: *RecordIterator.State) !std.mem.Allocator {
         if (state.fallback_arena) |f| return f.allocator();
         if (state.options.parse_allocator == .none) return error.AllocationRequired;
@@ -894,9 +920,9 @@ pub const RecordIterator = struct {
                 value.item_value == null or
                 value.item_value.? == .string) null else blk: {
                 // Basic parse to find raw value
-                const sep = std.mem.indexOfScalar(u8, rest, ':') orelse break :blk null;
+                const sep = std.mem.findScalar(u8, rest, ':') orelse break :blk null;
                 const after = rest[sep + 1 ..];
-                const end = std.mem.indexOfScalar(u8, after, state.field_delimiter) orelse after.len;
+                const end = std.mem.findScalar(u8, after, state.field_delimiter) orelse after.len;
                 break :blk after[0..end];
             };
 
@@ -965,7 +991,7 @@ pub const RecordIterator = struct {
                                 "attempting recovery from invalid srf (length prefix underflow of {d}). check diagnostics for more information",
                                 .{value.length_prefix_detected_underflow},
                             );
-                            state.current_line = if (std.mem.indexOfScalar(u8, state.current_line.?, state.field_delimiter)) |i|
+                            state.current_line = if (std.mem.findScalar(u8, state.current_line.?, state.field_delimiter)) |i|
                                 state.current_line.?[i + 1 ..]
                             else
                                 // Nothing on this line to resync to. The diagnostic is already recorded, so
@@ -2672,7 +2698,7 @@ test "Tombstone round-trip: forever with reason" {
     const tomb = try Tombstone.parse("forever not found in tiingo");
     var buf: [512]u8 = undefined;
     const out = try std.fmt.bufPrint(&buf, "{f}", .{fmt(Rec, items, .{ .tombstoned = tomb })});
-    try std.testing.expect(std.mem.indexOf(u8, out, "#!tombstoned=forever not found in tiingo") != null);
+    try std.testing.expect(std.mem.find(u8, out, "#!tombstoned=forever not found in tiingo") != null);
 
     var reader = std.Io.Reader.fixed(out);
     var ri = try iterator(&reader, std.testing.allocator, .{});
@@ -2688,7 +2714,7 @@ test "Tombstone round-trip: numeric until with reason" {
     const tomb = try Tombstone.parse("1782852900 delisted");
     var buf: [512]u8 = undefined;
     const out = try std.fmt.bufPrint(&buf, "{f}", .{fmt(Rec, items, .{ .tombstoned = tomb })});
-    try std.testing.expect(std.mem.indexOf(u8, out, "#!tombstoned=1782852900 delisted") != null);
+    try std.testing.expect(std.mem.find(u8, out, "#!tombstoned=1782852900 delisted") != null);
 
     var reader = std.Io.Reader.fixed(out);
     var ri = try iterator(&reader, std.testing.allocator, .{});
@@ -2704,7 +2730,7 @@ test "Tombstone round-trip: forever without reason" {
     const tomb = try Tombstone.parse("forever");
     var buf: [512]u8 = undefined;
     const out = try std.fmt.bufPrint(&buf, "{f}", .{fmt(Rec, items, .{ .tombstoned = tomb })});
-    try std.testing.expect(std.mem.indexOf(u8, out, "#!tombstoned=forever\n") != null);
+    try std.testing.expect(std.mem.find(u8, out, "#!tombstoned=forever\n") != null);
 
     var reader = std.Io.Reader.fixed(out);
     var ri = try iterator(&reader, std.testing.allocator, .{});
@@ -3194,3 +3220,68 @@ test "compact: a value ending one byte short, deeper into the next line" {
         .{ .record = 2, .key = "final", .value = "v" },
     }, 1);
 }
+
+/// Drives a parse to completion and checks the first diagnostic's text.
+///
+/// Walks fields as well as records: `checkShortPrefix` runs during field parsing,
+/// so draining records alone can miss it. Parse failures are ignored because the
+/// diagnostic, not the control flow, is what is under test.
+fn expectFirstDiagnostic(data: []const u8, expected: []const u8) !void {
+    var reader = std.Io.Reader.fixed(data);
+    var diags: BoundedDiagnostics(10) = .empty;
+    var diag: Diagnostics = diags.diagnostics();
+
+    var it = try iterator(&reader, std.testing.allocator, .{ .diagnostics = &diag });
+    defer it.deinit();
+    while (it.next() catch null) |fields| {
+        while (fields.next() catch null) |_| {}
+    }
+
+    const errors = diags.errors();
+    try std.testing.expect(errors.len > 0);
+    try std.testing.expectEqualStrings(expected, errors[0].message);
+}
+test "compact: suggested length uses the last delimiter before the next colon" {
+    // Fails today with "2 additional bytes ... restated as 6?". The value is a
+    // number with thousands separators, so the nearest comma is the wrong anchor.
+    // The colon after "really" is where the next field's key ends, so the comma
+    // just before it is the separator: 4 + 10 = 14.
+    try expectFirstDiagnostic(
+        "#!srfv1\nim_worth:4:23,000,000,000,really:bool:false\n",
+        "field value has 10 additional bytes. Perhaps length should be restated as 14?",
+    );
+}
+
+test "compact: a value containing both a delimiter and a colon" {
+    // Fails today with "1 additional bytes ... restated as 3?", which would leave
+    // the next field's key as "c,next". Under the rule the value is "a:b,c" and
+    // the next key is "next".
+    try expectFirstDiagnostic(
+        "#!srfv1\nk:2:a:b,c,next::v\n",
+        "field value has 3 additional bytes. Perhaps length should be restated as 5?",
+    );
+}
+test "compact: a colon with no delimiter before it falls back to the first delimiter" {
+    // The first colon sits inside the value's URL, so there is no comma before it
+    // and step 2 declines. The fallback is what gets this right.
+    try expectFirstDiagnostic(
+        "#!srfv1\nk:3:http://x,next::v\n",
+        "field value has 5 additional bytes. Perhaps length should be restated as 8?",
+    );
+}
+
+test "compact: no colon and no delimiter runs to end of line" {
+    try expectFirstDiagnostic(
+        "#!srfv1\nk:2:abc\n",
+        "field value has 1 additional bytes. Perhaps length should be restated as 3?",
+    );
+}
+
+test "long format ignores delimiters entirely" {
+    // A comma is ordinary text in long format, so the whole remainder counts and
+    // the rule must not apply there.
+    try expectFirstDiagnostic(
+        "#!srfv1\n#!long\nim_worth:4:23,000,000\n",
+        "line has 6 additional bytes after field value. Perhaps length should be restated as 10?",
+    );
+}