Make Json::Value(std::string) etc... explicit

[aaron-michaux]

Describe the bug
As it stands, we cannot write code like:

error_code write(std::ostream& in, const string_view s) noexcept;  // 1
error_code write(std::ostream& in, const Json::Value& o) noexcept;

Because the call to '1' this is ambiguous:

write(in, "foo"s);

The problem is that C++ detects that it can automagically convert "foo"s to either a string_view OR a Json::Value.

It's an annoying reality that explicit isn't the default for constructors. It should be. Be that as it may, Jsoncpp would play nicer if it did use explicit.

To Reproduce
Steps to reproduce the behavior:

1. See code snippets above.

Expected behavior
A clear and concise description of what you expected to happen.

An error message like:

serialize.cpp:343:12: error: call to 'write' is ambiguous
      ec = write(in, str(o));
           ^~~~~
serialize.cpp:260:12: note: candidate function
error_code write(std::ostream& out, std::string_view x) noexcept
           ^
serialize.cpp:339:12: note: candidate function
error_code write(std::ostream& in, const Json::Value& o) noexcept

Desktop (please complete the following information):

Affects all version compiling under a conformant c++ compiler.

[dota17]

The problem is that C++ detects that it can automagically convert "foo"s to either a string_view OR a Json::Value.

In your case, I think it is better to call the write(in, "foo"s); with

write(in, Json::Value("foo"));

The work should not be jsoncpp's.

If we use explicit, Json::Value v = "foo" would not work.

[baylesj]

Hmmm, I do lean on the side of preferring explicit, since it's part of the google style guide I might be biased :P.

IMHO Json::Value v { "foo" } is probably better than Json::Value v = "foo" since it's not a string.

I don't think this is super high priority, if I end up having free time I'll look at marking explicit, otherwise I'm happy to review a patch.

[cdunn2001]

@baylesj, I too am a fan of the Google C++ Style Guide. I wish I could convince co-workers to adopt it where I work.

Anyway, we could probably add explicit before the next release. It would cause only very minor breakage in exist code. We're bumping the minor version for sure.

[jovere]

Why hasn't this been addressed yet?

[BillyDonahue]

I may have a minority opinion, but I'm not sure this is a bug that needs fixing in jsoncpp.

• Changing the constructor to be explicit would potentially break some convenient conversions that are being put to use in client code. I think there are going to be more users that break from such a change than previously anticipated. Like, if a function takes a value, like: void f(Json::Value v), I think there will be callers like: f(123); or f("hi"); out there. These would break from explicit.

• The OP code above was flawed because there was a possibility for overloads of write to appear and break it, and std::string_view has (and needs to have) implicit unary ctors well. When working with an open overload set like that, callers have to be more explicit and careful. I agree with @dota17's original assessment that such code is not really jsoncpp's problem to fix. If jsoncpp adds explicit unary ctors, that code would still be vulnerable to breaking in the future as more overloads of write appear.

[jovere]

In that case, I'll refer to #1159, where this is a real bug.

[BillyDonahue]

In that case, I'll refer to #1159, where this is a real bug.

It's still a caller bug, though. In that case someone essentially assigned value = &someOtherValue, which is obviously not something they should do. They were just surprised at the failure mode (accepting it as bool).

The suggested fix there is a little different. It's to disable implicit conversion from pointers. We might also explicitly accept const char* and force it to be a string Value, but disable other kinds of pointers.

[jovere]

Hi Billy,

To keep from decaying down from a pointer would be a wonderful thing. Unfortunately, that bug has been languishing for just as long as this open bug, and marked duplicate (which I don't necessarily agree with). It would be extremely helpful to make it difficult for somebody to do the wrong thing, as what can unfortunately happen here:

https://godbolt.org/z/EG7ojxvfe

#include <json/json.h>

class B {
};

int main() {

Json::Value invalid;
// incorrect and should never happen, since B isn't even a Json::Value, but the compiler takes it happily
invalid["data"] = new B();
}

Yes, it's bad code, but the compiler will take it happily without complaint, even with all the warnings turned on. As I said above, I feel like we should be striving to make it difficult to do something that is fundamentally bad.

Thanks for your time,
Jeremy