Horribly inconsistent behavior between const/non-const reference in operator [] ()

[kawing-chiu]

Things like

std::string tag;
try {
    tag = json["tag"];
} catch (const std::domain_error &e) {
    //  handle it
}

works nicely and as expected. However, if json is a const reference, the above code will fail by assertion. Why is it designed like so? Shouldn't it be better and more consistent to at least throw a domain_error instead of fail by assertion? It took me more than one hour to debug this issue when I pass the above json by const reference to another function to extract data...

[nlohmann]

Neither version of operator[](key) checks if the given key exists and only throw std::domain_error when called on a JSON value that is not of type object. Therefore, your code will - in both cases - only catch exceptions indicating that json is not a JSON object. The behavior differs in case key is not stored in the object:

• The nonconst version (1) silently converts a null value to an empty object, and (2) creates a null value at key if it does not exist before it returns a reference to the value at key. This reference is always valid, because of step (2): there will always be a value at key.
• The const version returns a const reference to the value at key. If there is no value at key, we can neither alter the object nor return a "null reference". The behavior is undefined in this situation (basically, the code is equivalent to return *object.find(key);). An assertion checks this.

I though about the semantics for quite a while as there is no similar operator[] for std::map returning a const reference. However, there is at for checked access, so operator[] is for the unchecked access. Checking whether key exists in the const version would result in overhead which would contradict the "not pay what you don't use" principle.

I would be happy for ideas how to improve this situation.

[Dka8]

Hello!
I think I have a solution how to

at least throw a domain_error

In that metod (string 3616):

template<typename T>
const_reference operator[](T* key) const;

We should simply change this:

assert(m_value.object != nullptr);
assert(m_value.object->find(key) != m_value.object->end());
return m_value.object->(key)->second;

to something like this:

assert(m_value.object != nullptr);
if(m_value.object->find(key) == m_value.object->end()) {
        throw std::domain_error("something");
}
return m_value.object->find(key)->second;

It works pretty nice for me :-)

[gregmarr]

That's exactly what nlohmann said he doesn't want to do

Checking whether key exists in the const version would result in overhead which would contradict the "not pay what you don't use" principle.

[kawing-chiu]

Well, I think this problem is not too hard, but it might require some API change. To see what is the appropriate behavior here, let's learn from how others do it.

Since JSON originates from and is heavily used by high-level scripting languages, we consider Python and Javascript's handling of json here (these two languages even have json functionalities built into the language, and these functionalites work intimately with built-in data structures).

[Javascript]

Here is the behavior of Javascript object, which maps to JSON objects:

• Reading. Reading a non-exist key does not throw exception, but returns undefined. But nothing is inserted into the object when reading:

  >> o = {}
  Object {}
  >> o.x
  undefined
  >> o
  Object {}
  

• Writing. Writing to non-exist key works as expected:

  >> o.x = {123: '321'}
  Object {123: "321"}
  >> o.y = null
  null
  >> o
  Object {x: Object, y: null}
  

• Note that Javascript has two different built-in types: undefined and null. undefined means variable does not exist. null maps to JSON null.

[Python]

Here is the behavior of Python dict, which maps to JSON objects:

• Reading. Reading a non-exist key throws KeyError exception, and the dict itself is of course not touched:

  In [1]: d = {}
  
  In [2]: d['x']
  ---------------------------------------------------------------------------
  KeyError                                  Traceback (most recent call last)
  <ipython-input-2-58142e8c3daa> in <module>()
  ----> 1 d['x']
  
  KeyError: 'x'
  
  In [3]: d
  Out[3]: {}
  

• Writing. Writing to non-exist key works as expected:

  In [3]: d
  Out[3]: {}
  
  In [4]: d['x'] = {'10': 100}
  
  In [5]: d['y'] = None
  
  In [6]: d
  Out[6]: {'x': {'10': 100}, 'y': None}
  

• Python does not have equivalent of Javascript's undefined. Python's None type maps to JSON null.
  Note that on reading, both languages don't touch the object at all.

Now return to the C++ world. I think these two principles are worth following:

• Const and non-const behavior should definitely match.
• A production quality library should really get rid of assertions. See the C++ standard library for an example. Grep assert in the source of clang's libcxx and you will find that assertions are only used when exception is not available or very sparingly for internal debug purpose.

So from the above discussions, IMHO the best solution is to mimic Python's behavior, which is well tested, widely used and already familiar by many programmers. There's really no need to invent a new behavior. We can just define some exceptions like key_error or something. (In Python's case, KeyError is a built-in standard exception. But there's no reason that we must stick to built-in exceptions. For example, <future> defines a bunch of exceptions for use by std::future.) The reasons for using an exception on reading a non-exist key include:

• Reading a non-exist key is indeed an exceptional case. Which maps well to exception conceptually.
• Using exception avoids the overhead of doing unnecessary tests. JSON is a highly dynamic data format. A common use case of a json library is on a server that processes high-throughput json requests from the outside world. In this case, 99.99% percent of the requests are just valid. Every field exists as expected. The remaining 0.01% are some malformed jsons, maybe just several out of tens of thousands a day. So we certainly don't want to test every field for existence and type conformance for the 99.99% requests. And this is the exact reason that why I abandon rapid_json and come to nlohmann/json.
• Note that in the current status of the library, if assertion error/UB may occur, this amounts to force the user of the library to write all of the tests on all of the json fields. So we come to the point again: assertions should really be eliminate in favor of exceptions in any production-ready libraries.

So, to sum up, IMHO, I think we really should follow the behavior of Javascript or Python. I would just choose Python's. Or, we can implement both and provide a global config option to let the user choose which flavor to use.

[gregmarr]

avoids the overhead of doing unnecessary tests. JSON is a highly dynamic data format. A common use case of a json library is on a server that processes high-throughput json requests from the outside world. In this case, 99.99% percent of the requests are just valid. Every field exists as expected. The remaining 0.01% are some malformed jsons, maybe just several out of tens of thousands a day. So we certainly don't want to test every field for existence and type conformance for the 99.99% requests. And this is the exact reason that why I abandon rapid_json and come to nlohmann/json.

This is exactly why the test isn't there.

Note that in the current status of the library, if assertion error/UB may occur, this amounts to force the user of the library to write all of the tests on all of the json fields. So we come to the point again: assertions should really be eliminate in favor of exceptions in any production-ready libraries.

The assertions are removed when you build with NDEBUG, so there are no assertions when building for production. They're debugging aids. Therefore, ignore them.

If the user of the library needs to know whether or not the desired field is there because it's reading from a const object constructed from an unknown source, then it is up to the user of the library to do the test. Adding this test makes the 99.99% slower so that the 0.01% can be exactly the same speed, but with the test in the library instead of outside the library.

[gregmarr]

Const and non-const behavior should definitely match.

That is simply not possible. It would mean that you could not do this:

json j;
j["foo"] = "bar";

[kawing-chiu]

@gregmarr This is not what I mean. I mean 'the const part behavior' should match, of course. In our specific case, it means the behavior on reading.

Removing the assertion by such debug option is pointless, since what you get instead is undefined behavior, it's essentially the same. Failed with undefined behavior is even worse IMHO. Have you read through my post? I did not advocate adding tests at all. Where did you get that impression from?

[nlohmann]

@Dka8 The fix you describe is (more or less) the code of the at function which offers checked access to the object. The operator[] function skips this check, just as std::vector or std::map do.

[gregmarr]

I confused your post with dka8's post.
What exactly are you proposing then?

[nlohmann]

@gregmarr I propose to leave everything as is:

• at for checked access
• operator[] for unchecked access

[nlohmann]

@kawing-chiu I am aware how other languages cope with JSON, and the motivation for this project was "What if JSON was part of Modern C++?". Where possible, I tried to make JSON feel like a part of the STL and basically combined the interfaces of std::vector and std::map. Both offer with at checked access to the elements. std::vector also has operator[] for both const and nonconst access - in both cases without bounds checking. As std::map has no const operator[], I basically had three choices:

1. not to offer a const operator[]
2. copy the behavior of at and provide checked access
3. mimic the behavior of vector and provide unchecked access

Option 1 makes no sense. There a so many Stack Overflow threads about people being confused that they cannot use operator[] for const std::map that I did not follow this.

Option 2 makes more sense, but would always check for an element even if such a check is not needed. This, in my opinion, is not very C++ like.

Option 3 is documented as such. Just like dereferencing the past-the-end iterator or using an invalid vector index, the behavior is undefined. One could argue that calling find, checking whether it's end() and returning the reference if not does not really add overhead, but in the end it is a check that no one really asked for when using operator[] rather than at.

About assertions: I shall think about switching them off by default and only to switch them on with a special preprocessor symbol. However, the current code only use assertions in cases of otherwise undefined behavior.

[gregmarr]

There is no need for a special preprocessor symbol, assert comes with one as part of the standard, as I mentioned.

I agree with leaving things as is.

[nlohmann]

@gregmarr I agree. I think the main issue of @kawing-chiu is that they did not expect an assertion in this case. I do mention assertions in the README - I am not sure what I can do to avoid such confusions.

[kawing-chiu]

@nlohmann OK, now I see why. Actually, I think Option 1 is the best of three and it makes some sense. If we're to follow STL's lead, then just follow as close as possible...

Another possibility I can think of is to define subclass of std::logic_error, throw it and let the user know what happen instead of the assertion error.

Either way, I think there should be a section dedicated to it in the readme.