Issue #4561 - use diagnostic positions in exceptions #4583

hnampally · 2025-01-05T06:14:45Z

This PR is for #4561, updates the diagnostics function in the exceptions.hpp to use diagnostic positions in exceptions.

Pull request checklist

Read the Contribution Guidelines for detailed information.

Changes are described in the pull request, or an existing issue is referenced.
The test suite compiles and runs without error.
Code coverage is 100%. Test cases can be added by editing the test suite.
The source code is amalgamated; that is, after making changes to the sources in the include/nlohmann directory, run make amalgamate to create the single-header files single_include/nlohmann/json.hpp and single_include/nlohmann/json_fwd.hpp. The whole process is described here.

Signed-off-by: Harinath Nampally <[email protected]>

coveralls · 2025-01-05T06:50:33Z

coverage: 99.607% (-0.03%) from 99.639%
when pulling 52f0703 on hnampally:issue4561-use-diagnostic-positions-in-exceptions
into 60c4875 on nlohmann:develop.

nlohmann

Please also check the failing test. Just as for JSON_DIAGNOSTICS, you may need to add #ifdefs to the other test files to accommodate for the different exception outputs.

docs/examples/diagnostic_positions.cpp

docs/examples/diagnostics_extended.cpp

include/nlohmann/detail/exceptions.hpp

Signed-off-by: Harinath Nampally <[email protected]>

github-actions · 2025-01-06T01:12:13Z

🔴 Amalgamation check failed! 🔴

The source code has not been amalgamated. @hnampally
Please read and follow the Contribution Guidelines.

Signed-off-by: Harinath Nampally <[email protected]>

nlohmann · 2025-01-06T07:07:13Z

docs/examples/diagnostic_positions.cpp

@@ -48,4 +48,4 @@ int main()
    std::cout << "1" << "\n";
    std::cout << "Parsed string: \n";
    std::cout << json_string.substr(j["address"]["housenumber"].start_pos(), j["address"]["housenumber"].end_pos() - j["address"]["housenumber"].start_pos()) << "\n\n";
-}
+}


Please revert this file.

nlohmann · 2025-01-06T07:07:18Z

docs/examples/diagnostic_positions.output

@@ -47,4 +47,3 @@ Original string:
 1
 Parsed string: 
 1
-


Please revert this file.

nlohmann · 2025-01-06T07:07:54Z

docs/examples/diagnostics_extended.cpp

@@ -19,4 +19,4 @@ int main()
    {
        std::cout << e.what() << '\n';
    }
-}
+}


Please revert this file.

nlohmann · 2025-01-06T07:07:59Z

docs/examples/diagnostics_extended.output

@@ -1 +1 @@
-[json.exception.type_error.302] (/address/housenumber) type must be number, but is string
+[json.exception.type_error.302] (/address/housenumber) type must be number, but is string


Please revert this file.

nlohmann · 2025-01-06T07:09:20Z

include/nlohmann/detail/exceptions.hpp

+            str += ", byte " + std::to_string(leaf_element->start_pos()) + "-" + std::to_string(leaf_element->end_pos());
+            return concat('(', str, ") ");
+        }
+#endif


This logic looks weird:

#if JSON_DIAGNOSTIC_POSITIONS #endif elif JSON_DIAGNOSTIC_POSITIONS

Yes, a line was lost, and it should still be between the #endif and the #elif:

return concat('(', str, ") ");

Actually, that should just be moved out of the if.

nlohmann · 2025-01-06T07:13:06Z

tests/src/unit-json_patch.cpp

@@ -60,14 +60,20 @@ TEST_CASE("JSON patch")
            json const doc2 = R"({ "q": { "bar": 2 } })"_json;

            // because "a" does not exist.
+#if defined(JSON_DIAGNOSTIC_POSITIONS)


Due to

json/include/nlohmann/detail/abi_macros.hpp

Lines 29 to 31 in 60c4875

#ifndef JSON_DIAGNOSTIC_POSITIONS

#define JSON_DIAGNOSTIC_POSITIONS 0

#endif

, JSON_DIAGNOSTIC_POSITIONS will always be defined. Either to 0 or 1. So checking for defined(JSON_DIAGNOSTIC_POSITIONS) will always be true.

I understand, unfortunately my workspace got messed up and I was unable to test it thoroughly, I didn't intend to push these changes, anyway I will set up a fresh workspace, I wonder if its okay to close this PR and open a new PR with fresh branch?

codenut · 2025-01-06T12:11:19Z

include/nlohmann/detail/exceptions.hpp

@@ -131,7 +130,20 @@ class exception : public std::exception
        {
            return concat(a, '/', detail::escape(b));
        });
-        return concat('(', str, ") ");
+#if JSON_DIAGNOSTIC_POSITIONS
+        if ((leaf_element->start_pos() != std::string::npos) && (leaf_element->end_pos() != std::string::npos))


Line 134-139 appears to be similar to 140-146. Should we consider extracting it into a helper function for better code reusability? Like below..

template<typename BasicJsonType> std::string get_byte_positions(const BasicJsonType* leaf_element) { #if JSON_DIAGNOSTIC_POSITIONS if ((leaf_element->start_pos() != std::string::npos) && (leaf_element->end_pos() != std::string::npos)) { return concat('(', "byte ", std::to_string(leaf_element->start_pos()), "-", std::to_string(leaf_element->end_pos()), ") "); } return ""; #else static_cast<void>(leaf_element); return ""; #endif }

thanks for the suggestion, yes I will do this, it looks like a good idea to me.

gregmarr · 2025-01-06T21:45:39Z

include/nlohmann/detail/exceptions.hpp

+        if ((leaf_element->start_pos() != std::string::npos) && (leaf_element->end_pos() != std::string::npos))
+        {
+            auto str = concat("byte ", std::to_string(leaf_element->start_pos()), "-", std::to_string(leaf_element->end_pos()));
+            return concat('(', str, ") ");


Shouldn't need two separate concat() calls here.

gregmarr · 2025-01-06T21:46:03Z

include/nlohmann/detail/exceptions.hpp

+#if JSON_DIAGNOSTIC_POSITIONS
+        if ((leaf_element->start_pos() != std::string::npos) && (leaf_element->end_pos() != std::string::npos))
+        {
+            str += ", byte " + std::to_string(leaf_element->start_pos()) + "-" + std::to_string(leaf_element->end_pos());


Should these be bytes instead of byte?

hnampally · 2025-01-06T22:42:04Z

I am closing this PR and creating a new one.

hnampally added 8 commits January 1, 2025 15:09

add a ci step for Json_Diagnostic_Positions

b615508

Signed-off-by: Harinath Nampally <[email protected]>

Update ci.cmake to address review comments

39434ce

Signed-off-by: Harinath Nampally <[email protected]>

address review comment

c78941a

Signed-off-by: Harinath Nampally <[email protected]>

fix typo in the comment

66de11c

Signed-off-by: Harinath Nampally <[email protected]>

fix typos in ci.cmake

ffbc778

Signed-off-by: Harinath Nampally <[email protected]>

invoke the new ci step from ubuntu.yml

c1e8021

Signed-off-by: Harinath Nampally <[email protected]>

Merge branch 'nlohmann:develop' into develop

05f8bb5

update exceptions.hpp to use the diagnostic positions

2cc096a

Signed-off-by: Harinath Nampally <[email protected]>

hnampally requested a review from nlohmann as a code owner January 5, 2025 06:14

hnampally changed the title ~~Issue4561 use diagnostic positions in exceptions~~ Issue #4561 use diagnostic positions in exceptions Jan 5, 2025

github-actions bot added documentation M labels Jan 5, 2025

hnampally changed the title ~~Issue #4561 use diagnostic positions in exceptions~~ Issue #4561 - use diagnostic positions in exceptions Jan 5, 2025

nlohmann requested changes Jan 5, 2025

View reviewed changes

docs/examples/diagnostic_positions.cpp Outdated Show resolved Hide resolved

docs/examples/diagnostics_extended.cpp Outdated Show resolved Hide resolved

include/nlohmann/detail/exceptions.hpp Outdated Show resolved Hide resolved

address review comments

52f0703

Signed-off-by: Harinath Nampally <[email protected]>

github-actions bot added L tests and removed M labels Jan 6, 2025

fix unit test failures for ci_test_diagnotic_positions

777bae3

Signed-off-by: Harinath Nampally <[email protected]>

hnampally force-pushed the issue4561-use-diagnostic-positions-in-exceptions branch from 5aa9a0f to 777bae3 Compare January 6, 2025 01:17

fix other unit test failures for ci_test_diagnotic_positions

c1d35e3

Signed-off-by: Harinath Nampally <[email protected]>

hnampally force-pushed the issue4561-use-diagnostic-positions-in-exceptions branch from fab4d58 to c1d35e3 Compare January 6, 2025 01:58

nlohmann requested changes Jan 6, 2025

View reviewed changes

codenut reviewed Jan 6, 2025

View reviewed changes

gregmarr reviewed Jan 6, 2025

View reviewed changes

hnampally closed this Jan 6, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #4561 - use diagnostic positions in exceptions #4583

Issue #4561 - use diagnostic positions in exceptions #4583

hnampally commented Jan 5, 2025 •

edited

Loading

coveralls commented Jan 5, 2025 •

edited

Loading

nlohmann left a comment

github-actions bot commented Jan 6, 2025

nlohmann Jan 6, 2025

nlohmann Jan 6, 2025

nlohmann Jan 6, 2025

nlohmann Jan 6, 2025

nlohmann Jan 6, 2025

gregmarr Jan 6, 2025 •

edited

Loading

nlohmann Jan 6, 2025

hnampally Jan 6, 2025

codenut Jan 6, 2025

hnampally Jan 7, 2025

gregmarr Jan 6, 2025

gregmarr Jan 6, 2025

hnampally commented Jan 6, 2025 •

edited

Loading

		@@ -1 +1 @@
		[json.exception.type_error.302] (/address/housenumber) type must be number, but is string
		[json.exception.type_error.302] (/address/housenumber) type must be number, but is string

	#ifndef JSON_DIAGNOSTIC_POSITIONS
	#define JSON_DIAGNOSTIC_POSITIONS 0
	#endif

Issue #4561 - use diagnostic positions in exceptions #4583

Issue #4561 - use diagnostic positions in exceptions #4583

Conversation

hnampally commented Jan 5, 2025 • edited Loading

Pull request checklist

coveralls commented Jan 5, 2025 • edited Loading

nlohmann left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 6, 2025

🔴 Amalgamation check failed! 🔴

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregmarr Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hnampally commented Jan 6, 2025 • edited Loading

hnampally commented Jan 5, 2025 •

edited

Loading

coveralls commented Jan 5, 2025 •

edited

Loading

gregmarr Jan 6, 2025 •

edited

Loading

hnampally commented Jan 6, 2025 •

edited

Loading