nlohmann / json

JSON for Modern C++
https://json.nlohmann.me
MIT License
41.33k stars 6.58k forks source link

gcc13.2 compile fail when using std::string to compare the json object #4165

Open zhangjian3032 opened 9 months ago

zhangjian3032 commented 9 months ago

Description

Kindly refer to this link: https://godbolt.org/z/c3fzjnqzG, okay with gcc 12.3 failed with gcc 13.1 and 13.2

Reproduction steps

refer to description

Expected vs. actual results

expect using gcc13.2 can be compiled successfully

    if (std::string("bar") == j["xxx"])
    {

    }

Minimal code example

#include <iostream>
#include <string>
#include <nlohmann/json.hpp>

int main()
{
    nlohmann::json j;

    if (std::string("bar") == j["xxx"])
    {

    }

    return 0;
}


### Error messages

_No response_

### Compiler and operating system

gcc12.3 and gcc13.1

### Library version

3.11.1

### Validation

- [ ] The bug also occurs if the latest version from the [`develop`](https://github.com/nlohmann/json/tree/develop) branch is used.
- [ ] I can successfully [compile and run the unit tests](https://github.com/nlohmann/json#execute-unit-tests).
gregmarr commented 9 months ago

This only happens with C++20 mode enabled, and appears to be a result of the updated C++ compare code. It works in one direction but not the other.

This fails

if (std::string("bar") == j["xxx"])

This is good

if (j["xxx"] == std::string("bar"))
colbychaskell commented 9 months ago

I can confirm I get the same behavior running on ubuntu 23.04 with gcc 13.2.

It looks like the reason why this only appears for c++20 is the template requirement for the operator==

template<typename ScalarType>
requires std::is_scalar_v<ScalarType>
bool operator==(ScalarType rhs) const noexcept
{
    return *this == basic_json(rhs);
}

If the 'requires' line is commented out it appears to use this version of the == operator for this case.

I believe the reason why it works when lhs and rhs are swapped is because of the operator value_t() definition which lets it work with the std::string == operator.

I'm not sure why this function isn't just setup to work with all compatible types which would work with the basic_json(rhs) conversion?

nlohmann commented 9 months ago

I did not write this code myself, and did not find the time to read into these operators. Help is more than welcome here!

fsandhei commented 9 months ago

Without having worked on the library details I cannot give good rationales, but I decided to try out changing the constraint so operator== is called for the compatible types, like the equivalent catch - all constructor.

@@ -3694,9 +3694,11 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec

     /// @brief comparison: equal
     /// @sa https://json.nlohmann.me/api/basic_json/operator_eq/
-    template<typename ScalarType>
-    requires std::is_scalar_v<ScalarType>
-    bool operator==(ScalarType rhs) const noexcept
+    template < typename CompatibleType,
+               typename U = detail::uncvref_t<CompatibleType>,
+               detail::enable_if_t <
+                   !detail::is_basic_json<U>::value && detail::is_compatible_type<basic_json_t, U>::value, int > = 0 >
+    bool operator==(CompatibleType rhs) const noexcept
     {
         return *this == basic_json(rhs);
     }

The minimal example compiles (with GCC 13.2.1 on Arch) and the tests pass on develop.

There might be some details that I'm missing here, like unforeseen effects of this, so any input is appreciated. If this is okay I'll gladly make a PR for it.

schummLee commented 8 months ago

thats becauseIn GCC 13.1 and 13.2, stricter checks might be in place that catch potential issues earlier. It could be related to the handling of non-existing keys in a nlohmann::json object. Accessing a non-existing key with the operator[] in nlohmann::json returns a nullptr, which may not have a direct comparison overload with std::string in later versions of the library or compiler, I ALSO HAV THE CODE REFER TO THIS

if u using .find(),this error wont occur in here
auto it = j.find("xxx"); if (it != j.end() && *it == std::string("bar")) {

} if (j["xxx"].is_string() && std::string("bar") == j["xxx"].get()) {

}

schummLee commented 8 months ago

I can confirm I get the same behavior running on ubuntu 23.04 with gcc 13.2.

It looks like the reason why this only appears for c++20 is the template requirement for the operator==

template<typename ScalarType>
requires std::is_scalar_v<ScalarType>
bool operator==(ScalarType rhs) const noexcept
{
    return *this == basic_json(rhs);
}

If the 'requires' line is commented out it appears to use this version of the == operator for this case.

I believe the reason why it works when lhs and rhs are swapped is because of the operator value_t() definition which lets it work with the std::string == operator.

I'm not sure why this function isn't just setup to work with all compatible types which would work with the basic_json(rhs) conversion?

When you write j["xxx"] == std::string("bar"), the operator== from the nlohmann::json library gets invoked, but due to the constrained template, it doesn't find a suitable match for a non-scalar right-hand side. This is different from std::string("bar") == j["xxx"], where the operator== for std::string comes into play, and the nlohmann::json object gets implicitly converted to std::string (if it can be), making the comparison possible.