fix: restore the character that terminates a number - #5344
Draft
nlohmann wants to merge 9 commits into
Draft
Conversation
operator>>'s notes state that it leaves the stream positioned right after the parsed value, so that concatenated JSON values can be read back to back. That does not hold when the value is a number: a number is only terminated by the character that follows it, and the lexer's unget() is simulated (it rewinds only the lexer's own bookkeeping), so that character stays consumed from the stream. Document the actual behaviour: the guarantee holds for all value types except numbers, which must be followed by whitespace. Also qualify the cross-reference on the JSON Lines page, which repeated the unqualified claim. Documentation only; the behaviour itself is tracked in #5340. Signed-off-by: Niels Lohmann <mail@nlohmann.me>
operator>> is documented to leave the stream positioned right after the
parsed value, so that concatenated JSON values can be read back to back.
That did not hold for numbers: a number is only terminated by the
character following it, and lexer::scan_number() reads that character
and calls unget() -- which is simulated and rewinds only the lexer's own
bookkeeping. input_stream_adapter consumes via sbumpc() with no matching
sungetc(), so the terminating character stayed consumed and the next
extraction started one byte too late ('1true' left the stream at 'rue').
Propagating unget() to the adapter directly does not work: next_unget
makes the following get() replay the cached character, so the terminator
would be delivered twice. Instead, restore the still-pending character
once at the end of a non-strict parse, where the input is handed back to
the caller:
- input_stream_adapter gains unget_character() (sungetc()) and advertises
it via supports_unget, detected the same way as supports_seek.
- lexer::restore_pending_unget() turns a pending simulated unget of a
real (non-EOF) character into a real one and clears next_unget so the
character is not also replayed. It is a no-op for adapters that cannot
unget, and reports failure when sungetc() fails, in which case the
input is left as it was before.
- parser calls it on the three non-strict paths, i.e. for operator>> and
sax_parse(strict = false).
Strict parse()/accept() are unaffected: they require the input to end
after the value, so the character is consumed by the end-of-input check
anyway. Parse error messages and reported positions are unchanged.
Signed-off-by: Niels Lohmann <mail@nlohmann.me>
Contributor
Have you looked at using |
Four CI failures, all in the new test code: - GCC (-Werror=useless-cast): drop the `json(...)` wrapper around `json::parse(...)`, which already returns a `json`. - GCC (-Werror=unused-result): assign the discarded `json::parse()` result to a dummy, the idiom used elsewhere in the test suite, and catch `json::parse_error&` for consistency. - clang-tidy (google-default-arguments): remove the default argument from the `pbackfail()` override; `sungetc()` supplies the base declaration's default. - MSVC (bad allocation): `no_putback_streambuf::underflow()` set a one-character get area without advancing `m_pos`, so an implementation whose `istream::get` peeks before it bumps re-read the same character forever. Keep no get area at all: `underflow()` peeks, `uflow()` consumes, and `sungetc()` still always lands in `pbackfail()`, which is what the test needs. Signed-off-by: Niels Lohmann <mail@nlohmann.me>
Signed-off-by: Niels Lohmann <mail@nlohmann.me>
Read the character following a number without consuming it, instead of consuming it and putting it back. input_stream_adapter now peeks with sgetc() and only steps over the character when the next one is requested or when the adapter is destroyed, so releasing it cannot fail - no putback position is required from the streambuf. Suggested by gregmarr in #5344. Signed-off-by: Niels Lohmann <mail@nlohmann.me>
Signed-off-by: Niels Lohmann <mail@nlohmann.me>
…restore-unget Signed-off-by: Niels Lohmann <mail@nlohmann.me>
The caveat added in #5343 describes the behavior this branch fixes: a number no longer consumes the character that terminates it, so concatenated values need no separator. Signed-off-by: Niels Lohmann <mail@nlohmann.me>
Folding the release_lookahead() call into the existing strict check left the "in strict mode" comment on an else-if branch, and made the strict condition in sax_parse() redundant with the branch it followed. Signed-off-by: Niels Lohmann <mail@nlohmann.me>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #5340. Draft: this changes observable behavior of
operator>>andsax_parse(strict = false), so it is a candidate for the next major release rather than a patch release. The version numbers in the docs assume 4.0.0 and need adjusting if that changes.The bug
A number is the only JSON value whose end can only be detected by looking at the character that follows it.
lexer::scan_number()reads that character and callslexer::unget(), butunget()is deliberately simulated — it rewinds only the lexer's own bookkeeping (chars_read_total,chars_read_current_line,token_string), not the input.input_stream_adapter::get_character()consumed viasbumpc(), so the terminating character stayed consumed:This is invisible to
parse()/accept(), which require the input to end after the value anyway. It is only observable where the caller keeps using the input:operator>>andsax_parse(strict = false).The fix
Following @gregmarr's suggestion, the adapter now peeks instead of consuming, rather than consuming and trying to put the character back afterwards.
input_stream_adapter::get_character()returnssb->sgetc()and remembers that a character is pending; the next call steps over it withsbumpc()before peeking again.get_elements()and the destructor commit the pending character the same way, so every other user of the adapter — and the stream the caller gets back — sees the position it saw before.input_stream_adapter::release_lookahead()drops the pending character instead of committing it, leaving it in the stream. The adapter advertises this withsupports_lookahead, detected with the sameis_detectedtag-dispatch idiom already used forsupports_seek; other adapters compile the call away to a no-op.lexer::release_lookahead()forwards a pending simulated unget to the adapter and clearsnext_unget, so the character is read from the input again rather than replayed fromcurrent. A pending unget of EOF needs no special case: reaching EOF leaves no lookahead to release.parsercalls it on the three non-strict paths (bothparse()branches andsax_parse), folded into the existingif (strict && …)checks.The invariant that makes this safe:
get()clearsnext_ungetwhenever it consumes it, so a pending unget always refers to the most recent adapter read, and there is never more than one.Why not
sungetc()The first version of this PR consumed with
sbumpc()and put the character back withsungetc(). That is best-effort:sungetc()fails when the streambuf has no putback position, and the character stays consumed. Peeking cannot fail — the character is never consumed in the first place, so no putback position is required. Theno_putback_streambuftest (astreambufwith no get area whosepbackfail()always fails) covers exactly that case and now passes.Verification
1truerue❌true✅1 truetrue❌true✅1[2]2]❌[2]✅1{}}❌{}✅1"a"a"❌"a"✅-0.5e3xx✅Note
1 truewas wrong before too — the issue's table lists it as fine because the swallowed byte happened to be whitespace, but the position was still off by one.tests/src/unit-deserialization.cpp(stream position after extraction (#5340)): number terminators for every following value type, self-delimiting values, a number at end of input, repeated extraction of1true[2]3"x"{"a":4}5,sax_parse(strict = false), strict parsing still rejecting trailing data, and thestreambufthat cannot put back. 8 assertions in this section fail againstdevelop(and the repeated-extraction subcase throws).unit-deserialization,unit-class_lexer,unit-class_parser,unit-class_parser_diagnostic_positions,unit-user_defined_input,unit-regression2, andunit-disabled_exceptionspass against bothinclude/andsingle_include/— 20,411 assertions each, 0 failures.develop.Public API
No breaking changes to the public API surface: no signature, type, or name changes; nothing added to or removed from the public interface.
lexer::release_lookahead(),input_stream_adapter::release_lookahead(), andinput_stream_adapter::supports_lookaheadare new members indetail::.There is a behavior change, which is why this is a draft targeting the next major release:
operator>>andsax_parse(strict = false)on astd::istreamnow leave the stream one byte earlier when the parsed value was a number. Code that relied on the terminating byte being swallowed will observe it again.parse(),accept(), and all non-stream inputs (strings, iterators, containers,FILE*) are unchanged.Follow-up
docs/mkdocs/docs/api/operator_gtgt.mdandsax_parse.mdsay "version 4.0.0"; these need updating if the change lands elsewhere. The admonition added in #5343 is replaced here by the version note.unit-deserialization.cpp; verified to fail without the fix).make amalgamatewas run;single_includematchesinclude(+132/−5, no unrelated reformatting).This pull request was written by Claude Code.