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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D78404#1990461 <https://reviews.llvm.org/D78404#1990461>, @broadwaylamb wrote:

> In D78404#1990192 <https://reviews.llvm.org/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 <https://godbolt.org/z/svejsJ>, 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.



================
Comment at: clang/include/clang/AST/Decl.h:3198
 /// alias-declaration.
-class TypeAliasDecl : public TypedefNameDecl {
+class TypeAliasDecl : public TypedefNameDecl, public DeclContext {
   /// The template for which this is the pattern, if any.
----------------
broadwaylamb wrote:
> I'm not sure about inheriting `TypeAliasDecl` from `DeclContext`, but (see below)
Yeah, this doesn't seem appropriate. Hopefully we can find another way.


================
Comment at: clang/lib/Parse/ParseDecl.cpp:5672
+      //   initializer.
+      SuppressAccessChecks diagsFromTag(*this);
+
----------------
broadwaylamb wrote:
> 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`. 
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.




================
Comment at: clang/test/CXX/temp/temp.decls/temp.class.spec/p10.cpp:58
+template <typename T>
+using alias3_1 = TemplateClass3<T, &TestClass::func>;
+
----------------
broadwaylamb wrote:
> I'm talking about declarations like this.
> 
> Previously, we didn't reject it (which I believe was incorrect), and now we do.
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78404/new/

https://reviews.llvm.org/D78404