@llvm/issue-subscribers-clang-frontend

Confirmed, looks like we had a similar issue but it no longer reproduces: #50643 I think the friend is messing up this particular case.

CC @erichkeane @royjacobson

@erichkeane Looks like babdef2 probably fixed the first case but not this one.

Right, there is work being done on trunk, there is a review up on phab (which I cannot seem to find!) that redoes how a lot of this works, but @zygoloid had a bunch of comments, so we're waiting on him to review.

The given example can be worked around by adding a forward declaration of struct Bar, so that the friend declaration is no longer the first occurence of Bar. However, there is no similar workaround for (what appears to be) a variation of the same bug involving nested classes:

template<typename T>
concept ok = true;

struct outer {
	template<typename T>
	requires ok<T>
	struct foo {};
};

template<typename U>
struct bar {
	template<typename T>
	requires ok<T>
	friend struct outer::foo;
};

bar<int> x;

$ clang++-16 --std=c++20 -Wall -Wextra main.cpp -o test
main.cpp:13:11: error: requires clause differs in template redeclaration
        requires ok<T>
                 ^
main.cpp:17:10: note: in instantiation of template class 'bar<int>' requested here
bar<int> x;
         ^
main.cpp:6:11: note: previous template declaration is here
        requires ok<T>
                 ^
1 error generated.

Godbolt

This issue appears to prevent Clang from compiling parts of ranges in libstdc++. In particular, there's a constrained friend declaration in zip_transform_view that causes issues. I tried massaging libstdc++'s implementation, and unfortunately the forward declaration workaround does not seem to work here.

So the problem here is that we're doing no work to try to make these two 'equal' despite their different depths because we aren't including the associated declaration. This is because we're using the TemplateParameterListsAreEqual that doesn't take the associated declarations in CheckClassTemplate (SemaTemplate.cpp ~1997).

We probably have to try harder there to figure out what the correct declaration these are associated with are.

This issue appears to prevent Clang from compiling parts of ranges in libstdc++.

MSVCSTL makes some private parts public to workaround this issue:

• https://github.com/microsoft/STL/blob/f362f7d7bea87bd199c927a89718e929ed89b187/stl/inc/ranges#L8213
• https://github.com/microsoft/STL/blob/f362f7d7bea87bd199c927a89718e929ed89b187/stl/inc/ranges#L9047

One of the more difficult problems in CheckClassTemplate is going to be that we check the template arguments before the new template is constructed. The PrevClassTemplate works for the 'before', but we likely need to figure out how to move the template parameter list checking 'later'? I don't have a good idea how that looks unfortunately.

An ALTERNATIVE is to find a way to implement TemplateParameterListsAreEqual and AreConstraintExprsEqual in terms of the 'containing' decl context in this case (that is, to automatically just re-insert the same template arguments, then do the normal substitution as before). This is only possible when the new Decl is an uninstantiated template, but that should be OK, since there aren't any valid arguments to be substituting anyway. That way we could pass PrevClassTemplate for Old, and this new situation for New. Others are encouraged to think on this, else I'll think about it over the weekend.

An ALTERNATIVE is to find a way to implement TemplateParameterListsAreEqual and AreConstraintExprsEqual in terms of the 'containing' decl context in this case (that is, to automatically just re-insert the same template arguments, then do the normal substitution as before). This is only possible when the new Decl is an uninstantiated template, but that should be OK, since there aren't any valid arguments to be substituting anyway. That way we could pass PrevClassTemplate for Old, and this new situation for New. Others are encouraged to think on this, else I'll think about it over the weekend.

I ended up deciding that we could figure out how to do the substitution with just the decl-context, so I submitted a patch to support that in this case. WE might end up having a few other cases like this we have to deal with in the future, but this ends up fixing both repros in this issue.