[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 ↗

(On Diff #111805)

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 ↗

(On Diff #111805)

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/SemaTemplateDeduction.cpp

for (unsigned ParamIdx = 0, NumParamTypes = ParamTypes.size(), ArgIdx = 0;
     }
 
     // Build argument packs for each of the parameter packs expanded by this
     // pack expansion.
     if (auto Result = PackScope.finish())
       return Result;
   }
 
+  // Capture the context in which the function call is made. This is the context
+  // that is needed when the accessibility of template arguments is checked.
+  DeclContext *CallingCtx = CurContext;
+
   return FinishTemplateArgumentDeduction(
       FunctionTemplate, Deduced, NumExplicitlySpecified, Specialization, Info,
-      &OriginalCallArgs, PartialOverloading,
-      [&]() { return CheckNonDependent(ParamTypesForArgChecking); });
+      &OriginalCallArgs, PartialOverloading, [&, CallingCtx]() {
+        ContextRAII SavedContext(*this, CallingCtx);
+        return CheckNonDependent(ParamTypesForArgChecking);
+      });
 }
 
 QualType Sema::adjustCCAndNoReturn(QualType ArgFunctionType,
                                    QualType FunctionType,
                                    bool AdjustExceptionSpec) {
   if (ArgFunctionType.isNull())
     return ArgFunctionType;
 

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(T::*pMethod)(int)>
+  void func1() {}
+
+  template<class T>
+  void func2(void(*fn)()) {} // expected-note 2 {{candidate function template not viable: no overload of 'func}}
+
+  class C {
+  private:
+    friend void friendFunc();
+    void overloadedMethod();
+  protected:
+    void overloadedMethod(int);
+  public:
+    void overloadedMethod(int, int);
+    void method() {
+      func2<int>(&func0<C, &C::overloadedMethod>);
+      func2<int>(&func1<C, &C::overloadedMethod>);
+    }
+  };
+
+  void friendFunc() {
+    func2<int>(&func0<C, &C::overloadedMethod>);
+    func2<int>(&func1<C, &C::overloadedMethod>);
+  }
+
+  void nonFriendFunc() {
+    func2<int>(&func0<C, &C::overloadedMethod>); // expected-error {{no matching function for call to 'func2'}}
+    func2<int>(&func1<C, &C::overloadedMethod>); // expected-error {{no matching function for call to 'func2'}}
+  }
+
+  // r325321 caused an assertion failure when the following code was compiled.
+  class A {
+    template <typename Type> static bool foo1() { return true; }
+
+  public:
+    void init(bool c) {
+      if (c) {
+        auto f = foo1<int>;
+      }
+    }
+  };
+}