Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

<tuple>: correct get couldn't be found for struct inheriting from tuple #121

Closed
andreyfe1 opened this issue Sep 20, 2019 · 10 comments
Closed
Labels
bug Something isn't working

Comments

@andreyfe1
Copy link

Hi,
It was found that the code below doesn't compile on Windows with clang++. Here we create a class inherits tuple and try to call get for it.

I use Visual Studio 2019. Version 16.2.3. Compiler clang++

#include <iostream>
#include <tuple>

template <typename... _Tp>
struct TupleWrapper : public std::tuple<_Tp...>
{
    typedef std::tuple<_Tp...> base_type;
    
    TupleWrapper(const base_type& t) : base_type(t) {}

#if 0 // 1
    template <size_t i>
    friend const auto&
        get(const TupleWrapper& t)
    {
        return std::get<i>(base_type{ t });
    }
#endif
};

int main()
{
    TupleWrapper<int, int> a = std::make_tuple(1, 2);
    using std::get;
    std::cout << get<0>(a) << " " << get<1>(a) << std::endl;
    return 0;
}
>clang++ -std=c++14 -O0 test.cpp -otest.exe
test.cpp:26:38: error: no matching function for call to 'get'
    std::cout << get<0>(a) << " " << get<1>(a) << std::endl;
                                     ^~~~~~
C:\Program Files (x86)\Microsoft Visual Studio\2019\Professional\VC\Tools\MSVC\14.22.27905\include\utility:452:63: note: candidate template ignored: could not match 'pair' against 'TupleWrapper'
_NODISCARD constexpr tuple_element_t<_Idx, pair<_Ty1, _Ty2>>& get(
                                                              ^
.........................................
C:\Program Files (x86)\Microsoft Visual Studio\2019\Professional\VC\Tools\MSVC\14.22.27905\include\tuple:638:65: note: candidate template ignored: failed template argument deduction
_NODISCARD constexpr tuple_element_t<_Index, tuple<_Types...>>& get(
                                                                ^
C:\Program Files (x86)\Microsoft Visual Studio\2019\Professional\VC\Tools\MSVC\14.22.27905\include\tuple:645:71: note: candidate template ignored: failed template argument deduction
_NODISCARD constexpr const tuple_element_t<_Index, tuple<_Types...>>& get(
                                                                      ^
C:\Program Files (x86)\Microsoft Visual Studio\2019\Professional\VC\Tools\MSVC\14.22.27905\include\tuple:652:66: note: candidate template ignored: failed template argument deduction
_NODISCARD constexpr tuple_element_t<_Index, tuple<_Types...>>&& get(

Could you please help to identify what is the root case of the problem?

@andreyfe1 andreyfe1 changed the title <tuple>: correct get couldn't be found for struct inherited from std::tuple <tuple>: correct get couldn't be found for struct inherited tuple Sep 20, 2019
@CaseyCarter
Copy link
Member

Aside: Your program formally has undefined behavior due to use of the reserved identifier _Tp per [lex.name]/3.1,

Could you please help to identify what is the root case of the problem?

Our tuple implementation is recursive; tuple<int, double, void*> derives from tuple<double, void*> derives from tuple<void*> derives from tuple<> (https://godbolt.org/z/Er2tqy). This is unfortunate for a few reasons, including the one you've stumbled across.

When you pass an instance of a specialization of tuple to std::get:

static_assert(std::get<0>(tuple<int, double, void*>{42, 3.14, nullptr}));

the compiler must deduce tuple<Types...> from e.g. tuple<int, double, void*>. The deduction is simple when the source is a specialization of the same template like this: Types... is deduced to be int, double, void*, and all is well (https://godbolt.org/z/pFtzMl).

If we instead derive a type from a specialization of tuple:

struct S : std::tuple<int, double, void*> {};

and attempt to pass an object of that type to std::get:

static_assert(std::get<0>(S{{42, 3.14, nullptr}}) == 42);

the compiler must deduce tuple<Types...> from S. Since S isn't a specialization of tuple, it looks for specializations of tuple from which S is publicly and unambiguously derived from which it can deduce Types. There are four such specializations in S's inheritance hierarchy:

  1. S's direct base std::tuple<int, double, void*>,
  2. std::tuple<int, double, void*>'s direct base std::tuple<double, void*> (an indirect base of S),
  3. std::tuple<double, void*>'s direct base std::tuple<void*> (an indirect base of S),
  4. std::tuple<void*>'s direct base std::tuple<> (an indirect base of S).

Now things get messy. The C++ Standard provides no rule to disambiguate which of these four different bases should be chosen, so a conforming compiler must throw its hands in the air, say the deduction fails due to ambiguity, and that it can't call std::get with an S (as does clang in your example). MSVC has an overload resolution bug which causes it to prefer direct bases in such cases so the deduction succeeds and deduces Types as the pack int, double, void*. We can't fix the compiler bug without breaking tons of MSVC-specific code with types that derive from tuple, and we can't change the design of tuple to avoid the unfortunate recursion in the first place without breaking ABI and hence even more code.

@andreyfe1
Copy link
Author

andreyfe1 commented Sep 20, 2019

@CaseyCarter, thanks for the explanation. What is the advise in this case?
Can the wrapper be improved anyhow to use std::get for it?

@StephanTLavavej StephanTLavavej changed the title <tuple>: correct get couldn't be found for struct inherited tuple <tuple>: correct get couldn't be found for struct inheriting from tuple Sep 20, 2019
@StephanTLavavej
Copy link
Member

We can't fix the compiler bug without breaking tons of MSVC-specific code with types that derive from tuple

/permissive- could guard such enforcement.

and we can't change the design of tuple to avoid the unfortunate recursion in the first place without breaking ABI and hence even more code.

I have a prototype indicating that we can flatten the inheritance while preserving layout, so it may be possible.

@StephanTLavavej StephanTLavavej added the bug Something isn't working label Sep 20, 2019
@CaseyCarter
Copy link
Member

@CaseyCarter, thanks for the explanation. What is the advise in this case?
Can the wrapper be improved anyhow to use std::get for it?

I don't know of a way to make the wrapper work with std::get. The best advice I have is to write get overloads for the wrapper, and either perform the ADL two-step:

using std::get; 
get<X>(e);

at callsites that need to handle both std::tuple and wrappers, or write a function to do the ADL two-step for you:

template<std::size_t I, class T>
constexpr decltype(auto) adl_get(T&& t) noexcept {
  using std::get;
  return get<I>(std::forward<T>(t));
}

and use that at your callsites.

(Caveat: pair and tuple are to be avoided whenever possible. Prefer types with properly-named members. Reuse via inheritance is also to be avoided when you can reuse via composition.)

@timsong-cpp
Copy link
Contributor

Now things get messy. The C++ Standard provides no rule to disambiguate which of these four different bases should be chosen, so a conforming compiler must throw its hands in the air, say the deduction fails due to ambiguity, and that it can't call std::get with an S (as does clang in your example).

This is outdated; CWG fixed it in Kona via CWG2303.

facebook-github-bot pushed a commit to facebook/hermes that referenced this issue Apr 24, 2020
Summary:
Previously, a derived class, WithTuple, was used.  This ran
into bugs in MSVC (see microsoft/STL#121).
Instead, use specialization to get the same result using std::tuple
directly.  This avoids the bug, and is a cleaner API.

Changelog: [Internal]

Reviewed By: dulinriley

Differential Revision: D21233677

fbshipit-source-id: 1d75991847164e525b4ba70f65a90627e5f8cd56
facebook-github-bot pushed a commit to facebook/react-native that referenced this issue Apr 24, 2020
Summary:
Previously, a derived class, WithTuple, was used.  This ran
into bugs in MSVC (see microsoft/STL#121).
Instead, use specialization to get the same result using std::tuple
directly.  This avoids the bug, and is a cleaner API.

Changelog: [Internal]

Reviewed By: dulinriley

Differential Revision: D21233677

fbshipit-source-id: 1d75991847164e525b4ba70f65a90627e5f8cd56
oliverlee added a commit to oliverlee/state-machine that referenced this issue May 16, 2020
If transition::Row inherits from std::tuple and (in a later commit)
transition::Table inherits from std::tuple, std::get may fail to type
deduction due to ambiguity[1].

This commit updates transition Row to contain a std::tuple as a private
data member instead of inheriting from std::tuple. As accessor is
provided for lookup. An rvalue reference qualified member function is
provided for extracting the tuple in order to update a Row with a new
Transition.

[1]: microsoft/STL#121
@andreyfe1
Copy link
Author

I have a prototype indicating that we can flatten the inheritance while preserving layout, so it may be possible.

@StephanTLavavej, is it implemented in STL now?

@MikeGitb
Copy link

To clarify for myself: Are you saying that the error appears, even, if

template <size_t i>
friend const auto&
    get(const TupleWrapper& t)
{
    return std::get<i>(base_type{ t });
}

is defined or only without it? In your example, that definition is disabled through the #if 0.

@andreyfe1
Copy link
Author

The error appears if that function is not defined

@StephanTLavavej
Copy link
Member

@capatober No, I didn't implement that tuple representation overhaul, and I now believe that attempting to do so near the end of the v19 ABI releases would be unnecessarily disruptive. I think we should just fix this in vNext even if we could technically fix it in v19.

@frederick-vs-ja
Copy link
Contributor

I think this has been fixed by implementing CWG-2303 in Clang 12.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

6 participants