Enable string_view keys in operator[] by updating is_usable_as_basic_json_key_type (#3912)

[khloodelhossiny]

This pull request adds std::string_view support for operator[] to improve usability with modern C++ (C++17 and later). The behavior now matches the existing std::string overloads. Additionally, the exception messages were updated for consistency when operator[] is used with non-object types and a string_view key.

This change addresses and fixes issue #3912.

• The changes are described in detail, both the what and why.
• An existing issue is referenced (Fixes inconsistency with string/string_view lookups #3912).
• The Code coverage remains at 100%.. A test case for every new line of code.
• If applicable, the documentation is updated.
• The source code is amalgamated by running make amalgamate.

Read the Contribution Guidelines for detailed information.

[gregmarr]

string_view support already exists through this function:

    /// @brief access specified object element
    /// @sa https://json.nlohmann.me/api/basic_json/operator%5B%5D/
    template<class KeyType, detail::enable_if_t<
                 detail::is_usable_as_basic_json_key_type<basic_json_t, KeyType>::value, int > = 0 >
    reference operator[](KeyType && key)

#3423

As you can see from the godbolt link in the original issue, the issue isn't that string_view doesn't work, it's that the compiler doesn't pick up that the operator string_view() from the type should allow it to work.

The thing that should really be updated is is_usable_as_basic_json_key_type, and then all of the existing functions will work. I think it currently asks "can this type be used as a key" when this scenario needs it to ask "can this type be converted to a type that can be used as a key".

You can update the test code like this to see the missing piece.

	std::cout << js[static_cast<std::string_view>(Bar{})] << "\n";  // Okay
	std::cout << js[Bar{}] << "\n"; // Not okay

[khloodelhossiny]

@gregmarr
Yes, I understand that string_view is supported through the existing template function, and the issue arises with js[Bar{}] due to ambiguity between the two overloads, causing the compiler to fail.

My idea is to add specific string_view overloads for operator[] to guide the compiler in choosing the correct one, resolving the ambiguity.

[github-actions]

🔴 Amalgamation check failed! 🔴

The source code has not been amalgamated. @khloodelhossiny
Please read and follow the Contribution Guidelines.

[gregmarr]

My idea is to add specific string_view overloads for operator[] to guide the compiler in choosing the correct one, resolving the ambiguity.

Okay, that's not what your PR description says, so it wasn't clear that you knew that.

I'd be worried about the compiler potentially having a problem selecting between operator[](KeyType && key) and operator[](std::string_view key) and there also being unnecessary duplication. I still think that making is_usable_as_basic_json_key_type work with it is a better route here, but @nlohmann may be okay with the new functions.

[nlohmann]

I think a cleaner approach would be indeed to fix is_usable_as_basic_json_key_type to also work with string views.

[khloodelhossiny]

My idea is to add specific string_view overloads for operator[] to guide the compiler in choosing the correct one, resolving the ambiguity.

Okay, that's not what your PR description says, so it wasn't clear that you knew that.

I'd be worried about the compiler potentially having a problem selecting between operator[](KeyType && key) and operator[](std::string_view key) and there also being unnecessary duplication. I still think that making is_usable_as_basic_json_key_type work with it is a better route here, but @nlohmann may be okay with the new functions.

Thanks a lot for the explanation. I see your point, and I’ll update the code to follow that approach.

[khloodelhossiny]

I think a cleaner approach would be indeed to fix is_usable_as_basic_json_key_type to also work with string views.

Thanks for the feedback! Yes, that makes sense — I will update the code.

[github-actions]

🔴 Amalgamation check failed! 🔴

The source code has not been amalgamated. @khloodelhossiny
Please read and follow the Contribution Guidelines.

[coveralls]

coverage: 99.191%. remained the same
when pulling 1d8ca15 on khloodelhossiny:fix_issue-3912
into c8b66cf on nlohmann:develop.

[nlohmann]

Please resolve the conflicts.

[github-actions]

🔴 Amalgamation check failed! 🔴

The source code has not been amalgamated. @khloodelhossiny
Please read and follow the Contribution Guidelines.

[github-actions]

🔴 Amalgamation check failed! 🔴

The source code has not been amalgamated. @khloodelhossiny
Please read and follow the Contribution Guidelines.

[gregmarr]

I was hoping for something that didn't have to name string_view directly, but I'm not sure that's possible.

Another possibility that seems to work and doesn't require any changes to the library is to make your type comparable to std::string.

struct Bar {
	operator std::string_view() const { return "bar"; }
};
bool operator<(const std::string &lhs, Bar const &rhs) {
    return lhs < static_cast<std::string_view>(rhs);
}

It could be a bit more generic, but that might be dangerous in the rest of the program:

template<typename T>
bool operator<(const std::string &lhs, T &&rhs) {
    return lhs < static_cast<std::string_view>(rhs);
}
template<typename T>
bool operator<(T&&lhs, const std::string &rhs) {
    return static_cast<std::string_view>(lhs) < rhs;
}

[khloodelhossiny]

I was hoping for something that didn't have to name string_view directly, but I'm not sure that's possible.

Another possibility that seems to work and doesn't require any changes to the library is to make your type comparable to std::string.

struct Bar {
	operator std::string_view() const { return "bar"; }
};
bool operator<(const std::string &lhs, Bar const &rhs) {
    return lhs < static_cast<std::string_view>(rhs);
}

It could be a bit more generic, but that might be dangerous in the rest of the program:

template<typename T>
bool operator<(const std::string &lhs, T &&rhs) {
    return lhs < static_cast<std::string_view>(rhs);
}
template<typename T>
bool operator<(T&&lhs, const std::string &rhs) {
    return static_cast<std::string_view>(lhs) < rhs;
}

I got your point — I understand that you want the library to fully support std::string_view.
For example, this code:

struct TestStruct {
    operator std::string_view() const { return "key"; }
};
std::cout << j.contains(TestStruct{}) << "\n";

I tried to handle that case, but I think for now it might make sense to let users define their own comparison operators if they have custom types.
At least we’ve fixed the issue that many people usually miss.
I also agree that in the future, we should aim to support full std::string_view handling as a proper feature of the library.

[khloodelhossiny]

@gregmarr
Can you help me? I’m having some trouble with the CI checks.
Every time I push, I get these errors:

• Ubuntu / ci_static_analysis_clang (ci_test_clang)
• Ubuntu / ci_test_standards_clang (17, libstdcxx)
• Windows / mingw (x64)

I tried to fix them several times, but after I push new changes, other errors appear.

I believe most of them are related to clang-format.
When I run the formatting command locally, it changes macros that should stay on one line into multiple lines, which then causes conflicts.

My current steps are:

1. Edit the two files
2. Run the tests
3. Run make amalgamate
4. Push

If I’m missing any step, please let me know — I’m starting to get a bit disappointed :(

At this point, I almost believe that even if I push the develop branch, it will still fail the same checks :)

[gregmarr]

@nlohmann will have to help you with the checks.

I also agree that in the future, we should aim to support full std::string_view handling as a proper feature of the library.

IMO, the library supports std::string_view as a proper feature already. The issue here is that the object isn't directly usable as a key but requires an intermediate conversion to a type that is usable as a key.

[nlohmann]

• ci_static_analysis_clang and ci_test_standards_clang (17, libstdcxx) complain about a missing new line at the end of a file
• The MinGW job looks like a glitch - I restarted it.
• Please take a look at https://github.com/nlohmann/json/pull/4958/checks?check_run_id=54217855538

[khloodelhossiny]

IMO, the library supports std::string_view as a proper feature already. The issue here is that the object isn't directly usable as a key but requires an intermediate conversion to a type that is usable as a key.

Got it, thank you so much for the clarification and guidance!
What I actually meant was that it doesn’t fully support types that can be implicitly converted to std::string_view.
But yes, I’ll try to find the best solution for that.

[khloodelhossiny]

@nlohmann Many thanks to you!