Better diagnostics #2562

nlohmann · 2021-01-01T16:32:25Z

This PR improves the diagnostic information provided by the library in case of exceptions. The idea (originally described in #1508 (comment) and #932 (comment)) is that every JSON value in an array or object contains a pointer to the enclosing container. From this parent relation, we can generate a JSON Pointer to the current value that allows to identify the erroneous value. As we need to store one additional pointer per JSON value, the diagnostic mode is guarded by a preprocessor macro JSON_DIAGNOSTICS that needs to be defined in order to enable the better diagnostics.

This PR is work in progress. There are a lot of open tasks, including

Any help, input, review, or discussion is greatly appreciated!

Closes #1508. Closes #932. Closes #2552.

coveralls · 2021-01-01T21:43:50Z

Coverage remained the same at 100.0% when pulling 56a6dec on diagnostics into e754ef7 on develop.

test/src/unit-json_patch.cpp

include/nlohmann/detail/conversions/from_json.hpp

include/nlohmann/json.hpp

nlohmann · 2021-01-07T13:10:32Z

Thanks @gregmarr for the comments! I will look at them in detail later this week.

include/nlohmann/json.hpp

…ostics

karzhenkov · 2021-01-08T10:56:49Z

There is a missed point. The following assigment will be lost after the object is moved to the container:

json/include/nlohmann/json.hpp

Line 1646 in d4a91b7

(*element.m_value.array)[1].m_parent = this;

nlohmann · 2021-01-08T15:04:14Z

There is a missed point. The following assigment will be lost after the object is moved to the container:

json/include/nlohmann/json.hpp

Line 1646 in d4a91b7

(*element.m_value.array)[1].m_parent = this;

Unfortunately, I cannot create an example where the parent is lost.

Would you propose setting the parent after calling emplace?

karzhenkov · 2021-01-08T15:46:43Z

I think it can be done something like this: karzhenkov/json@22bd1e1

include/nlohmann/json.hpp

include/nlohmann/detail/diagnostics_t.hpp

doc/mkdocs/docs/features/macros.md

nlohmann · 2021-01-23T09:05:53Z

@gregmarr @karzhenkov I think this branch is ready now. Any further comment or observation?

doc/examples/diagnostics_extended.link

include/nlohmann/detail/diagnostics_t.hpp

include/nlohmann/detail/json_pointer.hpp

karzhenkov · 2021-01-24T10:17:03Z

Here are also some proposals to undo unnecessary changes and make the code a bit more concise: karzhenkov#1

Co-authored-by: Alexander Karzhenkov <[email protected]>

nlohmann · 2021-01-25T19:37:08Z

Here are also some proposals to undo unnecessary changes and make the code a bit more concise: karzhenkov#1

I took over some of your ideas: 74cc0ab. What do you think?

karzhenkov · 2021-01-26T07:18:40Z

It would also be nice to get rid of the empty json objects that are created in many places just to indicate "no diagnostics". This can be achieved with an overload of exception::diagnostics taking std::nullptr_t and with some defaults (BasicJsonType = std::nullptr_t; context = nullptr) here:

json/include/nlohmann/detail/exceptions.hpp

Lines 187 to 188 in 74cc0ab

    
           template<typename BasicJsonType> 
        
           static parse_error create(int id_, const position_t& pos, const std::string& what_arg, const BasicJsonType& context)

Other similar function templates need the same defaults.

As a possible point of additional extension, we can also consider other types of context that may be appropriate if no relevant json object is available.

gregmarr

Ugh, started a review yesterday and forgot to submit it.

include/nlohmann/detail/exceptions.hpp

gregmarr · 2021-01-25T20:39:53Z

include/nlohmann/detail/input/binary_reader.hpp

@@ -137,7 +137,7 @@ class binary_reader
            if (JSON_HEDLEY_UNLIKELY(current != std::char_traits<char_type>::eof()))
            {
                return sax->parse_error(chars_read, get_token_string(),
-                                        parse_error::create(110, chars_read, exception_message(format, "expected end of input; last byte: 0x" + get_token_string(), "value")));
+                                        parse_error::create(110, chars_read, exception_message(format, "expected end of input; last byte: 0x" + get_token_string(), "value"), BasicJsonType()));


Can this just be {} instead of BasicJsonType()? (Not sure now what works in C++11 and what requires later versions.)

I'm assuming that you want to be explicit about not providing a json object here and so don't want a default parameter value.

I tried earlier to work with a default value, but I got an error that the type could not be derived. I'll give it another check later.

Ah, it's a templated parameter, yeah that makes it harder to have a default.

…ostics � Conflicts: � include/nlohmann/detail/input/parser.hpp � single_include/nlohmann/json.hpp

gregmarr · 2021-02-09T19:23:09Z

LGTM

🚧 better diagnostics

a4d491e

nlohmann self-assigned this Jan 1, 2021

This was referenced Jan 1, 2021

Add Key name to Exception #932

Closed

How to get position info or parser context with custom from_json() that may throw exceptions? #1508

Closed

nlohmann marked this pull request as draft January 1, 2021 16:37

nlohmann added 6 commits January 2, 2021 13:44

🚧 add diagnostics to exceptions

7b04786

🚧 add switch for diagnostics

ecaab32

🚧 fix preprocessor check

c6e7fa2

🚧 fix preprocessor check

09cd4ed

🚧 add tests

7323a8e

🚧 implement more parent relations

ec0b179

nlohmann commented Jan 2, 2021

View reviewed changes

test/src/unit-json_patch.cpp Outdated Show resolved Hide resolved

🐛 fix bug in move constructor

294fa34

gregmarr reviewed Jan 6, 2021

View reviewed changes

include/nlohmann/detail/conversions/from_json.hpp Outdated Show resolved Hide resolved

gregmarr reviewed Jan 6, 2021

View reviewed changes

include/nlohmann/json.hpp Outdated Show resolved Hide resolved

gregmarr reviewed Jan 6, 2021

View reviewed changes

include/nlohmann/json.hpp Outdated Show resolved Hide resolved

gregmarr reviewed Jan 6, 2021

View reviewed changes

include/nlohmann/json.hpp Outdated Show resolved Hide resolved

karzhenkov reviewed Jan 7, 2021

View reviewed changes

include/nlohmann/json.hpp Outdated Show resolved Hide resolved

include/nlohmann/json.hpp Outdated Show resolved Hide resolved

include/nlohmann/json.hpp Outdated Show resolved Hide resolved

include/nlohmann/json.hpp Outdated Show resolved Hide resolved

nlohmann added 6 commits January 8, 2021 10:20

Merge branch 'develop' of https://github.com/nlohmann/json into diagn…

7cdf34b

…ostics

👌 remove unnecessary assignment from destructor

ddc3bb1

👌 fix operator[]

0617bd2

👌 fix move constructor and move assignment

04a0a07

👌 fix operator[]

e4af1dd

👌 clean operator[]

d4a91b7

👌 fix constructor

43cd5c8

✅ improve coverage

3337968

gregmarr suggested changes Jan 19, 2021

View reviewed changes

include/nlohmann/json.hpp Outdated Show resolved Hide resolved

gregmarr reviewed Jan 19, 2021

View reviewed changes

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

gregmarr reviewed Jan 19, 2021

View reviewed changes

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

gregmarr reviewed Jan 19, 2021

View reviewed changes

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

nlohmann added 3 commits January 20, 2021 15:05

👌 addressed review comments

d6ff059

✅ improve coverage

51ac600

📝 update documentation

d00ad33

gregmarr reviewed Jan 22, 2021

View reviewed changes

doc/mkdocs/docs/features/macros.md Show resolved Hide resolved

📝 update documentation

7b7da08

nlohmann marked this pull request as ready for review January 23, 2021 09:05

nlohmann added the release item: ⚡ improvement label Jan 23, 2021

nlohmann added this to the Release 3.9.2 milestone Jan 23, 2021

karzhenkov suggested changes Jan 23, 2021

View reviewed changes

doc/examples/diagnostics_extended.link Show resolved Hide resolved

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

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

🐛 fix bug in diagnostics_t

380a613

karzhenkov reviewed Jan 24, 2021

View reviewed changes

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

nlohmann and others added 3 commits January 24, 2021 17:45

👌 apply suggestion

c190a72

Co-authored-by: Alexander Karzhenkov <[email protected]>

✅ add test

e8dba10

♻️ remove diagnostics_t class

74cc0ab

gregmarr reviewed Jan 26, 2021

View reviewed changes

nlohmann added 3 commits January 26, 2021 17:49

👌 remove unused template parameter

524eea5

Merge branch 'develop' of https://github.com/nlohmann/json into diagn…

4917e7c

…ostics � Conflicts: � include/nlohmann/detail/input/parser.hpp � single_include/nlohmann/json.hpp

🔀 merge develop branch

56a6dec

nlohmann merged commit 176d8e2 into develop Feb 10, 2021

nlohmann deleted the diagnostics branch February 10, 2021 06:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better diagnostics #2562

Better diagnostics #2562

nlohmann commented Jan 1, 2021 •

edited

Loading

coveralls commented Jan 1, 2021 •

edited

Loading

nlohmann commented Jan 7, 2021

karzhenkov commented Jan 8, 2021

nlohmann commented Jan 8, 2021

karzhenkov commented Jan 8, 2021 •

edited

Loading

nlohmann commented Jan 23, 2021

karzhenkov commented Jan 24, 2021

nlohmann commented Jan 25, 2021 •

edited

Loading

karzhenkov commented Jan 26, 2021 •

edited

Loading

gregmarr left a comment

gregmarr Jan 25, 2021

gregmarr Jan 25, 2021

nlohmann Jan 26, 2021

gregmarr Jan 26, 2021

gregmarr commented Feb 9, 2021

Better diagnostics #2562

Better diagnostics #2562

Conversation

nlohmann commented Jan 1, 2021 • edited Loading

coveralls commented Jan 1, 2021 • edited Loading

nlohmann commented Jan 7, 2021

karzhenkov commented Jan 8, 2021

nlohmann commented Jan 8, 2021

karzhenkov commented Jan 8, 2021 • edited Loading

nlohmann commented Jan 23, 2021

karzhenkov commented Jan 24, 2021

nlohmann commented Jan 25, 2021 • edited Loading

karzhenkov commented Jan 26, 2021 • edited Loading

gregmarr left a comment

Choose a reason for hiding this comment

gregmarr Jan 25, 2021

Choose a reason for hiding this comment

gregmarr Jan 25, 2021

Choose a reason for hiding this comment

nlohmann Jan 26, 2021

Choose a reason for hiding this comment

gregmarr Jan 26, 2021

Choose a reason for hiding this comment

gregmarr commented Feb 9, 2021

nlohmann commented Jan 1, 2021 •

edited

Loading

coveralls commented Jan 1, 2021 •

edited

Loading

karzhenkov commented Jan 8, 2021 •

edited

Loading

nlohmann commented Jan 25, 2021 •

edited

Loading

karzhenkov commented Jan 26, 2021 •

edited

Loading