Fix ADL leakage due to MSVC compatibility flag

[frederic-tingaud-sonarsource]

Summary

Patch https://reviews.llvm.org/D124613 introduced an ADL leakage of friend functions when in MSVC compatibility mode. It broke some C++ compliant code. That patch fixes the problem introduced.

The original fix flagged friend function declarations as declarations, to follow MSVC behavior. The problem it introduced is that this same flag is used to do ADL and that made friend functions visible at the namespace level instead of them being visible at the class level only.

[frederic-tingaud-sonarsource]

After checking, I realize that the previous fix was really mimicking MSVC behavior up to its mishandling of standard compliant code.
https://godbolt.org/z/cEPGfMz3f
I'll revert it for now and try to see if there is a way to have it under another flag or something.

clang/lib/Sema/SemaLookup.cpp

for (auto *D : R) {
         continue;
 
       // The declaration is visible to argument-dependent lookup if either
       // it's ordinarily visible or declared as a friend in an associated
       // class.
       bool Visible = false;
       for (D = D->getMostRecentDecl(); D;
            D = cast_or_null<NamedDecl>(D->getPreviousDecl())) {
-        if (D->getIdentifierNamespace() & Decl::IDNS_Ordinary) {
-          if (isVisible(D)) {
+        // We check the friend case first because in case of MSVC compatibility
+        // a friend declaration is also flagged as Ordinary but we don't want
+        // it to be visible outside its associated class.
+        if (D->getFriendObjectKind()) {
+          auto *RD = cast<CXXRecordDecl>(D->getLexicalDeclContext());
+          if (AssociatedClasses.count(RD) && isVisible(D)) {
             Visible = true;
             break;
           }
-        } else if (D->getFriendObjectKind()) {
-          auto *RD = cast<CXXRecordDecl>(D->getLexicalDeclContext());
-          if (AssociatedClasses.count(RD) && isVisible(D)) {
+        } else if (D->getIdentifierNamespace() & Decl::IDNS_Ordinary) {
+          if (isVisible(D)) {
             Visible = true;
             break;
           }
         }
       }
 
       // FIXME: Preserve D as the FoundDecl.
       if (Visible)

clang/test/SemaCXX/ms-friend-function-decl.cpp

-// RUN: %clang_cc1 -std=c++03 -fms-compatibility -fsyntax-only -verify %s
-// RUN: %clang_cc1 -std=c++17 -fms-compatibility -fsyntax-only -verify %s
-// RUN: %clang_cc1 -std=c++20 -fms-compatibility -fsyntax-only -verify=modern %s
-#if __cplusplus < 202002L
-// expected-no-diagnostics
-#endif
+// RUN: %clang_cc1 -std=c++17 -fms-compatibility -fsyntax-only -verify=expected %s
+// RUN: %clang_cc1 -std=c++20 -fms-compatibility -fsyntax-only -verify=modern,expected %s
 
 namespace ns {
 
 class C {
 public:
   template <typename T>
   friend void funtemp();
 
public:
 
   void test() {
     funGlob(); // modern-error {{use of undeclared identifier 'funGlob'}}
   }
 };
 
 void funGlob() {
 }
+
+// At some point, considering friend declaration as full declarations broke ADL detection.
+// The following test checks that the friend declaration is only visible inside the class it's declared in.
+namespace check_adl {
+template <typename T>
+struct A {};
+
+struct NotADLVisible {
+  template <typename T>
+  friend constexpr bool operator==(A<T>, NotADLVisible) noexcept {
+    static_assert(sizeof(T) < 0, "This should never be instantiated");
+    return false;
+  }
+
+  constexpr NotADLVisible(int) {}
+};
+} // namespace check_adl
+
+void shouldNotSeeAnyOperator() {
+  bool b = check_adl::A<int>() == 2; // expected-error {{invalid operands to binary expression ('check_adl::A<int>' and 'int')}}
+}
+
+// Check that we don't break the case of a function that is both visible to ADL and a friend
+namespace check_adl_compliant {
+struct A {};
+
+struct B {
+  friend constexpr bool operator==(A, B) noexcept;
+
+  constexpr B(int) {}
+};
+
+constexpr bool operator==(check_adl_compliant::A, check_adl_compliant::B) noexcept {
+  return false;
+}
+} // namespace check_adl_compliant
+
+void shouldSeeTheOperator() {
+  bool b = check_adl_compliant::A() == 2;
+}