Build errors with VS 15.3 and /permissive- conformance switch

[lmdsp]

With Visual Studio 15.3 which has just been released, enabling the /permissive- conformance switch results in build errors.
This switch is useful to adhere strictly to the C++ standard, and it also helps enforcing cross-platform compatibility, reducing the chance to write code that will build under VS but not under clang/gcc for example.

It seems MS have enforced stricter rules as this was building fine in VS 15.2.

The error(s) appear in example\cpp11\non_def_con_class.cpp:

non_def_con_class.cpp
\include\msgpack/v1/object_fwd.hpp(66): error C2039: 'value': is not a member of 'msgpack::v1::type'
\include\msgpack/v1/adaptor/detail/cpp11_define_map.hpp(22): note: see declaration of 'msgpack::v1::type'
\include\msgpack/v2/object_fwd.hpp(97): note: see reference to class template instantiation 'msgpack::v1::has_as<T>' being compiled
1>        with
1>        [
1>            T=int
1>        ]
example\cpp11\non_def_con_class.cpp(37): note: see reference to class template instantiation 'msgpack::v2::has_as<int>' being compiled
\include\msgpack/v1/object_fwd.hpp(66): error C2065: 'value': undeclared identifier
\include\msgpack/v1/object_fwd.hpp(66): error C2131: expression did not evaluate to a constant
\include\msgpack/v1/object_fwd.hpp(66): note: failure was caused by non-constant arguments or reference to a non-constant symbol
\include\msgpack/v1/object_fwd.hpp(66): note: see usage of 'value'
\include\msgpack/v2/object_fwd.hpp(98): error C2039: 'value': is not a member of 'msgpack::v2::type'
\include\msgpack/v2/adaptor/detail/cpp11_define_map_decl.hpp(19): note: see declaration of 'msgpack::v2::type'
\include\msgpack/v2/object_fwd.hpp(98): error C2065: 'value': undeclared identifier
\include\msgpack/v2/object_fwd.hpp(98): error C2131: expression did not evaluate to a constant
\include\msgpack/v2/object_fwd.hpp(98): note: failure was caused by non-constant arguments or reference to a non-constant symbol
\include\msgpack/v2/object_fwd.hpp(98): note: see usage of 'value'
example\cpp11\non_def_con_class.cpp(37): warning C4305: 'specialization': truncation from 'const bool *' to 'bool'
example\cpp11\non_def_con_class.cpp(37): error C2672: 'msgpack::v1::object::as': no matching overloaded function found
example\cpp11\non_def_con_class.cpp(37): error C2770: invalid explicit template argument(s) for 'std::enable_if<msgpack::v2::has_as<T>::value,T>::type msgpack::v1::object::as(void) const'
\include\msgpack/v1/object_fwd.hpp(121): note: see declaration of 'msgpack::v1::object::as'
1>Done building project "non_def_con_class.vcxproj" -- FAILED.

[redboltz]

@lmdsp , thank you for reporting the issue.

The error message indicates the following code is invalid.

include/msgpack/v1/object_fwd.hpp

template <typename T>
struct has_as {
private:
    template <typename U>
    static auto check(U*) ->
        // Check v1 specialization
        typename std::is_same<
            decltype(adaptor::as<U>()(std::declval<msgpack::object>())),
            T
        >::type;
    template <typename>
    static std::false_type check(...);
public:
    using type = decltype(check<T>(MSGPACK_NULLPTR));
    static constexpr bool value = type::value; // line 66 'value': is not a member of 'msgpack::v1::type'
};

https://github.com/msgpack/msgpack-c/blob/master/include/msgpack/v1/object_fwd.hpp#L65

However, the type type is defined in the line 65.

    using type = decltype(check<T>(MSGPACK_NULLPTR));

Compiler doesn't use this one but tying to looking for msgpack::v1::type.

type is defined as a namespace. See

msgpack-c/include/msgpack/v2/adaptor/nil_decl.hpp

Line 21 in 87ff7e4

namespace type {

I think that the type type introduced by using type = decltype(check<T>(MSGPACK_NULLPTR)); should be used.

[lmdsp]

@redboltz , thanks for your insight.

According to your remarks, fixing this would be a simple matter of replacing

using type = decltype(check<T>(MSGPACK_NULLPTR));
static constexpr bool value = type::value;

with

using t_type = decltype(check<T>(MSGPACK_NULLPTR));
static constexpr bool value = t_type::value;

I'll test this tomorrow, what do you think ?

[redboltz]

@lmdsp , I installed Visual Studio 15.3 and got the same error.

I wrote a (maybe) minimal code that reproduces the problem.

See

#include <type_traits>

namespace type {}

template <typename T>
struct test {
	static std::true_type check();
	using type = decltype(check());
	static constexpr bool value = type::value;
};

int main() {
	test<int>();
}

I reported the issue to Microsoft (developers community).

I think that it is msvc's bug. /permissive- is not implemented correctly.
clang++ and g++ with -pedantic option doesn't report any errors.

I'll test this tomorrow, what do you think ?

I think that it works. In my minimal example, if I replaced namespace type {} with namespace type2 {}, the error is disappeared.

I'm waiting Microsoft (developers community)'s response. If it is a bug of MSVC, I don't want to modify the code.

[redboltz]

The report title is [C++] invalid name lookup if /permissive- is set.
I couldn't get URL of the report. It is integrated to Visual Studio. If you vote the report from Visual Studio, the priority of the report might be higher.

[lmdsp]

Thanks, I'm a bit ouf of my depth on this one - I suppose I could dig in through the standard papers to check the rules of precedence / priority of namespaces over typedefs to check.

I know MS have a history of sometimes 'interpreting' the standard in quite surprising ways, but lately the VS compiler team has done a great deal of work to improve conformance and stay up to date with the latest C++ 17.

Anyway I've upvoted your report so it hopefully draws some attention. There seems to be a few similar reports, so hopefully this will get fixed soon.

[hiradyazdan]

Hi @redboltz I seem to be having a similar issue. I am trying to use msgpack-cxx through msgpack.hpp header (#include <msgpack.hpp>) using msvc (VS 2022 C++17) - Windows 10, tried both with or without boost, but then it throws compile errors on this specific file (i.e. object_fwd.hpp) for each version (v1, v2, v3):

msgpack-cxx\include\msgpack\v1\object_fwd.hpp(56): error C2988: unrecognizable template declaration/definition
msgpack-cxx\include\msgpack\v1\object_fwd.hpp(67): note: see reference to class template instantiation 'msgpack::v1::has_as<T>' being compiled
msgpack-cxx\include\msgpack\v1\object_fwd.hpp(56): error C2059: syntax error: '<end Parse>'
msgpack-cxx\include\msgpack\v1\object_fwd.hpp(56): error C2334: unexpected token(s) preceding '{'; skipping apparent function body
.
.
.
msgpack-cxx\include\msgpack\v1\object_fwd.hpp(63): error C2988: unrecognizable template declaration/definition
msgpack-cxx\include\msgpack\v1\object_fwd.hpp(63): error C2059: syntax error: '<end Parse>'
msgpack-cxx\include\msgpack\v1\object_fwd.hpp(63): error C2334: unexpected token(s) preceding '{'; skipping apparent function body
.
.
.

I tried both cpp_master branch directly and the released pkgs (6.1.0 & 6.0.0).

It basically complains about variadic or any other templates. My impression is you've added support for C++11 on vs2015 features as well as C++03 for compatibility (noticed your fix for vs2015 and lower versions mentioned in #463). Also, followed up on the issue mentioned above as well as #199, #193 and #177, that's quite similar to mine, and even though I made sure /permisssive- conformance switch is disabled, the errors still pop up. So I am at loss in this. I'd appreciate if you could help figure out this issue.

[redboltz]

@hiradyazdan

I think that you can try the exact same step as

msgpack-c/.github/workflows/gha.yml

Lines 191 to 239 in df1d126

	windows:
	name: ${{ format('Windows cxx{0}', matrix.cxx) }}
	runs-on: windows-2019
	strategy:
	fail-fast: false
	matrix:
	# MSVC2019 only supports /std:c++14, /std:c++17 and /std:c++latest
	cxx: [14, 17, 20]
	pp_flag: ["/Zc:preprocessor-", "/Zc:preprocessor"]
	steps:
	- uses: actions/checkout@v2
	- name: Cache vcpkg dependencies
	id: cache-vcpkg
	uses: actions/cache@v2
	with:
	path: C:/vcpkg/installed/x64-windows
	key: ${{ runner.os }}-vcpkg-2021-08-09
	- name: Install vcpkg dependencies
	if: steps.cache-vcpkg.outputs.cache-hit != 'true'
	shell: powershell
	run: |
	vcpkg update
	vcpkg install zlib:x64-windows
	vcpkg install boost-assert:x64-windows boost-numeric-conversion:x64-windows boost-variant:x64-windows boost-utility:x64-windows boost-fusion:x64-windows boost-optional:x64-windows boost-predef:x64-windows boost-preprocessor:x64-windows boost-timer:x64-windows boost-test:x64-windows
	- name: Build and test
	shell: powershell
	run: |
	$CPPVER="MSGPACK_CXX${{ matrix.cxx }}=ON"
	md build
	cmake `
	-A x64 `
	-G "Visual Studio 16 2019" `
	-D CMAKE_TOOLCHAIN_FILE="C:/vcpkg/scripts/buildsystems/vcpkg.cmake" `
	-D MSGPACK_BUILD_TESTS=ON `
	-D $CPPVER `
	-D CMAKE_CXX_FLAGS="${{ matrix.pp_flag }} /D_VARIADIC_MAX=10 /EHsc /D_SILENCE_TR1_NAMESPACE_DEPRECATION_WARNING /D_SILENCE_CXX17_ALLOCATOR_VOID_DEPRECATION_WARNING /D_SILENCE_CXX17_OLD_ALLOCATOR_MEMBERS_DEPRECATION_WARNING /W3 /WX" `
	-B build `
	-S .
	if ($LastExitCode -ne 0) { exit $LastExitCode }
	cmake --build build --config Release
	if ($LastExitCode -ne 0) { exit $LastExitCode }
	ctest -VV --test-dir build -C Release
	if ($LastExitCode -ne 0) { exit $LastExitCode }

on msvc2022 and msvc2019. Then fill the result as follows:

compiler	result
msvc2019
msvc2022

If msvc2019 reports the C2988 error, then the next step is checking the difference between your environment and CI(success).
If msvc 2019 doesn't reports the C2988 error, but msvc2022 reports error, it could be msvc2022 degrate issue.

[hiradyazdan]

@redboltz after some investigation I found this issue is also another example of naming conflict as mentioned earlier above, but this time with other libraries macro definitions, since the function name check is a common name, despite being within msgpack namespace, it still can cause ambiguous definitions. It will really help if you could rename the function name to something more specific to the library like mpc_check as it's only used within the same file (object_fwd.hpp) for each version (v1, v2, v3). This change can also resolve 1050

[redboltz]

I know check is common name, but it is private member function template of has_as class template int msgpack namespace. If it is not allowed, C++ naming rule could be destroyed. I guess that this is the tip of an iceberg.
If I understand correctly, it is MSVC problem but not msgpack-c problem. Please consider reporting MSVC issue to somewhere (Microsoft forlum of official suport) with https://stackoverflow.com/help/minimal-reproducible-example.

[hiradyazdan]

It is not msvc problem, as I mentioned it is a problem for other libraries to be used with it if they have macro definitions since macros can cause ambiguity/conflicts as have precedence over functions (regardless of being private and namespaced) when processed/compiled. As I mentioned this issue was raised before in your repo, and causes major problems when used with Unreal Engine as they have check macro definition in their core engine source (i.e. AssertionMacros.h) and simply cannot be modified.

[redboltz]

Do you mean if someone include AssertionMacros.h, then all check that is located after the header are replaced?

For example,

#include <AssertionMacros.h>

int main () {
    bool check = true;
    if (check) {
       // ...
    }
}

[hiradyazdan]

Yes exactly. But not a variable. Only functions. You can replicate this as below:

#ifndef check
  #define check(expr);
#endif

template <typename U>
static auto check(U*) ->
    typename std::is_same<
        decltype(adaptor::as<U>()(std::declval<msgpack::object>())),
        T
    >::type;
template <typename...>
static std::false_type check(...);

[redboltz]

I think that the macro definition of AssertionMacros.h has serious problem. It is very strange that requesting all other libraries to care for bad designed macro definition of AssertionMacros.h.

However, I consider a possible solution:

your_app.cpp

#include <msgpack.hpp> // Always before AssertionMacros.h
#include <AssertionMacros.h> // and other headers that couldi include this.

The concept is all your translation units ( .cpp files) include msgpack.hpp on the top of the file.

If your code is not an application but a library, this approach should work too.

E.g.)

your_lib1.hpp

#include <msgpack.hpp> // Always before AssertionMacros.h
#include <AssertionMacros.h> // and other headers that couldi include this.

your_lib2.hpp

#include <msgpack.hpp> // Always before AssertionMacros.h
#include <AssertionMacros.h> // and other headers that couldi include this.

users_app.cpp

#include <your_lib1.hpp>
#include <your_lib2.hpp> // msgpack.hpp is not included thanks to include guard

[hiradyazdan]

This won't work since in so many libraries incl. UE, you can't change the order of includes as they're deep in the engine/library source and the way they're built and used is that you should need to have their core engine run first in order to be able to add/run your own implementation. I'm not sure why changing function name in this library is a big hassle as it's only privately used and literally no one requires it except perhaps for internal tests or am I missing something? This fix can impact a lot of Unreal game engine developers and companies since msgpack can finally be used with it without hassle of writing their own serializers to match the spec.

[redboltz]

Ok, if you post the PR that replaces check with check_, I would merge it.
That's my final answer.

Here are my thoughts:

I'm not sure why changing function name in this library is a big hassle as it's only privately used and literally no one requires it except perhaps for internal tests or am I missing something

It is because my policy. Easy to fix is not related.
I don't want to help the bad code infration. At releast AssertionMacros.h macro definition is bad and arrogant.
Because it requires changing the function name for all other libraries in the world that conflict the name. This issue is one example. This time, luckyly it is a private member function, but if it were public one, request change like the username "X" in twitter?

I don't accept its design. See C++ books. These kinds of macro is very bad practice. I think AssertionMacros.h should be fixed.
I really expect the design of AssertionMacros.h would be fixed in the future.

I've just checked similar issue is reported and found it.
https://forums.unrealengine.com/t/ue-macros-and-3rd-party-library-conflicts/397848/4
https://github.com/hiili/UnrealMacroNuke

[hiradyazdan]

I totally understand and accept your argument. You're on the spot on this policy and I did not mean to offend. I carry the same frustration about UE and the way the market is going. Thanks for the links, yes we could use those and nuke the macros, but in this case it was much simpler to have this minor change than having to deal with a extremely bloated engine which at any point things can go very wrong (by disabling warnings etc.). Thank you for this and I'll make a PR soon.

[redboltz]

Thank you for the comment. I know you are not AssertionMacros.h developer. We both a kind of victims.
Please send some feedback to Unreal Engine to improve it, if you didn't send yet.