[Clang] Clean up the fix for deferred access checking

[zyn0217]

200f3bd introduced a parsing scope to avoid deferring access checking
for friend declarations. That turned out to be insufficient because it
only sets up the scope until we see the friend keyword, which can be
bypassed if the declaration occurs before the keyword.

That said, based on the discussion in #12361, we still want to
preserve the fix since a friend function declaration doesn't grant the
access to other redeclarations.

This patch implemented it by not considering the friend declaration
functions as effective contexts. So we don't end up checking the
canonical function declaration when we're supposed to check only
non-function friend declarations.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Younan Zhang (zyn0217)

Changes

200f3bd introduced a parsing scope to avoid deferring access checking
for friend declarations. That turned out to be insufficient because it
only sets up the scope until we see the friend keyword, which can be
bypassed if the declaration occurs before the keyword.

That said, based on the discussion in #12361, we still want to
preserve the fix since a friend function declaration doesn't grant the
access to other redeclarations.

This patch implemented it by not considering the friend declaration
functions as effective contexts. So we don't end up checking the
canonical function declaration when we're supposed to check only
non-function friend declarations.

---

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

6 Files Affected:

• (modified) clang/include/clang/Sema/Scope.h (-6)
• (modified) clang/lib/Parse/ParseDecl.cpp (+2-5)
• (modified) clang/lib/Sema/Scope.cpp (-1)
• (modified) clang/lib/Sema/SemaAccess.cpp (+34-26)
• (removed) clang/test/SemaCXX/PR12361.cpp (-30)
• (modified) clang/test/SemaCXX/access-control-check.cpp (+39-1)

diff --git a/clang/include/clang/Sema/Scope.h b/clang/include/clang/Sema/Scope.h
index ad12a3d73413b..77497918f54eb 100644
--- a/clang/include/clang/Sema/Scope.h
+++ b/clang/include/clang/Sema/Scope.h
@@ -160,9 +160,6 @@ class Scope {
 
     /// This is a scope of type alias declaration.
     TypeAliasScope = 0x20000000,
-
-    /// This is a scope of friend declaration.
-    FriendScope = 0x40000000,
   };
 
 private:
@@ -590,9 +587,6 @@ class Scope {
   /// Determine whether this scope is a type alias scope.
   bool isTypeAliasScope() const { return getFlags() & Scope::TypeAliasScope; }
 
-  /// Determine whether this scope is a friend scope.
-  bool isFriendScope() const { return getFlags() & Scope::FriendScope; }
-
   /// Returns if rhs has a higher scope depth than this.
   ///
   /// The caller is responsible for calling this only if one of the two scopes
diff --git a/clang/lib/Parse/ParseDecl.cpp b/clang/lib/Parse/ParseDecl.cpp
index 7a87cd2e340cc..bcebf14aa4fdc 100644
--- a/clang/lib/Parse/ParseDecl.cpp
+++ b/clang/lib/Parse/ParseDecl.cpp
@@ -4219,12 +4219,9 @@ void Parser::ParseDeclarationSpecifiers(
 
     // friend
     case tok::kw_friend:
-      if (DSContext == DeclSpecContext::DSC_class) {
+      if (DSContext == DeclSpecContext::DSC_class)
         isInvalid = DS.SetFriendSpec(Loc, PrevSpec, DiagID);
-        Scope *CurS = getCurScope();
-        if (!isInvalid && CurS)
-          CurS->setFlags(CurS->getFlags() | Scope::FriendScope);
-      } else {
+      else {
         PrevSpec = ""; // not actually used by the diagnostic
         DiagID = diag::err_friend_invalid_in_context;
         isInvalid = true;
diff --git a/clang/lib/Sema/Scope.cpp b/clang/lib/Sema/Scope.cpp
index 5bc7e79a68186..ec7fd92f53e6e 100644
--- a/clang/lib/Sema/Scope.cpp
+++ b/clang/lib/Sema/Scope.cpp
@@ -233,7 +233,6 @@ void Scope::dumpImpl(raw_ostream &OS) const {
       {LambdaScope, "LambdaScope"},
       {OpenACCComputeConstructScope, "OpenACCComputeConstructScope"},
       {TypeAliasScope, "TypeAliasScope"},
-      {FriendScope, "FriendScope"},
   };
 
   for (auto Info : FlagInfo) {
diff --git a/clang/lib/Sema/SemaAccess.cpp b/clang/lib/Sema/SemaAccess.cpp
index 890df09157aa0..40ee20419c2a5 100644
--- a/clang/lib/Sema/SemaAccess.cpp
+++ b/clang/lib/Sema/SemaAccess.cpp
@@ -1469,32 +1469,12 @@ static Sema::AccessResult CheckAccess(Sema &S, SourceLocation Loc,
   // specifier, like this:
   //   A::private_type A::foo() { ... }
   //
-  // friend declaration should not be delayed because it may lead to incorrect
-  // redeclaration chain, such as:
-  //   class D {
-  //    class E{
-  //     class F{};
-  //     friend  void foo(D::E::F& q);
-  //    };
-  //    friend  void foo(D::E::F& q);
-  //   };
+  // Or we might be parsing something that will turn out to be a friend:
+  //   void foo(A::private_type);
+  //   void B::foo(A::private_type);
   if (S.DelayedDiagnostics.shouldDelayDiagnostics()) {
-    // [class.friend]p9:
-    // A member nominated by a friend declaration shall be accessible in the
-    // class containing the friend declaration. The meaning of the friend
-    // declaration is the same whether the friend declaration appears in the
-    // private, protected, or public ([class.mem]) portion of the class
-    // member-specification.
-    Scope *TS = S.getCurScope();
-    bool IsFriendDeclaration = false;
-    while (TS && !IsFriendDeclaration) {
-      IsFriendDeclaration = TS->isFriendScope();
-      TS = TS->getParent();
-    }
-    if (!IsFriendDeclaration) {
-      S.DelayedDiagnostics.add(DelayedDiagnostic::makeAccess(Loc, Entity));
-      return Sema::AR_delayed;
-    }
+    S.DelayedDiagnostics.add(DelayedDiagnostic::makeAccess(Loc, Entity));
+    return Sema::AR_delayed;
   }
 
   EffectiveContext EC(S.CurContext);
@@ -1517,9 +1497,37 @@ void Sema::HandleDelayedAccessCheck(DelayedDiagnostic &DD, Decl *D) {
     DC = D->getLexicalDeclContext();
   } else if (FunctionDecl *FN = dyn_cast<FunctionDecl>(D)) {
     DC = FN;
+    // C++ [class.access.general]p4:
+    //   Access control is applied uniformly to declarations and expressions.
+    // C++ [class.access.general]p6:
+    //   All access controls in [class.access] affect the ability to name a
+    //   class member from the declaration, including parts of the declaration
+    //   preceding the name of the entity being declared [...]
+    //
+    // A friend function declaration might name an entity to which it has access
+    // in the particular context, but it doesn't implicitly grant access
+    // to that entity in other declaration contexts. For example:
+    //
+    //   class A {
+    //     using T = int;
+    //     friend void foo(T);     // #1
+    //   };
+    //   class B {
+    //     friend void foo(A::T);  // #2
+    //   };
+    //
+    // The friend declaration at #1 does not give B access to A::T, nor does it
+    // befriend B. Therefore, the friend declaration should not serve
+    // as an effective context.
+    if (FN->getFriendObjectKind())
+      DC = FN->getLexicalDeclContext();
   } else if (TemplateDecl *TD = dyn_cast<TemplateDecl>(D)) {
-    if (auto *D = dyn_cast_if_present<DeclContext>(TD->getTemplatedDecl()))
+    if (auto *D = dyn_cast_if_present<DeclContext>(TD->getTemplatedDecl())) {
       DC = D;
+      if (FunctionDecl *FN = dyn_cast<FunctionDecl>(DC);
+          FN && FN->getFriendObjectKind())
+        DC = FN->getLexicalDeclContext();
+    }
   } else if (auto *RD = dyn_cast<RequiresExprBodyDecl>(D)) {
     DC = RD;
   }
diff --git a/clang/test/SemaCXX/PR12361.cpp b/clang/test/SemaCXX/PR12361.cpp
deleted file mode 100644
index 95ceb45b7ba04..0000000000000
--- a/clang/test/SemaCXX/PR12361.cpp
+++ /dev/null
@@ -1,30 +0,0 @@
- // RUN: %clang_cc1 -fsyntax-only -verify -std=c++98 %s
- // RUN: %clang_cc1 -fsyntax-only -verify -std=c++17 %s
- 
-class D {
-    class E{
-        class F{}; // expected-note{{implicitly declared private here}}
-        friend  void foo(D::E::F& q);
-        };
-    friend  void foo(D::E::F& q); // expected-error{{'F' is a private member of 'D::E'}}
-    };
-
-void foo(D::E::F& q) {}
-
-class D1 {
-    class E1{
-        class F1{}; // expected-note{{implicitly declared private here}}
-        friend  D1::E1::F1 foo1();
-        };
-    friend  D1::E1::F1 foo1(); // expected-error{{'F1' is a private member of 'D1::E1'}}
-    };
-
-D1::E1::F1 foo1() { return D1::E1::F1(); }
-
-class D2 {
-    class E2{
-        class F2{};
-        friend  void foo2();
-        };
-    friend  void foo2(){ D2::E2::F2 c;}
-    };
diff --git a/clang/test/SemaCXX/access-control-check.cpp b/clang/test/SemaCXX/access-control-check.cpp
index 76d27ce3e55cb..f7661e5aaca44 100644
--- a/clang/test/SemaCXX/access-control-check.cpp
+++ b/clang/test/SemaCXX/access-control-check.cpp
@@ -1,4 +1,5 @@
-// RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wno-unused-variable -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wno-unused-variable -std=c++20 -verify=expected,since-cxx20 %s
 
 class M {
   int iM;
@@ -59,3 +60,40 @@ struct A {
 int i = f<B>();
 
 }
+
+namespace GH12361 {
+class D1 {
+  class E1 {
+    class F1 {}; // #F1
+
+    friend D1::E1::F1 foo1();
+    friend void foo2(D1::E1::F1);
+    friend void foo3() noexcept(sizeof(D1::E1::F1) == 1);
+    friend void foo4();
+#if __cplusplus >= 202002L
+    template <class T>
+    friend void foo5(T) requires (sizeof(D1::E1::F1) == 1);
+#endif
+  };
+
+  D1::E1::F1 friend foo1();     // expected-error{{'F1' is a private member of 'GH12361::D1::E1'}}
+  // expected-note@#F1 {{implicitly declared private}}
+  friend void foo2(D1::E1::F1); // expected-error{{'F1' is a private member of 'GH12361::D1::E1'}}
+  // expected-note@#F1 {{implicitly declared private}}
+
+  // FIXME: This should be diagnosed. We entered the function DC too early.
+  friend void foo3() noexcept(sizeof(D1::E1::F1) == 1);
+  friend void foo4() {
+    D1::E1::F1 V;
+  }
+#if __cplusplus >= 202002L
+  template <class T>
+  friend void foo5(T)
+    requires (sizeof(D1::E1::F1) == 1); // since-cxx20-error {{'F1' is a private member of 'GH12361::D1::E1'}}
+  // since-cxx20-note@#F1 {{implicitly declared private}}
+#endif
+};
+
+D1::E1::F1 foo1() { return D1::E1::F1(); }
+
+}