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

Serialize empty tuple into '[]' instead of null #4594

Merged
merged 1 commit into from
Jan 18, 2025

Conversation

codenut
Copy link
Contributor

@codenut codenut commented Jan 12, 2025

This PR fixes #4530 by serializing an empty tuple into [] instead of null.


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 files single_include/nlohmann/json.hpp and single_include/nlohmann/json_fwd.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 this kind of bug). 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.

@codenut codenut requested a review from nlohmann as a code owner January 12, 2025 11:12
@github-actions github-actions bot added the M label Jan 12, 2025
@codenut codenut force-pushed the develop branch 3 times, most recently from 0d8c626 to 0c84dd6 Compare January 12, 2025 23:43
@github-actions github-actions bot added S and removed M labels Jan 12, 2025
@gregmarr
Copy link
Contributor

Title has typo on empty

@codenut codenut changed the title WIP: Serialize emtpy tuple into instead of null WIP: Serialize empty tuple into instead of null Jan 13, 2025
@github-actions github-actions bot added M tests and removed S labels Jan 13, 2025
Copy link

🔴 Amalgamation check failed! 🔴

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

@github-actions github-actions bot removed the tests label Jan 13, 2025
@codenut codenut force-pushed the develop branch 2 times, most recently from ecc7154 to 1aa1039 Compare January 13, 2025 11:35
@codenut codenut changed the title WIP: Serialize empty tuple into instead of null WIP: Serialize empty tuple into '[]' instead of null Jan 13, 2025
@github-actions github-actions bot added the tests label Jan 14, 2025
@codenut codenut changed the title WIP: Serialize empty tuple into '[]' instead of null Serialize empty tuple into '[]' instead of null Jan 14, 2025
@nlohmann
Copy link
Owner

The coverage job is now fixed in the develop branch. Please update.

@nlohmann nlohmann added the please rebase Please rebase your branch to origin/develop label Jan 15, 2025
@coveralls
Copy link

coveralls commented Jan 16, 2025

Coverage Status

coverage: 99.185% (+0.001%) from 99.184%
when pulling 5bc954d on codenut:develop
into e72046e on nlohmann:develop.

@codenut codenut force-pushed the develop branch 3 times, most recently from 0b995ce to 29f1571 Compare January 16, 2025 03:11
Copy link

🔴 Amalgamation check failed! 🔴

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

Signed-off-by: Michael Valladolid <mikevalladolid@gmail.com>
@nlohmann nlohmann removed the please rebase Please rebase your branch to origin/develop label Jan 17, 2025
@nlohmann
Copy link
Owner

I had a look at #4530, and the issue stated

null used to de-serialize without a problem to std::tuple<> but since throws an error since 3.10.0.

and

Expected - null succesfully de-serializes when j.get<std::tuple<>>() is called on null.

However, when I execute this code

// deserialize null
nlohmann::json j_null;
CHECK_NOTHROW(j_null.get<std::tuple<>>());

on this branch, I still get the error


[json.exception.type_error.302] type must be array, but is null

I am fine with serializing and empty tuple to [], but in the end, we do not fix the linked issue, right?

@gregmarr
Copy link
Contributor

The linked issue title and first two sentences:

Title

Empty std::tuple<> cannot be serialized and de-serialized

1

The behavior of serializing and de-sereailzing an empty tuple changed from 3.9.1 to 3.10.0.

2

null used to de-serialize without a problem to std::tuple<> but since throws an error since 3.10.0.

So I would say it fixes the title first sentence (round trip from c++ to json to c++) by fixing the serializing. I think it's clear that the serializing to null is a bug and should be fixed.

It doesn't fix the second sentence (deserialize to std::tuple<> from null). I think the "Expected behavior" in that issue is wrong and this is desired behavior. You can't deserialize to std::string or int from null, so I think it's fine that you can't deserialize it to std::tuple<> either.

@nlohmann
Copy link
Owner

Thanks @gregmarr. I totally agree, but I wanted to make sure to what extent we address all aspects of the issue.

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.

@nlohmann nlohmann added this to the Release 3.11.4 milestone Jan 18, 2025
@nlohmann nlohmann merged commit bdb8d2b into nlohmann:develop Jan 18, 2025
130 checks passed
@nlohmann
Copy link
Owner

Thanks!

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.

Empty std::tuple<> cannot be serialized and de-serialized
4 participants