[Clang][Concepts] Fix a constraint comparison regression for out-of-line ClassTemplateDecls

[zyn0217]

Since 98191d7, we have been comparing constraints on out-of-line class template declarations by recovering surrounding template arguments from the surrouding declaration context if neither the new declaration nor its template arguments are available.

However, there is a problem in that the out-of-line class template's template argument is introspected from its lexical context rather than the semantic context.

This results in a discrepancy: out-of-line constraint expressions are not substituted while their corresponding inline expressions are transformed, hence the bogus error.

This patch fixes that by introspecting these template arguments from the semantic context, regardless of it being a friend declaration - we have handled friends in getTemplateInstantiationArgs().

Fixes #102320

[llvmbot]

@llvm/pr-subscribers-clang

Author: Younan Zhang (zyn0217)

Changes

Since 98191d7, we have been comparing constraints on out-of-line class template declarations by recovering surrounding template arguments from the DeclContext if neither the new declaration nor its template arguments are available.

However, there is a problem in that the out-of-line class template is introduced from its lexical context rather than the semantic context.

This results in a discrepancy: out-of-line constraint expressions are not substituted while their corresponding inline expressions are transformed, hence the bogus error.

This patch fixes that by introspecting these template arguments from the semantic context, regardless of it being a friend declaration - we have handled friends in getTemplateInstantiationArgs().

Fixes #102320

---

Full diff: https://github.com/llvm/llvm-project/pull/102587.diff

3 Files Affected:

• (modified) clang/docs/ReleaseNotes.rst (+2)
• (modified) clang/lib/Sema/SemaConcept.cpp (+7-4)
• (modified) clang/test/SemaTemplate/concepts-out-of-line-def.cpp (+21)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 0f1a4c1851911d..cb92ba25d1633f 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -186,6 +186,8 @@ Bug Fixes to C++ Support
   substitutions in concepts, so it doesn't incorrectly complain of missing
   module imports in those situations. (#GH60336)
 - Fix init-capture packs having a size of one before being instantiated. (#GH63677)
+- Clang now properly compares constraints on an out of line class template
+  declaration definition. (#GH102320)
 
 Bug Fixes to AST Handling
 ^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/Sema/SemaConcept.cpp b/clang/lib/Sema/SemaConcept.cpp
index d4c9d044985e34..2871f78868aea7 100644
--- a/clang/lib/Sema/SemaConcept.cpp
+++ b/clang/lib/Sema/SemaConcept.cpp
@@ -948,7 +948,7 @@ static const Expr *SubstituteConstraintExpressionWithoutSatisfaction(
     Sema &S, const Sema::TemplateCompareNewDeclInfo &DeclInfo,
     const Expr *ConstrExpr) {
   MultiLevelTemplateArgumentList MLTAL = S.getTemplateInstantiationArgs(
-      DeclInfo.getDecl(), DeclInfo.getLexicalDeclContext(), /*Final=*/false,
+      DeclInfo.getDecl(), DeclInfo.getDeclContext(), /*Final=*/false,
       /*Innermost=*/std::nullopt,
       /*RelativeToPrimary=*/true,
       /*Pattern=*/nullptr, /*ForConstraintInstantiation=*/true,
@@ -971,9 +971,12 @@ static const Expr *SubstituteConstraintExpressionWithoutSatisfaction(
   // this may happen while we're comparing two templates' constraint
   // equivalence.
   LocalInstantiationScope ScopeForParameters(S);
-  if (auto *FD = DeclInfo.getDecl()->getAsFunction())
-    for (auto *PVD : FD->parameters())
-      ScopeForParameters.InstantiatedLocal(PVD, PVD);
+  if (const NamedDecl *D = DeclInfo.getDecl()) {
+    const FunctionDecl *FD = D->getAsFunction();
+    if (FD)
+      for (auto *PVD : FD->parameters())
+        ScopeForParameters.InstantiatedLocal(PVD, PVD);
+  }
 
   std::optional<Sema::CXXThisScopeRAII> ThisScope;
 
diff --git a/clang/test/SemaTemplate/concepts-out-of-line-def.cpp b/clang/test/SemaTemplate/concepts-out-of-line-def.cpp
index 0142efcdc3ee86..5ea1ff79e572bc 100644
--- a/clang/test/SemaTemplate/concepts-out-of-line-def.cpp
+++ b/clang/test/SemaTemplate/concepts-out-of-line-def.cpp
@@ -599,3 +599,24 @@ template <class DerT>
 unsigned long DerivedCollection<DerTs...>::index() {}
 
 } // namespace GH72557
+
+namespace GH102320 {
+
+template <class, class>
+concept Constrained = true;
+
+template <class T> class C {
+  template <Constrained<T>> class D;
+  template <class U>
+    requires Constrained<T, U>
+  class E;
+};
+
+template <class T> template <Constrained<T>> class C<T>::D {};
+
+template <class T>
+template <class U>
+  requires Constrained<T, U>
+class C<T>::E {};
+
+} // namespace GH102320

[zyn0217]

Note that @jcsxky has a similar patch #102554 that fixes the same thing. I don't intend to contend against him, and admittedly, that patch works.

But I don't think that approach is so reasonable because it is hacky in that ForConstraintInstantiation is disabled in the presence of ClassTemplateDecls, which seemingly does a superfluous job that getTemplateInstantiationArgs() should do and blurs the meaning of the flag.

That would also leave us a concern about whether we should change all the other lines where ForConstraintInstantiation was true to the same pattern in SubstituteConstraintExpressionWithoutSatisfaction.

[cor3ntin]

When is D null?

[zyn0217]

(I roughly explained in the comment.)
This function SubstituteConstraintExpressionWithoutSatisfaction would be called in the context where a template parameter list is getting parsed, when the template declaration that the template parameter list would attach to is not yet available.

[jcsxky]

Note that @jcsxky has a similar patch #102554 that fixes the same thing. I don't intend to contend against him, and admittedly, that patch works.

But I don't think that approach is so reasonable because it is hacky in that ForConstraintInstantiation is disabled in the presence of ClassTemplateDecls, which seemingly does a superfluous job that getTemplateInstantiationArgs() should do and blurs the meaning of the flag.

That would also leave us a concern about whether we should change all the other lines where ForConstraintInstantiation was true to the same pattern in SubstituteConstraintExpressionWithoutSatisfaction.

The approach in this patch does not work on the following case:

template<typename>
concept Constrained = true;

template <typename T>
class C
{
    template<Constrained>
    class D;
};

template <>
template <Constrained>
class C<int>::D
{
};

while mine does.

[zyn0217]

@jcsxky Thanks for spotting that. That is indeed a regression and I think that can be resolved by not letting out-of-line specializations contribute to template arguments.

[jcsxky]

I don't think we need this regression. As pointed in this comment, we should not transform ConceptSpecializationExpr which makes the depth incorrect. I tested locally and my origin approach also makes sense to this case.

[zyn0217]

I DIDN’T say this is a regression, but rather we’ve been rejecting it in many versions.

The function per se is called SubstituteConstraintExpressionWithoutSatisfaction, so not transforming the constraint appears to go against the intention of it to me.

[zyn0217]

I think if we really need to end up with an approach that by not transforming it, we should do the check/special-casing inside getTemplateInstantiationArgs(), rather than doing that at the call site.

[zyn0217]

Actually there seems to be a slight discrepancy among the four compilers here:
https://gcc.godbolt.org/z/nxPoeGE5b
w/o constraints, MSVC and Clang reject such a pattern, while GCC and EDG accept it.
w/ constraints, only Clang complains. So I'm unclear about what is supposed to happen here.
@cor3ntin do you have any ideas?

[cor3ntin]

@zyn0217 I need more time to think about the concept case, but the first case looks definitively like a bug. The following looks related #53139

[zyn0217]

Closed in favor of #106585