Issues around get and pointers

[quicknir]

I use nlohmann json quite extensively to de-serialize many types from json conveniently. I've written from_json specializations for quite a few standard library types and third party types, and recently I've come across a very awkward situation.

I'm currently working with the Howard Hinnant date library, which is also slated for inclusion in C++20 IIRC. One of the aspects of this library is that it's not really possible to work directly with timezone objects. The timezone objects themselves are stored in a singleton that is typically constructed at initialization time from reading the OS IANA database. Instead, you call const date::timezone* date::locate_zone(string_view); that is in user code you mostly work with pointers to const timezones instead.

So, I write code like this:

template <>
struct adl_serializer<const date::time_zone *>
{
    static const date::time_zone * from_json(const json & j)
    {
        auto s = j.get<std::string>();
        return date::locate_zone(s);
    }
};

However, the problem is that when I call j.get<const date::time_zone*>, I get a compiler error for ambiguity, with candidates:

/n/nvme/nir/venv-mosaic/json/3.1.0/include/nlohmann/json.hpp:2513:15: note: candidate: ValueType nlohmann::basic_json<ObjectType, ArrayType, StringType, BooleanType, NumberIntegerType, NumberUnsignedType, NumberFloatType, AllocatorType, JSONSerializer>::get() const [with ValueTypeCV = const date::time_zone*; ValueType = const date::time_zone*; typename std::enable_if<((! std::is_same<nlohmann::basic_json<ObjectType, ArrayType, StringType, BooleanType, NumberIntegerType, NumberUnsignedType, NumberFloatType, AllocatorType, JSONSerializer>, ValueType>::value) && nlohmann::detail::has_non_default_from_json<nlohmann::basic_json<ObjectType, ArrayType, StringType, BooleanType, NumberIntegerType, NumberUnsignedType, NumberFloatType, AllocatorType, JSONSerializer>, ValueType>::value), int>::type <anonymous> = 0; ObjectType = std::map; ArrayType = std::vector; StringType = std::__cxx11::basic_string<char>; BooleanType = bool; NumberIntegerType = long int; NumberUnsignedType = long unsigned int; NumberFloatType = double; AllocatorType = std::allocator; JSONSerializer = nlohmann::adl_serializer]
     ValueType get() const noexcept(noexcept(
               ^~~
/n/nvme/nir/venv-mosaic/json/3.1.0/include/nlohmann/json.hpp:2562:33: note: candidate: constexpr const PointerType nlohmann::basic_json<ObjectType, ArrayType, StringType, BooleanType, NumberIntegerType, NumberUnsignedType, NumberFloatType, AllocatorType, JSONSerializer>::get() const [with PointerType = const date::time_zone*; typename std::enable_if<std::is_pointer<_Ptr>::value, int>::type <anonymous> = 0; ObjectType = std::map; ArrayType = std::vector; StringType = std::__cxx11::basic_string<char>; BooleanType = bool; NumberIntegerType = long int; NumberUnsignedType = long unsigned int; NumberFloatType = double; AllocatorType = std::allocator; JSONSerializer = nlohmann::adl_serializer]
     constexpr const PointerType get() const noexcept

One rather ugly workaround is to write my own type, TimeZone, that internally holds such a pointer. I consider this ugly because I would just have to do conversions back and forth to actually work with date code and it's just introducing a new type for no reason.

I was wondering if there is a better workaround that I could use, or if I'm misusing something?

Based on what I've seen, I think it is wrong that get has specializations that behave differently for pointer types; it seems like a convenience (kinda) that breaks generic code; this behavior should be saved for get_ptr. But perhaps I've misunderstood the situation.

[quicknir]

I checked with version 3.6.1 as well, same issue.

Leaving aside for the moment the question of how you want get to behave in 4.0 (i.e. if you could make a breaking change), this problem may be solveable if these get overloads were sfinaed properly. The documentation for the get pointer overloads say:

pointer type; must be a pointer to array_t, object_t, string_t, boolean_t, number_integer_t, number_unsigned_t, or number_float_t.

Since this is a requirement that's already documented, it's not a breaking change to actually sfinae on this. Right now we have:

    template<typename PointerType, typename std::enable_if<
                 std::is_pointer<PointerType>::value, int>::type = 0>
    PointerType get() noexcept

If we change std::is_pointer to an appropriate trait (is_pointer_json_type) then I think things will work properly.

[nlohmann]

Indeed there seems to be too much magic in get to choose get_ptr when a pointer is given. If I remember correctly, this was added in times where we did not yet have the possibility to convert from/to arbitrary types. And until now, I have not heard of the situation in which a pointer was queried in a from_json function. Sorry about the inconvenience.

And you're right - adding a better type restriction should not only solve the issue, but would also not be a breaking change, as get_ptr was always documented to only work with library pointer types in the first place.

I'd be happy if you had a PR for this, but I can also fix this myself.

[quicknir]

No worries, thanks for your response. Sure, happy to push a patch, hopefully tomorrow. How long do you think until you'll want to do a release (minor/patch) that incorporates this?

[nlohmann]

I'm planning to make the next release (this 3.7.0) some time in July.

[quicknir]

I'm genuinely confused. Tried to add a unit test that shows the failure first, and i wasn't able to produce it. I tried adding the following to unit-udt.cpp:

namespace udt {
struct as_pointer
{
};

constexpr as_pointer global_udt;
}


namespace nlohmann {
template <>
struct adl_serializer<const udt::as_pointer*>
{
    static const udt::as_pointer* from_json(const json& j)
    {
        return &udt::global_udt;
    }
};
}

TEST_CASE("Issue #1238")
{
    json j;
    auto* p = j.get<const udt::as_pointer*>();
    CHECK(p == &udt::global_udt);
}

And everything compiles fine. I'm not quite sure what I'm missing... Will have to look at it again.

[quicknir]

Based on my error message and looking at the upstream source, maybe this has already been solved, because the get function went from a regular return type to a trailing return type which might give further sfinae opportunities... I'll look at this again tomorrow.

[nlohmann]

Did you have the chance to have a look at this?

[quicknir]

I think I was mistaken and this was fixed (indirectly) in later versions. From version 3.6.1, there is return type sfinae in these get functions which seems to solve the issue. The confusion was caused by my package manager using 3.1.0 even though I had selected 3.6.1 :-(. So I don't think any action is needed for now. In a 4.0 release though, I'd still suggest that get should not given any special treatment to pointers.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[nlohmann]

The internal type of numbers is std::int64_t, std::uint64_t, or double. Therefore, you cannot ask for an int*.