Use the correct namespace for looking up matching operator!=

[usx95]

S.getScopeForContext determins the active scope associated with the given declContext.
This fails to find the matching operator!= if candidate operator== was found via ADL since that scope is not active.

Instead, just directly lookup using the namespace decl of operator==

Fixes #68901

[llvmbot]

@llvm/pr-subscribers-clang

Author: Utkarsh Saxena (usx95)

Changes

S.getScopeForContext determins the active scope associated with the given declContext.
This fails to find the matching operator!= if candidate operator== was found via ADL since that scope is not active.

Instead, just directly lookup using the namespace decl of operator==

Fixes #68901

---

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

2 Files Affected:

• (modified) clang/lib/Sema/SemaOverload.cpp (+1-6)
• (modified) clang/test/CXX/over/over.match/over.match.funcs/over.match.oper/p3-2a.cpp (+24)

diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index ce78994e6553814..e1e0b4a888e49fa 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -960,12 +960,7 @@ 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) {
+  for (NamedDecl *Op : EqFD->getEnclosingNamespaceContext()->lookup(NotEqOp)) {
     auto *FD = Op->getAsFunction();
     if(auto* UD = dyn_cast<UsingShadowDecl>(Op))
       FD = UD->getUnderlyingDecl()->getAsFunction();
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 5c6804eb7726b5f..7142ed22ddb22ca 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
@@ -324,6 +324,30 @@ bool x = X() == X(); // expected-warning {{ambiguous}}
 }
 } // namespace P2468R2
 
+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 ADL_GH68901
+
+
 #else // NO_ERRORS
 
 namespace problem_cases {

[zygoloid]

Does this work for function-scope operator declarations?

[cor3ntin]

Looks reasonable, but i found a typo

[usx95]

Does this work for function-scope operator declarations?

@zygoloid I am not sure I follow. Could you please give an example. If this is about member operator, the search scope is class scope of the first operand.

[zygoloid]

Does this work for function-scope operator declarations?

@zygoloid I am not sure I follow. Could you please give an example.

Sure:

struct X { operator int(); };

bool f(X x) {
  bool operator==(X, int);
  return x == x;
}


bool g(X x) {
  bool operator==(X, int);
  bool operator!=(X, int);
  return x == x;
}

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;
}

Based on the standard's rules, it looks like f and g should be invalid due to ambiguity, but h and i should be valid. (Which I think is a pretty surprising outcome, but that's what the rules say.)

If there's not already something like that in our test suite, it'd be good to add it.

[usx95]

Thanks @zygoloid.
This case was failing with my change. Corrected and added tests.

[ilya-biryukov]

Oh, one last thing! @usx95 maybe add a test for the visibility check?
Various tests with modules (search for files with the .cppm extension in the tests) should give a good starting point.