`nlohhman::to_string()` default implementation shadows user defined one

namaenonaimumei commented 1 year ago

Description

I am not really sure how to go about using user-defined nlohhman::to_string() (in a consistent manner).

Is there any reason for this template to be implemented on nlohmann side at all (since it's pretty much .dump() alias anyway).

If this wasn't intended, is there any chance to alter this behaviour.

Reproduction steps

Example for context below, granted this template behaviour is to be expected so I'm not really sure how provided example was supposed to work in the first place, unless user version is not supposed to be implemented inside nlohmann, which seems inconsistent for using with rest of the API (that or I missed something obvious in documentation linked).

Expected vs. actual results

Removing example implementation and leaving implementing up to user or providing macro to enable default one seems like a good alternative.

Minimal code example

Example user implementation

namespace nlohmann // same for namespace "ns" in examples below
{
template<typename BasicJsonType>
std::string to_string(const BasicJsonType &j)
{
    if (j.type() == nlohmann::json::value_t::string)
        return j.template get<std::string>();
    else
        return j.dump();
}
}

Example calls

// All examples below prioritize internal/first implementation
std::cout << nlohmann::to_string(jsonobj["key"]) << std::endl;
std::cout << to_string(jsonobj["key"]) << std::endl;
{
    using namespace nlohmann;
    std::cout << to_string(jsonobj["key"]) << std::endl;
}
{
    using namespace ns;
    std::cout << to_string(jsonobj["key"]) << std::endl;
}
// Calls like these still work
std::cout << ns::to_string(jsonobj["key"]) << std::endl;

Error messages

No response

Compiler and operating system

gcc 12.2.1

Library version

3.11.2

Validation

[ ] The bug also occurs if the latest version from the develop branch is used.
[ ] I can successfully compile and run the unit tests.

nlohmann commented 1 year ago

I am not exactly sure what you propose? Removing the to_string member function?

namaenonaimumei commented 1 year ago

That (and leaving documentation as it is) or guarding default implementation with some guard macro (and updating documentation to reflect that). I lean more towards removal as it's just another .dump() and doesn't seem necessary in its current form. But if you deem previous behaviour as a vital part of default API later may be a better choice.

For second option something around the lines of below for actual usage

// json_ext.h (user header, included instead of json.hpp by other units)
#define JSON_TO_STRING_IMPLEMENTED
#include <json.hpp>
namespace nlohmann
{
template<typename BasicJsonType>
std::string to_string(const BasicJsonType &j)
{
    if (j.type() == nlohmann::json::value_t::string)
        return j.template get<std::string>();
    else
        return j.dump();
}
}

// json.hpp
#ifndef JSON_TO_STRING_IMPLEMENTED
/// @brief user-defined to_string function for JSON values
/// @sa https://json.nlohmann.me/api/basic_json/to_string/
NLOHMANN_BASIC_JSON_TPL_DECLARATION
std::string to_string(const NLOHMANN_BASIC_JSON_TPL& j)
{
    return j.dump();
}
#endif

nlohmann / json