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

Inconsisten operator[] #4458

Open
2 tasks done
fekir opened this issue Sep 13, 2024 · 2 comments
Open
2 tasks done

Inconsisten operator[] #4458

fekir opened this issue Sep 13, 2024 · 2 comments
Labels
kind: bug solution: wontfix the issue will not be fixed (either it is impossible or deemed out of scope)

Comments

@fekir
Copy link

fekir commented Sep 13, 2024

Description

Operator [] behaves inconsistentnly, depending if the const overloads is picked or not

It should be considered a bug because const/mutable overloads (especially for operator[]) changing the meaning of code is problematic.

As examples:

  • std::vector, array, std::array have overloads for operator[] that behaves the same for const and mutable; in all variants, access to the underlying object is provided (otherwise UB). member function at, which acts like a checked operator[] throws if there is no object.
  • std::map has mutable operator[], but no const operator[]. While it is not nice, at least it causes an inconsistency at compile-time, and not runtime

Reproduction steps

void foo(nlohmann::json& j){
  const auto& subobject = j["example"]; // does not throw
}

vs

void foo(const nlohmann::json& j){
  const auto& subobject = j["example"]; // throws if "example" not present
}

Expected vs. actual results

Expected result would that either both variants throw if the object does not exist or none throws.

Since the pattern

nlohmann::json j = nlohmann::json::object();
j["example"] = ...;

is common, it would be problematic for operator[] to throw if someone want to add an object.

Thus the most sensible approach would be to have operator[] not throwing in both cases.

Currently I'm using

const auto& j = json.contains( "example" ) ? json["example"] : nlohmann::json::object();

as workaround, this snippet behaves consistently with const and mutable objects.
The main drawbacks are

  • needs to remember to use this pattern (with ternary operator in order to take advantage of lifetime extension to avoid copies from operator[])
  • duplicate the objects it want to search (once for contains, and once for operator[])
  • create a local nlohmann::json::object()

This pattern could be integrate in the const version of operator[].
If the library defines a global const nlohmann::json::object(), then operator[] (const overload) could return the value in the object, or, if nothing found, the global object.
Since it is constant, it should not be problematic that different json objects return a reference to the same object.

With this change, operator[] would provide a more consistent behavior.

nlohmann::json j = nlohmann::json::object();
j["example"];

j["example"] returns an empty object for both the mutable and const overload.

j["example"] = ... compiles only for a mutable object (like currently)

The end-user of the library cannot distinguish (unless it compares the addresses) if the object already exists, has been created new, or is the global empty object.

Minimal code example

// https://godbolt.org/z/cM8cdeGsq
#include "nlohmann/json.hpp"

int main(){
    {
        nlohmann::json j = nlohmann::json::object();
        j["example"]; // does not throw
    }
    {
        const nlohmann::json j = nlohmann::json::object();
        j["example"]; // throws
    }
}

Error messages

No response

Compiler and operating system

gcc14.2 on linux and other systems

Library version

trunk, 3.11.1

Validation

@nlohmann
Copy link
Owner

The behavior is documented here: https://json.nlohmann.me/features/element_access/unchecked_access/

You may consider it inconsistent, but it is the same since version 1.0.0 of the library. Changing it now would break existing code which is something I don't intent to do.

@nlohmann nlohmann added the solution: wontfix the issue will not be fixed (either it is impossible or deemed out of scope) label Sep 13, 2024
@fekir
Copy link
Author

fekir commented Sep 13, 2024

@nlohmann I did not want to imply that it was not documented

Do you see any possible improvement to my workaround?
Maybe have another accessor that behaves similarly to what I've described? (maybe it's already there and I've overlooked it?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug solution: wontfix the issue will not be fixed (either it is impossible or deemed out of scope)
Projects
None yet
Development

No branches or pull requests

2 participants