[clang] Implement P0692R1 from C++20 (access checking on specializations)

[broadwaylamb]

Summary

This patch implements paper P0692R1 from the C++20 standard.

This also fixes a bug (https://llvm.org/PR37424) where explicit instantiations of templates parameterized by overloaded private member functions were incorrectly rejected.

This is my first contribution to CFE, so please let me know if I did something horribly wrong.

[rsmith]

Thanks! Is this really the only case we were getting wrong? If so, great!

We should make sure we have test coverage for all the cases affected by P0692R1. Please add some complementary tests for the cases where diagnostics should still be produced. For example, in addition to testing:

template<typename T, void (TestClass::*)()> class TemplateClass3 {};
template<typename T> class TemplateClass3<T, &TestClass::func> {};

... please also test ...

template<typename T> class TemplateClass3<T, &TestClass::func> varTemplate3{};

... which we should diagnose, because that's a primary variable template definition, not a partial / explicit specialization or explicit instantiation.

Please also add testcases for specializations / instantiations of function templates, variable templates, and for explicit specializations of members of class templates:

template<typename T> struct X {
  struct A {};
  void f();
  enum E : int;
  static int var;
};
class Y { using Z = int; };
template<> struct X<Y::Z>::A {};
template<> void X<Y::Z>::f() {}
template<> enum X<Y::Z>::E : int {};
template<> int X<Y::Z>::var = 76;

(It looks like we incorrectly reject the function and variable cases here, and presumably will still do so after this patch, so there's a little more work to be done before we can call P0692R1 complete.)

[broadwaylamb]

In D78404#1990192, @rsmith wrote:

... please also test ...

template<typename T> class TemplateClass3<T, &TestClass::func> varTemplate3{};

... which we should diagnose, because that's a primary variable template definition, not a partial / explicit specialization or explicit instantiation.

Interestingly, the latest GCC doesn't diagnose here, but we do. Do we need to remain compatible with GCC in this case?

[broadwaylamb]

• Add more tests
• Allow class template member explicit specializations
• Inherit TypeAliasDecl from DeclContext (this is needed so that we could perform access checks when parsing 'using' declaration templates)

[broadwaylamb]

clang/include/clang/AST/Decl.h

3198

I'm not sure about inheriting TypeAliasDecl from DeclContext, but (see below)

clang/lib/Parse/ParseTemplate.cpp

211

…but otherwise I couldn't make it print access level diagnostics for a particular kind of using declaration template (see in the next inline comment).

Why? Because of this line — we need to be able to cast the TypeAliasDecl to DeclContext in order for delayed access check to be actually performed.

clang/test/CXX/temp/temp.decls/temp.class.spec/p10.cpp

58

I'm talking about declarations like this.

Previously, we didn't reject it (which I believe was incorrect), and now we do.

[broadwaylamb]

clang/lib/Parse/ParseDecl.cpp

5672

This is for things like

template<> void X<Y::Z>::f() {}

not to be rejected (here Z is a private member of class Y)

I wasn't sure how to suppress it only when we're parsing template parameter list, so we suppress it unconditionally here. All the tests pass though, but I'd appreciate any hints.

Note that testing that D.getContext() == DeclaratorContext::TemplateParamContext doesn't work — when we get here, we're actually in a FileContext.

[rsmith]

In D78404#1990461, @broadwaylamb wrote:

In D78404#1990192, @rsmith wrote:

... please also test ...

template<typename T> class TemplateClass3<T, &TestClass::func> varTemplate3{};

... which we should diagnose, because that's a primary variable template definition, not a partial / explicit specialization or explicit instantiation.

Interestingly, the latest GCC doesn't diagnose here, but we do. Do we need to remain compatible with GCC in this case?

GCC doesn't even diagnose

class TestClass { struct X {}; };
template<typename T> TestClass::X varTemplate3_1b{};
void use() { varTemplate3_1b<int> = {}; }

... which is a flagrant access violation. I don't think we should be compatible with that, it just seems like a regular GCC bug rather than any kind of intentional extension.

clang/include/clang/AST/Decl.h

3198

Yeah, this doesn't seem appropriate. Hopefully we can find another way.

clang/lib/Parse/ParseDecl.cpp

5672

The cases that this is going to incorrectly accept will be a bit weird. For example:

struct A { void f(); };
class B { using T = A; };
void B::T::f() {}

We should be able to feed through information on whether we're parsing after template or template<> here (in which case we should suppress access) -- it seems reasonable to track that on the Declarator. One thing that seems unclear is whether we should suppress access here:

template<typename T> struct A;
class X { struct Y {}; };
template<> struct A<X::Y> { void f(); };
void A<X::Y>::f() {} // suppress access here or not?

To get that right, we'd need to temporarily suppress access here and do a one-token lookahead past the scope specifier before diagnosing -- this case should still report an access error:

template<typename T> struct A;
class X { struct Y {}; };
template<> struct A<X::Y> { int f(); };
int A<X::Y>::*f() {} // do not suppress access here, this is a regular non-template function

Note that testing that D.getContext() == DeclaratorContext::TemplateParamContext doesn't work

Right, that context indicates that we're parsing a template parameter.

clang/test/CXX/temp/temp.decls/temp.class.spec/p10.cpp

58

Do you know where we're suppressing the diagnostic in the first place? After we parse the *template-head*, we may be able to just look at the next token (to see if it's using) to determine if we should defer access checks. Making TypeAliasDecl be a DeclContext seems like a very heavy-handed way of handling this.

clang/include/clang/AST/Decl.h

public:
 
   // Implement isa/cast/dyncast/etc.
   static bool classof(const Decl *D) { return classofKind(D->getKind()); }
   static bool classofKind(Kind K) { return K == Typedef; }
 };
 
 /// Represents the declaration of a typedef-name via a C++11
 /// alias-declaration.
-class TypeAliasDecl : public TypedefNameDecl {
+class TypeAliasDecl : public TypedefNameDecl, public DeclContext {
   /// The template for which this is the pattern, if any.
   TypeAliasTemplateDecl *Template;
 
   TypeAliasDecl(ASTContext &C, DeclContext *DC, SourceLocation StartLoc,
                 SourceLocation IdLoc, IdentifierInfo *Id, TypeSourceInfo *TInfo)
       : TypedefNameDecl(TypeAlias, C, DC, StartLoc, IdLoc, Id, TInfo),
-        Template(nullptr) {}
+        DeclContext(TypeAlias), Template(nullptr) {}
 
 public:
   static TypeAliasDecl *Create(ASTContext &C, DeclContext *DC,
                                SourceLocation StartLoc, SourceLocation IdLoc,
                                IdentifierInfo *Id, TypeSourceInfo *TInfo);
   static TypeAliasDecl *CreateDeserialized(ASTContext &C, unsigned ID);
 
   SourceRange getSourceRange() const override LLVM_READONLY;

clang/include/clang/AST/DeclBase.h

 ///   OMPDeclareMapperDecl
 ///   FunctionDecl
 ///   ObjCMethodDecl
 ///   ObjCContainerDecl
 ///   LinkageSpecDecl
 ///   ExportDecl
 ///   BlockDecl
 ///   CapturedDecl
+///   TypeAliasDecl
 class DeclContext {
   /// For makeDeclVisibleInContextImpl
   friend class ASTDeclReader;
   /// For reconcileExternalVisibleStorage, CreateStoredDeclsMap,
   /// hasNeedToReconcileExternalVisibleStorage
   friend class ExternalASTSource;
   /// For CreateStoredDeclsMap
   friend class DependentDiagnostic;

clang/include/clang/Basic/DeclNodes.td

 def Named : DeclNode<Decl, "named declarations", 1>;
   def Namespace : DeclNode<Named, "namespaces">, DeclContext;
   def UsingDirective : DeclNode<Named>;
   def NamespaceAlias : DeclNode<Named>;
   def Label : DeclNode<Named, "labels">;
   def Type : DeclNode<Named, "types", 1>;
     def TypedefName : DeclNode<Type, "typedefs", 1>;
       def Typedef : DeclNode<TypedefName>;
-      def TypeAlias : DeclNode<TypedefName>;
+      def TypeAlias : DeclNode<TypedefName>, DeclContext;
       def ObjCTypeParam : DeclNode<TypedefName>;
     def UnresolvedUsingTypename : DeclNode<Type>;
     def Tag : DeclNode<Type, "tag types", 1>, DeclContext;
       def Enum : DeclNode<Tag, "enums">;
       def Record : DeclNode<Tag, "structs, unions, classes">;
         def CXXRecord : DeclNode<Record, "classes">;
           def ClassTemplateSpecialization : DeclNode<CXXRecord>;
             def ClassTemplatePartialSpecialization

clang/lib/AST/DeclBase.cpp

DeclContext *DeclContext::getPrimaryContext() {
   case Decl::ExternCContext:
   case Decl::LinkageSpec:
   case Decl::Export:
   case Decl::Block:
   case Decl::Captured:
   case Decl::OMPDeclareReduction:
   case Decl::OMPDeclareMapper:
   case Decl::RequiresExprBody:
+  case Decl::TypeAlias:
     // There is only one DeclContext for these entities.
     return this;
 
   case Decl::Namespace:
     // The original namespace is our primary context.
     return static_cast<NamespaceDecl *>(this)->getOriginalNamespace();
 
   case Decl::ObjCMethod:

clang/lib/Parse/ParseDecl.cpp

if (getLangOpts().CPlusPlus &&
       (Tok.is(tok::coloncolon) || Tok.is(tok::kw_decltype) ||
        (Tok.is(tok::identifier) &&
         (NextToken().is(tok::coloncolon) || NextToken().is(tok::less))) ||
        Tok.is(tok::annot_cxxscope))) {
     bool EnteringContext =
         D.getContext() == DeclaratorContext::FileContext ||
         D.getContext() == DeclaratorContext::MemberContext;
     CXXScopeSpec SS;
+
+    {
+      // If this is an explicit specialization of a member of a class template,
+      // don't perform access checks in template parameters.
+      //
+      // See C++20 [temp.explicit] 17.8/6:
+      //   The usual access checking rules do not apply to names in
+      //   a declaration of an explicit instantiation or explicit
+      //   specialization, with the exception of names appearing in a function
+      //   body, default argument, base-clause, member-specification,
+      //   enumerator-list, or static data member or variable template
+      //   initializer.
+      SuppressAccessChecks diagsFromTag(*this);
+
       ParseOptionalCXXScopeSpecifier(SS, /*ObjectType=*/nullptr,
-                                   /*ObjectHadErrors=*/false, EnteringContext);
+                                     /*ObjectHasErrors=*/false,
+                                     EnteringContext);
+    }
 
     if (SS.isNotEmpty()) {
       if (Tok.isNot(tok::star)) {
         // The scope spec really belongs to the direct-declarator.
         if (D.mayHaveIdentifier())
           D.getCXXScopeSpec() = SS;
         else
           AnnotateScopeToken(SS, true);

clang/lib/Parse/ParseDeclCXX.cpp

void Parser::ParseClassSpecifier(tok::TokenKind TagTokKind,
   }
 
   if (Tok.is(tok::code_completion)) {
     // Code completion for a struct, class, or union name.
     Actions.CodeCompleteTag(getCurScope(), TagType);
     return cutOffParsing();
   }
 
-  // C++03 [temp.explicit] 14.7.2/8:
-  //   The usual access checking rules do not apply to names used to specify
-  //   explicit instantiations.
+  // C++20 [temp.class.spec] 17.6.5/10:
+  //   The usual access checking rules do not apply to non-dependent names used
+  //   to specify template arguments of the simple-template-id of the partial
+  //   specialization
   //
-  // As an extension we do not perform access checking on the names used to
-  // specify explicit specializations either. This is important to allow
-  // specializing traits classes for private types.
+  // C++20 [temp.explicit] 17.8/6:
+  //   The usual access checking rules do not apply to names in a declaration
+  //   of an explicit instantiation or explicit specialization, with
+  //   the exception of names appearing in a function body, default argument,
+  //   base-clause, member-specification, enumerator-list, or static data member
+  //   or variable template initializer.
   //
   // Note that we don't suppress if this turns out to be an elaborated
   // type specifier.
   bool shouldDelayDiagsInTag =
-    (TemplateInfo.Kind == ParsedTemplateInfo::ExplicitInstantiation ||
-     TemplateInfo.Kind == ParsedTemplateInfo::ExplicitSpecialization);
+      TemplateInfo.Kind != ParsedTemplateInfo::NonTemplate;
   SuppressAccessChecks diagsFromTag(*this, shouldDelayDiagsInTag);
 
   ParsedAttributesWithRange attrs(AttrFactory);
   // If attributes exist after tag, parse them.
   MaybeParseGNUAttributes(attrs);
   MaybeParseMicrosoftDeclSpecs(attrs);
 
   // Parse inheritance specifiers.
if (AttrRange.isValid()) {
         << FixItHint::CreateRemoval(AttrRange);
 
       // Recover by adding misplaced attributes to the attribute list
       // of the class so they can be applied on the class later.
       attrs.takeAllFrom(Attributes);
     }
   }
 
-  // If this is an elaborated type specifier, and we delayed
-  // diagnostics before, just merge them into the current pool.
-  if (shouldDelayDiagsInTag) {
-    diagsFromTag.done();
-    if (TUK == Sema::TUK_Reference)
-      diagsFromTag.redelay();
-  }
-
   if (!Name && !TemplateId && (DS.getTypeSpecType() == DeclSpec::TST_error ||
                                TUK != Sema::TUK_Definition)) {
     if (DS.getTypeSpecType() != DeclSpec::TST_error) {
       // We have a declaration or reference to an anonymous class.
       Diag(StartLoc, diag::err_anon_type_definition)
         << DeclSpec::getSpecifierName(TagType, Policy);
     }
 
if (TemplateId) {
     // less common call.
     if (IsDependent) {
       assert(TUK == Sema::TUK_Reference || TUK == Sema::TUK_Friend);
       TypeResult = Actions.ActOnDependentTag(getCurScope(), TagType, TUK,
                                              SS, Name, StartLoc, NameLoc);
     }
   }
 
+  // If this is an elaborated type specifier, and we delayed
+  // diagnostics before, just merge them into the current pool.
+  if (shouldDelayDiagsInTag) {
+    diagsFromTag.done();
+    if (TUK == Sema::TUK_Reference)
+      diagsFromTag.redelay();
+  }
+
   // If there is a body, parse it and inform the actions module.
   if (TUK == Sema::TUK_Definition) {
     assert(Tok.is(tok::l_brace) ||
            (getLangOpts().CPlusPlus && Tok.is(tok::colon)) ||
            isCXX11FinalKeyword());
     if (SkipBody.ShouldSkip)
       SkipCXXMemberSpecification(StartLoc, AttrFixitLoc, TagType,
                                  TagOrTempResult.get());

clang/lib/Parse/ParseTemplate.cpp

ParseCXXClassMemberDeclaration(AS, AccessAttrs, TemplateInfo,
                                    &DiagsFromTParams);
     return nullptr;
   }
 
   ParsedAttributesWithRange prefixAttrs(AttrFactory);
   MaybeParseCXX11Attributes(prefixAttrs);
 
   if (Tok.is(tok::kw_using)) {
+    ParsingDeclRAIIObject ParsingDeclRAII(*this, &DiagsFromTParams);
     auto usingDeclPtr = ParseUsingDirectiveOrDeclaration(Context, TemplateInfo, DeclEnd,
                                                          prefixAttrs);
     if (!usingDeclPtr || !usingDeclPtr.get().isSingleDecl())
       return nullptr;
-    return usingDeclPtr.get().getSingleDecl();
+    Decl *Decl = usingDeclPtr.get().getSingleDecl();
+    ParsingDeclRAII.complete(Decl);
+    return Decl;
   }
 
   // Parse the declaration specifiers, stealing any diagnostics from
   // the template parameters.
   ParsingDeclSpec DS(*this, &DiagsFromTParams);
 
   ParseDeclarationSpecifiers(DS, TemplateInfo, AS,
                              getDeclSpecContextFromDeclaratorContext(Context));

clang/test/CXX/drs/dr1xx.cpp

template <class T> struct C {
     void f();
     void g();
   };
 
   template <class T> void C<T>::f() {}
   template <class T> void C<T>::g() {}
 
   class A {
-    class B {}; // expected-note {{here}}
+    class B {};
     void f();
   };
 
   template void C<A::B>::f();
-  template <> void C<A::B>::g(); // expected-error {{private}}
+  template <> void C<A::B>::g();
 
   void A::f() {
     C<B> cb;
     cb.f();
   }
 }
 
 namespace dr183 { // dr183: sup 382

clang/test/CXX/temp/temp.decls/temp.class.spec/p10.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+// C++20 [temp.class.spec] 17.6.5/10:
+//   The usual access checking rules do not apply to non-dependent names used
+//   to specify template arguments of the simple-template-id of the partial
+//   specialization.
+
+class TestClass {
+  // expected-note@+1 4 {{declared private here}}
+  void func();
+
+  // expected-note@+1 4 {{declared private here}}
+  void funcOverloaded();
+
+  void funcOverloaded(int);
+
+  // expected-note@+1 2 {{declared private here}}
+  static void staticFunc();
+
+  // expected-note@+1 2 {{declared private here}}
+  static void staticFuncOverloaded();
+
+  static void staticFuncOverloaded(int);
+
+  // expected-note@+1 {{declared private here}}
+  class Nested {};
+
+  // expected-note@+1 {{declared private here}}
+  int field;
+};
+
+template <void (TestClass::*)()> class TemplateClass {};
+template <> class TemplateClass<&TestClass::func> {};
+template <> class TemplateClass<&TestClass::funcOverloaded> {};
+
+// expected-error@+1 {{'func' is a private member of 'TestClass'}}
+using alias1_1 = TemplateClass<&TestClass::func>;
+
+// expected-error@+1 {{'funcOverloaded' is a private member of 'TestClass'}}
+using alias1_2 = TemplateClass<&TestClass::funcOverloaded>;
+
+template <void (*)()> class TemplateClass2 { };
+template <> class TemplateClass2<&TestClass::staticFunc> {};
+template <> class TemplateClass2<&TestClass::staticFuncOverloaded> {};
+
+// expected-error@+1 {{'staticFunc' is a private member of 'TestClass'}}
+using alias2_1 = TemplateClass2<&TestClass::staticFunc>;
+
+// expected-error@+1 {{'staticFuncOverloaded' is a private member of 'TestClass'}}
+using alias2_2 = TemplateClass2<&TestClass::staticFuncOverloaded>;
+
+template<typename T, void (TestClass::*)()> class TemplateClass3 {};
+template<typename T> class TemplateClass3<T, &TestClass::func> {};
+template<typename T> class TemplateClass3<T, &TestClass::funcOverloaded> {};
+
+// expected-error@+2 {{'func' is a private member of 'TestClass'}}
+template <typename T>
+using alias3_1 = TemplateClass3<T, &TestClass::func>;
+
+// expected-error@+1 {{'func' is a private member of 'TestClass'}}
+using alias3_2 = TemplateClass3<int, &TestClass::func>;
+
+// expected-error@+2 {{'funcOverloaded' is a private member of 'TestClass'}}
+template <typename T>
+using alias3_3 = TemplateClass3<T, &TestClass::funcOverloaded>;
+
+// expected-error@+1 {{'funcOverloaded' is a private member of 'TestClass'}}
+using alias3_4 = TemplateClass3<int, &TestClass::funcOverloaded>;
+
+// expected-error@+2 {{'func' is a private member of 'TestClass'}}
+template <typename T>
+class TemplateClass3<T, &TestClass::func> varTemplate3_1 {};
+
+// expected-error@+2 {{'funcOverloaded' is a private member of 'TestClass'}}
+template <typename T>
+class TemplateClass3<T, &TestClass::funcOverloaded> varTemplate3_2 {};
+
+template<typename T, void (*)()> class TemplateClass4 {};
+template<typename T> class TemplateClass4<T, &TestClass::staticFunc> {};
+template<typename T> class TemplateClass4<T, &TestClass::staticFuncOverloaded> {};
+
+// expected-error@+2 {{'staticFunc' is a private member of 'TestClass'}}
+template <typename T>
+class TemplateClass4<T, &TestClass::staticFunc> varTemplate4_1 {};
+
+// expected-error@+2 {{'staticFuncOverloaded' is a private member of 'TestClass'}}
+template <typename T>
+class TemplateClass4<T, &TestClass::staticFuncOverloaded> varTemplate4_2 {};
+
+template<typename T> class TemplateClass5 {};
+template<> class TemplateClass5<TestClass::Nested> {};
+
+template<typename T, typename U> class TemplateClass6 {};
+template<typename T> class TemplateClass6<T, TestClass::Nested> {};
+
+// expected-error@+2 {{'Nested' is a private member of 'TestClass'}}
+template <typename T>
+class TemplateClass6<T, TestClass::Nested> varTemplate6_1 {};
+
+template <int TestClass::*> class TemplateClass7 {};
+template <> class TemplateClass7<&TestClass::field> {};
+
+template <typename T, int TestClass::*> class TemplateClass8 {};
+template <typename T> class TemplateClass8<T, &TestClass::field> {};
+
+// expected-error@+2 {{'field' is a private member of 'TestClass'}}
+template <typename T>
+class TemplateClass8<T, &TestClass::field> varTemplate8_1 {};
+
+template<class T>
+struct trait;
+
+class class_ {
+  template<class U>
+  struct impl;
+};
+
+template<class U>
+struct trait<class_::impl<U>>;

clang/test/CXX/temp/temp.spec/p6.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// expected-no-diagnostics
+
+class X {
+  template <typename T> class Y {};
+};
+
+class A {
+  class B {};
+  class C {};
+
+  void func();
+  static void staticFunc();
+
+  // See https://llvm.org/PR37424
+  void funcOverloaded();
+  void funcOverloaded(int);
+  static void staticFuncOverloaded();
+  static void staticFuncOverloaded(int);
+
+  int field;
+};
+
+// C++20 [temp.spec] 17.8/6:
+//   The usual access checking rules do not apply to names in a declaration of
+//   an explicit instantiation or explicit specialization, with the exception
+//   of names appearing in a function body, default argument, base-clause,
+//   member-specification, enumerator-list, or static data member or variable
+//   template initializer.
+template class X::Y<A::B>;
+
+template <void (A::*)()> class D {};
+template class D<&A::func>;
+template class D<&A::funcOverloaded>;
+
+template <void (*)()> class E { };
+template class E<&A::staticFunc>;
+template class E<&A::staticFuncOverloaded>;
+
+template <int A::*> class G {};
+template class G<&A::field>;
+
+template <> class X::Y<A::C> {};
+
+namespace member_spec {
+
+  template <typename T>
+  struct X {
+    struct A {};
+    void f();
+    enum E : int;
+    static int var;
+  };
+
+  class Y {
+    using Z = int;
+  };
+
+  template <>
+  struct X<Y::Z>::A {};
+
+  template <>
+  void X<Y::Z>::f() {}
+
+  template <>
+  enum X<Y::Z>::E : int {};
+
+  template <>
+  int X<Y::Z>::var = 76;
+
+}

clang/test/CXX/temp/temp.spec/temp.explicit/p11.cpp

-// RUN: %clang_cc1 -fsyntax-only -verify %s
-// expected-no-diagnostics
-
-class X {
-  template <typename T> class Y {};
-};
-
-class A {
-  class B {};
-  class C {};
-};
-
-// C++0x [temp.explicit] 14.7.2/11:
-//   The usual access checking rules do not apply to names used to specify
-//   explicit instantiations.
-template class X::Y<A::B>;
-
-// As an extension, this rule is applied to explicit specializations as well.
-template <> class X::Y<A::C> {};

clang/www/cxx_status.html

</tr>
       </tr>
       <tr>
         <td><a href="https://wg21.link/p2085r0">P2085R0</a></td>
         <td class="none" align="center">No</td>
       </tr>
     <tr>
       <td>Access checking on specializations</td>
       <td><a href="https://wg21.link/p0692r1">P0692R1</a></td>
-      <td class="partial" align="center">Partial</td>
+      <td class="unreleased" align="center">Clang 11</td>
     </tr>
     <tr>
       <td>Default constructible and assignable stateless lambdas</td>
       <td><a href="https://wg21.link/p0624r2">P0624R2</a></td>
       <td class="full" align="center">Clang 8</td>
     </tr>
     <tr>
       <td>Lambdas in unevaluated contexts</td>