Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove basic_json proxy type #315

Open
danielaparker opened this issue Apr 5, 2021 · 1 comment
Open

Remove basic_json proxy type #315

danielaparker opened this issue Apr 5, 2021 · 1 comment

Comments

@danielaparker
Copy link
Owner

danielaparker commented Apr 5, 2021

From the beginnings of this library in 2013, influenced by the proxy idiom described in Scott Meyers' book Effective Modern C++, the non const basic_json::operator[](const string_view_type&) returns a proxy instead of a basic_json&,

proxy_type operator[](const string_view_type& key)

The proxy allows the implementation to distinguish between assignment, e.g.

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

and access, e.g.

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

and throw an exception if attempting to access "foo" if "foo" is not present, similar to Python dictionary. That seemed to me in 2013 to be a good feature to have. This behavior is the same as that currently defined for the const overload

const basic_json& operator[](const string_view_type& key) const

(where assignment is an error, so no need for a proxy).

However, there are some problems.

  • Proxies don't work well with auto.
  • auto& val = j[0] is okay with a json array and returns a json reference as expected, but auto& val = j["foo"] is not okay with a json object because it returns a reference to a temporary proxy type. This will result in a compile time warning, for the temporary, but not an error. Instead it needs to be written explicitly as json& val = j["foo"].
  • auto val = j["foo"] will result in a compile time error, because the proxy copy and move constructors and assignment operators have been made private. But having private proxy assignment operators causes the issue discussed in Is it possible to have a source json where you copy a subkey to a new json? #312.
  • There is a 2017 proposal P0672R0 to solve the problem of proxies and auto, but it doesn't seem to have much traction.
  • The implementation of the proxy class is currently about 1000 lines of code, including deprecated functions.
  • It's different from the behavior of std::map<Key,T>, which will insert (key,T()) if key does not exist. std::map<Key,T> doesn't have a const overload of operator[]. Instead it has throwing const and non-const overloads of at, which may be used for access or replacement if the key already exists (jsoncons also supports at.)
  • It's different from other json libraries, which stay consistent with std::map<Key,T> for the non-const overload of operator[].

For these reasons, I'm considering removing the proxy, i.e. removing 1000 lines of code and changing to

basic_json& operator[](const string_view_type& key)

where, when used as an accessor, operator[] will insert (key,json()) if key does not exist. Note that json() is an empty object.

In terms of impact, there should be no impact for code on the "good path". In the absence of errors, all code should work as before. I've ran the tests with this change on a branch, and all tests pass except tests that are testing behavior with keys that don't exist.

But in the case of attempting to access a key that does not exist, the behavior will change from throwing to inserting (key,json()).

No change is proposed for the const overload operator[](const string_view_type& key) const. It will continue to throw.

Comments welcome.

@danielaparker danielaparker pinned this issue Apr 8, 2021
@danielaparker danielaparker unpinned this issue Feb 7, 2024
@danielaparker danielaparker pinned this issue Nov 1, 2024
@danielaparker
Copy link
Owner Author

danielaparker commented Nov 1, 2024

After going back and forth on this, we've decided that having the non-const overload operator[](const string_view_type&) return a proxy was a bad idea, and this will be removed in a future release.

The new behavior for the non-const overload of operator[](const string_view_type&) will follow the standard library std::map behavior, that is, return a reference to the value that is associated with the key, inserting a default constructed value with the key if no such key already exists.

This raises an issue regarding consistency with the const overload. In the current implementation with proxy, both const and non-const overloads throw when attempting to access a key that does not exist. It would be undesirable for one version to throw and the other not. We want

const jsoncons::json& j1 = j["key"]; 

to behave the same regardless if j is const or non-const. The standard library does not provide much guidance here, because std::map does not support a const overload of operator[].

Our solution is to make both overloads non-throwing (provided they're applied to a JSON object.) The new behavior for the const overload of operator[](const string_view_type&) will be to return a const reference to the value that is associated with key, returning a const reference to a default constructed value with static storage duration if no such key already exists.

@danielaparker danielaparker changed the title Does returning a proxy make basic_json::operator[] too complicated? Remove basic_json proxy Nov 2, 2024
@danielaparker danielaparker changed the title Remove basic_json proxy Remove basic_json proxy type Nov 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant