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

Allow disabling default enum conversions #3536

Merged
merged 1 commit into from
Jun 16, 2022

Conversation

richardhozak
Copy link
Contributor

@richardhozak richardhozak commented Jun 14, 2022

This PR adds define JSON_NO_ENUM which works in similar vein as JSON_NO_IO. When JSON_NO_ENUM is defined it disables default from_json/to_json functions that convert the enum to underlying integer value and serialize it. This means if this is defined you have to provide user-defined conversion for enums (either by providing from_json/to_json pairs, using NLOHMANN_JSON_SERIALIZE_ENUM or specializing adl_serializer) if you want to serialize them, otherwise you get compiler error.

This does not change default behavior of this library, meaning JSON_NO_ENUM is not defined by default, retaining backwards compatibility, users have to opt-in.

Motivation
In our project we serialize all enums to json automatically by using the popular magic_enum library. This is done by using adl_serializer that is specialized for all enums, which then uses magic_enum to serialize enum variants. This piece of code needs to be included in order for the enums to get serialized properly, if we fail to do so, the enums get implicitly serialized to integer, leading to silent failures.

This change fixes that by turning all these use cases into compiler errors, allowing us to quickly fix this.


I thought this would benefit more people which use the recently introduced NLOHMANN_JSON_SERIALIZE_ENUM as it goes quite nicely into ensuring that you covered all your enum conversions.

I only briefly skimmed through issues and PRs, sorry if this has already been brought up in some way or form.


Pull request checklist

Read the Contribution Guidelines for detailed information.

  • Changes are described in the pull request, or an existing issue is referenced.
  • The test suite compiles and runs without error.
  • Code coverage is 100%. Test cases can be added by editing the test suite.
  • The source code is amalgamated; that is, after making changes to the sources in the include/nlohmann directory, run make amalgamate to create the single-header file single_include/nlohmann/json.hpp. The whole process is described here.

Please don't

  • The C++11 support varies between different compilers and versions. Please note the list of supported compilers. Some compilers like GCC 4.7 (and earlier), Clang 3.3 (and earlier), or Microsoft Visual Studio 13.0 and earlier are known not to work due to missing or incomplete C++11 support. Please refrain from proposing changes that work around these compiler's limitations with #ifdefs or other means.
  • Specifically, I am aware of compilation problems with Microsoft Visual Studio (there even is an issue label for these kind of bugs). I understand that even in 2016, complete C++11 support isn't there yet. But please also understand that I do not want to drop features or uglify the code just to make Microsoft's sub-standard compiler happy. The past has shown that there are ways to express the functionality such that the code compiles with the most recent MSVC - unfortunately, this is not the main objective of the project.
  • Please refrain from proposing changes that would break JSON conformance. If you propose a conformant extension of JSON to be supported by the library, please motivate this extension.
  • Please do not open pull requests that address multiple issues.

Copy link
Contributor

@falbrechtskirchinger falbrechtskirchinger left a comment

Choose a reason for hiding this comment

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

Please add JSON_NO_ENUM to detail/macro_scope.hpp and detail/macro_unscope.hpp.

@richardhozak
Copy link
Contributor Author

Added

@richardhozak richardhozak force-pushed the feat/json-no-enum branch 2 times, most recently from c8aa255 to 61a4200 Compare June 14, 2022 14:20
Copy link
Contributor

@gregmarr gregmarr left a comment

Choose a reason for hiding this comment

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

I'm wondering if this flag should be more descriptive, such as JSON_DISABLE_ENUM_SERIALIZATION.

include/nlohmann/detail/conversions/from_json.hpp Outdated Show resolved Hide resolved
@richardhozak
Copy link
Contributor Author

richardhozak commented Jun 14, 2022

Sure, I don't mind either way, I think more descriptive names are better but I was trying to stick with some sort of convention I found in the code already, as mentioned in original comment, I made this similarly to JSON_NO_IO, which disables serialization for io stuff.

Should I change it to JSON_DISABLE_ENUM_SERIALIZATION, leave it, or change it to something different?

@gregmarr
Copy link
Contributor

@falbrechtskirchinger @nlohmann Thoughts on naming? It's not a huge deal for me, just trying to avoid future user confusion.

@falbrechtskirchinger
Copy link
Contributor

@falbrechtskirchinger @nlohmann Thoughts on naming? It's not a huge deal for me, just trying to avoid future user confusion.

I agree that a more descriptive name is preferable. JSON_DISABLE_ENUM_SERIALIZATION works for me. JSON_NO_ENUM_SERIALIZATION would be shorter. Though, that's not been a primary concern even in the recent past (see JSON_USE_LEGACY_DISCARDED_VALUE_COMPARISON). ;-)

@gregmarr
Copy link
Contributor

I thought about NO but was looking for something that conveyed that we were just disabling the built-in default serialization in favor of the user-provided serialization.

@falbrechtskirchinger
Copy link
Contributor

I thought about NO but was looking for something that conveyed that we were just disabling the built-in default serialization in favor of the user-provided serialization.

Agreed. Let's go with JSON_DISABLE_ENUM_SERIALIZATION.

And for future consideration:
Do we want to expose this and other settings through CMake? Currently, the only ones covered are implicit conversion, no exceptions, and diagnostics.

@gregmarr
Copy link
Contributor

Do we want to expose this and other settings through CMake? Currently, the only ones covered are implicit conversion, no exceptions, and diagnostics.

Makes sense to me.

@coveralls
Copy link

coveralls commented Jun 15, 2022

Coverage Status

Coverage remained the same at 100.0% when pulling 31b03ea on zxey:feat/json-no-enum into e80945d on nlohmann:develop.

@richardhozak richardhozak force-pushed the feat/json-no-enum branch 2 times, most recently from 9594315 to 7d7db8e Compare June 15, 2022 08:35
@richardhozak
Copy link
Contributor Author

  • Renamed JSON_NO_ENUM to JSON_DISABLE_ENUM_SERIALIZATION
  • Added cmake option JSON_DisableEnumSerialization, which set/unsets JSON_DISABLE_ENUM_SERIALIZATION, this is OFF by default, meaning original behavior is retained

Copy link
Contributor

@falbrechtskirchinger falbrechtskirchinger left a comment

Choose a reason for hiding this comment

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

We've been quite nitpicky when reviewing documentation lately. By that standard, there're a few issues but nothing major. If anyone else requests changes, I'll add mine.

Otherwise, looks good to me.

Copy link
Contributor

@falbrechtskirchinger falbrechtskirchinger left a comment

Choose a reason for hiding this comment

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

I'll add my suggestions as well.

I messed up by doing the review from two tabs and not noticing. Hopefully, I fixed everything.

docs/mkdocs/docs/features/enum_conversion.md Outdated Show resolved Hide resolved
docs/mkdocs/docs/features/macros.md Outdated Show resolved Hide resolved
@richardhozak
Copy link
Contributor Author

Applied all the changes, I agree it is much clearer and easier to digest.

@richardhozak richardhozak force-pushed the feat/json-no-enum branch 2 times, most recently from 0c37848 to 2e458ba Compare June 15, 2022 15:02
docs/mkdocs/mkdocs.yml Outdated Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
docs/docset/docSet.sql Outdated Show resolved Hide resolved
docs/mkdocs/docs/api/macros/index.md Outdated Show resolved Hide resolved
Copy link
Contributor

@falbrechtskirchinger falbrechtskirchinger left a comment

Choose a reason for hiding this comment

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

One last thing.

@falbrechtskirchinger
Copy link
Contributor

Some day we should strictly define what the terms "parsing", "serializing", "deserializing", and "type conversion" mean and how they relate.

Copy link
Owner

@nlohmann nlohmann left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@gregmarr
Copy link
Contributor

Some day we should strictly define what the terms "parsing", "serializing", "deserializing", and "type conversion" mean and how they relate.

#3540

@nlohmann nlohmann merged commit f6acdbe into nlohmann:develop Jun 16, 2022
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.

None yet

5 participants