Assertion failed when accessing non-existing object with const json object about json HOT 12 CLOSED

Fine, but I still deeply believe this is a bad design choice.

from json.

The issue is the const. See https://json.nlohmann.me/api/basic_json/operator%5B%5D/#notes

from json.

This may be a stupid question but I'll ask it anyway: why not raise an exception instead of using assert() ?

from json.

See https://json.nlohmann.me/features/element_access/unchecked_access/ design rationale

from json.

It seems to me to be a design choice to have this undefined behavior.
I still don't get why you don't throw an exception in const_reference operator[](const typename object_t::key_type& key) const when the item is not found.
For the non-const version I understand why you need to insert a null item that can be later filled. But here with the const version you have a way to detect that the item is missing and can throw an exception which would remove the undefined behavior.
Can you tell me more on this design choice ?

from json.

For the non-const version I understand why you need to insert a null item that can be later filled. But here with the const version you have a way to detect that the item is missing and can throw an exception which would remove the undefined behavior.

As the caller, you have the ability to detect whether or not the item is there before calling operator[]. Thus, users that don't need to check, because they've already checked or they know that their data is good and the item is there, don't incur the overhead of checking again in operator[]. If you have a const object, and you don't know if the key exists, don't use operator[]. This is no different than checking that your index is in range before using operator[] on std::vector.

There is no operator[](key) const on std::map, so this is purely a helper function over what is provided by the standard library.

from json.

I have indeed the ability to check for each element presence, but being able to process a whole message with one try/catch and having a single error management in the catch would result in far more clear code than testing the existence of each element and having an error management for each one.

The non-const version of operator[] is robust to a missing element, to me having the same for the const version makes sense (and it also removes an undefined behavior in the library). Also I don't bother the implied small overhead.

Concerning the std::vector example, to me a list a special case where you can iterate over each element. With dictionaries you have to look for a particular element and nothing guarantees that an element will be present which lead into a lot of sanity check code.

from json.

I have indeed the ability to check for each element presence, but being able to process a whole message with one try/catch and having a single error management in the catch would result in far more clear code than testing the existence of each element and having an error management for each one.

If you don't have error management for each one, then attempting to read a non-existing element aborts the whole operation. If that's what you want, then this would give you the same behavior you are seeking from operator[] const.

auto GetChild(const nlohmann::json &j, const std::string &name)
{
    if (!j.contains(name)) { throw "error"; }
    return j[name];
}


    try
    {
        const json foo = ex["foo"];
        const std::string bar = GetChild(foo, "bar"); // <= access to existing item
        const std::string baar = GetChild(foo, "baar"); // <= access to non-existing item
        const std::string baaar = GetChild(foo, "baaar"); // <= access to non-existing item
    }
    catch(const std::exception& e)
    {
        std::cerr << " > exception 2: " << e.what() << std::endl;
    }

If that's NOT what you want, and you just want a default value when the child isn't there, you should just use the .value() function, which takes the name of the child and a default value to use when the child isn't there.

    try
    {
        const json foo = ex["foo"];
        const std::string bar = foo.value("bar", ""); // <= access to existing item
        const std::string baar = foo.value("baar", ""); // <= access to non-existing item
        const std::string baaar = foo.value("baaar", ""); // <= access to non-existing item
    }
    catch(const std::exception& e)
    {
        std::cerr << " > exception 2: " << e.what() << std::endl;
    }

The non-const version of operator[] is robust to a missing element, to me having the same for the const version makes sense (and it also removes an undefined behavior in the library).

Removing the non-const version would match std::map and also remove the undefined behavior. As would removing the const from your object, though that would mean that suddenly reading the data could modify it, which is likely not what you want.

Also I don't bother the implied small overhead.

Others have different priorities. With the current behavior, you have the ability to add that overhead if you want/need it. Adding the check means that others that don't want the overhead can't avoid it.

Concerning the std::vector example, to me a list a special case where you can iterate over each element. With dictionaries you have to look for a particular element and nothing guarantees that an element will be present which lead into a lot of sanity check code.

That is absolutely not true, you can iterate over the dictionary just as well.

from json.

I think this whole discussion is about priorities.

To me having an undefined behavior by just adding a const to my json object is a real problem. It means that my object behaves differently when I read its content, the object being const or not. And it's not a small difference, in one case my application will throw an exception and in the other it will either stops abruptly (with the assert) or behave in an undefined manner.

You will probably tell me that there is no issue since this is documented, but for a library which is intended to be largely used I believe it is an issue.
In the STL the std::map::operator[] is only available for non-const objects and std::map::at() throws an exception if the item doesn't exists. I believe this is a safe design choice, you get no surprise during execution if you add a const to your object.

from json.

Well, the design decision is documented, and this is the behavior of the library since day 1. I can understand your point, but using unchecked access for a non-existing key is a problem in itself. The library offers (and documents) several APIs how to cope with this.

And what do you expect when you access j["foo"] for a const json object without key foo?

• An exception? Use at.
• A default value? Use value.
• ???

from json.

If all cases are already covered with at() and value(), why propose operator[] for const objects ?
Probably because one day you thought it would be nicer to use it compared to at(). If it's the case then I think it should have been done all the way handling non-existing key in a defined manner.

I'm insisting maybe too much on this, but I'm used to MISRA coding rules which require variables to be declared as const if you don't modify them. In all the projects I've worked on, adding a const wouldn't change the behavior. Here I see situations where people would add a const to comply with coding rules having no idea their program would now crash if it receives a json frame with parameters missing or malformed.
Having no way to prevent it except from telling people to read properly the documentation scares me.

from json.

No: operator[] is for unchecked access. Don't pay for what you don't use.

from json.