[Clang][Sema] Refactor collection of multi-level template argument lists

[sdkrystian]

Currently, clang rejects the following explicit specialization of f due to the constraints not being equivalent:

template<typename T>
struct A
{
    template<bool B>
    void f() requires B;
};

template<>
template<bool B>
void A<int>::f() requires B { }

This happens because, in most cases, we do not set the flag indicating whether a RedeclarableTemplate is an explicit specialization of a member of an implicitly instantiated class template specialization until after we compare constraints for equivalence. This patch addresses the issue (and a number of other issues) by:

• storing the flag indicating whether a declaration is a member specialization on a per declaration basis, and
• significantly refactoring Sema::getTemplateInstantiationArgs so we collect the right set of template argument in all cases.

Many of our declaration matching & constraint evaluation woes can be traced back to bugs in Sema::getTemplateInstantiationArgs. This change/refactor should fix a lot of them. It also paves the way for fixing #101330 and #105462 per my suggestion in #102267 (which I have implemented on top of this patch but will merge in a subsequent PR).

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[mizvekov]

Awesome, thanks!

I will give a review later, but just want to note a different (but much smaller) PR touching the same area: #102922

Just to keep in mind that the idea is to later further simplify getTemplateInstantiationArgs by removing the DeclContext parameter.

[zyn0217]

Thanks for working on the refactor! I did a cursory review, mostly on the getTemplateInstantiationArgs() part, and the general approach looks plausible.

Re @mizvekov : this patch could supersede my refactoring work not only because of the reuse of the logic of "which next decl context to pick up for innermost arguments" and the reduction of the bool states, but my cwg2369 patch could also rely on it. So I like this patch. (Finally I don't have to rack my brain over the refactoring of it!)

[zyn0217]

So... it turns out we don't have to move to the parent semantic declaration of TTPD even if Innermost is present?
Note that we still want to traverse its parents in the previous implementation anyway.

[zyn0217]

Also, do you plan to fix #102320 in this or the follow-up patches?

[sdkrystian]

Also, do you plan to fix #102320 in this or the follow-up patches?

@zyn0217 This patch (in its current state) fixes #102320 :)

[sdkrystian]

Ping @mizvekov

[erichkeane]

I'll have to take another look at this when I get more time for it, but I'm glad this is getting further attention. I'd refactored this only a year or two ago (preivously it was a single really long function with a loop), so it is disappointing that we're needing to refactor it again already.

Collection of these arguments is unfortunately quite a task and ends up being a bit of trial/error, so hopefully the previous version ends up providing enough tests/etc to help inform this one.

That said, I'd vastly prefer this patch get split up into a few. The straight refactor + new visitor, plus it seems like there are a handful of similar/related refactors that are happening here too.

[erichkeane]

Awe, I thought this was clever/useful :)

[sdkrystian]

That said, I'd vastly prefer this patch get split up into a few. The straight refactor + new visitor, plus it seems like there are a handful of similar/related refactors that are happening here too.

@erichkeane I could move the "early setting of isMemberSpecialization" changes to a separate patch, if that works for you.

[sdkrystian]

Ping @erichkeane

[erichkeane]

Thanks for the ping, I'd forgotten about this one! I'm happy for the most part, I have 1 question and a couple nits.

[erichkeane]

I have a preference for using an enum instead of bool params (so that callers do: SpecializationKind::None, or SpecializationKind::Member or something like that?). Even if there are two options, it improves readability of callers a ton.

[erichkeane]

I still have a preference for something like this, but otherwise

[erichkeane]

1 comment, otherwise LGTM.

[erichkeane]

I still have a preference for something like this, but otherwise

[sdkrystian]

Reduced further to:

template<int N>
struct A
{
    template<typename T>
    static constexpr bool f();
};

template<>
template<typename T>
constexpr bool A<0>::f()
{
    return A<1>::f<T>(); // note: undefined function 'f<int>' cannot be used in a constant expression
}

template<>
template<typename T>
constexpr bool A<1>::f()
{
    return true;
}

static_assert(A<0>::f<int>()); // error: static assertion expression is not an integral constant expression

Also, if the first declaration of f is defined to return false (and return A<1>::f<T>() is changed to return A<1>::f<int>()) the static_assert will fail even without this patch applied.

[sdkrystian]

Another example using class templates:

template<int N>
struct A
{
    template<typename T>
    struct B;
};

template<>
template<typename T>
struct A<0>::B : A<1>::B<T> { };

template<>
template<typename T>
struct A<1>::B
{
    static constexpr bool x = true;
};

static_assert(A<0>::B<int>::x); // error: implicit instantiation of undefined template 'A<1>::B<int>'

This currently results in a crash on trunk. With this patch applied, we instantiate the undefined class template declared in the definition of A and complain.

These differences in behavior arise from the fact that we bind names to the most recent redeclaration of an entity when creating Expr nodes that refer to that entity. In the above example, the explicit specialization of A<1>::B is declared after it is referenced in the explicit specialization of A<0>::B, resulting in the initial declaration being used to instantiate B<int>.

IMO this behavior is correct per [temp.expl.spec] p7:

If a template, a member template or a member of a class template is explicitly specialized, a declaration of that specialization shall be reachable from every use of that specialization that would cause an implicit instantiation to take place, in every translation unit in which such a use occurs; no diagnostic is required.

However, I would like to know what @erichkeane, @mizvekov, and @zygoloid think.

In any case, the QT example can be fixed by reordering the explicit specialization such that each specialization is declared before being referenced.

[erichkeane]

I agree I think with your take on temp.expl.spec, we run into this in quite a few places. I'd like to hear from the others, but this is a paragraph I've used in the past to justify a lot of this sort of diagnostics.

[mizvekov]

Yep, I agree that the reduction indicates the original code was IFNDR.

We need a new reduction with no rename in order to locate this on QT code base and know for sure.

[zygoloid]

In @sdkrystian's latest example, does the use of A<1>::B<T> within A<0>::B cause an implicit instantiation to take place? I would have expected that no implicit instantiation takes place at that point because A<1>::B<T> is a dependent type. I would expect that the specialization of A<1>::B would need to be visible from the point of instantiation of A<1>::B<int>, which it is, so that example looks to me like it should be valid. The wording seems a bit unclear though, so maybe it's worth checking with CWG.

My understanding is that the point of [temp.expl.spec]/7 is that an explicit specialization needs to be declared before the point where we'd otherwise consider implicitly instantiating a specialization from the primary template or a partial specialization, and in this example it seems to me that it is.

[zyn0217]

@sdkrystian do you plan to reland this patch in some other form these days? I'm going to restart my cwg 2369 work on top of it.

[sdkrystian]

@zyn0217 I plan to reapply the patch after addressing the issues discussed above, but I have to get some more pressing work stuff done first :)