Feat/explicit conversion operator #1559

theodelrieu · 2019-04-04T12:20:14Z

Here it is!

Thanks to @gracicot, who found a good solution to #958, I'm finally attempting to solve it.

This PR introduces a CMake option: JSON_ExplicitConversions, defaulted to OFF.

To help users move away from the implicit conversion operator, a warning will be issued each time it is used. This warning can be silenced by users defining JSON_USE_IMPLICIT_CONVERSIONS.

On the other hand, defining JSON_USE_EXPLICIT_CONVERSIONS will very likely break code, but will ease the transition to the next major version, where only explicit conversion operator will be kept.

Meanwhile, I've removed every implicit conversion from the library's code, including tests.
It might be a good idea to add a short ifdefed test case, which tests implicit conversions (to avoid duplicating a lot of test code).

With explicit conversions, we can also remove the constraints that are present today, e.g. forbidding conversion from char, std::initializer_list<char>. Note that this has not been done in this PR.

I think this change is more suited to be released in a minor version, so we might release 3.6.2 first?

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.

theodelrieu · 2019-04-04T13:11:20Z

Where is the LGTM config file?

gracicot

Yeah, sorry I didn't had the time to submit the patch. Thanks for the submission!

My proposed solution was not as perfect as I thought, see my comment. Other than that, the diff seems fine for me!

gracicot · 2019-04-04T13:26:24Z

include/nlohmann/detail/macro_scope.hpp

+#ifndef JSON_USE_EXPLICIT_CONVERSIONS
+    #define JSON_EXPLICIT
+    #ifndef JSON_USE_IMPLICIT_CONVERSIONS
+        #warning Implicit conversions are being deprecated and will be \


I realized that the #warning statement is not standard, and not supported on msvc. There is some solution like #pragma message() but it's not a real warning.

I think we could use [[deprecated("...")]] or the msvc non standard __declspec(deprecated("...")) for earlier versions in that case.

I've added JSON_DEPRECATED_MSG, and tried building the tests by removing the Wno-deprecated warnings, it works \o/

Thank you, the patch is great now. I would just wait for @nlohmann to do a review too and merge it!

I like the idea of having a switch for the conversions, but I am still not convinced that implicit conversions should be deprecated.

The main reason is because it is broken today. The pile of issues related to #958 is a good indicator of that. Not to mention the why is quite hard to grasp.

The required changes are quite minimal as well:

std::string s; s = j; // becomes std::string s; j.get_to(s); std::string s = j; // becomes std::string s(j);

After thinking about it I think @nlohmann is right on not deprecate right now (I still think it should eventually). The compile switch must be there for a while before any deprecation, and the documentation should also rely on explicit conversion and it should recommend enabling the switch.

I would like to see the implicit operator be deprecated too but I also agree that even though the change in the syntax is minimal, its use is widespread so it must be done very gradually.

Edit: moved the example into the issue

coveralls · 2019-04-04T14:12:30Z

Coverage remained the same at 100.0% when pulling 7973293 on theodelrieu:feat/explicit_conversion_operator into 2cd10a7 on nlohmann:develop.

nlohmann · 2019-04-04T15:48:13Z

Where is the LGTM config file?

There is no config file. Here is a link to the log: https://lgtm.com/projects/g/nlohmann/json/logs/rev/pr-e7a2ac9be19e57d05404374d62bf6a8ab3794bfe/lang:cpp/stage:Build%20merge_d94187b754143b1064d1f7bcd3000e3db0419555. Strange that the test fails.

theodelrieu · 2019-04-05T08:00:00Z

But where do you define how to build the library?

theodelrieu · 2019-04-09T09:00:19Z

Ok, the LGTM build uses default CMake arguments, therefore it builds with the implicit conversions.
All the other builds use explicit conversions.

@nlohmann do you wish to do like with JSON_MultipleHeaders? i.e. define a variable in the CI files.

stale · 2019-05-09T09:50:24Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

theodelrieu · 2019-05-09T10:05:59Z

It would be nice to merge this PR and discuss what is the long-term plan after.

stale · 2019-06-08T10:31:20Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale · 2019-07-08T10:58:35Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

theodelrieu · 2019-07-30T09:15:39Z

@nlohmann Shall we merge this PR now that 3.7.0 has been released?

stale · 2019-08-29T09:43:22Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

gracicot · 2019-10-02T16:55:52Z

Since tackling the conversion operator is being candidate for version 4.0.0, I'd vote to add the deprecation back when JSON_ExplicitConversions is off.

If we want to use explicit conversion on the next major version, deprecation and the switch is the first step to transition the community.

theodelrieu · 2020-07-21T12:07:12Z

@nlohmann Seems good finally 🙏

test/src/unit-conversions.cpp

include/nlohmann/detail/meta/type_traits.hpp

test/src/unit-conversions.cpp

nlohmann · 2020-07-21T20:20:17Z

CMakeLists.txt

@@ -24,6 +24,7 @@ endif ()
 option(JSON_BuildTests "Build the unit tests when BUILD_TESTING is enabled." ON)
 option(JSON_Install "Install CMake targets during install step." ON)
 option(JSON_MultipleHeaders "Use non-amalgamated version of the library." OFF)
+option(JSON_ExplicitConversions "Enable explicit conversion operator" OFF)


I'm still struggling with the wording. To me, the switch "enable explicit conversion" sounds like an additional feature. Instead, it breaks existing code, because the assumed implicit conversions do not work any longer. If the code already only used explicit conversions, then flipping the switch does not change anything.

theodelrieu · 2020-07-22T07:24:44Z

@nlohmann About your last comment: I can swap the CMake option definition

wrap implicit conversions tests around the JSON_USE_IMPLICIT_CONVERSIONS macro

theodelrieu · 2020-07-22T12:08:46Z

@nlohmann GitHub was unreachable again, sigh. Can you restart the job please? 🙏 (https://travis-ci.org/github/nlohmann/json/jobs/710669524#L5359)

nlohmann

Looks good to me.

theodelrieu · 2020-07-22T13:55:13Z

@nlohmann It seems appveyor is stuck as well 🤔

nlohmann · 2020-07-22T15:56:42Z

No, it's just slow...

aokellermann · 2020-07-23T03:09:45Z

CMakeLists.txt

@@ -24,6 +24,7 @@ endif ()
 option(JSON_BuildTests "Build the unit tests when BUILD_TESTING is enabled." ON)
 option(JSON_Install "Install CMake targets during install step." ON)
 option(JSON_MultipleHeaders "Use non-amalgamated version of the library." OFF)
+option(JSON_ImplicitConversions "Enable implicit conversions" ON)


Maybe put period at end of description for consistency's sake.

gracicot · 2020-07-23T04:10:59Z

I wonder if this can be removed to re-enable conversions to std::string_view once we switch to explicit conversions:

    && !std::is_same<ValueType, typename std::string_view>::value

I think it was disabled because of a problem with string views and visual studio, but I'm not sure. If someone can find the issue/pull request, that would be awesome.

nlohmann · 2020-07-23T05:07:00Z

@gracicot Was this changed in this PR?

gracicot · 2020-07-23T05:16:35Z

@gracicot Was this changed in this PR?

No. The pull request mentioned that this hasn't been done forstd::initializer_list but I was wondering if std::string_view was worth to do. Right now json_value.get<std: string_view>() works, but not the conversion and is_gettable returns false for it.

nlohmann · 2020-07-23T06:50:36Z

No. The pull request mentioned that this hasn't been done forstd::initializer_list but I was wondering if std::string_view was worth to do. Right now json_value.get<std: string_view>() works, but not the conversion and is_gettable returns false for it.

I see. Let's handle this in a follow-up issue. This PR has been open for too long...

nlohmann · 2020-07-23T06:53:05Z

Thank you all so much!

nlohmann · 2020-07-23T06:53:49Z

🔖 Release item

This issue/PR will be part of the next release of the library. This template helps preparing the release notes.

Type

Description

Add option to switch off implicit conversions. operator T() considered harmful #958 Feat/explicit conversion operator #1559

theodelrieu · 2020-07-23T08:40:12Z

@gracicot I think all these type of explicit SFINAE disabling can be removed when using explicit conversions. IIRC there is one for char as well somewhere.

gracicot · 2020-07-23T11:34:58Z

Thank you so much! 🎉

tlemo · 2020-07-23T21:59:05Z

Thanks @theodelrieu for driving this!

theodelrieu force-pushed the feat/explicit_conversion_operator branch from edc99df to 5ec90c8 Compare April 4, 2019 12:41

gracicot suggested changes Apr 4, 2019

View reviewed changes

theodelrieu force-pushed the feat/explicit_conversion_operator branch from 5ec90c8 to fb833af Compare April 4, 2019 14:08

theodelrieu force-pushed the feat/explicit_conversion_operator branch 4 times, most recently from 54a0f6b to 79639b5 Compare April 8, 2019 13:37

gracicot mentioned this pull request Apr 11, 2019

operator T() considered harmful #958

Closed

stale bot added the state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated label May 9, 2019

stale bot removed the state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated label May 9, 2019

stale bot added the state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated label Jun 8, 2019

nlohmann removed the state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated label Jun 15, 2019

stale bot added the state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated label Jul 8, 2019

nlohmann removed the state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated label Jul 13, 2019

stale bot added the state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated label Aug 29, 2019

nlohmann added pinned and removed state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated labels Sep 2, 2019

theodelrieu force-pushed the feat/explicit_conversion_operator branch from 8ce2e52 to ba4ee31 Compare December 2, 2019 09:19

nlohmann requested changes Jul 21, 2020

View reviewed changes

theodelrieu force-pushed the feat/explicit_conversion_operator branch from a3e6f5c to cc74efb Compare July 22, 2020 07:49

theodelrieu added 2 commits July 22, 2020 10:49

add a switch to enable implicit conversions (defaults to true)

74b446f

wrap implicit conversions tests around the JSON_USE_IMPLICIT_CONVERSIONS macro

add a CI job for explicit conversions on every os

7973293

theodelrieu force-pushed the feat/explicit_conversion_operator branch from cc74efb to 7973293 Compare July 22, 2020 08:49

nlohmann approved these changes Jul 22, 2020

View reviewed changes

nlohmann self-assigned this Jul 22, 2020

nlohmann added release item: ✨ new feature and removed pinned labels Jul 22, 2020

nlohmann added this to the Release 3.8.1 milestone Jul 22, 2020

nlohmann linked an issue Jul 22, 2020 that may be closed by this pull request

operator T() considered harmful #958

Closed

aokellermann reviewed Jul 23, 2020

View reviewed changes

nlohmann merged commit a048b72 into nlohmann:develop Jul 23, 2020

theodelrieu deleted the feat/explicit_conversion_operator branch July 23, 2020 08:38

theodelrieu mentioned this pull request Jul 28, 2020

nlohmann_json: add 3.9.0 (and implicit_conversions option) conan-io/conan-center-index#2279

Merged

4 tasks

thomasgt mentioned this pull request Jan 7, 2022

[nlohmann-json] Add port feature to control implicit conversions behaviour microsoft/vcpkg#22408

Closed

Feat/explicit conversion operator #1559

Feat/explicit conversion operator #1559

Conversation

theodelrieu commented Apr 4, 2019 • edited Loading

Pull request checklist

theodelrieu commented Apr 4, 2019

gracicot left a comment

Choose a reason for hiding this comment

gracicot Apr 4, 2019

Choose a reason for hiding this comment

theodelrieu Apr 4, 2019

Choose a reason for hiding this comment

gracicot Apr 4, 2019

Choose a reason for hiding this comment

nlohmann Apr 4, 2019

Choose a reason for hiding this comment

theodelrieu Apr 5, 2019

Choose a reason for hiding this comment

gracicot Apr 5, 2019 • edited Loading

Choose a reason for hiding this comment

coveralls commented Apr 4, 2019 • edited Loading

nlohmann commented Apr 4, 2019

theodelrieu commented Apr 5, 2019

theodelrieu commented Apr 9, 2019

stale bot commented May 9, 2019

theodelrieu commented May 9, 2019

stale bot commented Jun 8, 2019

stale bot commented Jul 8, 2019

theodelrieu commented Jul 30, 2019

stale bot commented Aug 29, 2019

gracicot commented Oct 2, 2019 • edited Loading

theodelrieu commented Jul 21, 2020

nlohmann Jul 21, 2020

Choose a reason for hiding this comment

theodelrieu commented Jul 22, 2020

theodelrieu commented Jul 22, 2020

nlohmann left a comment

Choose a reason for hiding this comment

theodelrieu commented Jul 22, 2020

nlohmann commented Jul 22, 2020

aokellermann Jul 23, 2020

Choose a reason for hiding this comment

gracicot commented Jul 23, 2020

nlohmann commented Jul 23, 2020

gracicot commented Jul 23, 2020 • edited Loading

nlohmann commented Jul 23, 2020

nlohmann commented Jul 23, 2020

nlohmann commented Jul 23, 2020

🔖 Release item

Type

Description

theodelrieu commented Jul 23, 2020

gracicot commented Jul 23, 2020

tlemo commented Jul 23, 2020

theodelrieu commented Apr 4, 2019 •

edited

Loading

gracicot Apr 5, 2019 •

edited

Loading

coveralls commented Apr 4, 2019 •

edited

Loading

gracicot commented Oct 2, 2019 •

edited

Loading

gracicot commented Jul 23, 2020 •

edited

Loading