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

[Clang][Sema] Improve support for explicit specializations of constrained member functions & member function templates #88963

Merged
merged 11 commits into from
May 8, 2024

Conversation

sdkrystian
Copy link
Member

@sdkrystian sdkrystian commented Apr 16, 2024

Consider the following snippet from the discussion of CWG2847 on the core reflector (godbolt link):

template<typename T>
concept C = sizeof(T) <= sizeof(long);

template<typename T>
struct A 
{
    template<typename U>
    void f(U) requires C<U>; // #1, declares a function template 

    void g() requires C<T>; // #2, declares a function

    template<>
    void f(char);  // #3, an explicit specialization of a function template that declares a function
};

template<>
template<typename U>
void A<short>::f(U) requires C<U>; // #4, an explicit specialization of a function template that declares a function template

template<>
template<>
void A<int>::f(int); // #5, an explicit specialization of a function template that declares a function

template<>
void A<long>::g(); // #6, an explicit specialization of a function that declares a function

A number of problems exist:

  • Clang rejects #4 because the trailing requires-clause has U substituted with the wrong template parameter depth when Sema::AreConstraintExpressionsEqual is called to determine whether it matches the trailing requires-clause of the implicitly instantiated function template.
  • Clang rejects #5 because the function template specialization instantiated from A<int>::f has a trailing requires-clause, but #5 does not (nor can it have one as it isn't a templated function).
  • Clang rejects #6 for the same reasons it rejects #5.

This patch resolves these issues by making the following changes:

  • To fix #4, Sema::AreConstraintExpressionsEqual is passed FunctionTemplateDecls when comparing the trailing requires-clauses of #4 and the function template instantiated from #1.
  • To fix #5 and #6, the trailing requires-clauses are not compared for explicit specializations that declare functions.

In addition to these changes, CheckMemberSpecialization now considers constraint satisfaction/constraint partial ordering when determining which member function is specialized by an explicit specialization of a member function for an implicit instantiation of a class template (we previously would select the first function that has the same type as the explicit specialization). With constraints taken under consideration, we match EDG's behavior for these declarations.

Note: This still needs a release note + more tests... I'll work on that tomorrow.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Apr 16, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Apr 16, 2024

@llvm/pr-subscribers-clang

Author: Krystian Stasiowski (sdkrystian)

Changes

Consider the following snippet from the discussion of CWG2847 on the core reflector (godbolt link):

template&lt;typename T&gt;
concept C = sizeof(T) &lt;= sizeof(long);

template&lt;typename T&gt;
struct A 
{
    template&lt;typename U&gt;
    void f(U) requires C&lt;U&gt;; // #<!-- -->1, declares a function template 

    void g() requires C&lt;T&gt;; // #<!-- -->2, declares a function

    template&lt;&gt;
    void f(char);  // #<!-- -->3, an explicit specialization of a function template that declares a function
};

template&lt;&gt;
template&lt;typename U&gt;
void A&lt;short&gt;::f(U) requires C&lt;U&gt;; // #<!-- -->4, an explicit specialization of a function template that declares a function template

template&lt;&gt;
template&lt;&gt;
void A&lt;int&gt;::f(int); // #<!-- -->5, an explicit specialization of a function template that declares a function

template&lt;&gt;
void A&lt;long&gt;::g(); // #<!-- -->6, an explicit specialization of a function that declares a function

A number of problems exist:

  • Clang rejects #<!-- -->4 because the trailing requires-clause has U substituted with the wrong template parameter depth when Sema::AreConstraintExpressionsEqual is called to determine whether it matches the trailing requires-clause of the implicitly instantiated function template.
  • Clang rejects #<!-- -->5 because the function template specialization instantiated from A&lt;int&gt;::f has a trailing requires-clause, but #<!-- -->5 does not (nor can it have one as it isn't a templated function).
  • Clang rejects #<!-- -->6 for the same reasons it rejects #<!-- -->5.

This patch resolves these issues by making the following changes:

  • To fix #<!-- -->4, Sema::AreConstraintExpressionsEqual is passed FunctionTemplateDecls when comparing the trailing requires-clauses of #<!-- -->4 and the function template instantiated from #<!-- -->1.
  • To fix #<!-- -->5 and #<!-- -->6, the trailing requires-clauses are not compared for explicit specializations that declare functions.

In addition to these changes, CheckMemberSpecialization now considers constraint satisfaction/constraint partial ordering when determining which member function is specialized by an explicit specialization of a member function for an implicit instantiation of a class template.


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

5 Files Affected:

  • (modified) clang/lib/Sema/SemaConcept.cpp (+1-1)
  • (modified) clang/lib/Sema/SemaOverload.cpp (+8-3)
  • (modified) clang/lib/Sema/SemaTemplate.cpp (+37-7)
  • (modified) clang/lib/Sema/SemaTemplateInstantiate.cpp (+4)
  • (added) clang/test/CXX/temp/temp.spec/temp.expl.spec/p8.cpp (+74)
diff --git a/clang/lib/Sema/SemaConcept.cpp b/clang/lib/Sema/SemaConcept.cpp
index e00c972602829e..7bfec4e11f7aab 100644
--- a/clang/lib/Sema/SemaConcept.cpp
+++ b/clang/lib/Sema/SemaConcept.cpp
@@ -811,7 +811,7 @@ static const Expr *SubstituteConstraintExpressionWithoutSatisfaction(
   // this may happen while we're comparing two templates' constraint
   // equivalence.
   LocalInstantiationScope ScopeForParameters(S);
-  if (auto *FD = llvm::dyn_cast<FunctionDecl>(DeclInfo.getDecl()))
+  if (auto *FD = DeclInfo.getDecl()->getAsFunction())
     for (auto *PVD : FD->parameters())
       ScopeForParameters.InstantiatedLocal(PVD, PVD);
 
diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index 227ef564ba3e08..594cfc58d2226a 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -1303,6 +1303,8 @@ static bool IsOverloadOrOverrideImpl(Sema &SemaRef, FunctionDecl *New,
   if (New->isMSVCRTEntryPoint())
     return false;
 
+  NamedDecl *OldDecl = Old;
+  NamedDecl *NewDecl = New;
   FunctionTemplateDecl *OldTemplate = Old->getDescribedFunctionTemplate();
   FunctionTemplateDecl *NewTemplate = New->getDescribedFunctionTemplate();
 
@@ -1347,6 +1349,8 @@ static bool IsOverloadOrOverrideImpl(Sema &SemaRef, FunctionDecl *New,
   // references to non-instantiated entities during constraint substitution.
   // GH78101.
   if (NewTemplate) {
+    OldDecl = OldTemplate;
+    NewDecl = NewTemplate;
     // C++ [temp.over.link]p4:
     //   The signature of a function template consists of its function
     //   signature, its return type and its template parameter list. The names
@@ -1506,13 +1510,14 @@ static bool IsOverloadOrOverrideImpl(Sema &SemaRef, FunctionDecl *New,
     }
   }
 
-  if (!UseOverrideRules) {
+  if (!UseOverrideRules &&
+      New->getTemplateSpecializationKind() != TSK_ExplicitSpecialization) {
     Expr *NewRC = New->getTrailingRequiresClause(),
          *OldRC = Old->getTrailingRequiresClause();
     if ((NewRC != nullptr) != (OldRC != nullptr))
       return true;
-
-    if (NewRC && !SemaRef.AreConstraintExpressionsEqual(Old, OldRC, New, NewRC))
+    if (NewRC &&
+        !SemaRef.AreConstraintExpressionsEqual(OldDecl, OldRC, NewDecl, NewRC))
       return true;
   }
 
diff --git a/clang/lib/Sema/SemaTemplate.cpp b/clang/lib/Sema/SemaTemplate.cpp
index 95171359f0ab17..3cb1ee1d5b795d 100644
--- a/clang/lib/Sema/SemaTemplate.cpp
+++ b/clang/lib/Sema/SemaTemplate.cpp
@@ -10338,6 +10338,25 @@ bool Sema::CheckFunctionTemplateSpecialization(
   return false;
 }
 
+static bool IsMoreConstrainedFunction(Sema &S, FunctionDecl *FD1,
+                                      FunctionDecl *FD2) {
+  if (FunctionDecl *MF = FD1->getInstantiatedFromMemberFunction())
+    FD1 = MF;
+  if (FunctionDecl *MF = FD2->getInstantiatedFromMemberFunction())
+    FD2 = MF;
+  llvm::SmallVector<const Expr *, 3> AC1, AC2;
+  FD1->getAssociatedConstraints(AC1);
+  FD2->getAssociatedConstraints(AC2);
+  bool AtLeastAsConstrained1, AtLeastAsConstrained2;
+  if (S.IsAtLeastAsConstrained(FD1, AC1, FD2, AC2, AtLeastAsConstrained1))
+    return false;
+  if (S.IsAtLeastAsConstrained(FD2, AC2, FD1, AC1, AtLeastAsConstrained2))
+    return false;
+  if (AtLeastAsConstrained1 == AtLeastAsConstrained2)
+    return false;
+  return AtLeastAsConstrained1;
+}
+
 /// Perform semantic analysis for the given non-template member
 /// specialization.
 ///
@@ -10372,15 +10391,26 @@ Sema::CheckMemberSpecialization(NamedDecl *Member, LookupResult &Previous) {
         QualType Adjusted = Function->getType();
         if (!hasExplicitCallingConv(Adjusted))
           Adjusted = adjustCCAndNoReturn(Adjusted, Method->getType());
+        if (!Context.hasSameType(Adjusted, Method->getType()))
+          continue;
+        if (Method->getTrailingRequiresClause()) {
+          ConstraintSatisfaction Satisfaction;
+          if (CheckFunctionConstraints(Method, Satisfaction,
+                                       /*UsageLoc=*/Member->getLocation(),
+                                       /*ForOverloadResolution=*/true) ||
+              !Satisfaction.IsSatisfied)
+            continue;
+          if (Instantiation &&
+              !IsMoreConstrainedFunction(*this, Method,
+                                         cast<CXXMethodDecl>(Instantiation)))
+            continue;
+        }
         // This doesn't handle deduced return types, but both function
         // declarations should be undeduced at this point.
-        if (Context.hasSameType(Adjusted, Method->getType())) {
-          FoundInstantiation = *I;
-          Instantiation = Method;
-          InstantiatedFrom = Method->getInstantiatedFromMemberFunction();
-          MSInfo = Method->getMemberSpecializationInfo();
-          break;
-        }
+        FoundInstantiation = *I;
+        Instantiation = Method;
+        InstantiatedFrom = Method->getInstantiatedFromMemberFunction();
+        MSInfo = Method->getMemberSpecializationInfo();
       }
     }
   } else if (isa<VarDecl>(Member)) {
diff --git a/clang/lib/Sema/SemaTemplateInstantiate.cpp b/clang/lib/Sema/SemaTemplateInstantiate.cpp
index 7cd428de0bb32d..09a7f34d651db2 100644
--- a/clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -275,6 +275,10 @@ Response HandleFunction(Sema &SemaRef, const FunctionDecl *Function,
                                      TemplateArgs->asArray(),
                                      /*Final=*/false);
 
+    if (RelativeToPrimary &&
+        Function->getTemplateSpecializationKind() == TSK_ExplicitSpecialization)
+      return Response::UseNextDecl(Function);
+
     // If this function was instantiated from a specialized member that is
     // a function template, we're done.
     assert(Function->getPrimaryTemplate() && "No function template?");
diff --git a/clang/test/CXX/temp/temp.spec/temp.expl.spec/p8.cpp b/clang/test/CXX/temp/temp.spec/temp.expl.spec/p8.cpp
new file mode 100644
index 00000000000000..87e10d10e4b453
--- /dev/null
+++ b/clang/test/CXX/temp/temp.spec/temp.expl.spec/p8.cpp
@@ -0,0 +1,74 @@
+// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify %s
+// expected-no-diagnostics
+
+template<typename T>
+concept C = sizeof(T) <= sizeof(long);
+
+template<typename T>
+struct A {
+  template<typename U>
+  void f(U) requires C<U>;
+
+  void g() requires C<T>;
+
+  template<typename U>
+  void h(U) requires C<T>;
+
+  constexpr int i() requires C<T> {
+    return 0;
+  }
+
+  constexpr int i() requires C<T> && true {
+    return 1;
+  }
+
+  template<>
+  void f(char);
+};
+
+template<>
+template<typename U>
+void A<short>::f(U) requires C<U>;
+
+template<>
+template<typename U>
+void A<short>::h(U) requires C<short>;
+
+template<>
+template<>
+void A<int>::f(int);
+
+template<>
+void A<long>::g();
+
+template<>
+constexpr int A<long>::i() {
+  return 2;
+}
+
+static_assert(A<long>().i() == 2);
+
+template<typename T>
+struct D {
+  template<typename U>
+  static constexpr int f(U);
+
+  template<typename U>
+  static constexpr int f(U) requires (sizeof(T) == 1);
+
+  template<>
+  constexpr int f(int) {
+    return 1;
+  }
+};
+
+template<>
+template<typename U>
+constexpr int D<signed char>::f(U) requires (sizeof(signed char) == 1) {
+  return 0;
+}
+
+static_assert(D<char>::f(0) == 1);
+static_assert(D<char[2]>::f(0) == 1);
+static_assert(D<signed char>::f(0) == 1);
+static_assert(D<signed char>::f(0.0) == 0);

Copy link
Collaborator

@erichkeane erichkeane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approach/fix looks fine, other than release notes + tests, LGTM.

@@ -275,6 +275,13 @@ Response HandleFunction(Sema &SemaRef, const FunctionDecl *Function,
TemplateArgs->asArray(),
/*Final=*/false);

if (RelativeToPrimary &&
(Function->getTemplateSpecializationKind() ==
TSK_ExplicitSpecialization ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this condition Function->getTemplateSpecializationKind() == TSK_ExplicitSpecialization necessary? I removed it and all testcases passed. Could you please add a testcase to cover it which would fail without this condition?

Copy link
Member Author

@sdkrystian sdkrystian May 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jcsxky There already is a test case which will fail without the condition in this PR:

template<typename T>
struct D {
  template<typename U>
  static constexpr int f(U);

  template<typename U>
  static constexpr int f(U) requires (sizeof(T) == 1);

  template<>
  constexpr int f(int) {
    return 1;
  }
};

template<>
template<typename U>
constexpr int D<signed char>::f(U) requires (sizeof(signed char) == 1) {
  return 0;
}

static_assert(D<signed char>::f(0) == 1); // fails

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This testcase runs OK before be79079 applied and it may not be related to the condition Function->getTemplateSpecializationKind() == TSK_ExplicitSpecialization.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sdkrystian but gcc fails on the testcase you provided.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gcc does not support class scope explicit specializations.

@sdkrystian
Copy link
Member Author

Added tests + diagnostics for ambiguous member specializations

@sdkrystian sdkrystian force-pushed the constr-mem-fct-spec branch 2 times, most recently from 8667117 to 4e49bae Compare May 7, 2024 12:31
Copy link

github-actions bot commented May 7, 2024

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

@sdkrystian
Copy link
Member Author

There is a crash that occurs in the clang-tidy test suite. It will go away once #91339 is merged (I plan to merge that first).

This will also fix #90349.

Copy link
Collaborator

@erichkeane erichkeane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need a release note? Else LGTM.

@@ -5437,6 +5437,11 @@ def note_function_template_spec_matched : Note<
def err_function_template_partial_spec : Error<
"function template partial specialization is not allowed">;

def err_function_member_spec_ambiguous : Error<
"ambiguous member function specialization of %q0">;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"ambiguous member function specialization of %q0">;
"ambiguous member function specialization of %0">;

@sdkrystian
Copy link
Member Author

Oh, forgot about the release note :) I'll add one

@sdkrystian
Copy link
Member Author

@erichkeane I added two release notes (one for the bug fix to constraint substitution, and one for allowing explicit specializations of constrained member functions).

constexpr static int f() requires C<I> && D<I> { return 1; }
constexpr static int f() requires C<I> { return 2; }

constexpr static int g() requires C<I> { return 0; } // expected-note {{member function specialization matches 'g'}}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, please use the bookmark tags for these notes, I already got confused here: )

Copy link
Collaborator

@erichkeane erichkeane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

happy now :)

@sdkrystian sdkrystian merged commit 34ae226 into llvm:main May 8, 2024
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants