Allow preventing implicit converions to basic_json

iFreilicht commented 3 months ago

Previously, JSON_USE_IMPLICIT_CONVERSIONS only prevented implicit conversions from basic_json to other types.

With this change, implicit conversions to basic_json are also prevented by that macro.

Fixes #2226.

I tested this change also in https://github.com/iFreilicht/json-questions/commit/c0b2f53ebe279b6efd933a73e2b26f1bf2030261, the original repro repo from https://github.com/nlohmann/json/issues/2226#issuecomment-2023142182.

Pull request checklist

Read the Contribution Guidelines for detailed information.

[x] Changes are described in the pull request, or an existing issue is referenced.
[x] The test suite compiles and runs without error.
[x] Code coverage is 100%. Test cases can be added by editing the test suite.
[x] 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.

coveralls commented 3 months ago

coverage: 100.0%. remained the same when pulling 7c7f7a8d4f5601f68c32c8576f3a622a723a6242 on iFreilicht:develop into 199dea11b17c533721b26249e2dcaee6ca1d51d3 on nlohmann:develop.

schaumb commented 3 months ago

As I thought, this will not work easily because a lot of code depends on implicit construction.

iFreilicht commented 3 months ago

Ah yes, it seems that the CI is running more tests that I didn't run locally. I'll have a look at that.

gregmarr commented 3 months ago

Could we use a new define JSON_USE_IMPLICIT_CONSTRUCTORS for this that defaults to true so that users can opt in when they're ready?

nlohmann commented 3 months ago

Good idea.

iFreilicht commented 3 months ago

So, after playing whack-a-mole with the tests for 2 hours, I think just making the constructor explicit is not enough, it breaks an incredible amount of features, including safe ones (see the latest WIP commit).

A lot of functions already rely on the implicit conversion, so it seems necessary to add overloads for CompatibleTypes to many functions that then convert internally. I'm not sure that's much better than the current state of things.

Maybe there's a smarter way to prevent nested temporary values from being implicitly converted? Maybe if they're required to be moved instead of allowing const references?

iFreilicht commented 2 months ago

I will not continuing work on this. If someone else is interested in picking it up, feel free to do so.

nlohmann / json

Allow preventing implicit converions to basic_json #4324

Pull request checklist