Diagnose invalid decl-specifiers in non-type template parameter declarations (original author miyuki!)

[faisalv]

Summary

Diagnose invalid decl-specifiers in non-type template parameter declarations

According to the C++ Standard [temp.param]p2:

A storage class shall not be specified in a template-parameter
declaration.

Additionally other decl-specifiers are also restricted.

This patch implements a diagnostic for these restrictions within Sema (inspired by how the restrictions are enforced within ActOnParamDeclarator).

[faisalv]

lib/Parse/ParseTemplate.cpp

692 ↗

(On Diff #125087)

I tend to prefer explicit captures (unless you have a good reason?) - favoring a clearer description of intention - and acknowledgement of required context - over programming convenience ...

[miyuki]

Use explicit lambda capture list.

[miyuki]

Ping

[rogfer01]

lib/Parse/ParseTemplate.cpp

702 ↗

(On Diff #125119)

You added tests for SCS_* but not for TSCS_*. Is it worth adding at least one for the latter as well?

template <thread_local int X> struct Z2; // expected-error {{storage class specified for template parameter 'X'}}

[miyuki]

Added a test for thead_local.

[faisalv]

Otherwise, I think this looks good enough to commit.

Do you have commit access? If not, let me know when you're ready for me to commit it on your behalf ...

Thank you for fixing this!

include/clang/Basic/DiagnosticParseKinds.td

671 ↗

(On Diff #126325)

What about folding both of these diagnostics into one with something along these lines:
def err_storage_class_template_parameter : Error<

"storage class specified for template parameter %select{|%1}0">;

And for no identifier, do << 0, and for the valid identifier case: do << 1 << ParamDecl.getIdentifier()

[miyuki]

Merged two diagnostics into one

[miyuki]

I don't have commit access. Please commit the patch on my behalf.

[faisalv]

Hmm - I think i might make some tweaks to this patch (to be largely symmetric with the similar handling of invalid decl-specifiers on function parameters in Sema::Actions.ActOnParamDeclarator)...

Would you want to try to make those changes and update this diff - or would you rather I make them and commit a patch acknowledging your contribution?

Sorry about not suggesting the changes earlier - but it occurred to me while I was re-reviewing the patch prior to committing it, that we don't handle inline and virtual - which led me to ActOnParamDeclarator ...

[miyuki]

I would appreciate, if you make those changes (because I'm very new to Clang and I'm not sure that I understand how to move these checks to Sema correctly).

[faisalv]

Sounds good - if I don't get this done over the next seven days - would you mind just pinging me!

Thanks!

[faisalv]

Miyuki - please take a look at the patch and let me know if you agree with the changes - or have any concerns...

Thank you!

P.S. While I can see the argument for having these syntactical checks be done by the Parser - since they could be described as context sensitive syntax analysis, moving the changes to Sema also seems quite appropriate.

[miyuki]

LGTM, thanks a lot for fixing the patch.

[faisalv]

Added via https://reviews.llvm.org/rC321339

include/clang/Basic/DiagnosticSemaKinds.td

   "%select{|template parameter }0redeclaration">;
 def note_template_param_different_kind : Note<
   "template parameter has a different kind in template argument">;
+
+def err_invalid_decl_specifier_in_nontype_parm : Error<
+  "invalid declaration specifier in template non-type parameter">;
   
 def err_template_nontype_parm_different_type : Error<
   "template non-type parameter has a different type %0 in template "

lib/Sema/SemaTemplate.cpp

                                           Expr *Default) {
   TypeSourceInfo *TInfo = GetTypeForDeclarator(D, S);
 
+  // Check that we have valid decl-specifiers specified.
+  auto CheckValidDeclSpecifiers = [this, &D] {
+    // C++ [temp.param]
+    // p1 
+    //   template-parameter:
+    //     ...
+    //     parameter-declaration
+    // p2 
+    //   ... A storage class shall not be specified in a template-parameter
+    //   declaration.
+    // [dcl.typedef]p1: 
+    //   The typedef specifier [...] shall not be used in the decl-specifier-seq
+    //   of a parameter-declaration
+    const DeclSpec &DS = D.getDeclSpec();
+    auto EmitDiag = [this](SourceLocation Loc) {
+      Diag(Loc, diag::err_invalid_decl_specifier_in_nontype_parm)
+          << FixItHint::CreateRemoval(Loc);
+    };
+    if (DS.getStorageClassSpec() != DeclSpec::SCS_unspecified)
+      EmitDiag(DS.getStorageClassSpecLoc());
+    
+    if (DeclSpec::TSCS TSCS = DS.getThreadStorageClassSpec())
+      EmitDiag(DS.getThreadStorageClassSpecLoc());
+    
+    // [dcl.inline]p1: 
+    //   The inline specifier can be applied only to the declaration or 
+    //   definition of a variable or function.
+    
+    if (DS.isInlineSpecified())
+      EmitDiag(DS.getInlineSpecLoc());
+    
+    // [dcl.constexpr]p1:
+    //   The constexpr specifier shall be applied only to the definition of a 
+    //   variable or variable template or the declaration of a function or 
+    //   function template.
+    
+    if (DS.isConstexprSpecified())
+      EmitDiag(DS.getConstexprSpecLoc());
+
+    // [dcl.fct.spec]p1:
+    //   Function-specifiers can be used only in function declarations.
+
+    if (DS.isVirtualSpecified())
+      EmitDiag(DS.getVirtualSpecLoc());
+
+    if (DS.isExplicitSpecified())
+      EmitDiag(DS.getExplicitSpecLoc());
+
+    if (DS.isNoreturnSpecified())
+      EmitDiag(DS.getNoreturnSpecLoc());
+  };
+
+  CheckValidDeclSpecifiers();
+  
   if (TInfo->getType()->isUndeducedType()) {
     Diag(D.getIdentifierLoc(),
          diag::warn_cxx14_compat_template_nontype_parm_auto_type)

test/CXX/temp/temp.param/p2.cpp

-// RUN: %clang_cc1 -fsyntax-only -verify %s
-// expected-no-diagnostics
+// RUN: %clang_cc1 -fsyntax-only -verify %s 
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -verify %s -DCPP11
+// RUN: %clang_cc1 -std=c++17 -fsyntax-only -verify %s -DCPP17
 
 // There is no semantic difference between class and typename in a
 // template-parameter. typename followed by an unqualified-id names a

 template<typename T, typename X<T>::type Value> struct Y1;
 
 // A storage class shall not be specified in a template-parameter declaration.
-template<static int Value> struct Z; // FIXME: expect an error
+template<static int Value> struct Z; //expected-error{{invalid declaration specifier}}
+template<typedef int Value> struct Z0; //expected-error{{invalid declaration specifier}}
+template<extern inline int Value> struct Z0; //expected-error2{{invalid declaration specifier}}
+template<virtual int Value> struct Z0; //expected-error{{invalid declaration specifier}}
+template<explicit int Value> struct Z0; //expected-error{{invalid declaration specifier}}
+template<inline int Value> struct Z0; //expected-error{{invalid declaration specifier}}
+template<extern int> struct Z0; //expected-error{{invalid declaration specifier}}
+template<static int> struct Z0;  //expected-error{{invalid declaration specifier}}
+template<explicit int Value> struct Z0; //expected-error{{invalid declaration specifier}}
+template<mutable int> struct Z0; //expected-error{{invalid declaration specifier}}
 
+template<const int> struct Z0; // OK 
+template<volatile int> struct Z0; // OK
+
+
+
+#ifdef CPP11
+template<thread_local int> struct Z0; //expected-error{{invalid declaration specifier}}
+template<constexpr int> struct Z0; //expected-error{{invalid declaration specifier}}
+
+#endif
+
+#ifdef CPP17
+template<auto> struct Z1; // OK
+#endif
+
 // Make sure that we properly disambiguate non-type template parameters that
 // start with 'class'.
 class X1 { };