boostorg / core

Boost Core Utilities
132 stars 86 forks source link

core/swap.hpp(81,1): fatal error C1202: recursive type or function dependency context too complex #148

Closed afabri closed 10 months ago

afabri commented 1 year ago

We get a boost_1_82_0\boost/core/swap.hpp(81,1): fatal error C1202: recursive type or function dependency context too complex when compiling this program with VC2019 or VC2022 and stdc++17 enabled. link Neither g++ nor clang have a problem. The code is nonsensical and we only boiled down the real code to make it a minimal example. Also #include <boost/swap.hpp> is not used but only needed for getting the error. Maybe this is more a VC++ issue than a boost issue.

afabri commented 12 months ago

Hi @Lastique Do you by any chance have access to the VC++ compiler? I ask because your commit concerning the noexception triggers the problem.

Lastique commented 12 months ago

Do you by any chance have access to the VC++ compiler?

Not at the moment or in the coming few weeks.

If you are willing to try something, you could try the following:

  1. Envelop boost::swap definition here in a new namespace within namespace boost, say namespace swap_adl_block.
  2. After that namespace closes, import boost::swap_adl_block::swap into namespace boost by adding using swap_adl_block::swap declaration.
  3. Try if it fixes your problem.
pdimov commented 12 months ago

Reduced example:

#include <boost/swap.hpp> // compiles if removed
#include <boost/checked_delete.hpp>

template<class T> class X
{
    X( X const& );

public:

    X() {}
};

int main()
{
    X< boost::checked_deleter<int> > x;
    return noexcept( swap( x, x ) );
}

Fails under all compilers (e.g. https://godbolt.org/z/ea65PWz9b). The reason is that std::swap is SFINAEd away when the type isn't move constructible, so the unqualified swap call ends up calling boost::swap recursively, forever.

pdimov commented 12 months ago

Why did we need the change in https://github.com/boostorg/core/commit/8a8738a981e1d46146939f93ac0d751b827e7704?

Lastique commented 12 months ago

On July 9, 2023 8:28:34 PM Peter Dimov @.***> wrote:

Why did we need the change in https://github.com/boostorg/core/commit/8a8738a981e1d46146939f93ac0d751b827e7704?

This reflects std::swap. I also needed this in some of my code.

Transparently noexcept swap is the right thing to do. The fix should be to prevent boost::swap from being found by ADL.

pdimov commented 12 months ago

To prevent boost::swap from being found by ADL, we'll have to make it a function object. That might be the right thing to do, but it's a significant change and might have other unintended consequences.

Lastique commented 12 months ago

Reduced example:

#include <boost/swap.hpp> // compiles if removed
#include <boost/checked_delete.hpp>

template<class T> class X
{
    X( X const& );

public:

    X() {}
};

int main()
{
    X< boost::checked_deleter<int> > x;
    return noexcept( swap( x, x ) );
}

Fails under all compilers (e.g. https://godbolt.org/z/ea65PWz9b). The reason is that std::swap is SFINAEd away when the type isn't move constructible, so the unqualified swap call ends up calling boost::swap recursively, forever.

This code is incorrect either way. If boost::swap is found via ADL, the call is recursive and fails due to noexcept. Before the addition of noexcept, it would compile into an infinite recursion in runtime. If the boost::swap is not found via ADL, then there is no swap that accepts X - std::swap requires the type to be copyable.

I'm not sure this repro is equivalent to the code posted by the OP. Their code involves std::tuple, and without seeing its implementation I can't tell what triggers swap lookup and why it fails specifically on MSVC and not other compilers. I'm assuming, the OP's code compiles without noexcept specification and fails with it.

Lastique commented 12 months ago

@afabri Could you check if 987ffb3 resolves the problem for you?

Lastique commented 12 months ago

To prevent boost::swap from being found by ADL, we'll have to make it a function object.

Not necessarily, a using directive would do.

pdimov commented 12 months ago

My guess is that it uses std::is_nothrow_swappable somewhere.

Not necessarily, a using directive would do.

I don't think it would.

Lastique commented 12 months ago

Not necessarily, a using directive would do.

I don't think it would.

It does work in my local testing, but it turns out it has unexpected side effect that shows as swap_std_vector_of_boost test failure. Basically, if there is an overload of boost::swap for some type (besides Boost.Swap) then the boost::swap from Boost.Swap is no longer found, even if that overload is not suitable for the call arguments. I believe, we would have had the same problem with the function object.

I don't see a way around this. Would we be willing to ban swap overloads in namespace boost other than Boost.Swap, Boost-wide?

pdimov commented 12 months ago

Would we be willing to ban swap overloads in namespace boost other than Boost.Swap, Boost-wide?

I don't think we can do that. Types defined in namespace boost have to have their swap functions defined there as well.

Lastique commented 12 months ago

Would we be willing to ban swap overloads in namespace boost other than Boost.Swap, Boost-wide?

I don't think we can do that. Types defined in namespace boost have to have their swap functions defined there as well.

The idea was to move those types to their own namespaces within boost and then import them into boost with using declarations. But maybe that's too much to ask of maintainers.

Another solution would be to rename swap from Boost.Swap e.g. to adl_swap or move it to a namespace of its own (any naming suggestions?). For backward compatibility, we would keep the boost::swap name available (as a forwarding function or a using declaration) but marked as deprecated, which we would eventually remove. The problem would persist until we remove it.

pdimov commented 12 months ago

We can add adl_swap or whatever with proper noexcept forwarding, and leave boost::swap as it was (in addition to deprecating it) to avoid the regression.

boost::core::swap would be the obvious candidate but we can't do that because there are types in boost::core for which it will be found by ADL. So maybe just changing the name to something other than swap would be best. Maybe swap_adl or invoke_swap. (Or even boost::core::invoke_swap.)