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

(On Diff #258555)

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

clang/lib/Parse/ParseTemplate.cpp

211 ↗

(On Diff #258555)

…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

59

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 ↗

(On Diff #258555)

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 ↗

(On Diff #258555)

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

clang/lib/Parse/ParseDecl.cpp

5672 ↗

(On Diff #258555)

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

59

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/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/test/CXX/temp/temp.decls/temp.class.spec/p10.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// expected-no-diagnostics
+
+// 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 {
+private:
+  void func();
+  void funcOverloaded();
+  void funcOverloaded(int);
+
+  static void staticFunc();
+  static void staticFuncOverloaded();
+  static void staticFuncOverloaded(int);
+
+  class Nested {};
+
+  int field;
+};
+
+template <void (TestClass::*)()> class TemplateClass {};
+template <> class TemplateClass<&TestClass::func> {};
+template <> class TemplateClass<&TestClass::funcOverloaded> {};
+
+template <void (*)()> class TemplateClass2 { };
+template <> class TemplateClass2<&TestClass::staticFunc> {};
+template <> class 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> {};
+
+template<typename T, void (*)()> class TemplateClass4 {};
+template<typename T> class TemplateClass4<T, &TestClass::staticFunc> {};
+template<typename T> class TemplateClass4<T, &TestClass::staticFuncOverloaded> {};
+
+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> {};
+
+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> {};
+
+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>;
+
+// As an extension, this rule is applied to explicit specializations as well.
+template <> class X::Y<A::C> {};

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>