Bug: returning reference to local temporary object

[nlohmann]

Clang reports the following warning:

../single_include/nlohmann/json.hpp:18265:20: error: returning reference to local temporary object [-Werror,-Wreturn-stack-address]
            return *ptr;
                   ^~~~

Context:

template<typename ReferenceType, typename ThisType>
static ReferenceType get_ref_impl(ThisType& obj)
{
    // delegate the call to get_ptr<>()
    auto ptr = obj.template get_ptr<typename std::add_pointer<ReferenceType>::type>();

    if (JSON_HEDLEY_LIKELY(ptr != nullptr))
    {
        return *ptr;          // <----- warning
    }

    JSON_THROW(type_error::create(303, "incompatible ReferenceType for get_ref, actual type is " + std::string(obj.type_name())));
}

[nlohmann]

More context:

In file included from ../test/src/unit-udt.cpp:32:
../include/nlohmann/json.hpp:2779:20: warning: returning reference to local temporary object [-Wreturn-stack-address]
            return *ptr;
                   ^~~~
../include/nlohmann/json.hpp:3136:16: note: in instantiation of function template specialization 'nlohmann::basic_json<std::map, std::vector, std::__1::basic_string<char>, bool, long long, unsigned long long, double, std::allocator, adl_serializer, std::__1::vector<unsigned char, std::__1::allocator<unsigned char> > >::get_ref_impl<const nlohmann::basic_json<std::map, std::vector, std::__1::basic_string<char>, bool, long long, unsigned long long, double, std::allocator, adl_serializer, std::__1::vector<unsigned char, std::__1::allocator<unsigned char> > >::internal_binary_t &, const nlohmann::basic_json<std::map, std::vector, std::__1::basic_string<char>, bool, long long, unsigned long long, double, std::allocator, adl_serializer, std::__1::vector<unsigned char, std::__1::allocator<unsigned char> > > >' requested here
        return get_ref_impl<ReferenceType>(*this);
               ^
../include/nlohmann/json.hpp:1490:77: note: in instantiation of function template specialization 'nlohmann::basic_json<std::map, std::vector, std::__1::basic_string<char>, bool, long long, unsigned long long, double, std::allocator, adl_serializer, std::__1::vector<unsigned char, std::__1::allocator<unsigned char> > >::get_ref<const nlohmann::basic_json<std::map, std::vector, std::__1::basic_string<char>, bool, long long, unsigned long long, double, std::allocator, adl_serializer, std::__1::vector<unsigned char, std::__1::allocator<unsigned char> > >::internal_binary_t &, 0>' requested here
                JSONSerializer<other_binary_t>::to_json(*this, val.template get_ref<const other_binary_t&>());
                                                                            ^
../test/src/unit-udt.cpp:714:26: note: in instantiation of function template specialization 'nlohmann::basic_json<std::map, std::vector, std::__1::basic_string<char>, bool, long long, unsigned long long, double, std::allocator, another_adl_serializer, std::__1::vector<unsigned char, std::__1::allocator<unsigned char> > >::basic_json<nlohmann::basic_json<std::map, std::vector, std::__1::basic_string<char>, bool, long long, unsigned long long, double, std::allocator, adl_serializer, std::__1::vector<unsigned char, std::__1::allocator<unsigned char> > >, 0>' requested here
        custom_json cj = j;
                         ^

and

In file included from ../test/src/unit-regression.cpp:37:
../include/nlohmann/json.hpp:2779:20: warning: returning reference to local temporary object [-Wreturn-stack-address]
            return *ptr;
                   ^~~~
../include/nlohmann/json.hpp:3136:16: note: in instantiation of function template specialization 'nlohmann::basic_json<std::map, std::vector, std::__1::basic_string<char>, bool, long long, unsigned long long, double, std::allocator, adl_serializer, std::__1::vector<unsigned char, std::__1::allocator<unsigned char> > >::get_ref_impl<const nlohmann::basic_json<std::map, std::vector, std::__1::basic_string<char>, bool, long long, unsigned long long, double, std::allocator, adl_serializer, std::__1::vector<unsigned char, std::__1::allocator<unsigned char> > >::internal_binary_t &, const nlohmann::basic_json<std::map, std::vector, std::__1::basic_string<char>, bool, long long, unsigned long long, double, std::allocator, adl_serializer, std::__1::vector<unsigned char, std::__1::allocator<unsigned char> > > >' requested here
        return get_ref_impl<ReferenceType>(*this);
               ^
../include/nlohmann/json.hpp:1490:77: note: in instantiation of function template specialization 'nlohmann::basic_json<std::map, std::vector, std::__1::basic_string<char>, bool, long long, unsigned long long, double, std::allocator, adl_serializer, std::__1::vector<unsigned char, std::__1::allocator<unsigned char> > >::get_ref<const nlohmann::basic_json<std::map, std::vector, std::__1::basic_string<char>, bool, long long, unsigned long long, double, std::allocator, adl_serializer, std::__1::vector<unsigned char, std::__1::allocator<unsigned char> > >::internal_binary_t &, 0>' requested here
                JSONSerializer<other_binary_t>::to_json(*this, val.template get_ref<const other_binary_t&>());
                                                                            ^
../test/src/unit-regression.cpp:1623:23: note: in instantiation of function template specialization 'nlohmann::basic_json<my_workaround_fifo_map, std::vector, std::__1::basic_string<char>, bool, long long, unsigned long long, double, std::allocator, adl_serializer, std::__1::vector<unsigned char, std::__1::allocator<unsigned char> > >::basic_json<nlohmann::basic_json<std::map, std::vector, std::__1::basic_string<char>, bool, long long, unsigned long long, double, std::allocator, adl_serializer, std::__1::vector<unsigned char, std::__1::allocator<unsigned char> > >, 0>' requested here
        my_json foo = R"([1, 2, 3])"_json;
                      ^

and

In file included from ../test/src/unit-regression.cpp:37:
../include/nlohmann/json.hpp:2779:20: warning: returning reference to local temporary object [-Wreturn-stack-address]
            return *ptr;
                   ^~~~
../include/nlohmann/json.hpp:3136:16: note: in instantiation of function template specialization 'nlohmann::basic_json<std::map, std::vector, std::__1::basic_string<char>, bool, long long, unsigned long long, double, std::allocator, ns::foo_serializer, std::__1::vector<unsigned char, std::__1::allocator<unsigned char> > >::get_ref_impl<const nlohmann::basic_json<std::map, std::vector, std::__1::basic_string<char>, bool, long long, unsigned long long, double, std::allocator, ns::foo_serializer, std::__1::vector<unsigned char, std::__1::allocator<unsigned char> > >::internal_binary_t &, const nlohmann::basic_json<std::map, std::vector, std::__1::basic_string<char>, bool, long long, unsigned long long, double, std::allocator, ns::foo_serializer, std::__1::vector<unsigned char, std::__1::allocator<unsigned char> > > >' requested here
        return get_ref_impl<ReferenceType>(*this);
               ^
../include/nlohmann/json.hpp:1490:77: note: in instantiation of function template specialization 'nlohmann::basic_json<std::map, std::vector, std::__1::basic_string<char>, bool, long long, unsigned long long, double, std::allocator, ns::foo_serializer, std::__1::vector<unsigned char, std::__1::allocator<unsigned char> > >::get_ref<const nlohmann::basic_json<std::map, std::vector, std::__1::basic_string<char>, bool, long long, unsigned long long, double, std::allocator, ns::foo_serializer, std::__1::vector<unsigned char, std::__1::allocator<unsigned char> > >::internal_binary_t &, 0>' requested here
                JSONSerializer<other_binary_t>::to_json(*this, val.template get_ref<const other_binary_t&>());
                                                                            ^
../test/src/unit-regression.cpp:1634:29: note: in instantiation of function template specialization 'nlohmann::basic_json<std::map, std::vector, std::__1::basic_string<char>, bool, long long, unsigned long long, double, std::allocator, adl_serializer, std::__1::vector<unsigned char, std::__1::allocator<unsigned char> > >::basic_json<nlohmann::basic_json<std::map, std::vector, std::__1::basic_string<char>, bool, long long, unsigned long long, double, std::allocator, ns::foo_serializer, std::__1::vector<unsigned char, std::__1::allocator<unsigned char> > >, 0>' requested here
        nlohmann::json nj = lj;                // This line works as expected
                            ^

[nlohmann]

This issue was introduced in #1662. Compared to the other structured types (string, array, object), the binary types do not coincide:

        using other_string_t = typename BasicJsonType::string_t;
        using other_object_t = typename BasicJsonType::object_t;
        using other_array_t = typename BasicJsonType::array_t;
        using other_binary_t = typename BasicJsonType::internal_binary_t;

So the call

JSONSerializer<other_binary_t>::to_json(*this, val.template get_ref<const other_binary_t&>());

calls get_ref with const internal_binary_t&, which calls get_ptr with const internal_binary_t*:

However, we only have overloads for const binary_t*:

    /// get a pointer to the value (binary)
    constexpr const binary_t* get_impl_ptr(const binary_t* /*unused*/) const noexcept
    {
        return is_binary() ? m_value.binary : nullptr;
    }

[nlohmann]

@OmnipotentEntity Any idea on this one?

[OmnipotentEntity]

Oh dear.

So there's two ways to fix this, neither of which I particularly like:

1. Expose internal_binary_t by returning a reference to it.
2. Move the subtype handling somewhere else (into the main object probably).

Downsides of (1) are exposing an implementation detail, and the structure of the internally represented type being slightly surprising (but convertible to the expected type, and having the same interface as the expected type.)

Downsides of (2) is every json object will have to pay an extra 2 bytes of space + padding.

I'm personally for (1), and I can submit a patch to fix it next weekend. But the patch should be reasonably trivial to write if you can't wait (I'm just away from my computer for a while due to a nature vacation).

[nlohmann]

Thanks for the quick response!

I'm not sure whether it is what you meant with (1), but adding these overloads fixes the warning:

/// get a pointer to the value (binary)
internal_binary_t* get_impl_ptr(internal_binary_t* /*unused*/) noexcept
{
    return is_binary() ? m_value.binary : nullptr;
}

/// get a pointer to the value (binary)
constexpr const internal_binary_t* get_impl_ptr(const internal_binary_t* /*unused*/) const noexcept
{
    return is_binary() ? m_value.binary : nullptr;
}

What do you think?

[OmnipotentEntity]

That's what I meant by (1). I had misgivings about it because it would involve exposing an internal_binary_t. But it's probably the only reasonable way.