[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 , @broadwaylamb 
wrote:

> In D78404#1990192 , @rsmith wrote:
>
> > ... please also test ...
> >
> >   template class TemplateClass3 
> > 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 TestClass::X varTemplate3_1b{};
  void use() { varTemplate3_1b = {}; }

... 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::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 struct A;
class X { struct Y {}; };
template<> struct A { void f(); };
void A::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 struct A;
class X { struct Y {}; };
template<> struct A { int f(); };
int A::*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 
+using alias3_1 = TemplateClass3;
+

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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sergej Jaskiewicz via Phabricator via cfe-commits]

broadwaylamb marked an inline comment as done.
broadwaylamb added inline comments.



Comment at: clang/lib/Parse/ParseDecl.cpp:5672
+  //   initializer.
+  SuppressAccessChecks diagsFromTag(*this);
+

This is for things like

```
template<> void X::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`. 


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

https://reviews.llvm.org/D78404



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sergej Jaskiewicz via Phabricator via cfe-commits]

broadwaylamb marked 3 inline comments as done.
broadwaylamb added inline comments.



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.

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



Comment at: clang/lib/Parse/ParseTemplate.cpp:211
+Decl *Decl = usingDeclPtr.get().getSingleDecl();
+ParsingDeclRAII.complete(Decl);
+return Decl;

…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](https://github.com/llvm/llvm-project/blob/8e0c9e21bf5f3e7a427b07e3eaf3bc80d2c74cb6/clang/lib/Sema/SemaAccess.cpp#L1479)
 — we need to be able to cast the `TypeAliasDecl` to `DeclContext` in order for 
delayed access check to be actually performed.



Comment at: clang/test/CXX/temp/temp.decls/temp.class.spec/p10.cpp:58
+template 
+using alias3_1 = TemplateClass3;
+

I'm talking about declarations like this.

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


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

https://reviews.llvm.org/D78404



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sergej Jaskiewicz via Phabricator via cfe-commits]

broadwaylamb updated this revision to Diff 258555.
broadwaylamb added a comment.

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


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

https://reviews.llvm.org/D78404

Files:
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/DeclBase.h
  clang/include/clang/Basic/DeclNodes.td
  clang/lib/AST/DeclBase.cpp
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/lib/Parse/ParseTemplate.cpp
  clang/test/CXX/drs/dr1xx.cpp
  clang/test/CXX/temp/temp.decls/temp.class.spec/p10.cpp
  clang/test/CXX/temp/temp.spec/p6.cpp
  clang/test/CXX/temp/temp.spec/temp.explicit/p11.cpp
  clang/www/cxx_status.html

Index: clang/www/cxx_status.html
===
--- clang/www/cxx_status.html
+++ clang/www/cxx_status.html
@@ -966,7 +966,7 @@
 
   Access checking on specializations
   https://wg21.link/p0692r1;>P0692R1
-  Partial
+  Clang 11
 
 
   Default constructible and assignable stateless lambdas
Index: clang/test/CXX/temp/temp.spec/temp.explicit/p11.cpp
===
--- clang/test/CXX/temp/temp.spec/temp.explicit/p11.cpp
+++ /dev/null
@@ -1,19 +0,0 @@
-// RUN: %clang_cc1 -fsyntax-only -verify %s
-// expected-no-diagnostics
-
-class X {
-  template  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;
-
-// As an extension, this rule is applied to explicit specializations as well.
-template <> class X::Y {};
Index: clang/test/CXX/temp/temp.spec/p6.cpp
===
--- /dev/null
+++ clang/test/CXX/temp/temp.spec/p6.cpp
@@ -0,0 +1,71 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// expected-no-diagnostics
+
+class X {
+  template  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;
+
+template  class D {};
+template class D<::func>;
+template class D<::funcOverloaded>;
+
+template  class E { };
+template class E<::staticFunc>;
+template class E<::staticFuncOverloaded>;
+
+template  class G {};
+template class G<::field>;
+
+template <> class X::Y {};
+
+namespace member_spec {
+
+  template 
+  struct X {
+struct A {};
+void f();
+enum E : int;
+static int var;
+  };
+
+  class Y {
+using Z = int;
+  };
+
+  template <>
+  struct X::A {};
+
+  template <>
+  void X::f() {}
+
+  template <>
+  enum X::E : int {};
+
+  template <>
+  int X::var = 76;
+
+}
Index: clang/test/CXX/temp/temp.decls/temp.class.spec/p10.cpp
===
--- /dev/null
+++ clang/test/CXX/temp/temp.decls/temp.class.spec/p10.cpp
@@ -0,0 +1,119 @@
+// 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  class TemplateClass {};
+template <> class TemplateClass<::func> {};
+template <> class TemplateClass<::funcOverloaded> {};
+
+// expected-error@+1 {{'func' is a private member of 'TestClass'}}
+using alias1_1 = TemplateClass<::func>;
+
+// expected-error@+1 {{'funcOverloaded' is a private member of 'TestClass'}}
+using alias1_2 = TemplateClass<::funcOverloaded>;
+
+template  class TemplateClass2 { };
+template <> class TemplateClass2<::staticFunc> {};
+template <> class 

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

[Sergej Jaskiewicz via Phabricator via cfe-commits]

broadwaylamb added a comment.

In D78404#1990192 , @rsmith wrote:

> ... please also test ...
>
>   template class TemplateClass3 
> 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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78404



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

rsmith added a comment.

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 class TemplateClass3 {};
  template class TemplateClass3 {};

... please also test ...

  template class TemplateClass3 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 struct X {
struct A {};
void f();
enum E : int;
static int var;
  };
  class Y { using Z = int; };
  template<> struct X::A {};
  template<> void X::f() {}
  template<> enum X::E : int {};
  template<> int X::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.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78404



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sergej Jaskiewicz via Phabricator via cfe-commits]

broadwaylamb created this revision.
broadwaylamb added reviewers: asl, rsmith, doug.gregor, rjmccall, triton.
broadwaylamb added a project: clang.
Herald added subscribers: cfe-commits, dexonsmith.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D78404

Files:
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/test/CXX/temp/temp.decls/temp.class.spec/p10.cpp
  clang/test/CXX/temp/temp.spec/p6.cpp
  clang/test/CXX/temp/temp.spec/temp.explicit/p11.cpp
  clang/www/cxx_status.html

Index: clang/www/cxx_status.html
===
--- clang/www/cxx_status.html
+++ clang/www/cxx_status.html
@@ -966,7 +966,7 @@
 
   Access checking on specializations
   https://wg21.link/p0692r1;>P0692R1
-  Partial
+  Clang 11
 
 
   Default constructible and assignable stateless lambdas
Index: clang/test/CXX/temp/temp.spec/temp.explicit/p11.cpp
===
--- clang/test/CXX/temp/temp.spec/temp.explicit/p11.cpp
+++ /dev/null
@@ -1,19 +0,0 @@
-// RUN: %clang_cc1 -fsyntax-only -verify %s
-// expected-no-diagnostics
-
-class X {
-  template  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;
-
-// As an extension, this rule is applied to explicit specializations as well.
-template <> class X::Y {};
Index: clang/test/CXX/temp/temp.spec/p6.cpp
===
--- /dev/null
+++ clang/test/CXX/temp/temp.spec/p6.cpp
@@ -0,0 +1,44 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// expected-no-diagnostics
+
+class X {
+  template  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;
+
+template  class D {};
+template class D<::func>;
+template class D<::funcOverloaded>;
+
+template  class E { };
+template class E<::staticFunc>;
+template class E<::staticFuncOverloaded>;
+
+template  class G {};
+template class G<::field>;
+
+// As an extension, this rule is applied to explicit specializations as well.
+template <> class X::Y {};
Index: clang/test/CXX/temp/temp.decls/temp.class.spec/p10.cpp
===
--- /dev/null
+++ clang/test/CXX/temp/temp.decls/temp.class.spec/p10.cpp
@@ -0,0 +1,61 @@
+// 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  class TemplateClass {};
+template <> class TemplateClass<::func> {};
+template <> class TemplateClass<::funcOverloaded> {};
+
+template  class TemplateClass2 { };
+template <> class TemplateClass2<::staticFunc> {};
+template <> class TemplateClass2<::staticFuncOverloaded> {};
+
+template class TemplateClass3 {};
+template class TemplateClass3 {};
+template class TemplateClass3 {};
+
+template class TemplateClass4 {};
+template class TemplateClass4 {};
+template class TemplateClass4 {};
+
+template class TemplateClass5 {};
+template<> class TemplateClass5 {};
+
+template class TemplateClass6 {};
+template class TemplateClass6 {};
+
+template  class TemplateClass7 {};
+template <> class TemplateClass7<::field> {};
+
+template  class TemplateClass8 {};
+template  class TemplateClass8 {};
+
+template
+struct trait;
+
+class class_ {
+  template
+  struct impl;
+};
+
+template
+struct trait>;
Index: clang/lib/Parse/ParseDeclCXX.cpp