Fix: std.json: Writing inf values returns InvalidJSON error #23258
Conversation
|
It looks like one of the errors in CI is related to this: #22107 . Anyone with more experience working on the std lib have any thoughts? I can cast the comptime floats to regular floats before the checks, but perhaps |
|
#22107 (comment) |
There was a problem hiding this comment.
- You'll have to update the error sets of all
stringifyfunctions, not juststringifyAlloc - The doc comment bulletpoint
Zig floats -> JSON number or string.should be updated to mention this behavior error.InvalidJsondoes not accurately describe the failure - this is serialization to JSON and the input value is invalid, not the other way around
|
@linusg Thank you for the feedback! I've updated the error type and split out the handling of comptime floats since they don't need the check. Additionally, I've updated the docstring comment as you suggested! I did have a question about the error return value from other functions. It looks like they're specifically returning the Error type that is defined earlier in the file. I looked at some of the other return values for all public methods and they look to be referencing that type, which I did already updated, too! Is there something else that needs to be done here? |
|
|
I see! Thank you for explaining that! I don't fully follow the types there, but will continue looking into it to get a better understanding of how things fit together. In the meantime, I did update the return type of those functions and the tests for that file are passing for me, so I went ahead and pushed that up! :) Edit: I think I have a better understanding of how those types come into play and I do believe this is a more correct approach than what I had before! :) |
| return; | ||
| }, | ||
| .float => { | ||
| if (std.math.isInf(value) or std.math.isNan(value)) { |
There was a problem hiding this comment.
I'm not 100% sure, but I think you can combine the .float and .comptime_float cases back together if you do the f64 cast first (which also makes a comptime type a runtime type) and use that as the argument to isInf/isNan:
const f64val: f64 = @floatCast(value);
if (std.math.isInf(f64val) or std.math.isNan(f64val)) {
return error.InvalidInput;
}
if (f64val == value) {
...
(I think you should also use the fval in the first print too.... You might even refactor this code a bit and move the valueStart/valueDone such that they're done once? Not really your change though.)
Also, I'm not sure ... but is the fval == value check redundant with the isNan check? Ah, no... this is for detecting floats that don't fit in an f64 and rendering them as strings.
| try std.testing.expectError(error.InvalidInput, stringifyAlloc(std.testing.allocator, std.math.inf(f32), .{})); | ||
| try std.testing.expectError(error.InvalidInput, stringifyAlloc(std.testing.allocator, std.math.inf(f64), .{})); | ||
| try std.testing.expectError(error.InvalidInput, stringifyAlloc(std.testing.allocator, std.math.nan(f32), .{})); | ||
| try std.testing.expectError(error.InvalidInput, stringifyAlloc(std.testing.allocator, std.math.nan(f64), .{})); |
There was a problem hiding this comment.
I think its worth adding an f128 test case too here to make sure the doesn't-fit-in-an-f64 path is handled.
There was a problem hiding this comment.
Doesn't look like there is test coverage of the does-not-fit-in-f64 path elsewhere, so for bonus points you could add some success-path coverage of that too.
I believe this should fix #23253. First time contributor here, so please feel free to be as thorough as you'd like with feedback.
Abstract: Writing an infinite float in JSON produces the value
infwhich is not valid JSON. Since we can not convert an infinite value to JSON, introduce a new error type namedInvalidJsonand return that when attempting to write a value that can not be converted to valid JSON.