Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JSON WriteStream functionality regression #16519

Closed
karlseguin opened this issue Jul 24, 2023 · 10 comments
Closed

JSON WriteStream functionality regression #16519

karlseguin opened this issue Jul 24, 2023 · 10 comments
Labels
standard library This issue involves writing Zig code for the standard library. use case Describes a real use case that is difficult or impossible, but does not propose a solution.
Milestone

Comments

@karlseguin
Copy link
Contributor

Zig Version

0.11.0-dev.4191+1bf16b172

Steps to Reproduce and Observed Behavior

The issue with the new WriteStream API introduced in #16405 is that you can no longer compose values, trying to force this by reaching into ws.stream will crash.

Here's code that crashes:

pub fn main() !void {
	var gpa = std.heap.GeneralPurposeAllocator(.{}){};
	const allocator = gpa.allocator();

	const user = User{
		.name = .{.first = "Leto"},
	};

	var arr = std.ArrayList(u8).init(allocator);
	try std.json.stringify(user, .{}, arr.writer());
	std.debug.print("{s}\n", .{arr.items});
}

const User = struct {
	name: Name,
};

const Name = struct {
	first: []const u8,

	pub fn jsonStringify(self: Name, out: anytype) !void {
		// this works:
		// return out.write(self.first);

		return std.json.encodeJsonString(self.first, .{}, out.stream);
	}
};

Maybe it's wrong to use WriteStream.stream directly, but without this functionality, it's hard to have fine-grained controlled over the output. writePreformatted is the only available option, but now you need to add another writer to format your value...WriteStream isn't really a "stream" API.

Expected Behavior

I expect WriteStream to continue to expose an io.Writer interface.

@karlseguin karlseguin added the bug Observed behavior contradicts documented or intended behavior label Jul 24, 2023
@andrewrk andrewrk added standard library This issue involves writing Zig code for the standard library. use case Describes a real use case that is difficult or impossible, but does not propose a solution. and removed bug Observed behavior contradicts documented or intended behavior labels Jul 24, 2023
@andrewrk andrewrk added this to the 0.12.0 milestone Jul 24, 2023
@andrewrk
Copy link
Member

cc @thejoshwolfe

@karlseguin
Copy link
Contributor Author

karlseguin commented Jul 24, 2023

If anyone else runs into this issue, one hack that seems to work, is calling try out.writePreformatted(""); before you write directly to the stream. Not sure it'll work in all cases though.

If you change the jsonStringify code in the original issue to:

pub fn jsonStringify(self: Name, out: anytype) !void {
  try out.writePreformatted("");
  return std.json.encodeJsonString(self.first, .{}, out.stream);
}

It'll work.

@thejoshwolfe
Copy link
Contributor

Are you looking to write long strings in streaming chunks? is that the use case? I'd like to better understand the actual use case. The example code in the post above includes a working line commented out, so presumably that example wasn't complex enough to demonstrate the real issue, right?

I'd like to see if your use case can be properly supported rather than being hacked in by reaching into the state or abusing the preformatted function.

@karlseguin
Copy link
Contributor Author

I wanted explicit control over how floats and integers are serialized. I know both these are documented, but integers being serialized as strings in some cases, and the way floats are treated (e..g e+00) are problematic issues for me.

Before this change, I could just use out_stream.writeAll to get the exact output that I wanted. Adding options for integers and floats would solve my specific issue, but I still think WriteStream should act as anio.Writer for fine-grained control.

@thejoshwolfe
Copy link
Contributor

Ok that makes sense! So doing something like std:fmt.bufPrint() into a small local buffer and then calling writePreformatted() would work, but it does an extra copy that you want to avoid, right?

@karlseguin
Copy link
Contributor Author

Yes, that's what we've done for the time being.

@thejoshwolfe
Copy link
Contributor

How about an API like this:

        /// An alternative to calling `write` that formats the value with `std.fmt`.
        /// This function does the usual punctuation and indentation formatting
        /// assuming the resulting formatted string represents a single complete value.
        /// This function is useful for doing your own number formatting.
        pub fn print(self: *Self, comptime fmt: []const u8, args: anytype) Error!void {
            try self.valueStart();
            try self.stream.print(fmt, args);
            self.valueDone();
        }

@thejoshwolfe
Copy link
Contributor

And then, it's not clear why we'd need writePreformatted anymore 🤔. probably should just use print("{s}", .{s}) instead.

@karlseguin
Copy link
Contributor Author

That would solve my specific case, yes.

@andrewrk
Copy link
Member

I'm going to declare this fixed by #16543. I'm happy to re-open if anyone wants to elaborate on the use case that is still not addressed here.

@andrewrk andrewrk modified the milestones: 0.12.0, 0.11.0 Jul 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
standard library This issue involves writing Zig code for the standard library. use case Describes a real use case that is difficult or impossible, but does not propose a solution.
Projects
None yet
Development

No branches or pull requests

3 participants