Swap diagnostic positions and cover remaining JSON_DIAGNOSTIC_POSITIONS cases

[22elix3r]

Summary

With JSON_DIAGNOSTIC_POSITIONS, copy construction copies start_pos/end_pos and move construction resets the source to npos, but basic_json::swap only swapped the payload. std::swap therefore left diagnostic positions attached to the wrong object. That is a behavior fix, not just extra coverage.

The rest of #5420 was test/docs holes around that API. This PR now covers them instead of leaving a follow-up.

Related Issue

Fixes #5420

Changes Made

Behavior

• Exchange start_position/end_position in basic_json::swap() when positions are enabled (type-specific array_t/object_t/string_t/binary_t overloads still leave positions on the basic_json object)

Tests (the rest of #5420)

• Copy (children keep offsets into the original input; mutating the original does not affect the copy)
• Move (source is npos)
• Swap of two basic_json values, plus type-specific container swap
• Mutation: operator[], push_back, erase leave parent/sibling spans pointing at the original text; replacements are npos
• Input adapters: istringstream, ifstream, iterator pairs, contiguous vector, UTF-8 BOM (start_pos() == 3)
• Wide strings: positions are UTF-8 byte offsets of the transcoded stream and cannot index the original u16string/u32string
• Binary formats (from_cbor/from_msgpack/from_ubjson/from_bjdata/from_bson) yield npos
• User-constructed json_sax_dom_parser (no lexer) yields npos
• ignore_trailing_commas: end_pos covers the skipped comma; accept()/sax_parse() with the flag on (the hole Test suite: unit-class_parser_diagnostic_positions.cpp has drifted from unit-class_parser.cpp — deduplicate via define-based recompilation #5417 / Exercise trailing commas with diagnostic positions enabled #5431 left open)

Docs

• UTF-8 byte offsets, BOM, copy/move/swap, mutation invalidation, SAX/binary npos

Testing

Commands executed:

• python3 tools/amalgamate/amalgamate.py is not sufficient by itself; single_include/nlohmann/json.hpp is restored from develop with only the swap change (CI amalgamation check failed on the first commit because amalgamate.py was run without astyle)
• cmake -S . -B build -DJSON_BuildTests=ON -DCMAKE_BUILD_TYPE=Debug -DJSON_FastTests=ON
• cmake --build build --target test-diagnostic-positions_cpp11 test-class_parser_diagnostic_positions_cpp11 test-deserialization_cpp11
• ./tests/test-diagnostic-positions_cpp11 --no-skip -tce='*downloaded*'
• ./tests/test-class_parser_diagnostic_positions_cpp11 --no-skip -tce='*downloaded*'
• ./tests/test-deserialization_cpp11 --no-skip -tce='*downloaded*'

Results:

• diagnostic-positions: 1 passed, 137 assertions
• class_parser_diagnostic_positions: 2 passed, 10790 assertions
• deserialization: 11 passed, 468 assertions

Notes

Assignment already swapped positions (operator=(basic_json other)). Member swap was the hole. Positions describe a span in the original parse input; they are not recomputed when the value is later mutated.

• The changes are described in detail, both the what and why.
• If applicable, an existing issue is referenced.
• The Code coverage remained at 100%. A test case for every new line of code.
• If applicable, the documentation is updated.
• The source code is amalgamated by running make amalgamate.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[gregmarr]

The type-specific swap functions below this one don't swap, and they can't because they take type-specific values, so I'm not sure what those should do, if anything.

[22elix3r]

Those overloads swap only the contained array_t / object_t / string_t / binary_t with an external container of the same type. The basic_json object itself is unchanged (same type, same identity), so its diagnostic start_position / end_position should stay put: they describe this value's span in the original document, not the heap storage of the container.

The other argument is not a basic_json, so there is no second pair of positions to exchange. Clearing or touching the positions on a container-only swap would be the wrong invariant. I left those helpers as value-only on purpose.

[nlohmann]

Thanks for fixing. However, this is only part 1 of #5420. Please address the other items or remove the "fixes #5420" from the description.

[nlohmann]

To spell out what "part 1 of #5420" leaves open — and one gap inside part 1 itself:

Item 1 is not complete. Copy, move and swap are now asserted, but the mutation case is not:

Mutating a parsed document (j["a"] = 42;, push_back, erase) leaves sibling/parent positions pointing at the old text — no test pins whether that is intended.

Item 2 — input adapters. None of it: wide strings (where chars_read_total counts transcoded UTF-8 bytes, so positions cannot index the original wstring), BOM-prefixed input (root start_pos() should be 3), ifstream/istringstream, iterator pairs, contiguous containers, and binary formats yielding npos.

Item 3 — user-driven SAX. Untouched. A user-constructed json_sax_dom_parser has no lexer ref, so every #if JSON_DIAGNOSTIC_POSITIONS block is skipped and positions come out npos; nothing asserts that either way.

Item 4 — ignore_trailing_commas × positions. Still zero coverage. #5417 was meant to close this and its PR (#5431) was withdrawn, so it is not covered elsewhere.

Note the swap change itself is a behavior fix, not just coverage — worth calling out in the description, since #5420 was filed as a test-coverage issue and this quietly changes what swap does.

---

Generated by Claude Code

[github-actions]

🔴 Amalgamation check failed! 🔴

The source code has not been amalgamated and/or formatted correctly.

📎 A ready-to-apply patch is attached to the failed workflow run as the amalgamation-patch artifact. Download it, then apply it locally from the repository root with:

git apply amalgamation.patch

This does not require installing astyle yourself.

[22elix3r]

Follow-up for the review: this is no longer just part 1 of #5420.

Item 1 (lifetime). Copy/move/swap stay. Mutation is now pinned: j["a"] = 42, push_back, and erase leave parent/sibling positions pointing at the original text; replacements are npos. That matches the existing invalidation note, which is now spelled out with those operations. Type-specific swap(array_t&) (and friends) still do not touch positions — they are not a basic_json↔basic_json exchange.

Item 2 (adapters). Covered: istringstream/ifstream, iterator pairs, contiguous vector, BOM (start_pos() == 3), wide strings (UTF-8 offsets of the transcoded stream, not indexes into the original u16string/u32string), and binary formats as npos.

Item 3 (user SAX). A user-constructed json_sax_dom_parser (no lexer) is asserted to yield npos, including from parser_helper where operator== had been hiding it.

Item 4 (trailing commas). ignore_trailing_commas is exercised with positions (end_pos covers the skipped comma), and accept()/sax_parse() with the flag are tested in unit-deserialization.cpp. This is the coverage hole #5431 was withdrawn over, not the CMake dual-compile dedup from #5417.

swap itself is called out in the description as a behavior fix. The amalgamated header from the first commit is restored (amalgamate.py without astyle was the CI failure).

[gregmarr]

Those overloads swap only the contained array_t / object_t / string_t / binary_t with an external container of the same type. > The basic_json object itself is unchanged (same type, same identity), so its diagnostic start_position / end_position should stay put: they describe this value's span in the original document, not the heap storage of the container.

Is that useful? If the content is no longer what was parsed from that document, should it continue to refer to the positions in the document, possibly creating an inaccurate error message later?

Related to this comment:

Item 1 is not complete. Copy, move and swap are now asserted, but the mutation case is not:

Mutating a parsed document (j["a"] = 42;, push_back, erase) leaves sibling/parent positions pointing at the old text — no test pins whether that is intended.

[nlohmann]

@22elix3r Thanks for pushing the follow-up commit covering the rest of #5420. CI is still red on the latest push though: msvc (Debug, Win32, default), msvc-arm64 (Debug), ci_nvhpc, and the AppVeyor build are failing. Could you take a look?

[nlohmann]

Thanks for this — the underlying finding is real and well-diagnosed: basic_json::swap() copies the payload but never exchanges start_position/end_position, unlike copy-assignment (which does via copy-and-swap). Good catch, and the fix itself (swapping the two position members under #if JSON_DIAGNOSTIC_POSITIONS) is correct.

We're folding that fix into our own diagnostic-positions work (the #5474/#5482 stack for #5417/#5420), which took a different structural approach to the test-file duplication (deleting unit-class_parser_diagnostic_positions.cpp and folding it into unit-class_parser.cpp behind a second CMake compile target) — that made a direct merge of this PR's test changes conflict-prone, and its CI is currently red on MSVC/nvhpc/AppVeyor due to an unrelated wide-string transcoding assertion in the new test. So we're going to move forward with our stack (crediting this PR for the swap fix) and close this one. Thanks again for tracking it down.

— closed by Claude Code on behalf of @nlohmann