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

include/clang/Sema/Sema.h

 
   TemplateDeductionResult FinishTemplateArgumentDeduction(
       FunctionTemplateDecl *FunctionTemplate,
       SmallVectorImpl<DeducedTemplateArgument> &Deduced,
       unsigned NumExplicitlySpecified, FunctionDecl *&Specialization,
       sema::TemplateDeductionInfo &Info,
       SmallVectorImpl<OriginalCallArg> const *OriginalCallArgs = nullptr,
       bool PartialOverloading = false,
-      llvm::function_ref<bool()> CheckNonDependent = []{ return false; });
+      llvm::function_ref<bool(DeclContext *)> CheckNonDependent =
+          [](DeclContext *){ return false; });
 
   TemplateDeductionResult DeduceTemplateArguments(
       FunctionTemplateDecl *FunctionTemplate,
       TemplateArgumentListInfo *ExplicitTemplateArgs, ArrayRef<Expr *> Args,
       FunctionDecl *&Specialization, sema::TemplateDeductionInfo &Info,
       bool PartialOverloading,
       llvm::function_ref<bool(ArrayRef<QualType>)> CheckNonDependent);
 

lib/Sema/SemaTemplateDeduction.cpp

 /// \param OriginalCallArgs If non-NULL, the original call arguments against
 /// which the deduced argument types should be compared.
 Sema::TemplateDeductionResult Sema::FinishTemplateArgumentDeduction(
     FunctionTemplateDecl *FunctionTemplate,
     SmallVectorImpl<DeducedTemplateArgument> &Deduced,
     unsigned NumExplicitlySpecified, FunctionDecl *&Specialization,
     TemplateDeductionInfo &Info,
     SmallVectorImpl<OriginalCallArg> const *OriginalCallArgs,
-    bool PartialOverloading, llvm::function_ref<bool()> CheckNonDependent) {
+    bool PartialOverloading,
+    llvm::function_ref<bool(DeclContext *)> CheckNonDependent) {
   // Unevaluated SFINAE context.
   EnterExpressionEvaluationContext Unevaluated(
       *this, Sema::ExpressionEvaluationContext::Unevaluated);
   SFINAETrap Trap(*this);
 
   // Enter a new template instantiation context while we instantiate the
   // actual function declaration.
   SmallVector<TemplateArgument, 4> DeducedArgs(Deduced.begin(), Deduced.end());
   InstantiatingTemplate Inst(
       *this, Info.getLocation(), FunctionTemplate, DeducedArgs,
       CodeSynthesisContext::DeducedTemplateArgumentSubstitution, Info);
   if (Inst.isInvalid())
     return TDK_InstantiationDepth;
 
+  DeclContext *CallingCtx = CurContext;
   ContextRAII SavedContext(*this, FunctionTemplate->getTemplatedDecl());
 
   // C++ [temp.deduct.type]p2:
   //   [...] or if any template argument remains neither deduced nor
   //   explicitly specified, template argument deduction fails.
   SmallVector<TemplateArgument, 4> Builder;
   if (auto Result = ConvertDeducedTemplateArguments(
           *this, FunctionTemplate, /*IsDeduced*/true, Deduced, Info, Builder,
           CurrentInstantiationScope, NumExplicitlySpecified,
           PartialOverloading))
     return Result;
 
   // C++ [temp.deduct.call]p10: [DR1391]
   //   If deduction succeeds for all parameters that contain
   //   template-parameters that participate in template argument deduction,
   //   and all template arguments are explicitly specified, deduced, or
   //   obtained from default template arguments, remaining parameters are then
   //   compared with the corresponding arguments. For each remaining parameter
   //   P with a type that was non-dependent before substitution of any
   //   explicitly-specified template arguments, if the corresponding argument
   //   A cannot be implicitly converted to P, deduction fails.
-  if (CheckNonDependent())
+  if (CheckNonDependent(CallingCtx))
     return TDK_NonDependentConversionFailure;
 
   // Form the template argument list from the deduced template arguments.
   TemplateArgumentList *DeducedArgumentList
     = TemplateArgumentList::CreateCopy(Context, Builder);
   Info.reset(DeducedArgumentList);
 
   // Substitute the deduced template arguments into the function template
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;
   }
 
   return FinishTemplateArgumentDeduction(
       FunctionTemplate, Deduced, NumExplicitlySpecified, Specialization, Info,
-      &OriginalCallArgs, PartialOverloading,
-      [&]() { return CheckNonDependent(ParamTypesForArgChecking); });
+      &OriginalCallArgs, PartialOverloading, [&](DeclContext *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 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'}}
+  }
+}