fix access checking about function overloading

[Backl1ght]

fix #107629

After some more debugging, I find out that we will check access here at

llvm-project/clang/lib/Sema/SemaInit.cpp

Line 7807 in 8e010ac

S.CheckAddressOfMemberAccess(CurInit.get(), Step->Function.FoundDecl);

And for f() inside code below, Found.getAccess() is AS_none hence CheckAddressOfMemberAccess return AR_accessible directly.

struct Base {
public:
  int f(int);
private:
  int f();  // expect-note {{declared private here}}
};

struct Derived : public Base {};

void f() {
  int(Derived::* public_f)(int) = &Derived::f;
  int(Derived::* private_f)() = &Derived::f;  // expect-error {{'f' is a private member of 'Base'}}
}

I think the Found.getAccess() is intended to be AS_none so I just add one more access check for the UnresolvedLookupExpr when Found.getAccess() is AS_none. If add the check unconditionally clang will report lots of duplicate errors and cause several unit tests to fail.

I also test the UB mentioned in #107629 and clang now display 4 false as expecetd.

[zyn0217]

What's the status here?

[Backl1ght]

Sorry for the late response, I will continue to look into this issue this sunday.

[Backl1ght]

After some more debugging, I find out that we will check access here at

llvm-project/clang/lib/Sema/SemaInit.cpp

Line 7807 in 8e010ac

S.CheckAddressOfMemberAccess(CurInit.get(), Step->Function.FoundDecl);

And for f() inside code below, Found.getAccess() is AS_none hence CheckAddressOfMemberAccess return AR_accessible directly.

struct Base {
public:
  int f(int);
private:
  int f();  // expect-note {{declared private here}}
};

struct Derived : public Base {};

void f() {
  int(Derived::* public_f)(int) = &Derived::f;
  int(Derived::* private_f)() = &Derived::f;  // expect-error {{'f' is a private member of 'Base'}}
}

[llvmbot]

@llvm/pr-subscribers-clang

Author: Zhikai Zeng (Backl1ght)

Changes

fix #107629

After some more debugging, I find out that we will check access here at

llvm-project/clang/lib/Sema/SemaInit.cpp

Line 7807 in 8e010ac

S.CheckAddressOfMemberAccess(CurInit.get(), Step->Function.FoundDecl);

And for f() inside code below, Found.getAccess() is AS_none hence CheckAddressOfMemberAccess return AR_accessible directly.

struct Base {
public:
  int f(int);
private:
  int f();  // expect-note {{declared private here}}
};

struct Derived : public Base {};

void f() {
  int(Derived::* public_f)(int) = &Derived::f;
  int(Derived::* private_f)() = &Derived::f;  // expect-error {{'f' is a private member of 'Base'}}
}

I think the Found.getAccess() is intended to be AS_none so I just add one more access check for the UnresolvedLookupExpr when Found.getAccess() is AS_none. If add the check unconditionally clang will report lots of duplicate errors and cause several unit tests to fail.

I also test the UB mentioned in #107629 and clang now display 4 false as expecetd.

---

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

3 Files Affected:

• (modified) clang/docs/ReleaseNotes.rst (+1)
• (modified) clang/lib/Sema/SemaOverload.cpp (+3)
• (modified) clang/test/CXX/class.access/p4.cpp (+14-2)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index e44aefa90ab386..0de249ced8309a 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -763,6 +763,7 @@ Bug Fixes to C++ Support
 - Fixed a bug where bounds of partially expanded pack indexing expressions were checked too early. (#GH116105)
 - Fixed an assertion failure caused by using ``consteval`` in condition in consumed analyses. (#GH117385)
 - Fix a crash caused by incorrect argument position in merging deduced template arguments. (#GH113659)
+- Fix a bug where private access specifier of overloaded function not respected. (#GH107629)
 
 Bug Fixes to AST Handling
 ^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index 4c9e37bd286dee..35389957adf32d 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -16305,6 +16305,9 @@ ExprResult Sema::FixOverloadedFunctionReference(Expr *E, DeclAccessPair Found,
   }
 
   if (UnresolvedLookupExpr *ULE = dyn_cast<UnresolvedLookupExpr>(E)) {
+    if (Found.getAccess() == AS_none) {
+      CheckUnresolvedLookupAccess(ULE, Found);
+    }
     // FIXME: avoid copy.
     TemplateArgumentListInfo TemplateArgsBuffer, *TemplateArgs = nullptr;
     if (ULE->hasExplicitTemplateArgs()) {
diff --git a/clang/test/CXX/class.access/p4.cpp b/clang/test/CXX/class.access/p4.cpp
index ca98c9f90bd89e..6d4c8c004911db 100644
--- a/clang/test/CXX/class.access/p4.cpp
+++ b/clang/test/CXX/class.access/p4.cpp
@@ -21,11 +21,13 @@ namespace test0 {
   public:
     void foo(Public&);
   protected:
-    void foo(Protected&); // expected-note 2 {{declared protected here}}
+    void foo(Protected&); // expected-note 4 {{declared protected here}}
   private:
-    void foo(Private&); // expected-note 2 {{declared private here}}
+    void foo(Private&); // expected-note 4 {{declared private here}}
   };
 
+  class B : public A {};
+
   void test(A *op) {
     op->foo(PublicInst);
     op->foo(ProtectedInst); // expected-error {{'foo' is a protected member}}
@@ -35,6 +37,16 @@ namespace test0 {
     void (A::*b)(Protected&) = &A::foo; // expected-error {{'foo' is a protected member}}
     void (A::*c)(Private&) = &A::foo; // expected-error {{'foo' is a private member}}
   }
+
+  void test(B *op) {
+    op->foo(PublicInst);
+    op->foo(ProtectedInst); // expected-error {{'foo' is a protected member}}
+    op->foo(PrivateInst); // expected-error {{'foo' is a private member}}
+
+    void (B::*a)(Public&) = &B::foo;
+    void (B::*b)(Protected&) = &B::foo; // expected-error {{'foo' is a protected member}}
+    void (B::*c)(Private&) = &B::foo; // expected-error {{'foo' is a private member}}
+  }
 }
 
 // Member operators.

[erichkeane]

This seems reasonable to me? And passes tests?

The AS_none has 'different meanings in different contexts, which is confusing here, so I have no idea what it is supposed to mean here. But unless someone has a problem with this, I suspect we should just see what the fallout of this is...

THOUGH, because I'm not sure, perhaps we wait another ~week for the branch and only do this in Clang21+?

[erichkeane]

DID a merge commit to see if we can get CI to approve this one, then I'll merge it (or someone else feel free to if you see it green!).