Page MenuHomePhabricator

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

Authored by broadwaylamb on Apr 17 2020, 2:26 PM.

Details

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.

Diff Detail

Event Timeline

broadwaylamb created this revision.Apr 17 2020, 2:26 PM

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.)

... 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?

  • 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 marked 3 inline comments as done.Apr 18 2020, 2:06 PM
broadwaylamb added inline comments.
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 marked an inline comment as done.Apr 18 2020, 2:11 PM
broadwaylamb added inline comments.
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.

... 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.