[Sema] Take into account the current context when checking the accessibility of a member function pointer

[ahatanak]

Summary

Sema::CheckAddressOfMemberAccess was disregarding the context in which the member pointer was referenced.

This patch fixes PR32898.

rdar://problem/33737747

[ahatanak]

Test access to protected member functions.

[ahatanak]

ping

[ahatanak]

ping

[Rakete1111]

lib/Sema/SemaAccess.cpp

1798

You don't need that scope resolution operator there. Also, I guess you don't have to create EC, you can just pass EffectiveContext(CurScope->getEntity()) directly to IsAccessible.

[ahatanak]

Address review comments.

lib/Sema/SemaAccess.cpp

1798

The scope resolution operator is needed here because IsAccessible's return type is AccessResult defined at the top of SemaAccess.cpp, not the one defined in Sema.h. If I remove the scope resolution operator, clang issues a warning ("comparison of two values with different enumeration types").

EffectiveContext is passed as a temporary now as you suggested.

[ahatanak]

ping

[arphaman]

LGTM, I didn't find any issues

[ahatanak]

The patch got reverted in r325335 as it broke the Chromium build. I'm reopening this review.

[ahatanak]

The new patch passes the calling context to CheckNonDependent so that Sema::CheckAddressOfMemberAccess uses the correct context to check accessibility of explicit template arguments. I initially tried moving the declaration of SavedContext in Sema::FinishTemplateArgumentDeduction to after the call to CheckNonDependent, but that caused test/SemaCXX/cxx1z-class-template-argument-deduction.cpp to fail. It seems like it changes the way lookup of default template arguments is done when the context (which is a CXXDeductionGuideDecl in the failing test case) is not set before ConvertDeducedTemplateArguments is called.

[ahatanak]

ping

[ahatanak]

ping

[rnk]

I don't see a new test case for the issue reduced out of Chromium. I'd recommend adding that.

I have to resign because I don't know enough about deduction to stamp this...

[ahatanak]

Add a test case for the Chromium failure. Also, simplify a bit by capturing the calling context in Sema::DeduceTemplateArguments.

[ahatanak]

ping

[jkorous]

I don't particularly like that between setting the DeclContext (SemaTemplateDeduction.cpp:3814) and actually using it (CheckAccess() in SemaAccess.cpp:1459) are some 20 stack frames but it looks like you already tried fixing this "locally" in your initial approach.

I assume we can't get the appropriate DeclContext from Expr *OvlExpr in CheckAddressOfMemberAccess(), right?

[ahatanak]

It doesn't look like it's possible to get the context needed to do accessibility check (CXXMethodDecl 'method' for 'C::overloadedMethod' in the test case) from 'Expr *OvlExpr' in CheckAddressOfMemberAccess. It's possible to get the class in which the member is defined (class ''C' for C::overloadedMethod'), but we want the context in which the member is used (that is 'method').

[jkorous]

LGTM.

Thank you for the explanation!

lib/Sema/SemaAccess.cpp

if (!getLangOpts().AccessControl ||
       Found.getAccess() == AS_public)
     return AR_accessible;
 
   OverloadExpr *Ovl = OverloadExpr::find(OvlExpr).Expression;
   CXXRecordDecl *NamingClass = Ovl->getNamingClass();
 
   AccessTarget Entity(Context, AccessTarget::Member, NamingClass, Found,
                       /*no instance context*/ QualType());
+
+  EffectiveContext EC(CurScope->getEntity());
+  if (IsAccessible(*this, EC, Entity) == ::AR_accessible)
+    return AR_accessible;
+
   Entity.setDiag(diag::err_access)
     << Ovl->getSourceRange();
 
   return CheckAccess(*this, Ovl->getNameLoc(), Entity);
 }
 
 /// Checks access for a hierarchy conversion.
 ///

test/SemaCXX/access.cpp

class A {
     friend class D;
     static void foo(); // expected-note {{here}}
   };
   template <class T> void foo() {
     []() { A::foo(); }(); // expected-error {{private}}
   }
   void bar() { foo<void>(); }
 }
+
+namespace OverloadedMemberFunctionPointer {
+  template<class T, void(T::*pMethod)()>
+  void func0() {}
+
+  template<class T>
+  void func1(void(*fn)()) {} // expected-note {{candidate function not viable: no overload of 'func0' matching 'void (*)()' for 1st argument}}
+
+  class C {
+    friend void friendFunc();
+    void overloadedMethod();
+  public:
+    void overloadedMethod(int);
+    void method() {
+      func1<int>(&func0<C, &C::overloadedMethod>);
+    }
+  };
+
+  void friendFunc() {
+    func1<int>(&func0<C, &C::overloadedMethod>);
+  }
+
+  void nonFriendFunc() {
+    func1<int>(&func0<C, &C::overloadedMethod>); // expected-error {{no matching function for call to 'func1'}}
+  }
+}