Check access to friend declarations. There's a number of different

things going on here that were problematic:
  - We were missing the actual access check, or rather, it was suppressed
    on account of being a redeclaration lookup.
  - The access check would naturally happen during delay, which isn't
    appropriate in this case.
  - We weren't actually emitting dependent diagnostics associated with
    class templates, which was unfortunate.
  - Access was being propagated incorrectly for friend method declarations
    that couldn't be matched at parse-time.

llvm-svn: 161652

clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -909,6 +909,9 @@ def err_access_field_ctor : Error<
   "field of type %0 has %select{private|protected}2 "
   "%select{default |copy |move |*ERROR* |*ERROR* |*ERROR* |}1constructor">,
   AccessControl;
+def err_access_friend_function : Error<
+  "friend function %1 is a %select{private|protected}0 member of %3">,
+  AccessControl;
 
 def err_access_dtor : Error<
   "calling a %select{private|protected}1 destructor of class %0">, 

clang/include/clang/Sema/Sema.h

@@ -4427,9 +4427,7 @@ public:
                                      CXXDestructorDecl *Dtor,
                                      const PartialDiagnostic &PDiag,
                                      QualType objectType = QualType());
-  AccessResult CheckDirectMemberAccess(SourceLocation Loc,
-                                       NamedDecl *D,
-                                       const PartialDiagnostic &PDiag);
+  AccessResult CheckFriendAccess(NamedDecl *D);
   AccessResult CheckMemberOperatorAccess(SourceLocation Loc,
                                          Expr *ObjectExpr,
                                          Expr *ArgExpr,

clang/lib/Sema/SemaAccess.cpp

@@ -1632,25 +1632,6 @@ Sema::AccessResult Sema::CheckConstructorAccess(SourceLocation UseLoc,
   return CheckAccess(*this, UseLoc, AccessEntity);
 } 
 
-/// Checks direct (i.e. non-inherited) access to an arbitrary class
-/// member.
-Sema::AccessResult Sema::CheckDirectMemberAccess(SourceLocation UseLoc,
-                                                 NamedDecl *Target,
-                                           const PartialDiagnostic &Diag) {
-  AccessSpecifier Access = Target->getAccess();
-  if (!getLangOpts().AccessControl ||
-      Access == AS_public)
-    return AR_accessible;
-
-  CXXRecordDecl *NamingClass = cast<CXXRecordDecl>(Target->getDeclContext());
-  AccessTarget Entity(Context, AccessTarget::Member, NamingClass,
-                      DeclAccessPair::make(Target, Access),
-                      QualType());
-  Entity.setDiag(Diag);
-  return CheckAccess(*this, UseLoc, Entity);
-}
-                                           
-
 /// Checks access to an overloaded operator new or delete.
 Sema::AccessResult Sema::CheckAllocationAccess(SourceLocation OpLoc,
                                                SourceRange PlacementRange,
@@ -1693,6 +1674,44 @@ Sema::AccessResult Sema::CheckMemberOperatorAccess(SourceLocation OpLoc,
   return CheckAccess(*this, OpLoc, Entity);
 }
 
+/// Checks access to the target of a friend declaration.
+Sema::AccessResult Sema::CheckFriendAccess(NamedDecl *target) {
+  assert(isa<CXXMethodDecl>(target) ||
+         (isa<FunctionTemplateDecl>(target) &&
+          isa<CXXMethodDecl>(cast<FunctionTemplateDecl>(target)
+                               ->getTemplatedDecl())));
+
+  // Friendship lookup is a redeclaration lookup, so there's never an
+  // inheritance path modifying access.
+  AccessSpecifier access = target->getAccess();
+
+  if (!getLangOpts().AccessControl || access == AS_public)
+    return AR_accessible;
+
+  CXXMethodDecl *method = dyn_cast<CXXMethodDecl>(target);
+  if (!method)
+    method = cast<CXXMethodDecl>(
+                     cast<FunctionTemplateDecl>(target)->getTemplatedDecl());
+  assert(method->getQualifier());
+
+  AccessTarget entity(Context, AccessTarget::Member,
+                      cast<CXXRecordDecl>(target->getDeclContext()),
+                      DeclAccessPair::make(target, access),
+                      /*no instance context*/ QualType());
+  entity.setDiag(diag::err_access_friend_function)
+    << method->getQualifierLoc().getSourceRange();
+
+  // We need to bypass delayed-diagnostics because we might be called
+  // while the ParsingDeclarator is active.
+  EffectiveContext EC(CurContext);
+  switch (CheckEffectiveAccess(*this, EC, target->getLocation(), entity)) {
+  case AR_accessible: return Sema::AR_accessible;
+  case AR_inaccessible: return Sema::AR_inaccessible;
+  case AR_dependent: return Sema::AR_dependent;
+  }
+  llvm_unreachable("falling off end");
+}
+
 Sema::AccessResult Sema::CheckAddressOfMemberAccess(Expr *OvlExpr,
                                                     DeclAccessPair Found) {
   if (!getLangOpts().AccessControl ||

clang/lib/Sema/SemaDeclCXX.cpp

@@ -10342,9 +10342,11 @@ Decl *Sema::ActOnFriendFunctionDecl(Scope *S, Declarator &D,
   FrD->setAccess(AS_public);
   CurContext->addDecl(FrD);
 
-  if (ND->isInvalidDecl())
+  if (ND->isInvalidDecl()) {
     FrD->setInvalidDecl();
-  else {
+  } else {
+    if (DC->isRecord()) CheckFriendAccess(ND);
+
     FunctionDecl *FD;
     if (FunctionTemplateDecl *FTD = dyn_cast<FunctionTemplateDecl>(ND))
       FD = FTD->getTemplatedDecl();

clang/lib/Sema/SemaTemplateInstantiate.cpp

@@ -2005,6 +2005,9 @@ Sema::InstantiateClass(SourceLocation PointOfInstantiation,
   }
 
   if (!Instantiation->isInvalidDecl()) {
+    // Perform any dependent diagnostics from the pattern.
+    PerformDependentDiagnostics(Pattern, TemplateArgs);
+
     // Instantiate any out-of-line class template partial
     // specializations now.
     for (TemplateDeclInstantiator::delayed_partial_spec_iterator 

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

@@ -935,8 +935,6 @@ TemplateDeclInstantiator::VisitFunctionTemplateDecl(FunctionTemplateDecl *D) {
   if (!Instantiated)
     return 0;
 
-  Instantiated->setAccess(D->getAccess());
-
   // Link the instantiated function template declaration to the function
   // template from which it was instantiated.
   FunctionTemplateDecl *InstTemplate
@@ -954,8 +952,12 @@ TemplateDeclInstantiator::VisitFunctionTemplateDecl(FunctionTemplateDecl *D) {
     InstTemplate->setInstantiatedFromMemberTemplate(D);
 
   // Make declarations visible in the appropriate context.
-  if (!isFriend)
+  if (!isFriend) {
     Owner->addDecl(InstTemplate);
+  } else if (InstTemplate->getDeclContext()->isRecord() &&
+             !D->getPreviousDecl()) {
+    SemaRef.CheckFriendAccess(InstTemplate);
+  }
 
   return InstTemplate;
 }
@@ -1526,7 +1528,15 @@ TemplateDeclInstantiator::VisitCXXMethodDecl(CXXMethodDecl *D,
   if (D->isPure())
     SemaRef.CheckPureMethod(Method, SourceRange());
 
-  Method->setAccess(D->getAccess());
+  // Propagate access.  For a non-friend declaration, the access is
+  // whatever we're propagating from.  For a friend, it should be the
+  // previous declaration we just found.
+  if (isFriend && Method->getPreviousDecl())
+    Method->setAccess(Method->getPreviousDecl()->getAccess());
+  else 
+    Method->setAccess(D->getAccess());
+  if (FunctionTemplate)
+    FunctionTemplate->setAccess(Method->getAccess());
 
   SemaRef.CheckOverrideControl(Method);
 
@@ -1536,18 +1546,28 @@ TemplateDeclInstantiator::VisitCXXMethodDecl(CXXMethodDecl *D,
   if (D->isDeletedAsWritten())
     Method->setDeletedAsWritten();
 
+  // If there's a function template, let our caller handle it.
   if (FunctionTemplate) {
-    // If there's a function template, let our caller handle it.
+    // do nothing
+
+  // Don't hide a (potentially) valid declaration with an invalid one.
   } else if (Method->isInvalidDecl() && !Previous.empty()) {
-    // Don't hide a (potentially) valid declaration with an invalid one.
-  } else {
-    NamedDecl *DeclToAdd = (TemplateParams
-                            ? cast<NamedDecl>(FunctionTemplate)
-                            : Method);
-    if (isFriend)
-      Record->makeDeclVisibleInContext(DeclToAdd);
-    else if (!IsClassScopeSpecialization)
-      Owner->addDecl(DeclToAdd);
+    // do nothing
+
+  // Otherwise, check access to friends and make them visible.
+  } else if (isFriend) {
+    // We only need to re-check access for methods which we didn't
+    // manage to match during parsing.
+    if (!D->getPreviousDecl())
+      SemaRef.CheckFriendAccess(Method);
+
+    Record->makeDeclVisibleInContext(Method);
+
+  // Otherwise, add the declaration.  We don't need to do this for
+  // class-scope specializations because we'll have matched them with
+  // the appropriate template.
+  } else if (!IsClassScopeSpecialization) {
+    Owner->addDecl(Method);
   }
 
   if (D->isExplicitlyDefaulted()) {

clang/test/CXX/class.access/class.friend/p1.cpp

@@ -64,6 +64,7 @@ namespace test0 {
   };
 
   class MemberFriend {
+  public:
     void test();
   };
 
@@ -309,6 +310,7 @@ namespace test10 {
 // PR8705
 namespace test11 {
   class A {
+  public:
     void test0(int);
     void test1(int);
     void test2(int);

clang/test/CXX/class.access/class.friend/p9-cxx0x.cpp

@@ -0,0 +1,117 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+// C++98 [class.friend]p7:
+// C++11 [class.friend]p9:
+//   A name nominated by a friend declaration shall be accessible in
+//   the scope of the class containing the friend declaration.
+
+// PR12328
+// Simple, non-templated case.
+namespace test0 {
+  class X {
+    void f(); // expected-note {{implicitly declared private here}}
+  };
+
+  class Y {
+    friend void X::f(); // expected-error {{friend function 'f' is a private member of 'test0::X'}}
+  };
+}
+
+// Templated but non-dependent.
+namespace test1 {
+  class X {
+    void f(); // expected-note {{implicitly declared private here}}
+  };
+
+  template <class T> class Y {
+    friend void X::f(); // expected-error {{friend function 'f' is a private member of 'test1::X'}}
+  };
+}
+
+// Dependent but instantiated at the right type.
+namespace test2 {
+  template <class T> class Y;
+
+  class X {
+    void f();
+    friend class Y<int>;
+  };
+
+  template <class T> class Y {
+    friend void X::f();
+  };
+
+  template class Y<int>;
+}
+
+// Dependent and instantiated at the wrong type.
+namespace test3 {
+  template <class T> class Y;
+
+  class X {
+    void f(); // expected-note {{implicitly declared private here}}
+    friend class Y<int>;
+  };
+
+  template <class T> class Y {
+    friend void X::f(); // expected-error {{friend function 'f' is a private member of 'test3::X'}}
+  };
+
+  template class Y<float>; // expected-note {{in instantiation}}
+}
+
+// Dependent because dependently-scoped.
+namespace test4 {
+  template <class T> class X {
+    void f();
+  };
+
+  template <class T> class Y {
+    friend void X<T>::f();
+  };
+}
+
+// Dependently-scoped, no friends.
+namespace test5 {
+  template <class T> class X {
+    void f(); // expected-note {{implicitly declared private here}}
+  };
+
+  template <class T> class Y {
+    friend void X<T>::f(); // expected-error {{friend function 'f' is a private member of 'test5::X<int>'}}
+  };
+
+  template class Y<int>; // expected-note {{in instantiation}}
+}
+
+// Dependently-scoped, wrong friend.
+namespace test6 {
+  template <class T> class Y;
+
+  template <class T> class X {
+    void f(); // expected-note {{implicitly declared private here}}
+    friend class Y<float>;
+  };
+
+  template <class T> class Y {
+    friend void X<T>::f(); // expected-error {{friend function 'f' is a private member of 'test6::X<int>'}}
+  };
+
+  template class Y<int>; // expected-note {{in instantiation}}
+}
+
+// Dependently-scoped, right friend.
+namespace test7 {
+  template <class T> class Y;
+
+  template <class T> class X {
+    void f();
+    friend class Y<int>;
+  };
+
+  template <class T> class Y {
+    friend void X<T>::f();
+  };
+
+  template class Y<int>;
+}

clang/test/CXX/temp/temp.decls/temp.friend/p1.cpp

@@ -302,6 +302,7 @@ namespace test14 {
   };
 
   template <class T> class B {
+  public:
     void foo() { return A<long>::foo(); } // expected-error {{'foo' is a private member of 'test14::A<long>'}}
   };
 
@@ -320,10 +321,12 @@ namespace test15 {
   };
 
   template <class T> class B {
+  public:
     void foo() { return A<long>::foo(); } // expected-error {{'foo' is a private member of 'test15::A<long>'}}
   };
 
   template <> class B<float> {
+  public:
     void foo() { return A<float>::foo(); }
     template <class U> void bar(U u) {
       (void) A<float>::foo();