Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add strict enum de/serialization macro #4612

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

hnampally
Copy link
Contributor

@hnampally hnampally commented Jan 20, 2025

#3992

  • The changes are described in detail, both the what and why.
  • If applicable, an #3992 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.

Signed-off-by: Harinath Nampally <harinath922@gmail.com>
Copy link

🔴 Amalgamation check failed! 🔴

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

@coveralls
Copy link

coveralls commented Jan 20, 2025

Coverage Status

coverage: 99.186%. remained the same
when pulling c6d9ea0 on hnampally:issue-3992
into f06604f on nlohmann:develop.

Signed-off-by: Harinath Nampally <harinath922@gmail.com>
Signed-off-by: Harinath Nampally <harinath922@gmail.com>
Signed-off-by: Harinath Nampally <harinath922@gmail.com>
- If an enum value appears more than once in the mapping, only the first occurrence will be used for serialization,
subsequent mappings for the same enum value will be ignored.
- If a JSON value appears more than once in the mapping, only the first occurrence will be used for deserialization,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a thought: shouldn't a strict macro also take care of this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for catching that! Yes, that makes sense. Could you clarify which exception should be thrown in this case?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good question! As C++11 most probably does not allow to detect such duplicates at compile time and an exception could only be thrown the first time the from_json function is used, such an exception would be rather unexpected. I therefore think we should leave this as is.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be possible to do a static_assert that there are no duplicate values in this array, but I'm not sure what the compile time cost would be. The hardest part would be supporting that in C++11 where constexpr is pretty limited.

       static const std::pair<ENUM_TYPE, BasicJsonType> m[] = __VA_ARGS__;

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though intellectually interesting, the question is at what price such a check would come.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, I can only get this to work in C++20, as in order to static_assert that there are no duplicates in the array, the array has to be constexpr and constexpr prior to C++20 doesn't support std::pair<ENUM_TYPE, BasicJsonType> m[] = ....

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand. And I guess there is no way to map the pairs to only cope with an array of ENUM_TYPE?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For posterity, the implementation that requires C++20:

template<typename Iter>
constexpr bool enum_array_has_no_duplicates(Iter begin, Iter end)
{
    for (auto it1 = begin; it1 != end; it1++)
    {
        for (auto it2 = it1 + 1; it2 != end; it2++)
        {
           if (it1->first == it2->first || it1->second == it2->second)
           {
            return false;
           } 
        }
    }

    return true;
}

    static constexpr std::pair<ENUM_TYPE, BasicJsonType> m[] = { ...

    static_assert(enum_array_has_no_duplicates(std::begin(m), std::end(m)));

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That code prevents this definition:

enum Color
{
    RED = 1,
    Red = 1,
    Green = 2
};

NLOHMANN_JSON_SERIALIZE_ENUM_STRICT(Color,
{
    {RED, "RED"},
    {Red, "Red"}, // fails due to matching RED and Red
    {Green, "RED"} // fails to matching "RED" and "RED"
})

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand. And I guess there is no way to map the pairs to only cope with an array of ENUM_TYPE?

It should check both ENUM_TYPE and BasicJsonType duplication because there is mapping in both directions.

This could be a C++20-only feature, but that would be messy, because it's not just the added function and static assert, but you also have to make m either const or constexpr . That could maybe be done with a NLOHMANN_JSON_SERIALIZE_ENUM_STRICT_CONSTEXPR that is constexpr in C++20 and const in earlier versions, and use that instead of const on the definition of m.

It could make three different arrays, one of pairs, one of enums, and one of jsons, but you'd need to remove all the braces and do the alternating between enum and json that is done in the new named serialization macro, which means that you'd have limits on the number of enums as you have there.

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

#if !defined(JSON_NOEXCEPTION) && !defined(JSON_THROW_USER) && !defined(JSON_THROW)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed? The other examples work with exceptions without this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshot from 2025-01-19 20-47-22
Yes, for some reason, I encountered a compilation issue without this. I noticed that JSON_THROW is already defined in json.hpp, but the issue persists. Could you let me know if I'm overlooking something?

Copy link
Contributor Author

@hnampally hnampally Jan 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like it is being undefined in macro_unscope.hpp at the end of json.hpp. Please find the relevant line at nlohmann/json.hpp.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that other examples, such as nlohmann_json_serialize_enum_2.cpp, do not encounter this compilation issue because they use macro functions like NLOHMANN_JSON_SERIALIZE_ENUM, which do not throw exceptions. In macro_scope.hpp, NLOHMANN_JSON_SERIALIZE_ENUM_STRICT is the first macro to trigger the JSON_THROW exception. Therefore, I believe I need to redefine it in this example code.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this definitely needs to be fixed - a user must not worry about any macros for exceptions. In this case, the exception from NLOHMANN_JSON_SERIALIZE_ENUM_STRICT should be thrown via a plain throw. The JSON_THROW is only used for exceptions that can occur inside of the library whereas it makes no sense for a user to use a different throw mechanism for the the exception in NLOHMANN_JSON_SERIALIZE_ENUM_STRICT.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the other hand, if you don't use JSON_THROW, then NLOHMANN_JSON_SERIALIZE_ENUM_STRICT is not usable with exceptions disabled. An alternative solution to having the macro escape the library could be to have an internal function like this just for this particular usage of throwing from inside one of the macros:

    template<typename T> 
    void json_throw_from_serialize_macro(T&& exception) 
    {
        JSON_THROW std::forward<T>(exception);
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nlohmann Yes, I completely agree. I will give Greg's proposed solution a try.
@gregmarr thanks for your code snippet.

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

#if !defined(JSON_NOEXCEPTION) && !defined(JSON_THROW_USER) && !defined(JSON_THROW)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this definitely needs to be fixed - a user must not worry about any macros for exceptions. In this case, the exception from NLOHMANN_JSON_SERIALIZE_ENUM_STRICT should be thrown via a plain throw. The JSON_THROW is only used for exceptions that can occur inside of the library whereas it makes no sense for a user to use a different throw mechanism for the the exception in NLOHMANN_JSON_SERIALIZE_ENUM_STRICT.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants