Serialize enum

[nugentcaillin]

[Describe your pull request here. Please read the text below the line and make sure you follow the checklist.]

• The changes are described in detail, both the what and why.
• If applicable, an existing issue is referenced.
• The Code coverage remained 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.

• added NLOHMANN_SERIALIZE_ENUM_STRICT to throw error on unrecognized enum values, solving 3992.
• Added new unit tests to verify behavior.
• updated documentation where NLOHMANN_SERIALIZE_ENUM to mention alongside warning that a throwing version exists - i didn't add a separate page for it as it is very close functionally to NLOHMANN_SERIALIZE_ENUM, but can add.

I wasn't able to call JSON_THROW inside detail/macro_scope.hpp, so this PR may have issues when exceptions are disabled - but am unsure if this is an issue if this macro has been added with the purpose of throwing - I'd appreciate some guidance on how to proceed here.

[nlohmann]

Can you please add a dedicated page for the enum like https://github.com/nlohmann/json/blob/develop/docs/mkdocs/docs/api/macros/nlohmann_json_serialize_enum.md ?

[nugentcaillin]

Thanks for taking the time to review! I've added the page and links where seemed appropriate.

[github-actions]

🔴 Amalgamation check failed! 🔴

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

[github-actions]

🔴 Amalgamation check failed! 🔴

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

[github-actions]

🔴 Amalgamation check failed! 🔴

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

[nugentcaillin]

Thanks for the feedback, I've applied those changes - however for JSON_THROW I couldn't call it directly and had to add a templated function to wrap JSON_THROW.

I would get a compiler error that would be fixed by #defining JSON_THROW in the test file - I'm guessing something to do with the serialize macro creating a template along with JSON_THROW being undef'd and calling JSON_THROW through a wrapper function templated on exception changes the timing on when the JSON_THROW macro is expanded?

I don't know for sure though but this was the only way I could get the serialization macro to compile without #defining JSON_THROW again wherever it needs to be called. If there's a better way to do this i'd appreciate some guidance.

I also had to void cast exception in this templated function to avoid error if compiled with Werror and Wunused-parameter as JSON_THROW doesn't use exception if exceptions are turned off - not sure if this goes against code style.

maybe a void cast could be added to the JSON_THROW macro when exceptions are turned off to suppress this? - changing JSON_THROW felt beyond the scope of this PR.

[nlohmann]

Some small comments, otherwise looks good.

[gregmarr]

FYI, two alternative versions of this function from previous "strict enum" PRs:

https://github.com/nlohmann/json/pull/4612/changes#diff-ed4d9ac7996b56500f709b366672a844db294ffa884bbbcda8c690e6eb8a7711R245-R260
https://github.com/nlohmann/json/pull/4989/changes#diff-0af3903deeaf48f62fcb01acf3c6702a376ece2bb801a80d806c9757f826f41cR32-R41

The first one basically reimplements JSON_THROW, the second one uses JSON_THROW and is much more focused.

I'm not judging if any of them are better, just pointing out other versions of this same fix for not being able to use JSON_THROW in the macros.

[nugentcaillin]

Thanks for bringing these up!

I don't think the first would be a good fit as re-implementing JSON_THROW ignores the undef and redef of JSON_THROW if JSON_THROW_USER is defined, and would make the code less maintainable as JSON_THROW would have to be updated in an additional place if changes are needed, so I think it's important that JSON_THROW is called

https://github.com/nugentcaillin/json/blob/21fe58fa43745a846c6987f772094a6a0a4736c0/include/nlohmann/detail/macro_scope.hpp#L188-L192
https://github.com/Ash-Jose/json_issue/blob/12319044db42395e8709fd58a04e79dad51fceb1/include/nlohmann/detail/conversions/from_json.hpp#L32-L40

// override exception macros
#if defined(JSON_THROW_USER)
    #undef JSON_THROW
    #define JSON_THROW JSON_THROW_USER
#endif

As for the second, looking at them side by side they seem roughly equivalent with the exception of the previous one being inlined and templated on BasicJsonType instead of ExceptionType, and hardcoding the specific exception.
https://github.com/nugentcaillin/json/blob/21fe58fa43745a846c6987f772094a6a0a4736c0/include/nlohmann/detail/macro_scope.hpp#L258-L271

// current

/*!
@brief function to wrap JSON_THROW_MACRO - NLOHMANN_SERIALIZE_ENUM_STRICT has a
       compilation warning about there being no arguments to JSON_THROW that depend on
       template arguments otherwise
*/
template<typename ExceptionType>
void templated_json_throw(ExceptionType exception)
{
    JSON_THROW(exception);


    /* JSON_THROW(exception) discards exception and aborts - void cast needed to supress
       compilation error if compiled with -Werror and Wunused-parameter */
    (void)exception;
}

// from previous pull

/* helper for strict enum error reporting */
template<typename BasicJsonType>
inline void throw_enum_error(const BasicJsonType& j, const char* enum_type)
{
    JSON_THROW(::nlohmann::detail::type_error::create(
                   302,
                   std::string("invalid value for ") + enum_type + ": " + j.dump(),
                   &j));
}

I do worry that emulating this would make the change less extensible as hard-coding in the exception would mean that if another strict macro was implemented in the future it would need to implement its own throw function instead of using the current one.

I also don't know if it would make much sense to pass json object when throwing error in to_json as no object would have been created from VA_ARGS, and dump cannot be called on nullptr. It does make a lot of sense in from_json though - it seems in that PR they decided not to throw in to_json.

If that behavior is more desirable I could make a change like this and remove throwing in to_json - it does seem against the spirit of a strict serialization macro to not throw though:

/*!
@brief function to wrap JSON_THROW_MACRO - NLOHMANN_SERIALIZE_ENUM_STRICT has a
       compilation warning about there being no arguments to JSON_THROW that depend on
       template arguments otherwise
*/
template<typename BasicJsonType>
void nlohmann_serialize_enum_throw(const BasicJsonType& j, const char *enum_type)
{
    JSON_THROW(nlohmann::detail::out_of_range::create(
        410,
        std::string("invalid value for") + enum_type + ": " + j.dump(),
        &j));
}

I could also leave the templated function for throwing unchanged and change the call site to have a more descriptive error message in from_json like the function from the previous pull request and do something like this:

template<typename BasicJsonType>
inline void to_json(BasicJsonType& j, const ENUM_TYPE& e)
{
    ...
    // add stringification of enum type
    else templated_json_throw<nlohmann::detail::out_of_range>(nlohmann::detail::out_of_range::create(410,"enum value out of range for " + #ENUM_TYPE, nullptr));
}
template<typename BasicJsonType>
inline void from_json(BasicJsonType& j, const ENUM_TYPE& e)
{
    ...
    // add stringification of enum type, json dump and pointer
    else templated_json_throw<nlohmann::detail::out_of_range>(nlohmann::detail::out_of_range::create(410,"enum value out of range for " + #ENUM_TYPE + ": " + j.dump(), &j));
}

This would take the more descriptive error message from that PR whilst keeping the function available for use for any future strict macros. I've also left the inline out here since it's a templated function and I don't think inline will change how the compiler handles it at all - happy to add it in though.

Please advise which of these solutions, if any would be most appropriate. I'll start on the last one for now as that seems more appropriate but happy to switch.

[nlohmann]

Looks good to me.

[nlohmann]

Thanks!