[clang] Recognize friend operator != to fix ambiguity with operator== (PR #70213)

Utkarsh Saxena via cfe-commits cfe-commits at lists.llvm.org
Wed Oct 25 07:37:30 PDT 2023


https://github.com/usx95 created https://github.com/llvm/llvm-project/pull/70213

The namespace lookup should give a matching friend operator!= as friend functions belong to the namespace scope (as opposed to the lexical class scope).

Thus to resolve ambiguity of operator== with itself, defining a corresponding friend operator!= should suffice.

>From 8aa439a97a56ef80bfc9ccc90a9f093680e455f5 Mon Sep 17 00:00:00 2001
From: Utkarsh Saxena <usx at google.com>
Date: Sun, 22 Oct 2023 11:11:53 +0200
Subject: [PATCH 1/4] rebase...

---
 clang/docs/ReleaseNotes.rst                   |  4 +
 clang/lib/Sema/SemaOverload.cpp               | 19 ++---
 .../over.match.oper/p3-2a.cpp                 | 78 +++++++++++++++++++
 3 files changed, 89 insertions(+), 12 deletions(-)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 3a9a67f7451c092..cef857244387e8b 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -85,6 +85,10 @@ C/C++ Language Potentially Breaking Changes
     ``__has_c_attribute(warn_unused_result)``,          202003, 0
     ``__has_c_attribute(gnu::warn_unused_result)``,     202003, 1
 
+- Fixed a bug in finding matching `operator!=` while adding reversed `operator==` as
+  outlined in "The Equality Operator You Are Looking For" (`P2468 <http://wg21.link/p2468r2>`_).
+  Fixes (`#68901: <https://github.com/llvm/llvm-project/issues/68901>`_).
+
 C++ Specific Potentially Breaking Changes
 -----------------------------------------
 - The name mangling rules for function templates has been changed to take into
diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index d57a7ad8f46859a..75e4184c8aa6cb8 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -960,18 +960,13 @@ static bool shouldAddReversedEqEq(Sema &S, SourceLocation OpLoc,
     return true;
   }
   // Otherwise the search scope is the namespace scope of which F is a member.
-  LookupResult NonMembers(S, NotEqOp, OpLoc,
-                          Sema::LookupNameKind::LookupOperatorName);
-  S.LookupName(NonMembers,
-               S.getScopeForContext(EqFD->getEnclosingNamespaceContext()));
-  NonMembers.suppressAccessDiagnostics();
-  for (NamedDecl *Op : NonMembers) {
-    auto *FD = Op->getAsFunction();
-    if(auto* UD = dyn_cast<UsingShadowDecl>(Op))
-      FD = UD->getUnderlyingDecl()->getAsFunction();
-    if (FunctionsCorrespond(S.Context, EqFD, FD) &&
-        declaresSameEntity(cast<Decl>(EqFD->getDeclContext()),
-                           cast<Decl>(Op->getDeclContext())))
+  for (NamedDecl *Op : EqFD->getEnclosingNamespaceContext()->lookup(NotEqOp)) {
+    auto *NotEqFD = Op->getAsFunction();
+    if (auto *UD = dyn_cast<UsingShadowDecl>(Op))
+      NotEqFD = UD->getUnderlyingDecl()->getAsFunction();
+    if (FunctionsCorrespond(S.Context, EqFD, NotEqFD) &&
+        declaresSameEntity(cast<Decl>(EqFD->getEnclosingNamespaceContext()),
+                           cast<Decl>(Op->getLexicalDeclContext())))
       return false;
   }
   return true;
diff --git a/clang/test/CXX/over/over.match/over.match.funcs/over.match.oper/p3-2a.cpp b/clang/test/CXX/over/over.match/over.match.funcs/over.match.oper/p3-2a.cpp
index 5727d506fe5e61d..9dc5ee8db565341 100644
--- a/clang/test/CXX/over/over.match/over.match.funcs/over.match.oper/p3-2a.cpp
+++ b/clang/test/CXX/over/over.match/over.match.funcs/over.match.oper/p3-2a.cpp
@@ -374,6 +374,84 @@ bool fine(A a, B b) { return a == b; } // Ok. Found a matching operator!=.
 }
 }
 
+
+namespace ADL_GH68901{
+namespace A {
+struct S {};
+bool operator==(S, int); // expected-note {{no known conversion from 'int' to 'S' for 1st argument}}
+bool operator!=(S, int);
+} // namespace A
+bool a = 0 == A::S(); // expected-error {{invalid operands to binary expression ('int' and 'A::S')}}
+
+namespace B {
+struct Derived {};
+struct Base : Derived {};
+
+bool operator==(Derived& a, Base& b);
+bool operator!=(Derived& a, Base& b);
+}  // namespace B
+
+void foo() {
+  // B::Base{} == B::Base{};
+  B::Base a,b;
+  bool v = a == b;
+}
+
+namespace ns {
+template <class T>
+struct A {
+};
+
+template <class T>
+struct B : A<T> {
+};
+
+template <class T> bool operator == (B<T>, A<T>); // expected-note {{candidate template ignored: could not match 'B' against 'A'}}
+template <class T> bool operator != (B<T>, A<T>);
+}
+
+void test() {
+    ns::A<int> a;
+    ns::B<int> b;
+    a == b; // expected-error {{invalid operands to binary expression}}
+}
+
+
+} //namespace ADL_GH68901
+
+namespace function_scope_operator_eqeq {
+// For non-members, we always lookup for matching operator!= in the namespace scope of
+// operator== (and not in the scope of operator==).
+struct X { operator int(); };
+namespace test1{
+bool h(X x) {
+  bool operator==(X, int); // expected-note {{reversed}}
+  return x == x; // expected-warning {{ambiguous}}
+}
+
+bool g(X x) {
+  bool operator==(X, int); // expected-note {{reversed}}
+  bool operator!=(X, int);
+  return x == x;  // expected-warning {{ambiguous}}
+}
+} // namespace test1
+
+namespace test2 {
+bool operator!=(X, int);
+
+bool h(X x) {
+  bool operator==(X, int);
+  return x == x;
+}
+
+bool i(X x) {
+  bool operator==(X, int);
+  bool operator!=(X, int);
+  return x == x;
+}
+} // namespace test2
+} // namespace function_scope_operator_eqeq
+
 namespace non_member_template_2 {
 struct P {};
 template<class S>

>From df6f585d8cd1dbe328733ecb1dde164a312d2f9f Mon Sep 17 00:00:00 2001
From: Utkarsh Saxena <usx at google.com>
Date: Sun, 22 Oct 2023 21:27:17 +0200
Subject: [PATCH 2/4] added more test and visibility check.

---
 clang/lib/Sema/SemaOverload.cpp               |  2 +-
 .../over.match.oper/p3-2a.cpp                 | 49 ++++++++++++++-----
 2 files changed, 38 insertions(+), 13 deletions(-)

diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index 75e4184c8aa6cb8..070e4218ea3afcd 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -964,7 +964,7 @@ static bool shouldAddReversedEqEq(Sema &S, SourceLocation OpLoc,
     auto *NotEqFD = Op->getAsFunction();
     if (auto *UD = dyn_cast<UsingShadowDecl>(Op))
       NotEqFD = UD->getUnderlyingDecl()->getAsFunction();
-    if (FunctionsCorrespond(S.Context, EqFD, NotEqFD) &&
+    if (FunctionsCorrespond(S.Context, EqFD, NotEqFD) && S.isVisible(NotEqFD) &&
         declaresSameEntity(cast<Decl>(EqFD->getEnclosingNamespaceContext()),
                            cast<Decl>(Op->getLexicalDeclContext())))
       return false;
diff --git a/clang/test/CXX/over/over.match/over.match.funcs/over.match.oper/p3-2a.cpp b/clang/test/CXX/over/over.match/over.match.funcs/over.match.oper/p3-2a.cpp
index 9dc5ee8db565341..016eaf7f52876d5 100644
--- a/clang/test/CXX/over/over.match/over.match.funcs/over.match.oper/p3-2a.cpp
+++ b/clang/test/CXX/over/over.match/over.match.funcs/over.match.oper/p3-2a.cpp
@@ -376,13 +376,17 @@ bool fine(A a, B b) { return a == b; } // Ok. Found a matching operator!=.
 
 
 namespace ADL_GH68901{
+namespace test1 {
 namespace A {
 struct S {};
 bool operator==(S, int); // expected-note {{no known conversion from 'int' to 'S' for 1st argument}}
+bool a = 0 == A::S(); // Ok. Operator!= not visible.
 bool operator!=(S, int);
 } // namespace A
 bool a = 0 == A::S(); // expected-error {{invalid operands to binary expression ('int' and 'A::S')}}
+} // namespace test1
 
+namespace test2 {
 namespace B {
 struct Derived {};
 struct Base : Derived {};
@@ -391,23 +395,20 @@ bool operator==(Derived& a, Base& b);
 bool operator!=(Derived& a, Base& b);
 }  // namespace B
 
-void foo() {
-  // B::Base{} == B::Base{};
+bool foo() {
   B::Base a,b;
-  bool v = a == b;
+  return a == b;
 }
+} // namespace test2
 
-namespace ns {
-template <class T>
-struct A {
-};
 
-template <class T>
-struct B : A<T> {
-};
+namespace template_ {
+namespace ns {
+template <class T> struct A {};
+template <class T> struct B : A<T> {};
 
-template <class T> bool operator == (B<T>, A<T>); // expected-note {{candidate template ignored: could not match 'B' against 'A'}}
-template <class T> bool operator != (B<T>, A<T>);
+template <class T> bool operator==(B<T>, A<T>); // expected-note {{candidate template ignored: could not match 'B' against 'A'}}
+template <class T> bool operator!=(B<T>, A<T>);
 }
 
 void test() {
@@ -415,7 +416,31 @@ void test() {
     ns::B<int> b;
     a == b; // expected-error {{invalid operands to binary expression}}
 }
+} // namespace test3
+
+namespace using_not_eq {
+namespace A {
+struct S {};
+namespace B {
+bool operator!=(S, int);
+}
+bool operator==(S, int); // expected-note {{candidate}}
+using B::operator!=;
+}  // namespace A
+bool a = 0 == A::S();  // expected-error {{invalid operands to binary expression}}
+} // namespace reversed_lookup_not_like_ADL
 
+namespace using_eqeq {
+namespace A {
+struct S {};
+namespace B {
+bool operator==(S, int); // expected-note {{candidate}}
+bool operator!=(S, int);
+}
+using B::operator==;
+}  // namespace A
+bool a = 0 == A::S();  // expected-error {{invalid operands to binary expression}}
+}
 
 } //namespace ADL_GH68901
 

>From 24e874ca5e6f1b4ac58068cbfb68cbb48fd5e2e3 Mon Sep 17 00:00:00 2001
From: Utkarsh Saxena <usx at google.com>
Date: Mon, 23 Oct 2023 14:04:42 +0200
Subject: [PATCH 3/4] added test for visibility

---
 .../over.match.oper/p2468R2.cppm              | 24 +++++++++++++++++++
 1 file changed, 24 insertions(+)
 create mode 100644 clang/test/CXX/over/over.match/over.match.funcs/over.match.oper/p2468R2.cppm

diff --git a/clang/test/CXX/over/over.match/over.match.funcs/over.match.oper/p2468R2.cppm b/clang/test/CXX/over/over.match/over.match.funcs/over.match.oper/p2468R2.cppm
new file mode 100644
index 000000000000000..05891618991da07
--- /dev/null
+++ b/clang/test/CXX/over/over.match/over.match.funcs/over.match.oper/p2468R2.cppm
@@ -0,0 +1,24 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -std=c++20 %t/A.cppm -emit-module-interface -o %t/A.pcm
+// RUN: %clang_cc1 -std=c++20 -fprebuilt-module-path=%t -I%t %t/p2468r2.cpp -verify
+
+//--- A.cppm
+module;
+export module A;
+export {
+namespace NS {
+struct S {};
+bool operator==(S, int);
+} // namespace NS
+}
+
+namespace NS { bool operator!=(S, int); } // Not visible.
+
+
+//--- p2468r2.cpp
+// expected-no-diagnostics
+import A;
+bool x = 0 == NS::S(); // Ok. operator!= from module A is not visible.

>From 183209f71b08a1fdba335d89c906cd26483115c1 Mon Sep 17 00:00:00 2001
From: Utkarsh Saxena <usx at google.com>
Date: Wed, 25 Oct 2023 16:32:52 +0200
Subject: [PATCH 4/4] friend operator!=

---
 clang/lib/Sema/SemaOverload.cpp               | 18 ++++++++++-----
 .../over.match.oper/p3-2a.cpp                 | 22 +++++++++++++++++++
 2 files changed, 34 insertions(+), 6 deletions(-)

diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index 070e4218ea3afcd..30bde49636b2f69 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -14,6 +14,7 @@
 #include "clang/AST/ASTLambda.h"
 #include "clang/AST/CXXInheritance.h"
 #include "clang/AST/DeclCXX.h"
+#include "clang/AST/DeclFriend.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/DependenceFlags.h"
 #include "clang/AST/Expr.h"
@@ -960,13 +961,16 @@ static bool shouldAddReversedEqEq(Sema &S, SourceLocation OpLoc,
     return true;
   }
   // Otherwise the search scope is the namespace scope of which F is a member.
-  for (NamedDecl *Op : EqFD->getEnclosingNamespaceContext()->lookup(NotEqOp)) {
+  DeclContext *EqDC = EqFD->getEnclosingNamespaceContext();
+  for (NamedDecl *Op : EqDC->lookup(NotEqOp)) {
     auto *NotEqFD = Op->getAsFunction();
+    DeclContext *NotEqDC = Op->getFriendObjectKind()
+                               ? NotEqFD->getEnclosingNamespaceContext()
+                               : Op->getLexicalDeclContext();
     if (auto *UD = dyn_cast<UsingShadowDecl>(Op))
       NotEqFD = UD->getUnderlyingDecl()->getAsFunction();
     if (FunctionsCorrespond(S.Context, EqFD, NotEqFD) && S.isVisible(NotEqFD) &&
-        declaresSameEntity(cast<Decl>(EqFD->getEnclosingNamespaceContext()),
-                           cast<Decl>(Op->getLexicalDeclContext())))
+        declaresSameEntity(cast<Decl>(EqDC), cast<Decl>(NotEqDC)))
       return false;
   }
   return true;
@@ -10086,9 +10090,11 @@ static bool allowAmbiguity(ASTContext &Context, const FunctionDecl *F1,
                            const FunctionDecl *F2) {
   if (declaresSameEntity(F1, F2))
     return true;
-  if (F1->isTemplateInstantiation() && F2->isTemplateInstantiation() &&
-      declaresSameEntity(F1->getPrimaryTemplate(), F2->getPrimaryTemplate())) {
-    return true;
+  if (F1->isTemplateInstantiation() && F2->isTemplateInstantiation()) {
+    FunctionTemplateDecl *P1 = F1->getPrimaryTemplate();
+    FunctionTemplateDecl *P2 = F2->getPrimaryTemplate();
+    if (declaresSameEntity(P1, P2))
+      return true;
   }
   // TODO: It is not clear whether comparing parameters is necessary (i.e.
   // different functions with same params). Consider removing this (as no test
diff --git a/clang/test/CXX/over/over.match/over.match.funcs/over.match.oper/p3-2a.cpp b/clang/test/CXX/over/over.match/over.match.funcs/over.match.oper/p3-2a.cpp
index 016eaf7f52876d5..2292822e230dde6 100644
--- a/clang/test/CXX/over/over.match/over.match.funcs/over.match.oper/p3-2a.cpp
+++ b/clang/test/CXX/over/over.match/over.match.funcs/over.match.oper/p3-2a.cpp
@@ -374,6 +374,28 @@ bool fine(A a, B b) { return a == b; } // Ok. Found a matching operator!=.
 }
 }
 
+namespace friend_opNE_GH{
+namespace test1 {
+struct S {
+    operator int();
+    friend bool operator==(const S &, int); // expected-note {{reversed}}
+};
+struct A : S {};
+struct B : S {};
+bool x = A{} == B{}; // expected-warning {{ambiguous}}
+}
+
+namespace test2 {
+struct S {
+    operator int();
+    friend bool operator==(const S &, int);
+    friend bool operator!=(const S &, int);
+};
+struct A : S {};
+struct B : S {};
+bool x = A{} == B{};
+}
+}
 
 namespace ADL_GH68901{
 namespace test1 {



More information about the cfe-commits mailing list