Two issues I'm having with std.json

[thick oasis]

Hello! I'm trying to use the new std.json interface to efficiently parse json, and I'm experiencing 2 issues. It's likely that I just don't know how to use the interface very well, so here are my questions that someone might be able to fix:

1. The Value polymorphic parsing seems useful, but pretty wasteful for my use case.
2. Attempting to parse from a Scanner into a struct containing strings appears to cause some sort of memory corruption (The strings reference the internal Reader's long-gone memory).

I have a json that looks like this:

{
  "outer": {
    "key1": {
      // Large struct with known keys/values, including arrays and strings
    },
    "key2": {
      // Same struct
    },
    // Many more unknown keys containing the same struct.
  }
}

So, my json is only partially polymorphic. My understanding is that parsing the entire json with the std.json.Value type essentially creates a bunch of nested StringArrayHashMaps all the way down. Really, what I want is one StringArrayHashMap, mapping the string keys (key1, key2, ...) to the actual struct. I could parse the whole thing to this Value type, then pull my struct out of them... but I'm learning Zig to be fast an efficient ! Instead, What would be nice would be to tell the parser to only make 1 hashmap. Here's a solution that I came up with, which actually works really well!

<message too long, see solution below>

However, this has an issue; it totally doesn't work. Specifically because of this pesky line here:
https://github.com/ziglang/zig/blob/689f3163af48fd6e0c08bb76fadfb711db30c97c/lib/std/json/static.zig#L128

Basically, all of the std.json.parseFrom* functions won't let you parse partial json objects with them. Which I guess makes sense, it prevents you from parsing malformed json, it just blocks us from being able to do this kind of parsing while streaming.

<long post continued below>

Here's the almost working implementation of the partially-polymorphic parser:

fn JsonHashmap(comptime T: type) type {
    return struct {
        map: std.StringArrayHashMap(T),

        pub fn jsonParse(
            allocator: std.mem.Allocator,
            source: anytype,
            options: std.json.ParseOptions,
        ) !@This() {
            var map = std.StringArrayHashMap(T).init(allocator);
            errdefer map.deinit();

            if (.object_begin != try source.next()) return error.UnexpectedToken;
            while (true) {
                switch (try source.nextAlloc(allocator, .alloc_always)) {
                    .allocated_string => |string| {
                        var resolved_options = options;
                        // This allow_partial option doesn't exist in std.json
                        resolved_options.allow_partial = true;
                        const value = try std.json.parseFromTokenSourceLeaky(
                            T,
                            allocator,
                            source,
                            resolved_options,
                        );
                        try map.put(string, value);
                    },
                    .object_end => {
                        return @This(){ .map = map };
                    },

                    else => unreachable,
                }
            }
        }
    };
}
const JsonOverallStruct = struct {
    outer: JsonHashmap(MyInternalStruct),
};

(Note: I haven't checked how leaky this implementation is, I just know so far that doesn't panic)
Now, The user can just call parseFromTokenSourceLeaky with the JsonOverallStruct, then grab the hashmap out from with the result. Much more efficient.
For this to work, a new std.json.ParseOptions options would be added: .allow_partial, which would simply silence that pesky assertion.

While I'm here, one other note: there appears to be a problem with the streaming variant of these parsers, where the result of a parsed struct containing strings can return pointers to the json Scanner's internal buffer that get overwritten on later scans. There was another post somewhere in #1019652020308824145 talking about it. They claimed these lines were at fault for possibly returning pointers to a buffer that would get overwritten: https://github.com/ziglang/zig/blob/de227ace14e09c7c17a60f716b88d2327fee201d/lib/std/json/static.zig#L461
So, that would also need to get fixed if we wanted super-fast streaming json parsing.

Ultimately I have 2 questions:

1. Am I misusing std.json? Is there a better way of achieving what I want using it?
2. I don't believe I see any proposals to the effect of fixing these two "issues", would they be considered? I don't assume std is set in stone, I'm just not sure about the process of adding to it.
   If I am using std correctly, and these are problems, it would be nice if these updates could be made to std, rather than needing to pretty much copy most of std just to change a few lines for some kind of external library.

Thank you!

[thick oasis]

Here's a diff that addresses issue 1, allowing for partial parsing:

diff --git a/lib/std/json/static.zig b/lib/std/json/static.zig
index f1926660f..70b0204a2 100644
--- a/lib/std/json/static.zig
+++ b/lib/std/json/static.zig
@@ -34,6 +34,11 @@ pub const ParseOptions = struct {
     /// The default for `parseFromTokenSource` with a `*std.json.Reader` is `std.json.default_max_value_len`.
     /// Ignored for `parseFromValue` and `parseFromValueLeaky`.
     max_value_len: ?usize = null,
+
+    /// If false, calling any parseFromSlice* or parseFromTokenSource* functions
+    /// where the given json object is only partially parsed is an assertion failure.
+    /// Setting this to true allows parseFrom* functions to parse only part of a json.
+    allow_partial: bool = false,
 };
 
 pub fn Parsed(comptime T: type) type {
@@ -125,7 +130,9 @@ pub fn parseFromTokenSourceLeaky(
 
     const value = try internalParse(T, allocator, scanner_or_reader, resolved_options);
 
-    assert(.end_of_document == try scanner_or_reader.next());
+    if (!resolved_options.allow_partial) {
+        assert(.end_of_document == try scanner_or_reader.next());
+    }
 
     return value;
 }

[thick oasis]

As for the second issue, The below diff appears to solve the problem (my strings aren't getting visibly corrupted), buy I'm not familiar enough with the parser to be confident that this is the best solution.

@@ -458,17 +465,10 @@ fn internalParse(
                                 _ = try source.allocNextIntoArrayList(&value_list, .alloc_always);
                                 return try value_list.toOwnedSliceSentinel(@as(*const u8, @ptrCast(sentinel_ptr)).*);
                             }
-                            if (ptrInfo.is_const) {
-                                switch (try source.nextAllocMax(allocator, .alloc_if_needed, options.max_value_len.?)) {
-                                    inline .string, .allocated_string => |slice| return slice,
-                                    else => unreachable,
-                                }
-                            } else {
-                                // Have to allocate to get a mutable copy.
-                                switch (try source.nextAllocMax(allocator, .alloc_always, options.max_value_len.?)) {
-                                    .allocated_string => |slice| return slice,
-                                    else => unreachable,
-                                }
+                            // Have to allocate to get a mutable copy.
+                            switch (try source.nextAllocMax(allocator, .alloc_always, options.max_value_len.?)) {
+                                .allocated_string => |slice| return slice,
+                                else => unreachable,
                             }
                         },
                         else => return error.UnexpectedToken,

[maiden musk]

Yeah, it would seem to me the default parsing strategy assumes that the output is able to indefinitely reference the input

Which is only true for fully buffered parsing and such

[thick oasis]

Instead of simply removing the is_const prong in the second example, perhaps another solutions would be another ParserOption that specified whether the input buffer is trusted to be indefinitely referencable, something like .always_alloc, or .buffer_is_temporary. Unsure if this would be helpful, or too bloated, just thinking of ideas

[maiden musk]

yeah, it might be wise for the scanner or reader in question to pass .alloc_always or .alloc_if_needed based on whether they know thye have stable or unstable buffers

[hexed sierra]

maybe this is related #1123359158469664879 message

what do you think @nimble vapor ?

[thick oasis]

You're correct, that's the question where i'd seen people talking about the scanner buffer invalidation issue, and what allowed me to debug and find a temporary fix. Just didn't know how to link other questions into this one 😛

[maiden musk]

for now, I think the most efficient approach would be to not try to interface with jsonParse at any lower level, and instead manually parse, whilst still leveraging the std.json.Reader and std.json.Scanner interface

not the most ergonomic

but it works

[thick oasis]

Yep, I guess that the most efficient (processing-wise, not coding-wise) way with std as it is now if I'm not missing anything. Though it'd be a shame to rewrite std/json/static/internalParse()...

Looking at the linked quetsion, it seems like they decide whether we trust our buffer or not based on if the scanner given is a JsonScanner object. It doesn't look like that would cover all cases as std is now (I think that question was referencing an earlier version of std.json?), as Scanner has two modes of operation, an initCompleteInput mode (where we can trust the buffer), and a initStreaming one (where we can't). So I don't know if any introspection would tell us if the given scanner can trust it's buffer or not, maybe i'm not seeing it

[thick oasis]

Okay, I think I've got something that fixes both issues

[hexed sierra]

can you create PR? or issue?

[thick oasis]

Can I? I mean I'm sure I could look up and figure out how to do it, I mean will the language accept it. This page says they aren't accepting any "proposals", I don't know if that means no PRs or no issues. https://github.com/ziglang/zig/wiki/Language-Proposals . Is there any more formal process I should go through before submitting one of those, or just submit one? I don't wanna be presumptuous, never done this before

[thick oasis]

Since that's a different question, I'll move it to a new post