https://github.com/zyn0217 created https://github.com/llvm/llvm-project/pull/141340
https://github.com/llvm/llvm-project/commit/200f3bd39562f4d605f13567398025d30fa27d61 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 https://github.com/llvm/llvm-project/issues/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. >From 57fd785b2dde19e287c2478644312bc112db3045 Mon Sep 17 00:00:00 2001 From: Younan Zhang <zyn7...@gmail.com> Date: Sat, 24 May 2025 16:02:15 +0800 Subject: [PATCH 1/2] Revert "[Clang][Sema] access checking of friend declaration should not be delayed (#91430)" This reverts commit 200f3bd39562f4d605f13567398025d30fa27d61. --- clang/include/clang/Sema/Scope.h | 6 ------ clang/lib/Parse/ParseDecl.cpp | 7 ++----- clang/lib/Sema/Scope.cpp | 1 - clang/lib/Sema/SemaAccess.cpp | 30 +++++------------------------- clang/test/SemaCXX/PR12361.cpp | 30 ------------------------------ 5 files changed, 7 insertions(+), 67 deletions(-) delete mode 100644 clang/test/SemaCXX/PR12361.cpp 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..92da29b47dbf7 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); 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;} - }; >From dd5dee18e899f56c5785e0a3f040ca6277f813ac Mon Sep 17 00:00:00 2001 From: Younan Zhang <zyn7...@gmail.com> Date: Sat, 24 May 2025 17:35:04 +0800 Subject: [PATCH 2/2] [Clang] Clean up the fix for deferred access checking 200f3bd39 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. --- clang/lib/Sema/SemaAccess.cpp | 30 +++++++++++++++- clang/test/SemaCXX/access-control-check.cpp | 40 ++++++++++++++++++++- 2 files changed, 68 insertions(+), 2 deletions(-) diff --git a/clang/lib/Sema/SemaAccess.cpp b/clang/lib/Sema/SemaAccess.cpp index 92da29b47dbf7..40ee20419c2a5 100644 --- a/clang/lib/Sema/SemaAccess.cpp +++ b/clang/lib/Sema/SemaAccess.cpp @@ -1497,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/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(); } + +} _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
