[PATCH] D125526: Fix ADL leakage due to MSVC compatibility flag

[Fred Tingaud via Phabricator via cfe-commits]

frederic-tingaud-sonarsource created this revision.
frederic-tingaud-sonarsource added a reviewer: rnk.
Herald added a project: All.
frederic-tingaud-sonarsource requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D125526

Files:
  clang/lib/Sema/SemaLookup.cpp
  clang/test/SemaCXX/ms-friend-function-decl.cpp


Index: clang/test/SemaCXX/ms-friend-function-decl.cpp
===================================================================
--- clang/test/SemaCXX/ms-friend-function-decl.cpp
+++ clang/test/SemaCXX/ms-friend-function-decl.cpp
@@ -1,9 +1,5 @@
-// 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 {
 
@@ -43,3 +39,43 @@
 
 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;
+}
Index: clang/lib/Sema/SemaLookup.cpp
===================================================================
--- clang/lib/Sema/SemaLookup.cpp
+++ clang/lib/Sema/SemaLookup.cpp
@@ -3634,14 +3634,17 @@
       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;
           }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D125526.429166.patch
Type: text/x-patch
Size: 3180 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20220513/72a427e5/attachment.bin>