MSVC 2015 build fails when attempting to compare object_t

[erichkeane]

See #144 and #165 for examples of the build.

It seems that the following results in a test to see if a value is streamable in catch.hpp:
unit.cpp:2588 CHECK(*p1 == value.get<test_type>());

This causes a call to operator == in catch.hpp (17771):

template<typename RhsT>
ResultBuilder& operator == ( RhsT const& rhs ) {
return captureExpression<Internal::IsEqualTo>( rhs );
}

THAT causes a call to the captureExpression function on catch.hpp:1829

template<Internal::Operator Op, typename RhsT>
ResultBuilder& captureExpression( RhsT const& rhs ) {
    return m_rb
        .setResultType( Internal::compare<Op>( m_lhs, rhs ) )
         .setLhs( Catch::toString( m_lhs ) )
        .setRhs( Catch::toString( rhs ) )
        .setOp( Internal::OperatorTraits<Op>::getName() );
}

The Catch::toString results in a call to catch.hpp: 1731's toString->StringMaker:

template<typename T>
std::string toString( T const& value ) {
return StringMaker<T>::convert( value );
}

StringMaker simply offloads to Detail::StringMakerBase, but first goes through the Detail::IsStreamInsertable::value! This happens on 1575:

 template<typename T>
 struct IsStreamInsertable {
     static std::ostream &s;
     static T  const&t;
     enum { value = sizeof( testStreamable(s << t) ) == sizeof( TrueType ) };
 };

On Linux, this appears to realize there is NO overload for operator << that works, however MSVC doesn't seem to know that, and decides that it is an ambiguous call between just about every possible overload.

[erichkeane]

A few more comments before I call it a day:
1- I find it odd that the == operator is coming from catch, rather than comparing the items as std::map objects. Do we know what clang/gcc does with these?

2- It seems that the IsStreamInsertable intends for the proper overload to be:

FalseType operator<<(std::ostream const&, BorgType const&);

BorgType has a CTOR that should just work for that:

template<typename T> BorgType (T const &);

This should be an implcit conversion to BorgType, which should result in the operator << being called above. That version DOES come up in the function lookup (see error on line 82):

c:\projects\json\test\catch.hpp(1569): note: or       'Catch::Detail::FalseType Catch::Detail::operator <<(const std::ostream &,const Catch::Detail::BorgType &)'

however it for some reason doesn't realize it is convertable?

3- I'm not sure this is meaningful, but it seems that template-instance listed has T=test_type instead of the std::map that it always was before? It seems that on the referred to line that it is using the CLASS templateto captureExpression, though if the previous lines where it lists it as test_type, these should be the same??

[jinCN]

1. Because the CHECK(A == B) takes A==B as a macro parameter and turns it into a part of this kind of expression:
   __catchResult <=A==B
   __catchResult <=A saves A inside and then compares to B in a different way, using a different operator==.
2. This sounds not good. I don't think msvc can be like this. ^_^

[erichkeane]

Ah, thanks! I missed reading through the macro, and got caught up by the rest of it! Thanks! Hopefully that'll help my debugging today.

[jinCN]

As to the the familiar error C2593, It's just the compiler has more than one implicit convertion choices so it doesn't choose anymore. This may help, but I can not figure out where are the choices right now.

[erichkeane]

Ya, this one is pretty annoying. The super annoying part is that the compiler lists EVERY operator<< overload as ambigious, and I strongly suspect that it is only ambigious between a few choices. I am going to binary search/build through the code today I think, otherwise no real idea on how to proceed.

[jinCN]

No, you got it wrong. The list is just all the overloads the compiler search from, which means nothing.
The quickest way to find the one who conflict with BorgType is write these under same namespace of BorgType:
std::ostream s();
s<<T();//where T is the type you want to check
Then compiler will do a real check, then fail, then show a error that point out the conversion may be BorgType ,or be XXX, then say it is ambiguous.

[erichkeane]

So, here is something CRAZY interesting that makes me think we're getting some weird bug....

In Master, the existience of this unit test line on test/unit.cpp:11219
nlohmann::basic_json<std::map, std::vector, std::string, bool, int32_t, float> j;

causes the problem to happen. Additionally, the unordered_map test that was added in #165 does the exact same thing!

If I comment out those lines, (all of issue #89 on master), everything compiles fine!

The only thing I can think of is that since that is the ONLY line that customizes the basic_json object, that the compiler is getting confused about the type of object_t?

@mysyljr @nlohmann Any ideas/thoughts?

[erichkeane]

Additionally, changing that line to match the default template (so that it isn't actually specialized) makes it work. ANY difference int he template causes the ambiguous operator << issue.

[erichkeane]

@mysyljr : Sorry, missed your last comment until now. It didnt' come up until I refreshed github for some reason? What I meant was, in CLANG/G++ in an ambiguous overload situation it will list only the possible ones, not EVERY possible overload.

I'll try your suggestion.

[jinCN]

@erichkeane My point is that the "sizeof( testStreamable(s << t) ) }; " way makes only an ambiguous error. But the "s<<t" way makes a real execution and will tell what exactly can be the choices, one more error in fact.

[erichkeane]

Ah, good debug tip, thanks! I'll give it a try. I don't really get much experience with MSVC, so I am just used the clang/gcc error messages. Seems like compiler error message are a foreign language of their own.

[jinCN]

I'm using msvc2013 and it's not so much full of magic than gcc. It's only incompatible in details. When you found an error, maybe it's only gcc let it hides.

[jinCN]

And who designed the BorgType? I'm not sure if I'm right, but I think anything who can be implicit converted to a type and call operator<< will get a bug like this, which seems not fine. ^_^

[erichkeane]

In this case, both GCC and CLANG compile this properly (and both are well known to be extremely standards compliant), so I suspect that the issue is an oddity with MSVC.

I'll note that I added

 void p()
 {
     std::stringstream s;
     s << nlohmann::json::object_t{};
 }

to the namespace with the BorgType struct, and this added zero additional errors.

I DO notice that this only happens when there is multiple template specializations, and now that I look closer, lines 83-86 (below) in the error list seems to show that it is considering BOTH overloads of operator << via argument dependent lookup, which I believe is a mistake. I believe that the ones on json.hpp:4898 (1st and 3rd lines below) are the ones it is confused by, however my understanding of the lookup rules would make the 2nd set not valid, since they are different types.

C:\projects\json\src\json.hpp(4936): note: or       'std::ostream &nlohmann::operator <<(std::ostream &,const nlohmann::basic_json<std::map,std::vector,std::string,bool,int64_t,double,std::allocator> &)' [found using argument-dependent lookup]
C:\projects\json\src\json.hpp(5058): note: or       'std::istream &nlohmann::operator <<(nlohmann::basic_json<std::map,std::vector,std::string,bool,int64_t,double,std::allocator> &,std::istream &)' [found using argument-dependent lookup]
C:\projects\json\src\json.hpp(4936): note: or       'std::ostream &nlohmann::basic_json<std::map,std::vector,std::string,bool,int32_t,float,std::allocator>::operator <<(std::ostream &,const nlohmann::basic_json<std::map,std::vector,std::string,bool,int32_t,float,std::allocator> &)' [found using argument-dependent lookup]
C:\projects\json\src\json.hpp(5058): note: or       'std::istream &nlohmann::basic_json<std::map,std::vector,std::string,bool,int32_t,float,std::allocator>::operator <<(nlohmann::basic_json<std::map,std::vector,std::string,bool,int32_t,float,std::allocator> &,std::istream &)' [found using argument-dependent lookup]

[erichkeane]

BTW, BorgType was generated as a part of the Catch library, here: https://github.com/philsquared/Catch

[jinCN]

@erichkeane I really think it's a bug of Catch. I'll check it tommorrow.^_^
As to the type that conflicts, I believe that you've got it right. And it's suggesting nothing wrong with our codes.

[erichkeane]

Alright, cool. I've run out of ideas/thoughts on how to push further, I suspect my lack of experience with MSVC is holding me back. If you could post any further things you identify when you look at it tomorrow, I'd love the chance to poke at it some more with any additional ideas you get.

[jinCN]

It's not Catch library's fault. Because only if you define a conflicting implicit conversion operator<< overload in the same namespace of BorgType, you will get an ambiguous error, that happens when you hack into it not when you use it.

Now it suggests that even without BorgType, without CHECK(*p1 == value.get()), you will fail if you do a test "s<<t", for it's ambiguous at json library's design.

I thought it was a simple problem, but it was in fact a design problem. Maybe I should install a VS2015 to get a whole view.

[nlohmann]

Hi there! I really appreciate that you're digging into this issue. Unfortunately, I have no idea what goes wrong there... I would love to see the code compile with MSVC again, and if I can help, please let me know!

(And Happy New Year to all of you! 🎆)

[erichkeane]

@nlohmann:You're welcome! I had a few spare hours with a VM, but unfortunately couldn't come up with the solution. I PERSONALLY suspect that this is a defect with MSVC. For NOW, the work around is (on windows), only instantiate a single template of the basic_json object.

I believe that since it isn't a regression, that #144 should be able to be merged. You could merge #165 if you cared to, however the alternative proposal in #164 is a great solution that I can start working on (if you're OK with the API change).

Happy new year to you as well!

[jinCN]

This case failed for msvc:
nlohmann::basic_json< std::map, std::vector, std::string, bool, int32_t, float > j;
template< typename T >
static void convert(T const& _value) {
std::ostringstream oss;
oss << _value;

}

convert(json::object_t());

This line:
oss << _value; //wrong because _value can be converted to a nlohmann::basic_json< std::map, std::vector, std::string, bool, int32_t, float > or a nlohmann::basic_json< std::map, std::vector, std::string, bool, int64_t, double >, both have a operator<<.

I find that the problem is linked to implicit construction from object_t and friend operator<< in basic_json, but of course both are not the final root of the problem because both are simple and can't be wrong.

[jinCN]

Though I may not get it solved this day, I find that there are two bugs all together, for compiler stops when it meets the first one. If I can't solve them I'll post a new issue in detail. Here is where the second wrong in json.hpp:
template <class CompatibleArrayType, typename
std::enable_if<
not std::is_same<CompatibleArrayType, typename basic_json_t::iterator>::value and
not std::is_same<CompatibleArrayType, typename basic_json_t::const_iterator>::value and
not std::is_same<CompatibleArrayType, typename basic_json_t::reverse_iterator>::value and
not std::is_same<CompatibleArrayType, typename basic_json_t::const_reverse_iterator>::value and
not std::is_same<CompatibleArrayType, typename array_t::iterator>::value and
not std::is_same<CompatibleArrayType, typename array_t::const_iterator>::value and
std::is_constructible<basic_json, typename CompatibleArrayType::value_type>::value, int>::type
= 0>
basic_json(const CompatibleArrayType& val)
: m_type(value_t::array)
{
using std::begin;
using std::end;
m_value.array = create<array_t>(begin(val), end(val));
}

[erichkeane]

IMO (And in Clang/G++'s opinion), the ambigious line of os << _value; should not be ambigious, as the two are totally different types.

This is an issue with the MSVC compiler I suspect, though I'm not sure how to work around it.

Not sure the issue with the 2nd one, since I didn't see the error message with it.

[jinCN]

Just comment out the whole section: SECTION("pointer access to object_t"), then the 2nd error will show.
The ambigious line of os << _valueThere is ambigious because the _value need to be converted to basic_json to perform the action, when it can be converted to another version of basic_json, the functionality then breaks. The bug is that there shouldn't be a conversion from
basic_json< std::map, std::vector, std::string, bool, int64_t, double >::object_t
to
basic_json< std::map, std::vector, std::string, bool, int32_t, float >
but there is.

Now we need a way to detect which conversion caused the conflict.

[jinCN]

    template <class CompatibleArrayType, typename
              std::enable_if<
                  not std::is_same<CompatibleArrayType, typename basic_json_t::iterator>::value and
                  not std::is_same<CompatibleArrayType, typename basic_json_t::const_iterator>::value and
                  not std::is_same<CompatibleArrayType, typename basic_json_t::reverse_iterator>::value and
                  not std::is_same<CompatibleArrayType, typename basic_json_t::const_reverse_iterator>::value and
                  not std::is_same<CompatibleArrayType, typename array_t::iterator>::value and
                  not std::is_same<CompatibleArrayType, typename array_t::const_iterator>::value and
                  std::is_constructible<basic_json, typename CompatibleArrayType::value_type>::value, int>::type
              = 0>
    basic_json(const CompatibleArrayType& val)
        : m_type(value_t::array)
    {
        using std::begin;
        using std::end;
        m_value.array = create<array_t>(begin(val), end(val));
    }

Just beautify the code I pasted ✨ 😌

[jinCN]

Problem can be concluded to this:

std::is_constructible<
basic_json<std::map, std::vector, bool, int32_t, float, std::allocator>,
basic_json<std::map, std::vector, bool, int64_t, double, std::allocator>
>::value

is true somehow.
I use an older version of msvc2015 then the bug is gone. It only happens on my newly installed msvc2015. 😏

[jinCN]

I think my explanation still can't directly point to a compiler issue. Anyone can help to introduce the conversion relationship inside the json library? @erichkeane @nlohmann 😌

[gregmarr]

Well, that sounds like it could be the implicit conversion to the type enumeration and then construction from it.

[jinCN]

@gregmarr But I don't think so. I'll test.

[twelsby]

The problem can be minimally reproduced with this:

nhohmann::json x;
nhohmann::basic_json<std::map, std::vector, std::string, bool, int32_t, float> y { { "a", 1 } };
std::cout << y;

This is just in a main() - no Catch so it is either VS2015 or us.

Intellisense is actually more helpful than the compiler output and points directly to the compiler being unable to decide between two versions of operator<<, the 32 bit and the 64 bit.

Helping the compiler find the right one by explicitly casting using this:

nhohmann::json x;
nhohmann::basic_json<std::map, std::vector, std::string, bool, int32_t, float> y { { "a", 1 } };
std::cout << (nhohmann::basic_json<std::map, std::vector, std::string, bool, int32_t, float>) y;

gets it to work, but this is merely covering up the underlying issue which is that the compiler bizarrely can't seem to decide the right one even though it seems so obvious.

For what its worth I'm leaning towards it being the compiler although try as I might I can't reproduce the problem with a simplified version of the code outside of the json library.

Without fixing the problem, there are a few workarounds we could employ to get AppVeyor to work so that at least we can get all current tests to run under VS2015 (even the custom basic_json). We could overload the toString() functions of the problematic types in unit.hpp so that they no longer call operator<< but do something else useful for debugging - we could even explicitly cast the operator<< to get the original functionality back. We could also wrap the problematic CHECK expressions in parenthesis so that they evaluate before going into Catch - this also prevents the use of operator<< (or do the same for all CHECK statements by adding just one pair of parentheses in catch.hpp).

The problematic types are (in addtion array_t only it doesn't seem to get used in this way in the tests so it wouldn't have to be overloaded):

iterator
const_iterator
reverse_iterator
const_reverse_iterator
array_t::iterator
object_t

The other types are either primitive types that are already overloaded, and probably wouldn't cause a problem even if they weren't, the string type, and pointers that are dealt with differently in Catch.

The problematic CHECK statements are obviously those that involve these types and there are many so it would be preferable not to have to fix it that way.

Going down either road means that the tests would still function as tests, although the debugging info would be slightly less helpful (but we would still have line numbers). It would be possible to only switch on the fix for VS2015 using macros of course.

None of this solves the underlying issue however and VS2015 support would still be incomplete however even having multiple versions of basic_json is achievable with explicit casting so its not completely terrible .

[nlohmann]

@twelsby Thanks for the detailed analysis. So far, I would be happy if the code would "just compile" with MSVC. As it seems not to be an issue with catch, wrapping CATCH expressions with parentheses seems pragmatic enough. Did you try this locally? How many tests would be affected?

[twelsby]

@nlohmann, my local testing did not include modifying 100% of the affected CHECK statements - I only got to about 75% before I decided to try other things, but with the remaining 25% commented out it compiles. It also compiles with the parentheses applied in catch.hpp so I have absolutely no doubt it would work - its just the laborious job of making the changes. I don't think this is the best way to go though as I will explain.

To summarise some of the available workarounds and their implications (as I see it) more explicitly, they include:

Modify catch.hpp to wrap expr in paretheses
Change this line at line 2023 of catch.hpp so that expr becomes (expr):

( __catchResult <= expr ).endExpression();

Pros:

1. All current tests will function correctly as tests with all compilers.
2. The easiest imaginable modification - just two characters and it will compile under VS2015.
3. There is no possibility that any future test using CHECK we may add could ever break, irrespective of the object type.

Cons:

1. catch.hpp is not our code to modify so any time a new version of catch.hpp is issued it would have to be checked and modified accordingly. This would not be difficult as it is only two characters.
2. This change applies to all uses of CHECK even if they would not cause problems. This has the effect of reducing the quality of debugging information as I will explain further.

Modify all problematic CHECK invocations by wrapping the test in parentheses
As an example CHECK(it == j.begin()) becomes CHECK((it == j.begin()))

Pros:

1. All current tests will function correctly as tests with all compilers.
2. Only requires modification of our code.
3. More targeted - only affects those uses of CHECK that actually cause problems.

Cons:

1. The number of required modifications is HUGE - well into the hundreds (think of all the iterator comparisons in the form of the example above). I have done this for 75% of the version 2.0.0 unit.cpp on my repo but it would really have to be done on 1.0.1 as well.
2. Future tests using CHECK that uses different types (such as array_t) in different ways could cause problems. This is not a huge problem as they could just be wrapped in parentheses as well.
3. The same problem of reduced debug info quality would apply as for the first workaround.

Provide overloads for Check::toString() for the problematic types
This is even hinted at in the source for catch.hpp. It would require adding this to the top of unit.cpp:

#ifdef _MSC_VER
namespace Catch {
    std::string toString(json::iterator value) { return Detail::unprintableString; }
    std::string toString(json::const_iterator value) { return Detail::unprintableString; }
    std::string toString(json::reverse_iterator value) { return Detail::unprintableString; }
    std::string toString(json::const_reverse_iterator value) { return Detail::unprintableString; }
    std::string toString(json::array_t::iterator value) { return Detail::unprintableString; }
    std::string toString(json::object_t value) {
        return (static_cast<std::ostringstream&>(std::ostringstream() << static_cast<json>(value))).str();
    }
}
#endif

Pros:

1. All current tests will function correctly as tests with all compilers.
2. Only requires modification of our code.
3. A much easier modification than the second workaround and only slightly more difficult than the first.
4. More targeted - only affects those types that actually cause problems.
5. Only applies the fix to VS2015.
6. Preserves the quality of debug info because this replicates the normal behaviour under non-VS2015.

Cons:

1. Future tests that use different types (such as array_t) in different ways could cause problems. This is not a huge problem as additional overloads could be added.
2. If additional unit tests were added that use operator<< in connection with the custom basic_json object then more work would be needed in the overload for object_t.

Loss of debug quality
The loss of debug quality that is the result of wrapping the CHECK expression in parentheses arises from the fact that Catch can no longer provide the evaluation of the LHS and RHS of the expression (either way you still get line numbers and the string representation of the expression in its entirety so its not all bad). This is not meaningful for the iterators anyway and probably not too important for object_t. It is most important for the primitive types where you really do want to know the numerical value of both sides of the expression - but primitive types don't cause a problem and therefore aren't affected by workaround two so the loss of debug quality is only really a problem with the first workaround.

For the record I prefer workaround three (as you can probably tell).

Once it has been successfully built all unit tests pass by the way. My local build settings and the unit test parameters were as per the AppVeyor.yml file and I used the current master versions of all relevant files except for the modifications noted. I used VS2015 14.0.24720.00 Update 1 with compiler version 19.0.23506.0.

[twelsby]

If you want I can add a commit with this code to my pull request and it will show the AppVeyor results or submit another pull request (via a different branch). I just don't know how to do it without the version 2.0.0 changes short of reverting them, applying this code and then reapplying them (it would be nice if github allowed two forks).

[twelsby]

Well I have some more insight into the underlying problem... and another possible fix.

I wish I could claim that it was due to my deep understanding of c++ or inherent genius but alas this is not so because I cheated and took the path of least resistance.

The answer is commit 457bfc2 - this is where it all went wrong. It was introduced to solve the problem of implicit conversion to strings (issue #144), per:

s2 = o["name"];

To achieve this additional conditions were placed on the instantiation of operator ValueType() const in order to prevent another ambiguity, namely:

not std::is_same<ValueType, typename string_t::value_type>::value
not std::is_same<ValueType, std::initializer_list<typename string_t::value_type>>::value

Comment out the second one and all is well. Compilation works under VS2015 and all tests pass. It doesn't even affect the fix for #144.

Now could someone who does have a deep understanding of c++ or inherent genius please explain why this works. Don't be modest.

Of course this 'fix' introduces its own problem because gcc (and no doubt clang) still complains about the line from issue #144. The obvious workaround is to omit adding the extra condition for only VS2015 by wrapping it in a #ifndef _MSC_VER. Not pretty but effective. Then we are back to 100% Visual Studio support.

I get the feeling there is an even better solution out there that doesn't require conditional compilation.

By the way, I am now getting floating point -Wfloat-equal warnings at 2913 and 2917 which is in the new code from commit 663ad13. They are typical gcc paranoia so probably could be ignored, suppressed or whatever (its not like we are adding floats like 0.8+0.1 != 0.9 - comparing a float with one you saved earlier should not be unsafe because there is no possibility for rounding error).

[twelsby]

Rather than incorporate this in pull #183, I have split it off so that it can be merged for version 1.0.1 (what I should have done to begin with).

[nlohmann]

Fixed with #188.