[Clang] Only compare template params of potential overload after checking their decl context by cor3ntin · Pull Request #78139 · llvm/llvm-project

cor3ntin · 2024-01-15T09:59:09Z

Fixes a regression from 69066ab in which we compared the template lists of potential overloads before checkings their declaration contexts.

This would cause a crash when doing constraint substitution as part of that template check, because we would try to refer to not yet instantiated entities (the underlying cause is unclear).

This patch reorders (again) when we look at template parameter so we don't when checkings friends in different lexical contexts.

Fixes #77953
Fixes #78101

llvmbot · 2024-01-15T09:59:38Z

@llvm/pr-subscribers-clang

Author: cor3ntin (cor3ntin)

Changes

Fixes a regression from 69066ab in which we compared the template lists of potential overloads before checkings their declaration contexts.

This would cause a crash when doing constraint substitution as part of that template check, because we would try to refer to not yet instantiated entities (the underlying cause is unclear).

This patch reorders (again) when we look at template parameter so we don't when checkings friends in different lexical contexts.

Fixes #77953
Fixes #78101

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

3 Files Affected:

(modified) clang/lib/Sema/SemaOverload.cpp (+34-26)
(modified) clang/test/CXX/over/over.load/p2-0x.cpp (+27-5)
(added) clang/test/Modules/GH77953.cpp (+28)

diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index 23b9bc0fe2d6e2e..4b84a60d4780b59 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -1259,6 +1259,40 @@ static bool IsOverloadOrOverrideImpl(Sema &SemaRef, FunctionDecl *New,
   if ((OldTemplate == nullptr) != (NewTemplate == nullptr))
     return true;
 
+  // Is the function New an overload of the function Old?
+  QualType OldQType = SemaRef.Context.getCanonicalType(Old->getType());
+  QualType NewQType = SemaRef.Context.getCanonicalType(New->getType());
+
+  // Compare the signatures (C++ 1.3.10) of the two functions to
+  // determine whether they are overloads. If we find any mismatch
+  // in the signature, they are overloads.
+
+  // If either of these functions is a K&R-style function (no
+  // prototype), then we consider them to have matching signatures.
+  if (isa<FunctionNoProtoType>(OldQType.getTypePtr()) ||
+      isa<FunctionNoProtoType>(NewQType.getTypePtr()))
+    return false;
+
+  const FunctionProtoType *OldType = cast<FunctionProtoType>(OldQType);
+  const FunctionProtoType *NewType = cast<FunctionProtoType>(NewQType);
+
+  // The signature of a function includes the types of its
+  // parameters (C++ 1.3.10), which includes the presence or absence
+  // of the ellipsis; see C++ DR 357).
+  if (OldQType != NewQType && OldType->isVariadic() != NewType->isVariadic())
+    return true;
+
+  // For member-like friends, the enclosing class is part of the signature.
+  if ((New->isMemberLikeConstrainedFriend() ||
+       Old->isMemberLikeConstrainedFriend()) &&
+      !New->getLexicalDeclContext()->Equals(Old->getLexicalDeclContext()))
+    return true;
+
+  // Compare the parameter lists.
+  // This can only be done once we have establish that friend functions
+  // inhabit the same context, otherwise we might tried to instantiate
+  // references to non-instantiated entities during constraint substitution.
+  // GH78101.
   if (NewTemplate) {
     // C++ [temp.over.link]p4:
     //   The signature of a function template consists of its function
@@ -1296,34 +1330,8 @@ static bool IsOverloadOrOverrideImpl(Sema &SemaRef, FunctionDecl *New,
       return true;
   }
 
-  // Is the function New an overload of the function Old?
-  QualType OldQType = SemaRef.Context.getCanonicalType(Old->getType());
-  QualType NewQType = SemaRef.Context.getCanonicalType(New->getType());
-
-  // Compare the signatures (C++ 1.3.10) of the two functions to
-  // determine whether they are overloads. If we find any mismatch
-  // in the signature, they are overloads.
 
-  // If either of these functions is a K&R-style function (no
-  // prototype), then we consider them to have matching signatures.
-  if (isa<FunctionNoProtoType>(OldQType.getTypePtr()) ||
-      isa<FunctionNoProtoType>(NewQType.getTypePtr()))
-    return false;
 
-  const FunctionProtoType *OldType = cast<FunctionProtoType>(OldQType);
-  const FunctionProtoType *NewType = cast<FunctionProtoType>(NewQType);
-
-  // The signature of a function includes the types of its
-  // parameters (C++ 1.3.10), which includes the presence or absence
-  // of the ellipsis; see C++ DR 357).
-  if (OldQType != NewQType && OldType->isVariadic() != NewType->isVariadic())
-    return true;
-
-  // For member-like friends, the enclosing class is part of the signature.
-  if ((New->isMemberLikeConstrainedFriend() ||
-       Old->isMemberLikeConstrainedFriend()) &&
-      !New->getLexicalDeclContext()->Equals(Old->getLexicalDeclContext()))
-    return true;
   const auto *OldMethod = dyn_cast<CXXMethodDecl>(Old);
   const auto *NewMethod = dyn_cast<CXXMethodDecl>(New);
 
diff --git a/clang/test/CXX/over/over.load/p2-0x.cpp b/clang/test/CXX/over/over.load/p2-0x.cpp
index 8fd9a1ce1e87ab4..a86e2d2a1d81e51 100644
--- a/clang/test/CXX/over/over.load/p2-0x.cpp
+++ b/clang/test/CXX/over/over.load/p2-0x.cpp
@@ -24,11 +24,6 @@ class Y {
   void k() &&; // expected-error{{cannot overload a member function with ref-qualifier '&&' with a member function without a ref-qualifier}}
 };
 
-struct GH76358 {
-    template<int> void f() && {}
-    template<typename T> void f() const {}
-};
-
 
 #if __cplusplus >= 202002L
 namespace GH58962 {
@@ -55,4 +50,31 @@ static_assert(not test<type<2>&>);
 static_assert(test<type<2>&&>);
 
 }
+
+namespace GH78101 {
+
+template<typename T, typename U, int i>
+concept True = true;
+
+template<typename T, int I>
+struct Template {
+    static constexpr int i = I;
+    friend constexpr auto operator+(True<T, i> auto f) {
+        return i;
+    }
+};
+
+template<int I>
+struct Template<float, I> {
+    static constexpr int i = I;
+    friend constexpr auto operator+(True<float, i> auto f) {
+        return i;
+    }
+};
+
+Template<void, 4> f{};
+static_assert(+Template<float, 5>{} == 5);
+
+}
+
 #endif
diff --git a/clang/test/Modules/GH77953.cpp b/clang/test/Modules/GH77953.cpp
new file mode 100644
index 000000000000000..aaca8153c3d2121
--- /dev/null
+++ b/clang/test/Modules/GH77953.cpp
@@ -0,0 +1,28 @@
+// From https://github.com/llvm/llvm-project/issues/77953
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface %t/a.cppm -o %t/a.pcm
+// RUN: %clang_cc1 -std=c++20 -fmodule-file=a=%t/a.pcm %t/b.cppm
+
+//--- a.cppm
+export module a;
+
+template<typename, typename>
+concept c = true;
+
+export template<typename... Ts>
+struct a {
+	template<typename... Us> requires(... and c<Ts, Us>)
+	friend bool operator==(a, a<Us...>) {
+		return true;
+	}
+};
+
+template struct a<>;
+
+//--- b.cppm
+import a;
+
+template struct a<int>;

llvmbot · 2024-01-15T09:59:38Z

@llvm/pr-subscribers-clang-modules

Author: cor3ntin (cor3ntin)

Changes

Fixes a regression from 69066ab in which we compared the template lists of potential overloads before checkings their declaration contexts.

This would cause a crash when doing constraint substitution as part of that template check, because we would try to refer to not yet instantiated entities (the underlying cause is unclear).

This patch reorders (again) when we look at template parameter so we don't when checkings friends in different lexical contexts.

Fixes #77953
Fixes #78101

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

3 Files Affected:

(modified) clang/lib/Sema/SemaOverload.cpp (+34-26)
(modified) clang/test/CXX/over/over.load/p2-0x.cpp (+27-5)
(added) clang/test/Modules/GH77953.cpp (+28)

diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index 23b9bc0fe2d6e2..4b84a60d4780b5 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -1259,6 +1259,40 @@ static bool IsOverloadOrOverrideImpl(Sema &SemaRef, FunctionDecl *New,
   if ((OldTemplate == nullptr) != (NewTemplate == nullptr))
     return true;
 
+  // Is the function New an overload of the function Old?
+  QualType OldQType = SemaRef.Context.getCanonicalType(Old->getType());
+  QualType NewQType = SemaRef.Context.getCanonicalType(New->getType());
+
+  // Compare the signatures (C++ 1.3.10) of the two functions to
+  // determine whether they are overloads. If we find any mismatch
+  // in the signature, they are overloads.
+
+  // If either of these functions is a K&R-style function (no
+  // prototype), then we consider them to have matching signatures.
+  if (isa<FunctionNoProtoType>(OldQType.getTypePtr()) ||
+      isa<FunctionNoProtoType>(NewQType.getTypePtr()))
+    return false;
+
+  const FunctionProtoType *OldType = cast<FunctionProtoType>(OldQType);
+  const FunctionProtoType *NewType = cast<FunctionProtoType>(NewQType);
+
+  // The signature of a function includes the types of its
+  // parameters (C++ 1.3.10), which includes the presence or absence
+  // of the ellipsis; see C++ DR 357).
+  if (OldQType != NewQType && OldType->isVariadic() != NewType->isVariadic())
+    return true;
+
+  // For member-like friends, the enclosing class is part of the signature.
+  if ((New->isMemberLikeConstrainedFriend() ||
+       Old->isMemberLikeConstrainedFriend()) &&
+      !New->getLexicalDeclContext()->Equals(Old->getLexicalDeclContext()))
+    return true;
+
+  // Compare the parameter lists.
+  // This can only be done once we have establish that friend functions
+  // inhabit the same context, otherwise we might tried to instantiate
+  // references to non-instantiated entities during constraint substitution.
+  // GH78101.
   if (NewTemplate) {
     // C++ [temp.over.link]p4:
     //   The signature of a function template consists of its function
@@ -1296,34 +1330,8 @@ static bool IsOverloadOrOverrideImpl(Sema &SemaRef, FunctionDecl *New,
       return true;
   }
 
-  // Is the function New an overload of the function Old?
-  QualType OldQType = SemaRef.Context.getCanonicalType(Old->getType());
-  QualType NewQType = SemaRef.Context.getCanonicalType(New->getType());
-
-  // Compare the signatures (C++ 1.3.10) of the two functions to
-  // determine whether they are overloads. If we find any mismatch
-  // in the signature, they are overloads.
 
-  // If either of these functions is a K&R-style function (no
-  // prototype), then we consider them to have matching signatures.
-  if (isa<FunctionNoProtoType>(OldQType.getTypePtr()) ||
-      isa<FunctionNoProtoType>(NewQType.getTypePtr()))
-    return false;
 
-  const FunctionProtoType *OldType = cast<FunctionProtoType>(OldQType);
-  const FunctionProtoType *NewType = cast<FunctionProtoType>(NewQType);
-
-  // The signature of a function includes the types of its
-  // parameters (C++ 1.3.10), which includes the presence or absence
-  // of the ellipsis; see C++ DR 357).
-  if (OldQType != NewQType && OldType->isVariadic() != NewType->isVariadic())
-    return true;
-
-  // For member-like friends, the enclosing class is part of the signature.
-  if ((New->isMemberLikeConstrainedFriend() ||
-       Old->isMemberLikeConstrainedFriend()) &&
-      !New->getLexicalDeclContext()->Equals(Old->getLexicalDeclContext()))
-    return true;
   const auto *OldMethod = dyn_cast<CXXMethodDecl>(Old);
   const auto *NewMethod = dyn_cast<CXXMethodDecl>(New);
 
diff --git a/clang/test/CXX/over/over.load/p2-0x.cpp b/clang/test/CXX/over/over.load/p2-0x.cpp
index 8fd9a1ce1e87ab..a86e2d2a1d81e5 100644
--- a/clang/test/CXX/over/over.load/p2-0x.cpp
+++ b/clang/test/CXX/over/over.load/p2-0x.cpp
@@ -24,11 +24,6 @@ class Y {
   void k() &&; // expected-error{{cannot overload a member function with ref-qualifier '&&' with a member function without a ref-qualifier}}
 };
 
-struct GH76358 {
-    template<int> void f() && {}
-    template<typename T> void f() const {}
-};
-
 
 #if __cplusplus >= 202002L
 namespace GH58962 {
@@ -55,4 +50,31 @@ static_assert(not test<type<2>&>);
 static_assert(test<type<2>&&>);
 
 }
+
+namespace GH78101 {
+
+template<typename T, typename U, int i>
+concept True = true;
+
+template<typename T, int I>
+struct Template {
+    static constexpr int i = I;
+    friend constexpr auto operator+(True<T, i> auto f) {
+        return i;
+    }
+};
+
+template<int I>
+struct Template<float, I> {
+    static constexpr int i = I;
+    friend constexpr auto operator+(True<float, i> auto f) {
+        return i;
+    }
+};
+
+Template<void, 4> f{};
+static_assert(+Template<float, 5>{} == 5);
+
+}
+
 #endif
diff --git a/clang/test/Modules/GH77953.cpp b/clang/test/Modules/GH77953.cpp
new file mode 100644
index 00000000000000..aaca8153c3d212
--- /dev/null
+++ b/clang/test/Modules/GH77953.cpp
@@ -0,0 +1,28 @@
+// From https://github.com/llvm/llvm-project/issues/77953
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface %t/a.cppm -o %t/a.pcm
+// RUN: %clang_cc1 -std=c++20 -fmodule-file=a=%t/a.pcm %t/b.cppm
+
+//--- a.cppm
+export module a;
+
+template<typename, typename>
+concept c = true;
+
+export template<typename... Ts>
+struct a {
+	template<typename... Us> requires(... and c<Ts, Us>)
+	friend bool operator==(a, a<Us...>) {
+		return true;
+	}
+};
+
+template struct a<>;
+
+//--- b.cppm
+import a;
+
+template struct a<int>;

github-actions · 2024-01-15T10:01:27Z

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

…king their decl context Fixes a regression from 69066ab in which we compared the template lists of potential overloads before checkings their declaration contexts. This would cause a crash when doing constraint substitution as part of that template check, because we would try to refer to not yet instantiated entities (the underlying cause is unclear). This patch reorders (again) when we look at templaste parameter so we don't when checkings friends in different lexical contexts. Fixes llvm#77953 Fixes llvm#78101

tbaederr · 2024-01-15T11:06:48Z

+  const FunctionProtoType *OldType = cast<FunctionProtoType>(OldQType);
+  const FunctionProtoType *NewType = cast<FunctionProtoType>(NewQType);


Suggested change

const FunctionProtoType *OldType = cast<FunctionProtoType>(OldQType);

const FunctionProtoType *NewType = cast<FunctionProtoType>(NewQType);

const auto *OldType = cast<FunctionProtoType>(OldQType);

const auto *NewType = cast<FunctionProtoType>(NewQType);

Fixed. Note that this code was just moved so it's all preexisting

AaronBallman · 2024-01-15T14:34:06Z

  void k() &&; // expected-error{{cannot overload a member function with ref-qualifier '&&' with a member function without a ref-qualifier}}
 };

-struct GH76358 {


Why remove this test?

Completely accidental, i did not notice...

AaronBallman

LGTM

ChuanqiXu9 · 2024-01-16T03:18:58Z

+// RUN: split-file %s %t
+
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface %t/a.cppm -o %t/a.pcm
+// RUN: %clang_cc1 -std=c++20 -fmodule-file=a=%t/a.pcm %t/b.cppm


nit: %clang_cc1 -std=c++20 -fmodule-file=a=%t/a.pcm %t/b.cpp -fsyntax-only -verify

I didn't look this in time. I'll try to improve this in a seperate NFC commit.

Good catch, thanks!

…king their decl context (llvm#78139) Fixes a regression from 69066ab in which we compared the template lists of potential overloads before checkings their declaration contexts. This would cause a crash when doing constraint substitution as part of that template check, because we would try to refer to not yet instantiated entities (the underlying cause is unclear). This patch reorders (again) when we look at template parameter so we don't do it when checkings friends in different lexical contexts. Fixes llvm#77953 Fixes llvm#78101

…king their decl context (llvm#78139) Fixes a regression from c4e066a in which we compared the template lists of potential overloads before checkings their declaration contexts. This would cause a crash when doing constraint substitution as part of that template check, because we would try to refer to not yet instantiated entities (the underlying cause is unclear). This patch reorders (again) when we look at template parameter so we don't do it when checkings friends in different lexical contexts. Fixes llvm#77953 Fixes llvm#78101

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:modules C++20 modules and Clang Header Modules labels Jan 15, 2024

cor3ntin requested a review from erichkeane January 15, 2024 09:59

cor3ntin force-pushed the GH77953 branch from e5be422 to e0eb639 Compare January 15, 2024 10:04

tbaederr reviewed Jan 15, 2024

View reviewed changes

AaronBallman reviewed Jan 15, 2024

View reviewed changes

Address review feedback

7352ce5

AaronBallman approved these changes Jan 15, 2024

View reviewed changes

cor3ntin merged commit c58bc24 into llvm:main Jan 15, 2024

ChuanqiXu9 reviewed Jan 16, 2024

View reviewed changes

Navigation Menu

Repository navigation

[Clang] Only compare template params of potential overload after checking their decl context - #78139

[Clang] Only compare template params of potential overload after checking their decl context#78139
cor3ntin merged 2 commits into
llvm:mainfrom
cor3ntin:GH77953

cor3ntin commented Jan 15, 2024

Uh oh!

llvmbot commented Jan 15, 2024

Uh oh!

llvmbot commented Jan 15, 2024

Uh oh!

github-actions Bot commented Jan 15, 2024 •

edited

Loading

Uh oh!

tbaederr Jan 15, 2024

Uh oh!

cor3ntin Jan 15, 2024

Uh oh!

AaronBallman Jan 15, 2024

Uh oh!

cor3ntin Jan 15, 2024

Uh oh!

AaronBallman left a comment

Uh oh!

ChuanqiXu9 Jan 16, 2024

Uh oh!

cor3ntin Jan 16, 2024

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

		const FunctionProtoType *OldType = cast<FunctionProtoType>(OldQType);
		const FunctionProtoType *NewType = cast<FunctionProtoType>(NewQType);

Conversation

cor3ntin commented Jan 15, 2024

Uh oh!

llvmbot commented Jan 15, 2024

Uh oh!

llvmbot commented Jan 15, 2024

Uh oh!

github-actions Bot commented Jan 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tbaederr Jan 15, 2024

Choose a reason for hiding this comment

Uh oh!

cor3ntin Jan 15, 2024

Choose a reason for hiding this comment

Uh oh!

AaronBallman Jan 15, 2024

Choose a reason for hiding this comment

Uh oh!

cor3ntin Jan 15, 2024

Choose a reason for hiding this comment

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

ChuanqiXu9 Jan 16, 2024

Choose a reason for hiding this comment

Uh oh!

cor3ntin Jan 16, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

github-actions Bot commented Jan 15, 2024 •

edited

Loading