[Clang][Sema] access checking of friend declaration should not be delayed #91430

jcsxky · 2024-05-08T03:43:03Z

attempt to fix #12361
Consider this example:

class D {
    class E{
        class F{};
        friend  void foo(D::E::F& q);
        };
    friend  void foo(D::E::F& q);
    };

void foo(D::E::F& q) {}

The first friend declaration of foo is correct. After that, the second friend declaration delayed access checking and set its previous declaration to be the first one. When doing access checking of F(which is private filed of E), we put its canonical declaration(the first friend declaration) into EffectiveContext.Functions. Actually, we are still checking the first one. This is incorrect due to the delayed checking.
Creating a new scope to indicate we are parsing a friend declaration and doing access checking in time.

llvmbot · 2024-05-08T03:43:32Z

@llvm/pr-subscribers-clang

Author: Qizhi Hu (jcsxky)

Changes

attempt to fix #12361
Consider this example:

class D {
    class E{
        class F{}; // expected-note{{implicitly declared private here}}
        friend  void foo(D::E::F&amp; q);
        };
    friend  void foo(D::E::F&amp; q);
    };

void foo(D::E::F&amp; q) {}

The first friend declaration of foo is correct. After that, the second friend declaration delayed access checking and set its previous declaration to be the first one. When doing access checking of F(which is private filed of E), we put its canonical declaration(the first friend declaration) into EffectiveContext.Functions. Actually, we are still checking the first one. This is incorrect due to the delayed checking.
Creating a new scope to indicate we are parsing a friend declaration and doing access checking in time.

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

5 Files Affected:

(modified) clang/include/clang/Sema/Scope.h (+6)
(modified) clang/lib/Parse/ParseDecl.cpp (+5-2)
(modified) clang/lib/Sema/Scope.cpp (+2)
(modified) clang/lib/Sema/SemaAccess.cpp (+7-1)
(added) clang/test/SemaCXX/PR12361.cpp (+30)

diff --git a/clang/include/clang/Sema/Scope.h b/clang/include/clang/Sema/Scope.h
index 1752a25111a77..084db73034219 100644
--- a/clang/include/clang/Sema/Scope.h
+++ b/clang/include/clang/Sema/Scope.h
@@ -159,6 +159,9 @@ class Scope {
 
     /// This is a scope of type alias declaration.
     TypeAliasScope = 0x20000000,
+
+    /// This is a scope of friend declaration.
+    FriendScope = 0x40000000,
   };
 
 private:
@@ -586,6 +589,9 @@ 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 4e4b05b21383e..78a81c77f48c6 100644
--- a/clang/lib/Parse/ParseDecl.cpp
+++ b/clang/lib/Parse/ParseDecl.cpp
@@ -4332,9 +4332,12 @@ void Parser::ParseDeclarationSpecifiers(
 
     // friend
     case tok::kw_friend:
-      if (DSContext == DeclSpecContext::DSC_class)
+      if (DSContext == DeclSpecContext::DSC_class) {
         isInvalid = DS.SetFriendSpec(Loc, PrevSpec, DiagID);
-      else {
+        Scope *CurS = getCurScope();
+        if (!isInvalid && CurS)
+          CurS->setFlags(CurS->getFlags() | Scope::FriendScope);
+      } 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 11a41753a1bda..780aa898b1085 100644
--- a/clang/lib/Sema/Scope.cpp
+++ b/clang/lib/Sema/Scope.cpp
@@ -229,6 +229,8 @@ void Scope::dumpImpl(raw_ostream &OS) const {
       {ClassInheritanceScope, "ClassInheritanceScope"},
       {CatchScope, "CatchScope"},
       {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 6a707eeb66d01..ded88da510525 100644
--- a/clang/lib/Sema/SemaAccess.cpp
+++ b/clang/lib/Sema/SemaAccess.cpp
@@ -1476,7 +1476,13 @@ static Sema::AccessResult CheckAccess(Sema &S, SourceLocation Loc,
   // 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()) {
+  Scope *TS = S.getCurScope();
+  bool IsFriendDeclaration = false;
+  while (TS && !IsFriendDeclaration) {
+    IsFriendDeclaration = TS->isFriendScope();
+    TS = TS->getParent();
+  }
+  if (S.DelayedDiagnostics.shouldDelayDiagnostics() && !IsFriendDeclaration) {
     S.DelayedDiagnostics.add(DelayedDiagnostic::makeAccess(Loc, Entity));
     return Sema::AR_delayed;
   }
diff --git a/clang/test/SemaCXX/PR12361.cpp b/clang/test/SemaCXX/PR12361.cpp
new file mode 100644
index 0000000000000..95ceb45b7ba04
--- /dev/null
+++ b/clang/test/SemaCXX/PR12361.cpp
@@ -0,0 +1,30 @@
+ // 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;}
+    };

cor3ntin · 2024-05-08T08:31:34Z

clang/lib/Sema/SemaAccess.cpp

@@ -1477,8 +1477,16 @@ static Sema::AccessResult CheckAccess(Sema &S, SourceLocation Loc,
  //   void foo(A::private_type);


The comment above needs tweaking

cor3ntin · 2024-05-08T08:31:58Z

clang/lib/Sema/Scope.cpp

@@ -229,6 +229,8 @@ void Scope::dumpImpl(raw_ostream &OS) const {
      {ClassInheritanceScope, "ClassInheritanceScope"},
      {CatchScope, "CatchScope"},
      {OpenACCComputeConstructScope, "OpenACCComputeConstructScope"},
+      {TypeAliasScope, "TypeAliasScope"},


Note: Drive by fix

Removed. This will be done in a follow-up.

cor3ntin · 2024-05-08T08:38:40Z

clang/lib/Sema/SemaAccess.cpp

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


We might want to quote the standard
https://eel.is/c++draft/class.friend#9

cor3ntin · 2024-05-08T08:39:03Z

Can you add a changelog entry?

jcsxky · 2024-05-08T09:01:55Z

Can you add a changelog entry?

Ah, I forgot that! will be added when applying other reviews.

…ayed

cor3ntin

LGTM, thanks!

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels May 8, 2024

jcsxky force-pushed the fix-12361 branch from 836a66f to a4eee6d Compare May 8, 2024 04:17

cor3ntin reviewed May 8, 2024

View reviewed changes

jcsxky force-pushed the fix-12361 branch from bce02c3 to 77cfb6f Compare May 8, 2024 13:24

jcsxky requested review from cor3ntin, AaronBallman and erichkeane May 8, 2024 13:26

jcsxky force-pushed the fix-12361 branch from 77cfb6f to 97723ff Compare May 9, 2024 00:39

jcsxky added 2 commits May 10, 2024 06:28

[Clang][Sema] access checking of friend declaration should not be del…

0027514

…ayed

apply reviews

113c850

jcsxky force-pushed the fix-12361 branch from 97723ff to 113c850 Compare May 9, 2024 22:29

cor3ntin approved these changes May 10, 2024

View reviewed changes

jcsxky merged commit 200f3bd into llvm:main May 10, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Clang][Sema] access checking of friend declaration should not be delayed #91430

[Clang][Sema] access checking of friend declaration should not be delayed #91430

Uh oh!

jcsxky commented May 8, 2024 •

edited

Loading

Uh oh!

llvmbot commented May 8, 2024

Uh oh!

cor3ntin May 8, 2024

Uh oh!

jcsxky May 8, 2024

Uh oh!

cor3ntin May 8, 2024

Uh oh!

jcsxky May 8, 2024

Uh oh!

cor3ntin May 8, 2024

Uh oh!

jcsxky May 8, 2024

Uh oh!

cor3ntin commented May 8, 2024

Uh oh!

jcsxky commented May 8, 2024

Uh oh!

cor3ntin left a comment

Uh oh!

Uh oh!

Uh oh!

		@@ -1477,8 +1477,16 @@ static Sema::AccessResult CheckAccess(Sema &S, SourceLocation Loc,
		// void foo(A::private_type);

[Clang][Sema] access checking of friend declaration should not be delayed #91430

[Clang][Sema] access checking of friend declaration should not be delayed #91430

Uh oh!

Conversation

jcsxky commented May 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented May 8, 2024

Uh oh!

cor3ntin May 8, 2024

Choose a reason for hiding this comment

Uh oh!

jcsxky May 8, 2024

Choose a reason for hiding this comment

Uh oh!

cor3ntin May 8, 2024

Choose a reason for hiding this comment

Uh oh!

jcsxky May 8, 2024

Choose a reason for hiding this comment

Uh oh!

cor3ntin May 8, 2024

Choose a reason for hiding this comment

Uh oh!

jcsxky May 8, 2024

Choose a reason for hiding this comment

Uh oh!

cor3ntin commented May 8, 2024

Uh oh!

jcsxky commented May 8, 2024

Uh oh!

cor3ntin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jcsxky commented May 8, 2024 •

edited

Loading