ned14 / outcome

Provides very lightweight outcome<T> and result<T> (non-Boost edition)
https://ned14.github.io/outcome
Other
674 stars 62 forks source link

hooks.cpp, expected-pass.cpp fail on msvc-14.2; expected-pass.cpp fails on msvc-14.3 #273

Closed pdimov closed 1 year ago

pdimov commented 1 year ago

I'm trying to setup CI for the Boost superproject, and the Windows CMake tests are failing because of Outcome under both windows-2019 and windows-2022 images. I can replicate the same errors using b2, so the issue isn't CMake-specific. b2 toolset=msvc-14.2 fails with

compile-c-c++ ..\..\bin.v2\libs\outcome\test\hooks.test\msvc-14.2\debug\threading-multi\tests\hooks.obj
hooks.cpp
test\tests\hooks.cpp(73): error C2280: 'boost::outcome_v2::basic_result<int,hook_test::error_code,boost::outcome_v2::policy::exception_ptr_rethrow<R,S,void>>::basic_result<int,void>(T &&,boost::outcome_v2::basic_result<R,S,boost::outcome_v2::policy::exception_ptr_rethrow<T,S,void>>::implicit_constructors_disabled_tag)': attempting to reference a deleted function
        with
        [
            R=int,
            S=hook_test::error_code,
            T=int
        ]

and

compile-c-c++ ..\..\bin.v2\libs\outcome\test\expected-pass.test\msvc-14.2\debug\threading-multi\expected-pass.obj
expected-pass.cpp
test\expected-pass.cpp(592): error C2664: 'stde::expected<int,std::exception_ptr>::expected(const boost::outcome_v2::success_type<void> &)': cannot convert argument 1 from 'boost::outcome_v2::failure_type<std::exception_ptr,void>' to 'const boost::outcome_v2::success_type<void> &'

and b2 toolset=msvc-14.3 fails with

compile-c-c++ ..\..\bin.v2\libs\outcome\test\expected-pass.test\msvc-14.3\debug\threading-multi\expected-pass.obj
expected-pass.cpp
test\expected-pass.cpp(592): error C2665: 'stde::expected<int,std::exception_ptr>::expected': none of the 4 overloads could convert all the argument types

If these failures are expected, would it be possible to #ifdef these tests out for MSVC?

ned14 commented 1 year ago

If you have a look at https://www.boost.org/development/tests/develop/developer/outcome.html, for some MSVC's it passes and for other MSVC's it fails. This has always been the case since Outcome entered Boost, about half the MSVC test runners fail those tests. I have always assumed that the cause was the test runner setup, as I have never been able to reproduce the failure on my hardware. I would suggest maybe investigating what differences there are, and if we can figure those out, we can either ifdef for those combinations or have the test runners fixed or something along those lines.

I wouldn't want to disable the tests on MSVC for all MSVCs because they work perfectly fine on "normal" MSVCs, where "normal" is defined as "my system".

You mentioned you could reproduce the errors yourself locally? I have never successfully achieved this. I don't suppose you could tell me how you configured your system, such that I might be able to replicate exactly your system and then I could reproduce these failures locally?

pdimov commented 1 year ago

I don't remember these tests ever passing for me locally, whenever I tried them, no matter on what MSVC update. But my msvc-14.3 just updated itself to 17.4.0, so I'll give them a try again.

pdimov commented 1 year ago

Same result as before, two failures,

testing.capture-output ..\..\bin.v2\libs\outcome\test\issue0255.test\msvc-14.3\debug\threading-multi\issue0255.run
====== BEGIN OUTPUT ======
Test setup error: test tree is empty

EXIT STATUS: 200
====== END OUTPUT ======

and

compile-c-c++ ..\..\bin.v2\libs\outcome\test\expected-pass.test\msvc-14.3\debug\threading-multi\expected-pass.obj
expected-pass.cpp
test\expected-pass.cpp(592): error C2665: 'stde::expected<int,std::exception_ptr>::expected': no overloaded function could convert all the argument types
test\expected-pass.cpp(95): note: could be 'stde::expected<int,std::exception_ptr>::expected(const boost::outcome_v2::success_type<void> &)', which inherits 'boost::outcome_v2::basic_result<T,E,boost::outcome_v2::policy::throw_bad_result_access<E,void>>::basic_result(const boost::outcome_v2::success_type<void> &) noexcept(<expr>)' via base class 'stde::detail::enable_default_constructor<T,E>'
        with
        [
            T=int,
            E=std::exception_ptr
        ]
test\expected-pass.cpp(592): note: 'stde::expected<int,std::exception_ptr>::expected(const boost::outcome_v2::success_type<void> &)': cannot convert argument 1 from 'boost::outcome_v2::failure_type<std::exception_ptr,void>' to 'const boost::outcome_v2::success_type<void> &'
test\expected-pass.cpp(592): note: Reason: cannot convert from 'boost::outcome_v2::failure_type<std::exception_ptr,void>' to 'const boost::outcome_v2::success_type<void>'
test\expected-pass.cpp(592): note: No user-defined-conversion operator available that can perform this conversion, or the operator cannot be called
test\expected-pass.cpp(95): note: or       'stde::expected<int,std::exception_ptr>::expected(boost::outcome_v2::failure_type<T,void> &&,boost::outcome_v2::basic_result<T,E,boost::outcome_v2::policy::throw_bad_result_access<E,void>>::explicit_make_exception_ptr_compatible_move_conversion_tag)', which inherits 'boost::outcome_v2::basic_result<T,E,boost::outcome_v2::policy::throw_bad_result_access<E,void>>::basic_result(boost::outcome_v2::failure_type<T,void> &&,boost::outcome_v2::basic_result<T,E,boost::outcome_v2::policy::throw_bad_result_access<E,void>>::explicit_make_exception_ptr_compatible_move_conversion_tag) noexcept(<expr>)' via base class 'stde::detail::enable_default_constructor<T,E>'
        with
        [
            T=int,
            E=std::exception_ptr
        ]
test\expected-pass.cpp(592): note: 'stde::expected<int,std::exception_ptr>::expected(boost::outcome_v2::failure_type<T,void> &&,boost::outcome_v2::basic_result<T,E,boost::outcome_v2::policy::throw_bad_result_access<E,void>>::explicit_make_exception_ptr_compatible_move_conversion_tag)': expects 2 arguments - 1 provided
        with
        [
            T=int,
            E=std::exception_ptr
        ]

plus quite a bit more of output.

I didn't configure anything in any special way; just installed VS2022 and then

C:\boost-git\develop\libs\outcome>b2 test toolset=msvc-14.3
pdimov commented 1 year ago

FWIW, with clang-cl from the VS2022 installation, three tests fail.

testing.capture-output ..\..\bin.v2\libs\outcome\test\issue0255.test\clang-win-15.0.1\debug\threading-multi\issue0255.run
====== BEGIN OUTPUT ======
Test setup error: test tree is empty

EXIT STATUS: 200
====== END OUTPUT ======
compile-c-c++ ..\..\bin.v2\libs\outcome\test\issue0059.test\clang-win-15.0.1\debug\threading-multi\tests\issue0059.obj
test\tests\issue0059.cpp(53,48): error: no matching constructor for initialization of 'result<udt>' (aka 'basic_result<udt, boost::system::error_code, boost::outcome_v2::policy::error_code_throw_as_system_error<udt, boost::system::error_code, void>>')
    auto g = [niall]() -> result<udt> { return {niall}; };
                                               ^~~~~~~
compile-c-c++ ..\..\bin.v2\libs\outcome\test\swap.test\clang-win-15.0.1\debug\threading-multi\tests\swap.obj
In file included from test\tests\swap.cpp:30:
In file included from ..\..\boost/outcome.hpp:32:
In file included from ..\..\boost/outcome/iostream_support.hpp:34:
In file included from ..\..\boost/outcome/outcome.hpp:35:
In file included from ..\..\boost/outcome/boost_outcome.hpp:34:
In file included from ..\..\boost/outcome/std_outcome.hpp:34:
..\..\boost/outcome/basic_outcome.hpp(998,13): error: 'boost::outcome_v2::basic_outcome<ErrorCode, Throwy<false, false>, ErrorCode2, boost::outcome_v2::policy::all_narrow>::swap(boost::outcome_v2::basic_outcome<ErrorCode, Throwy<false, false>, ErrorCode2, boost::outcome_v2::policy::all_narrow> &)::_::~_()::_::all_good' is not a member of class '_'
        if(!all_good)
            ^
pdimov commented 1 year ago

If you have a look at https://www.boost.org/development/tests/develop/developer/outcome.html, for some MSVC's it passes and for other MSVC's it fails.

Looking at that, the only pass is for msvc-14.3 with /std:c++latest. msvc-14.1, msvc-14.2, msvc-14.3 without latest all fail in the way I'm seeing.

pdimov commented 1 year ago

msvc-14.3 with c++17 passes expected-pass.cpp, but still fails issue0255.cpp. msvc-14.3 with c++20 passes all.

ned14 commented 1 year ago

Is it /std:c++latest, or is it the /permissive- which /std:c++latest forces on? Also, are you using the 32 bit or 64 bit MSVC binaries? So, not the binary type being compiled, but rather the MSVC compiler itself. I have found in the past the 32 bit one tends to be more sensitive to whatever (I suspect) memory corruption the compiler internally does to itself, due to the much decreased internal address space.

pdimov commented 1 year ago

Whatever b2 defaults to. I definitely don't think it's memory corruption.

Not /permissive-; same two tests fail with that flag as well, when using the default c++14, and the results with the other c++ modes are likewise identical.

ned14 commented 1 year ago

Alright, let me set up a fresh windows VM with fresh VS2019 and see if I can replicate your experience. You in a rush to get this fixed?

pdimov commented 1 year ago

Not particularly. For now I've just disabled the Outcome tests on the Windows job: https://github.com/boostorg/boost/blob/6df9e61d1a5ccd0661471ba21f2715781582f980/.github/workflows/ci.yml#L192

BurningEnlightenment commented 1 year ago

I've just observed a build failure similar to the hooks.cpp one with llfio:

C:\PROGRA~2\MICROS~1\2019\ENTERP~1\VC\Tools\MSVC\1429~1.301\bin\Hostx64\x64\cl.exe   /TP -DLLFIO_DISABLE_OPENSSL=1 -DLLFIO_ENABLE_TEST_IO_MULTIPLEXERS=1 -DLLFIO_EXPERIMENTAL_STATUS_CODE=1 -DLLFIO_INCLUDE_STORAGE_PROFILE=1 -DLLFIO_SOURCE=1 -DLLFIO_STATIC_LINK=1 -DOUTCOME_USE_SYSTEM_STATUS_CODE=1 -DQUICKCPPLIB_ENABLE_VALGRIND=1 -D_WIN32_WINNT=0x601 -external:ID:\devel\source\deeplex\deeplog\build\x64-windows-clang\vcpkg_installed\x64-windows-static\include -external:W0 /nologo /DWIN32 /D_WINDOWS /W3 /utf-8   /MP /utf-8 /Zc:__cplusplus /std:c++20 /D_DEBUG /MTd /Z7 /Ob0 /Od /RTC1  /EHsc /MTd /W4 /showIncludes /FoCMakeFiles\llfio_sl.dir\src\llfio.cpp.obj /FdCMakeFiles\llfio_sl.dir\llfio_sl.pdb /FS -c D:\devel\source\vcpkg\buildtrees\llfio\src\6fd1a48a8c-81198ed700.clean\src\llfio.cpp
cl : Command line warning D9025 : overriding '/W3' with '/W4'
D:\devel\source\vcpkg\buildtrees\llfio\src\6fd1a48a8c-81198ed700.clean\include\llfio\v2.0\detail/impl/windows/directory_handle.ipp(302): error C2280: 'outcome_v2::basic_result<llfio_v2_85eb6946::directory_handle::buffers_type,llfio_v2_85eb6946::file_io_error,outcome_v2::experimental::policy::status_code_throw<R,S,void>>::basic_result<llfio_v2_85eb6946::directory_handle::buffers_type>(T &&,outcome_v2::basic_result<R,S,outcome_v2::experimental::policy::status_code_throw<T,S,void>>::implicit_constructors_disabled_tag)': attempting to reference a deleted function
        with
        [
            R=llfio_v2_85eb6946::directory_handle::buffers_type,
            S=llfio_v2_85eb6946::file_io_error,
            T=llfio_v2_85eb6946::directory_handle::buffers_type
        ]
D:\devel\source\deeplex\deeplog\build\x64-windows-clang\vcpkg_installed\x64-windows-static\include\outcome\experimental\../basic_result.hpp(398): note: see declaration of 'outcome_v2::basic_result<llfio_v2_85eb6946::directory_handle::buffers_type,llfio_v2_85eb6946::file_io_error,outcome_v2::experimental::policy::status_code_throw<R,S,void>>::basic_result'
        with
        [
            R=llfio_v2_85eb6946::directory_handle::buffers_type,
            S=llfio_v2_85eb6946::file_io_error
        ]

I only stumbled accross this, because vcpkg erroneously selected VS 2019 (16.11.23/19.29.30147) instead of VS 2022. The error persists if you add /permissive to or replace /std:c++20 with /std:c++latest in the above command. Given that clang-cl and cl 19.34.31937 don't reject above code I'd guess it is a concepts bug in cl 19.29.30147.

ned14 commented 1 year ago

Not particularly. For now I've just disabled the Outcome tests on the Windows job: https://github.com/boostorg/boost/blob/6df9e61d1a5ccd0661471ba21f2715781582f980/.github/workflows/ci.yml#L192

@pdimov Can you undisable Outcome on Windows CI now please as https://github.com/boostorg/outcome/pull/5 contains many fixes and improvements. Thanks to @alandefreitas for doing the donkey work there as I didn't have the time. In any case, if CI is reenabled, I can test getting it fixed completely for the next Boost release.

pdimov commented 1 year ago

This PR is still not merged so I can't test whether the issues are fixed.

ned14 commented 1 year ago

You mean https://github.com/boostorg/outcome/pull/5?

It can't be merged as boostorg/outcome is script generated.

If you undisable Outcome on Windows CI over the next few weeks I will be merging parts of said PR translated into the pre-script generated Outcome and watching the Boost regression matrix until it goes all green. This is why I ask for it to be undisabled.

pdimov commented 1 year ago

The above doesn't affect the Boost regression matrix; it's the Github Actions CI for the superproject that only runs on tags (that is, on beta and on release.) So it wouldn't matter much whether I undisable it there or not.

ned14 commented 1 year ago

Ah okay. So the lack of MSVC runners at https://www.boost.org/development/tests/develop/developer/outcome.html has nothing to do with your CI disable?

Now I click around that matrix into other projects, it seems we no longer run MSVC runners for any project. I didn't realise that.

Ok, I'll start merging bits from Alan's work in and pushing them through into boostorg edition and see how his new CI stuff reveals things. It is deeply unhelpful that Boost.Outcome works just fine with my VS2019 here, even if it doesn't for others.

ned14 commented 1 year ago

@pdimov Can I confirm that the CI failure results at https://github.com/boostorg/outcome/actions/runs/4789737947 match your expectations?

What would be your preferred mechanism for me disabling the tests for MSVC 14.2 and GCC 10 such that your superproject CI would pass? I assume it doesn't read the regression tests blacklist, so there is surely an alternative mechanism?

pdimov commented 1 year ago

I see the same errors under msvc-14.2 in c++14 mode. However your CI doesn't test msvc-14.3 in c++14 mode, which still has one of the two failures (the stde::expected one).

The only way to reliably make these "go away" under both b2 and CMake would be to ifdef the failing tests in the source code, based on the appropriate conditions.

ned14 commented 1 year ago

Ok, I'll fix up the CI for MSVC14.3 in C++ 14 mode.

I'm not keen on disabling the tests for MSVC. They don't fail here, to be specific, and I don't know why they're failing elsewhere, and it feels wrong to disable them unilaterally as a result. I don't suppose you define an environment variable or something similar when your superproject CI runs? Then I could key test disablement based on it being run in your specific environment, whilst not disabling it elsewhere.

I'm keen you see that the MSVC devs see the test failure when they test MSVC against Boost releases, though I suspect given that they already do that they're also not seeing failures same as me.

ned14 commented 1 year ago

https://github.com/boostorg/outcome/actions/runs/4809587810 says that things ought to now be fixed on MSVC.

pdimov commented 1 year ago

I confirm that the develop tests now pass for toolset=msvc-14.2,msvc-14.3 cxxstd=14,17,20,latest.

You might want to add clang-cl to your CI, because it fails a few tests at the moment.

...failed compile-c-c++ ..\..\bin.v2\libs\outcome\test\issue0059.test\clang-win-15.0.1\debug\cxxstd-14-iso\threading-multi\tests\issue0059.obj...
...failed compile-c-c++ ..\..\bin.v2\libs\outcome\test\swap.test\clang-win-15.0.1\debug\cxxstd-14-iso\threading-multi\tests\swap.obj...
...failed compile-c-c++ ..\..\bin.v2\libs\outcome\test\swap.test\clang-win-15.0.1\debug\cxxstd-17-iso\threading-multi\tests\swap.obj...
...failed compile-c-c++ ..\..\bin.v2\libs\outcome\test\swap.test\clang-win-15.0.1\debug\cxxstd-20-iso\threading-multi\tests\swap.obj...
...failed compile-c-c++ ..\..\bin.v2\libs\outcome\test\swap.test\clang-win-15.0.1\debug\cxxstd-latest-iso\threading-multi\tests\swap.obj...
...failed updating 5 targets...
ned14 commented 1 year ago

clang-cl is now fixed. It was failing due to parser bugs, I worked around them.

https://github.com/boostorg/outcome/actions/runs/4812666803/jobs/8568198656

I'm considering this issue closed now. Thanks for reporting it!