Skip to content

Add missing diagnostic-positions test coverage (lifetime, input adapters, SAX) - #5482

Merged
nlohmann merged 2 commits into
developfrom
issue-5420-diagnostic-positions-coverage
Sep 16, 2026
Merged

nlohmann merged 2 commits into
developfrom
issue-5420-diagnostic-positions-coverage

Conversation

@nlohmann

@nlohmann nlohmann commented Sep 5, 2026

Copy link
Copy Markdown
Owner

Summary

Stacked on #5474 (fix for #5417), this PR adds the coverage requested in #5420 to the (now merged) tests/src/unit-class_parser.cpp, inside a new TEST_CASE("diagnostic positions: value lifetime, input adapters, and SAX") guarded by #if JSON_DIAGNOSTIC_POSITIONS. Trailing-comma coverage under positions (item 4 of #5420) is already closed by #5417, so it is skipped here as instructed.

Covered, as characterization tests (regression protection for existing behavior, not new behavior):

  • Value lifetime
    • Copy constructor copies start_pos()/end_pos() recursively (root and every nested value).
    • Move constructor resets the moved-from value's positions to npos; the destination keeps the original positions recursively.
    • swap() — see "Found while characterizing" below.
    • Mutating a parsed document (operator[] adding a key, push_back, erase) leaves the parent's own (now stale) span and existing siblings' positions untouched; newly-added values get npos.
  • Input adapters
    • json::parse(std::wstring): positions count transcoded UTF-8 bytes, not wide characters (demonstrated with L"{\"a\":\"éé\"}", where end_pos() == 12 but ws.size() == 10).
    • BOM-prefixed input: root's start_pos() is 3, reflecting the skipped BOM.
    • std::istringstream, std::ifstream, and iterator-pair inputs all report fully consistent (non-npos) positions.
    • Binary formats (from_cbor, from_msgpack, from_ubjson, from_bson) always report npos, since binary_reader never sets a text position.
  • User-driven SAX: a user-constructed json_sax_dom_parser (no lexer pointer, e.g. built directly instead of going through json::parse()) reports npos for every produced value, since every if (m_lexer_ref) guard in json_sax.hpp is skipped without one.

Found while characterizing (flagging for a maintainer, not fixed here)

basic_json::swap(reference) (include/nlohmann/json.hpp, around line 3540) and the friend swap(reference, reference) that forwards to it swap m_data.m_type/m_data.m_value but never swap start_position/end_position — unlike copy-assignment's operator=(basic_json) (around line 1291), which does swap them as part of its copy-and-swap implementation. Concretely:

json a = json::parse(R"({"a":1})");
json b = json::parse(R"([1,2,3,4,5])");
swap(a, b);
// a is now [1,2,3,4,5], b is now {"a":1} -- values did swap
// but a.start_pos()/a.end_pos() still describe the OLD `{"a":1}` span,
// and b.start_pos()/b.end_pos() still describe the OLD `[1,2,3,4,5]` span

This looks like an oversight/inconsistency rather than intended behavior. Per the scope of this PR (test-only, characterization, no include/ changes), it is not fixed here — the test only pins the current behavior, with a comment pointing at the exact lines, so a future fix (or a deliberate decision to keep it) shows up as an intentional, visible change rather than a silent regression.

Validation

Compiled and ran unit-class_parser.cpp offline with -DJSON_DIAGNOSTIC_POSITIONS=1 (new tests exercised, 10511 assertions passed) and without it (new TEST_CASE fully compiled out, 10299 assertions passed, unchanged from before). Also checked both variants compile warning-free under -Wall -Wextra -Wpedantic -Wshadow.

Breaking change?

No breaking changes. This is a test-only change; nothing under include/ was modified.

Fixes #5420

— opened by Claude Code on behalf of @nlohmann

@nlohmann nlohmann added the review needed It would be great if someone could review the proposed changes. label Sep 5, 2026
@nlohmann nlohmann self-assigned this Sep 5, 2026
@nlohmann
nlohmann force-pushed the issue-5420-diagnostic-positions-coverage branch from 0349745 to 4d22dd4 Compare September 9, 2026 11:16
Base automatically changed from issue-5417-dedup-diagnostic-positions-test to develop September 11, 2026 06:18
@nlohmann
nlohmann force-pushed the issue-5420-diagnostic-positions-coverage branch from 4d22dd4 to 53c9694 Compare September 11, 2026 06:18
…ers, SAX)

Building on the merged unit-class_parser.cpp from #5417, add
characterization tests (regression protection for existing behavior, not a
behavior change) for JSON_DIAGNOSTIC_POSITIONS:

- value lifetime: copy ctor copies positions recursively, move ctor resets
  the moved-from value to npos, and mutating a parsed document (operator[],
  push_back, erase) leaves the parent's stale span and siblings' positions
  untouched while new values get npos.
- input adapters: wide-string input positions count transcoded UTF-8 bytes
  (not wide characters), BOM-prefixed input's start_pos() reflects the
  skipped 3-byte BOM, istringstream/ifstream/iterator-pair inputs report
  consistent (non-npos) positions, and binary formats (CBOR, MessagePack,
  UBJSON, BSON) always report npos.
- a user-constructed json_sax_dom_parser with no lexer (as used when driving
  json::sax_parse() directly) reports npos for every value, since it has no
  m_lexer_ref to source positions from.

While characterizing swap(), found that basic_json::swap() (and the friend
swap() that forwards to it) does not swap start_position/end_position,
unlike copy-assignment's operator=(basic_json), which does as part of its
copy-and-swap implementation. This looks like a real inconsistency/bug, but
per the scope of this test-only change it is only pinned (not fixed) here;
see the comment at the "swap() does NOT exchange positions" section.

Fixes #5420

Signed-off-by: Niels Lohmann <mail@nlohmann.me>
Use é escapes instead of a literal UTF-8-encoded 'é' inside the L""
literal, so the wide string's content does not depend on the compiler's
assumed source character set (MSVC without /utf-8 decodes raw non-ASCII
source bytes using the system code page rather than as UTF-8, which was
producing a wstring of unexpected length/content and failing the
ws.size()/end_pos() assertions on Windows CI).

Also reworded a comment that unintentionally embedded the literal
substring "TODO check", which clang-tidy's google-readability-todo check
flags regardless of quoting context.

Signed-off-by: Niels Lohmann <mail@nlohmann.me>
@nlohmann
nlohmann force-pushed the issue-5420-diagnostic-positions-coverage branch from 53c9694 to b37df25 Compare September 11, 2026 15:56
@nlohmann nlohmann added this to the Release 3.13.0 milestone Sep 16, 2026
@nlohmann
nlohmann merged commit 29ba597 into develop Sep 16, 2026
159 checks passed
@nlohmann
nlohmann deleted the issue-5420-diagnostic-positions-coverage branch September 16, 2026 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

L review needed It would be great if someone could review the proposed changes. tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Missing test coverage: diagnostic positions — value lifetime, input adapters, SAX, trailing commas

1 participant