Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Clang][Sema] Diagnose variable template explicit specializations with storage-class-specifiers #93873

Merged
merged 5 commits into from
Jun 18, 2024

Conversation

sdkrystian
Copy link
Member

According to [temp.expl.spec] p2:

The declaration in an explicit-specialization shall not be an export-declaration. An explicit specialization shall not use a storage-class-specifier other than thread_local.

Clang partially implements this, but a number of issues exist:

  1. We don't diagnose class scope explicit specializations of variable templates with storage-class-specifiers, e.g.
    struct A
    {
        template<typename T>
        static constexpr int x = 0;
    
        template<>
        static constexpr int x<void> = 1; // ill-formed, but clang accepts
    };
  2. We incorrectly reject class scope explicit specializations of variable templates when static is not used, e.g.
    struct A
    {
        template<typename T>
        static constexpr int x = 0;
    
        template<>
        constexpr int x<void> = 1; // error: non-static data member cannot be constexpr; did you intend to make it static?
    };
  3. We don't diagnose dependent class scope explicit specializations of function templates with storage class specifiers, e.g.
    template<typename T>
    struct A
    {
        template<typename U>
        static void f();
    
        template<>
        static void f<int>(); // ill-formed, but clang accepts
    };

This patch addresses these issues as follows:

  • #1 is fixed by issuing a diagnostic when an explicit specialization of a variable template has storage class specifier
  • #2 is fixed by considering any non-function declaration with any template parameter lists at class scope to be a static data member. This also allows for better error recovery (it's more likely the user intended to declare a variable template than a "field template").
  • #3 is fixed by checking whether a function template explicit specialization has a storage class specifier even when the primary template is not yet known.

One thing to note is that it would be far simpler to diagnose this when parsing the decl-specifier-seq, but such an implementation would necessitate a refactor of ParsedTemplateInfo which I believe to be outside the scope of this patch.

@sdkrystian sdkrystian requested a review from Endilll as a code owner May 30, 2024 20:24
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:modules C++20 modules and Clang Header Modules labels May 30, 2024
@llvmbot
Copy link
Member

llvmbot commented May 30, 2024

@llvm/pr-subscribers-clang-modules

Author: Krystian Stasiowski (sdkrystian)

Changes

According to [[temp.expl.spec] p2](http://eel.is/c++draft/temp.expl.spec#2):
> The declaration in an explicit-specialization shall not be an export-declaration. An explicit specialization shall not use a storage-class-specifier other than thread_local.

Clang partially implements this, but a number of issues exist:

  1. We don't diagnose class scope explicit specializations of variable templates with storage-class-specifiers, e.g.
    struct A
    {
        template&lt;typename T&gt;
        static constexpr int x = 0;
    
        template&lt;&gt;
        static constexpr int x&lt;void&gt; = 1; // ill-formed, but clang accepts
    };
  2. We incorrectly reject class scope explicit specializations of variable templates when static is not used, e.g.
    struct A
    {
        template&lt;typename T&gt;
        static constexpr int x = 0;
    
        template&lt;&gt;
        constexpr int x&lt;void&gt; = 1; // error: non-static data member cannot be constexpr; did you intend to make it static?
    };
  3. We don't diagnose dependent class scope explicit specializations of function templates with storage class specifiers, e.g.
    template&lt;typename T&gt;
    struct A
    {
        template&lt;typename U&gt;
        static void f();
    
        template&lt;&gt;
        static void f&lt;int&gt;(); // ill-formed, but clang accepts
    };

This patch addresses these issues as follows:

  • #1 is fixed by issuing a diagnostic when an explicit specialization of a variable template has storage class specifier
  • #2 is fixed by considering any non-function declaration with any template parameter lists at class scope to be a static data member. This also allows for better error recovery (it's more likely the user intended to declare a variable template than a "field template").
  • #3 is fixed by checking whether a function template explicit specialization has a storage class specifier even when the primary template is not yet known.

One thing to note is that it would be far simpler to diagnose this when parsing the decl-specifier-seq, but such an implementation would necessitate a refactor of ParsedTemplateInfo which I believe to be outside the scope of this patch.


Patch is 49.31 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/93873.diff

18 Files Affected:

  • (modified) clang/lib/Parse/ParseDeclCXX.cpp (+3-2)
  • (modified) clang/lib/Sema/DeclSpec.cpp (+1)
  • (modified) clang/lib/Sema/SemaDecl.cpp (+134-114)
  • (modified) clang/lib/Sema/SemaDeclCXX.cpp (+5-3)
  • (modified) clang/test/CXX/dcl.dcl/dcl.spec/dcl.stc/p1.cpp (+3-2)
  • (modified) clang/test/CXX/drs/cwg7xx.cpp (+12-12)
  • (modified) clang/test/CXX/temp/temp.decls/temp.mem/p2.cpp (+1-1)
  • (modified) clang/test/CXX/temp/temp.decls/temp.variadic/p5.cpp (+1-1)
  • (modified) clang/test/CXX/temp/temp.spec/temp.expl.spec/p17.cpp (+8-7)
  • (modified) clang/test/CXX/temp/temp.spec/temp.expl.spec/p2-0x.cpp (+31-28)
  • (added) clang/test/CXX/temp/temp.spec/temp.expl.spec/p2-20.cpp (+274)
  • (modified) clang/test/Modules/Inputs/redecl-templates/a.h (+1-1)
  • (modified) clang/test/Modules/redecl-templates.cpp (+4-2)
  • (modified) clang/test/PCH/cxx-templates.h (+6-6)
  • (modified) clang/test/PCH/cxx1y-variable-templates.cpp (+16-8)
  • (modified) clang/test/SemaCXX/cxx1y-variable-templates_in_class.cpp (+18-12)
  • (modified) clang/test/SemaTemplate/explicit-specialization-member.cpp (+4-4)
  • (modified) clang/test/SemaTemplate/nested-template.cpp (+7-7)
diff --git a/clang/lib/Parse/ParseDeclCXX.cpp b/clang/lib/Parse/ParseDeclCXX.cpp
index 9a4a777f575b2..d02548f6441f9 100644
--- a/clang/lib/Parse/ParseDeclCXX.cpp
+++ b/clang/lib/Parse/ParseDeclCXX.cpp
@@ -3162,7 +3162,8 @@ Parser::DeclGroupPtrTy Parser::ParseCXXClassMemberDeclaration(
                      DeclSpec::SCS_static &&
                  DeclaratorInfo.getDeclSpec().getStorageClassSpec() !=
                      DeclSpec::SCS_typedef &&
-                 !DS.isFriendSpecified()) {
+                 !DS.isFriendSpecified() &&
+                 TemplateInfo.Kind == ParsedTemplateInfo::NonTemplate) {
         // It's a default member initializer.
         if (BitfieldSize.get())
           Diag(Tok, getLangOpts().CPlusPlus20
@@ -3261,7 +3262,7 @@ Parser::DeclGroupPtrTy Parser::ParseCXXClassMemberDeclaration(
       } else if (ThisDecl)
         Actions.AddInitializerToDecl(ThisDecl, Init.get(),
                                      EqualLoc.isInvalid());
-    } else if (ThisDecl && DS.getStorageClassSpec() == DeclSpec::SCS_static)
+    } else if (ThisDecl && DeclaratorInfo.isStaticMember())
       // No initializer.
       Actions.ActOnUninitializedDecl(ThisDecl);
 
diff --git a/clang/lib/Sema/DeclSpec.cpp b/clang/lib/Sema/DeclSpec.cpp
index 60e8189025700..96c90a60b9682 100644
--- a/clang/lib/Sema/DeclSpec.cpp
+++ b/clang/lib/Sema/DeclSpec.cpp
@@ -416,6 +416,7 @@ bool Declarator::isDeclarationOfFunction() const {
 bool Declarator::isStaticMember() {
   assert(getContext() == DeclaratorContext::Member);
   return getDeclSpec().getStorageClassSpec() == DeclSpec::SCS_static ||
+         (!isDeclarationOfFunction() && !getTemplateParameterLists().empty()) ||
          (getName().getKind() == UnqualifiedIdKind::IK_OperatorFunctionId &&
           CXXMethodDecl::isStaticOverloadedOperator(
               getName().OperatorFunctionId.Operator));
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 2a87b26f17a2b..cc36387c0e332 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -7602,80 +7602,8 @@ NamedDecl *Sema::ActOnVariableDeclarator(
                             NTCUC_AutoVar, NTCUK_Destruct);
   } else {
     bool Invalid = false;
-
-    if (DC->isRecord() && !CurContext->isRecord()) {
-      // This is an out-of-line definition of a static data member.
-      switch (SC) {
-      case SC_None:
-        break;
-      case SC_Static:
-        Diag(D.getDeclSpec().getStorageClassSpecLoc(),
-             diag::err_static_out_of_line)
-          << FixItHint::CreateRemoval(D.getDeclSpec().getStorageClassSpecLoc());
-        break;
-      case SC_Auto:
-      case SC_Register:
-      case SC_Extern:
-        // [dcl.stc] p2: The auto or register specifiers shall be applied only
-        // to names of variables declared in a block or to function parameters.
-        // [dcl.stc] p6: The extern specifier cannot be used in the declaration
-        // of class members
-
-        Diag(D.getDeclSpec().getStorageClassSpecLoc(),
-             diag::err_storage_class_for_static_member)
-          << FixItHint::CreateRemoval(D.getDeclSpec().getStorageClassSpecLoc());
-        break;
-      case SC_PrivateExtern:
-        llvm_unreachable("C storage class in c++!");
-      }
-    }
-
-    if (SC == SC_Static && CurContext->isRecord()) {
-      if (const CXXRecordDecl *RD = dyn_cast<CXXRecordDecl>(DC)) {
-        // Walk up the enclosing DeclContexts to check for any that are
-        // incompatible with static data members.
-        const DeclContext *FunctionOrMethod = nullptr;
-        const CXXRecordDecl *AnonStruct = nullptr;
-        for (DeclContext *Ctxt = DC; Ctxt; Ctxt = Ctxt->getParent()) {
-          if (Ctxt->isFunctionOrMethod()) {
-            FunctionOrMethod = Ctxt;
-            break;
-          }
-          const CXXRecordDecl *ParentDecl = dyn_cast<CXXRecordDecl>(Ctxt);
-          if (ParentDecl && !ParentDecl->getDeclName()) {
-            AnonStruct = ParentDecl;
-            break;
-          }
-        }
-        if (FunctionOrMethod) {
-          // C++ [class.static.data]p5: A local class shall not have static data
-          // members.
-          Diag(D.getIdentifierLoc(),
-               diag::err_static_data_member_not_allowed_in_local_class)
-              << Name << RD->getDeclName()
-              << llvm::to_underlying(RD->getTagKind());
-        } else if (AnonStruct) {
-          // C++ [class.static.data]p4: Unnamed classes and classes contained
-          // directly or indirectly within unnamed classes shall not contain
-          // static data members.
-          Diag(D.getIdentifierLoc(),
-               diag::err_static_data_member_not_allowed_in_anon_struct)
-              << Name << llvm::to_underlying(AnonStruct->getTagKind());
-          Invalid = true;
-        } else if (RD->isUnion()) {
-          // C++98 [class.union]p1: If a union contains a static data member,
-          // the program is ill-formed. C++11 drops this restriction.
-          Diag(D.getIdentifierLoc(),
-               getLangOpts().CPlusPlus11
-                 ? diag::warn_cxx98_compat_static_data_member_in_union
-                 : diag::ext_static_data_member_in_union) << Name;
-        }
-      }
-    }
-
     // Match up the template parameter lists with the scope specifier, then
     // determine whether we have a template or a template specialization.
-    bool InvalidScope = false;
     TemplateParams = MatchTemplateParametersToScopeSpecifier(
         D.getDeclSpec().getBeginLoc(), D.getIdentifierLoc(),
         D.getCXXScopeSpec(),
@@ -7683,8 +7611,7 @@ NamedDecl *Sema::ActOnVariableDeclarator(
             ? D.getName().TemplateId
             : nullptr,
         TemplateParamLists,
-        /*never a friend*/ false, IsMemberSpecialization, InvalidScope);
-    Invalid |= InvalidScope;
+        /*never a friend*/ false, IsMemberSpecialization, Invalid);
 
     if (TemplateParams) {
       if (!TemplateParams->size() &&
@@ -7727,6 +7654,102 @@ NamedDecl *Sema::ActOnVariableDeclarator(
              "should have a 'template<>' for this decl");
     }
 
+    bool IsExplicitSpecialization =
+        IsVariableTemplateSpecialization && !IsPartialSpecialization;
+
+    // C++ [temp.expl.spec]p2:
+    //   The declaration in an explicit-specialization shall not be an
+    //   export-declaration. An explicit specialization shall not use a
+    //   storage-class-specifier other than thread_local.
+    //
+    // We use the storage-class-specifier from DeclSpec because we may have
+    // added implicit 'extern' for declarations with __declspec(dllimport)!
+    if (SCSpec != DeclSpec::SCS_unspecified &&
+        (IsExplicitSpecialization || IsMemberSpecialization)) {
+      Diag(D.getDeclSpec().getStorageClassSpecLoc(),
+           diag::ext_explicit_specialization_storage_class)
+          << FixItHint::CreateRemoval(D.getDeclSpec().getStorageClassSpecLoc());
+    }
+
+    if (CurContext->isRecord()) {
+      if (SC == SC_Static) {
+        if (const CXXRecordDecl *RD = dyn_cast<CXXRecordDecl>(DC)) {
+          // Walk up the enclosing DeclContexts to check for any that are
+          // incompatible with static data members.
+          const DeclContext *FunctionOrMethod = nullptr;
+          const CXXRecordDecl *AnonStruct = nullptr;
+          for (DeclContext *Ctxt = DC; Ctxt; Ctxt = Ctxt->getParent()) {
+            if (Ctxt->isFunctionOrMethod()) {
+              FunctionOrMethod = Ctxt;
+              break;
+            }
+            const CXXRecordDecl *ParentDecl = dyn_cast<CXXRecordDecl>(Ctxt);
+            if (ParentDecl && !ParentDecl->getDeclName()) {
+              AnonStruct = ParentDecl;
+              break;
+            }
+          }
+          if (FunctionOrMethod) {
+            // C++ [class.static.data]p5: A local class shall not have static
+            // data members.
+            Diag(D.getIdentifierLoc(),
+                 diag::err_static_data_member_not_allowed_in_local_class)
+                << Name << RD->getDeclName()
+                << llvm::to_underlying(RD->getTagKind());
+          } else if (AnonStruct) {
+            // C++ [class.static.data]p4: Unnamed classes and classes contained
+            // directly or indirectly within unnamed classes shall not contain
+            // static data members.
+            Diag(D.getIdentifierLoc(),
+                 diag::err_static_data_member_not_allowed_in_anon_struct)
+                << Name << llvm::to_underlying(AnonStruct->getTagKind());
+            Invalid = true;
+          } else if (RD->isUnion()) {
+            // C++98 [class.union]p1: If a union contains a static data member,
+            // the program is ill-formed. C++11 drops this restriction.
+            Diag(D.getIdentifierLoc(),
+                 getLangOpts().CPlusPlus11
+                     ? diag::warn_cxx98_compat_static_data_member_in_union
+                     : diag::ext_static_data_member_in_union)
+                << Name;
+          }
+        }
+      } else if (IsVariableTemplate || IsPartialSpecialization) {
+        // There is no such thing as a member field template.
+        Diag(D.getIdentifierLoc(), diag::err_template_member)
+            << II << TemplateParams->getSourceRange();
+        // Recover by pretending this is a static data member template.
+        SC = SC_Static;
+      }
+    } else if (DC->isRecord()) {
+      // This is an out-of-line definition of a static data member.
+      switch (SC) {
+      case SC_None:
+        break;
+      case SC_Static:
+        Diag(D.getDeclSpec().getStorageClassSpecLoc(),
+             diag::err_static_out_of_line)
+            << FixItHint::CreateRemoval(
+                   D.getDeclSpec().getStorageClassSpecLoc());
+        break;
+      case SC_Auto:
+      case SC_Register:
+      case SC_Extern:
+        // [dcl.stc] p2: The auto or register specifiers shall be applied only
+        // to names of variables declared in a block or to function parameters.
+        // [dcl.stc] p6: The extern specifier cannot be used in the declaration
+        // of class members
+
+        Diag(D.getDeclSpec().getStorageClassSpecLoc(),
+             diag::err_storage_class_for_static_member)
+            << FixItHint::CreateRemoval(
+                   D.getDeclSpec().getStorageClassSpecLoc());
+        break;
+      case SC_PrivateExtern:
+        llvm_unreachable("C storage class in c++!");
+      }
+    }
+
     if (IsVariableTemplateSpecialization) {
       SourceLocation TemplateKWLoc =
           TemplateParamLists.size() > 0
@@ -7772,8 +7795,6 @@ NamedDecl *Sema::ActOnVariableDeclarator(
     // the variable (matching the scope specifier), store them.
     // An explicit variable template specialization does not own any template
     // parameter lists.
-    bool IsExplicitSpecialization =
-        IsVariableTemplateSpecialization && !IsPartialSpecialization;
     unsigned VDTemplateParamLists =
         (TemplateParams && !IsExplicitSpecialization) ? 1 : 0;
     if (TemplateParamLists.size() > VDTemplateParamLists)
@@ -10203,25 +10224,45 @@ Sema::ActOnFunctionDeclarator(Scope *S, Declarator &D, DeclContext *DC,
       NewFD->setImplicitlyInline(ImplicitInlineCXX20);
     }
 
-    if (SC == SC_Static && isa<CXXMethodDecl>(NewFD) &&
-        !CurContext->isRecord()) {
-      // C++ [class.static]p1:
-      //   A data or function member of a class may be declared static
-      //   in a class definition, in which case it is a static member of
-      //   the class.
+    if (!isFriend && SC != SC_None) {
+      // C++ [temp.expl.spec]p2:
+      //   The declaration in an explicit-specialization shall not be an
+      //   export-declaration. An explicit specialization shall not use a
+      //   storage-class-specifier other than thread_local.
+      //
+      // We diagnose friend declarations with storage-class-specifiers
+      // elsewhere.
+      if (isFunctionTemplateSpecialization || isMemberSpecialization) {
+        Diag(D.getDeclSpec().getStorageClassSpecLoc(),
+             diag::ext_explicit_specialization_storage_class)
+            << FixItHint::CreateRemoval(
+                   D.getDeclSpec().getStorageClassSpecLoc());
+      }
 
-      // Complain about the 'static' specifier if it's on an out-of-line
-      // member function definition.
+      if (SC == SC_Static && !CurContext->isRecord() && DC->isRecord()) {
+        assert(isa<CXXMethodDecl>(NewFD) &&
+               "Out-of-line member function should be a CXXMethodDecl");
+        // C++ [class.static]p1:
+        //   A data or function member of a class may be declared static
+        //   in a class definition, in which case it is a static member of
+        //   the class.
 
-      // MSVC permits the use of a 'static' storage specifier on an out-of-line
-      // member function template declaration and class member template
-      // declaration (MSVC versions before 2015), warn about this.
-      Diag(D.getDeclSpec().getStorageClassSpecLoc(),
-           ((!getLangOpts().isCompatibleWithMSVC(LangOptions::MSVC2015) &&
-             cast<CXXRecordDecl>(DC)->getDescribedClassTemplate()) ||
-           (getLangOpts().MSVCCompat && NewFD->getDescribedFunctionTemplate()))
-           ? diag::ext_static_out_of_line : diag::err_static_out_of_line)
-        << FixItHint::CreateRemoval(D.getDeclSpec().getStorageClassSpecLoc());
+        // Complain about the 'static' specifier if it's on an out-of-line
+        // member function definition.
+
+        // MSVC permits the use of a 'static' storage specifier on an
+        // out-of-line member function template declaration and class member
+        // template declaration (MSVC versions before 2015), warn about this.
+        Diag(D.getDeclSpec().getStorageClassSpecLoc(),
+             ((!getLangOpts().isCompatibleWithMSVC(LangOptions::MSVC2015) &&
+               cast<CXXRecordDecl>(DC)->getDescribedClassTemplate()) ||
+              (getLangOpts().MSVCCompat &&
+               NewFD->getDescribedFunctionTemplate()))
+                 ? diag::ext_static_out_of_line
+                 : diag::err_static_out_of_line)
+            << FixItHint::CreateRemoval(
+                   D.getDeclSpec().getStorageClassSpecLoc());
+      }
     }
 
     // C++11 [except.spec]p15:
@@ -10589,27 +10630,6 @@ Sema::ActOnFunctionDeclarator(Scope *S, Declarator &D, DeclContext *DC,
                                                 Previous))
           NewFD->setInvalidDecl();
       }
-
-      // C++ [dcl.stc]p1:
-      //   A storage-class-specifier shall not be specified in an explicit
-      //   specialization (14.7.3)
-      // FIXME: We should be checking this for dependent specializations.
-      FunctionTemplateSpecializationInfo *Info =
-          NewFD->getTemplateSpecializationInfo();
-      if (Info && SC != SC_None) {
-        if (SC != Info->getTemplate()->getTemplatedDecl()->getStorageClass())
-          Diag(NewFD->getLocation(),
-               diag::err_explicit_specialization_inconsistent_storage_class)
-            << SC
-            << FixItHint::CreateRemoval(
-                                      D.getDeclSpec().getStorageClassSpecLoc());
-
-        else
-          Diag(NewFD->getLocation(),
-               diag::ext_explicit_specialization_storage_class)
-            << FixItHint::CreateRemoval(
-                                      D.getDeclSpec().getStorageClassSpecLoc());
-      }
     } else if (isMemberSpecialization && isa<CXXMethodDecl>(NewFD)) {
       if (CheckMemberSpecialization(NewFD, Previous))
           NewFD->setInvalidDecl();
diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
index 8ab429e2a136e..55284d638efb1 100644
--- a/clang/lib/Sema/SemaDeclCXX.cpp
+++ b/clang/lib/Sema/SemaDeclCXX.cpp
@@ -3489,9 +3489,9 @@ Sema::ActOnCXXMemberDeclarator(Scope *S, AccessSpecifier AS, Declarator &D,
     break;
   }
 
-  bool isInstField = ((DS.getStorageClassSpec() == DeclSpec::SCS_unspecified ||
-                       DS.getStorageClassSpec() == DeclSpec::SCS_mutable) &&
-                      !isFunc);
+  bool isInstField = (DS.getStorageClassSpec() == DeclSpec::SCS_unspecified ||
+                      DS.getStorageClassSpec() == DeclSpec::SCS_mutable) &&
+                     !isFunc && TemplateParameterLists.empty();
 
   if (DS.hasConstexprSpecifier() && isInstField) {
     SemaDiagnosticBuilder B =
@@ -3541,6 +3541,7 @@ Sema::ActOnCXXMemberDeclarator(Scope *S, AccessSpecifier AS, Declarator &D,
 
     IdentifierInfo *II = Name.getAsIdentifierInfo();
 
+#if 0
     // Member field could not be with "template" keyword.
     // So TemplateParameterLists should be empty in this case.
     if (TemplateParameterLists.size()) {
@@ -3561,6 +3562,7 @@ Sema::ActOnCXXMemberDeclarator(Scope *S, AccessSpecifier AS, Declarator &D,
       }
       return nullptr;
     }
+#endif
 
     if (D.getName().getKind() == UnqualifiedIdKind::IK_TemplateId) {
       Diag(D.getIdentifierLoc(), diag::err_member_with_template_arguments)
diff --git a/clang/test/CXX/dcl.dcl/dcl.spec/dcl.stc/p1.cpp b/clang/test/CXX/dcl.dcl/dcl.spec/dcl.stc/p1.cpp
index cbb439ef5fecd..f6b5d2487e73d 100644
--- a/clang/test/CXX/dcl.dcl/dcl.spec/dcl.stc/p1.cpp
+++ b/clang/test/CXX/dcl.dcl/dcl.spec/dcl.stc/p1.cpp
@@ -7,7 +7,7 @@ template<typename T> void f(T) {}
 template<typename T> static void g(T) {}
 
 
-template<> static void f<int>(int); // expected-error{{explicit specialization has extraneous, inconsistent storage class 'static'}}
+template<> static void f<int>(int); // expected-warning{{explicit specialization cannot have a storage class}}
 template static void f<float>(float); // expected-error{{explicit instantiation cannot have a storage class}}
 
 template<> void f<double>(double);
@@ -29,4 +29,5 @@ int X<T>::value = 17;
 
 template static int X<int>::value; // expected-error{{explicit instantiation cannot have a storage class}}
 
-template<> static int X<float>::value; // expected-error{{'static' can only be specified inside the class definition}}
+template<> static int X<float>::value; // expected-warning{{explicit specialization cannot have a storage class}}
+                                       // expected-error@-1{{'static' can only be specified inside the class definition}}
diff --git a/clang/test/CXX/drs/cwg7xx.cpp b/clang/test/CXX/drs/cwg7xx.cpp
index 0300dae08d6d3..6d93e2948dadb 100644
--- a/clang/test/CXX/drs/cwg7xx.cpp
+++ b/clang/test/CXX/drs/cwg7xx.cpp
@@ -80,7 +80,7 @@ namespace cwg727 { // cwg727: partial
 
     template<> struct C<int>;
     template<> void f<int>();
-    template<> static int N<int>;
+    template<> int N<int>;
 
     template<typename T> struct C<T*>;
     template<typename T> static int N<T*>;
@@ -91,7 +91,7 @@ namespace cwg727 { // cwg727: partial
       //   expected-note@#cwg727-C {{explicitly specialized declaration is here}}
       template<> void f<float>();
       // expected-error@-1 {{no function template matches function template specialization 'f'}}
-      template<> static int N<float>;
+      template<> int N<float>;
       // expected-error@-1 {{variable template specialization of 'N' not in class 'A' or an enclosing namespace}}
       //   expected-note@#cwg727-N {{explicitly specialized declaration is here}}
 
@@ -109,7 +109,7 @@ namespace cwg727 { // cwg727: partial
       template<> void A::f<double>();
       // expected-error@-1 {{o function template matches function template specialization 'f'}}
       // expected-error@-2 {{non-friend class member 'f' cannot have a qualified name}}
-      template<> static int A::N<double>;
+      template<> int A::N<double>;
       // expected-error@-1 {{non-friend class member 'N' cannot have a qualified name}}
       // expected-error@-2 {{variable template specialization of 'N' not in class 'A' or an enclosing namespace}}
       //   expected-note@#cwg727-N {{explicitly specialized declaration is here}}
@@ -166,7 +166,7 @@ namespace cwg727 { // cwg727: partial
 
     template<> struct C<int> {};
     template<> void f<int>() {}
-    template<> static const int N<int>;
+    template<> const int N<int>;
 
     template<typename T> struct C<T*> {};
     template<typename T> static const int N<T*>;
@@ -208,18 +208,18 @@ namespace cwg727 { // cwg727: partial
 #if __cplusplus >= 201402L
     template<int> struct B {
       template<int> static const int u = 1;
-      template<> static const i...
[truncated]

@llvmbot
Copy link
Member

llvmbot commented May 30, 2024

@llvm/pr-subscribers-clang

Author: Krystian Stasiowski (sdkrystian)

Changes

According to [[temp.expl.spec] p2](http://eel.is/c++draft/temp.expl.spec#2):
> The declaration in an explicit-specialization shall not be an export-declaration. An explicit specialization shall not use a storage-class-specifier other than thread_local.

Clang partially implements this, but a number of issues exist:

  1. We don't diagnose class scope explicit specializations of variable templates with storage-class-specifiers, e.g.
    struct A
    {
        template&lt;typename T&gt;
        static constexpr int x = 0;
    
        template&lt;&gt;
        static constexpr int x&lt;void&gt; = 1; // ill-formed, but clang accepts
    };
  2. We incorrectly reject class scope explicit specializations of variable templates when static is not used, e.g.
    struct A
    {
        template&lt;typename T&gt;
        static constexpr int x = 0;
    
        template&lt;&gt;
        constexpr int x&lt;void&gt; = 1; // error: non-static data member cannot be constexpr; did you intend to make it static?
    };
  3. We don't diagnose dependent class scope explicit specializations of function templates with storage class specifiers, e.g.
    template&lt;typename T&gt;
    struct A
    {
        template&lt;typename U&gt;
        static void f();
    
        template&lt;&gt;
        static void f&lt;int&gt;(); // ill-formed, but clang accepts
    };

This patch addresses these issues as follows:

  • #1 is fixed by issuing a diagnostic when an explicit specialization of a variable template has storage class specifier
  • #2 is fixed by considering any non-function declaration with any template parameter lists at class scope to be a static data member. This also allows for better error recovery (it's more likely the user intended to declare a variable template than a "field template").
  • #3 is fixed by checking whether a function template explicit specialization has a storage class specifier even when the primary template is not yet known.

One thing to note is that it would be far simpler to diagnose this when parsing the decl-specifier-seq, but such an implementation would necessitate a refactor of ParsedTemplateInfo which I believe to be outside the scope of this patch.


Patch is 49.31 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/93873.diff

18 Files Affected:

  • (modified) clang/lib/Parse/ParseDeclCXX.cpp (+3-2)
  • (modified) clang/lib/Sema/DeclSpec.cpp (+1)
  • (modified) clang/lib/Sema/SemaDecl.cpp (+134-114)
  • (modified) clang/lib/Sema/SemaDeclCXX.cpp (+5-3)
  • (modified) clang/test/CXX/dcl.dcl/dcl.spec/dcl.stc/p1.cpp (+3-2)
  • (modified) clang/test/CXX/drs/cwg7xx.cpp (+12-12)
  • (modified) clang/test/CXX/temp/temp.decls/temp.mem/p2.cpp (+1-1)
  • (modified) clang/test/CXX/temp/temp.decls/temp.variadic/p5.cpp (+1-1)
  • (modified) clang/test/CXX/temp/temp.spec/temp.expl.spec/p17.cpp (+8-7)
  • (modified) clang/test/CXX/temp/temp.spec/temp.expl.spec/p2-0x.cpp (+31-28)
  • (added) clang/test/CXX/temp/temp.spec/temp.expl.spec/p2-20.cpp (+274)
  • (modified) clang/test/Modules/Inputs/redecl-templates/a.h (+1-1)
  • (modified) clang/test/Modules/redecl-templates.cpp (+4-2)
  • (modified) clang/test/PCH/cxx-templates.h (+6-6)
  • (modified) clang/test/PCH/cxx1y-variable-templates.cpp (+16-8)
  • (modified) clang/test/SemaCXX/cxx1y-variable-templates_in_class.cpp (+18-12)
  • (modified) clang/test/SemaTemplate/explicit-specialization-member.cpp (+4-4)
  • (modified) clang/test/SemaTemplate/nested-template.cpp (+7-7)
diff --git a/clang/lib/Parse/ParseDeclCXX.cpp b/clang/lib/Parse/ParseDeclCXX.cpp
index 9a4a777f575b2..d02548f6441f9 100644
--- a/clang/lib/Parse/ParseDeclCXX.cpp
+++ b/clang/lib/Parse/ParseDeclCXX.cpp
@@ -3162,7 +3162,8 @@ Parser::DeclGroupPtrTy Parser::ParseCXXClassMemberDeclaration(
                      DeclSpec::SCS_static &&
                  DeclaratorInfo.getDeclSpec().getStorageClassSpec() !=
                      DeclSpec::SCS_typedef &&
-                 !DS.isFriendSpecified()) {
+                 !DS.isFriendSpecified() &&
+                 TemplateInfo.Kind == ParsedTemplateInfo::NonTemplate) {
         // It's a default member initializer.
         if (BitfieldSize.get())
           Diag(Tok, getLangOpts().CPlusPlus20
@@ -3261,7 +3262,7 @@ Parser::DeclGroupPtrTy Parser::ParseCXXClassMemberDeclaration(
       } else if (ThisDecl)
         Actions.AddInitializerToDecl(ThisDecl, Init.get(),
                                      EqualLoc.isInvalid());
-    } else if (ThisDecl && DS.getStorageClassSpec() == DeclSpec::SCS_static)
+    } else if (ThisDecl && DeclaratorInfo.isStaticMember())
       // No initializer.
       Actions.ActOnUninitializedDecl(ThisDecl);
 
diff --git a/clang/lib/Sema/DeclSpec.cpp b/clang/lib/Sema/DeclSpec.cpp
index 60e8189025700..96c90a60b9682 100644
--- a/clang/lib/Sema/DeclSpec.cpp
+++ b/clang/lib/Sema/DeclSpec.cpp
@@ -416,6 +416,7 @@ bool Declarator::isDeclarationOfFunction() const {
 bool Declarator::isStaticMember() {
   assert(getContext() == DeclaratorContext::Member);
   return getDeclSpec().getStorageClassSpec() == DeclSpec::SCS_static ||
+         (!isDeclarationOfFunction() && !getTemplateParameterLists().empty()) ||
          (getName().getKind() == UnqualifiedIdKind::IK_OperatorFunctionId &&
           CXXMethodDecl::isStaticOverloadedOperator(
               getName().OperatorFunctionId.Operator));
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 2a87b26f17a2b..cc36387c0e332 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -7602,80 +7602,8 @@ NamedDecl *Sema::ActOnVariableDeclarator(
                             NTCUC_AutoVar, NTCUK_Destruct);
   } else {
     bool Invalid = false;
-
-    if (DC->isRecord() && !CurContext->isRecord()) {
-      // This is an out-of-line definition of a static data member.
-      switch (SC) {
-      case SC_None:
-        break;
-      case SC_Static:
-        Diag(D.getDeclSpec().getStorageClassSpecLoc(),
-             diag::err_static_out_of_line)
-          << FixItHint::CreateRemoval(D.getDeclSpec().getStorageClassSpecLoc());
-        break;
-      case SC_Auto:
-      case SC_Register:
-      case SC_Extern:
-        // [dcl.stc] p2: The auto or register specifiers shall be applied only
-        // to names of variables declared in a block or to function parameters.
-        // [dcl.stc] p6: The extern specifier cannot be used in the declaration
-        // of class members
-
-        Diag(D.getDeclSpec().getStorageClassSpecLoc(),
-             diag::err_storage_class_for_static_member)
-          << FixItHint::CreateRemoval(D.getDeclSpec().getStorageClassSpecLoc());
-        break;
-      case SC_PrivateExtern:
-        llvm_unreachable("C storage class in c++!");
-      }
-    }
-
-    if (SC == SC_Static && CurContext->isRecord()) {
-      if (const CXXRecordDecl *RD = dyn_cast<CXXRecordDecl>(DC)) {
-        // Walk up the enclosing DeclContexts to check for any that are
-        // incompatible with static data members.
-        const DeclContext *FunctionOrMethod = nullptr;
-        const CXXRecordDecl *AnonStruct = nullptr;
-        for (DeclContext *Ctxt = DC; Ctxt; Ctxt = Ctxt->getParent()) {
-          if (Ctxt->isFunctionOrMethod()) {
-            FunctionOrMethod = Ctxt;
-            break;
-          }
-          const CXXRecordDecl *ParentDecl = dyn_cast<CXXRecordDecl>(Ctxt);
-          if (ParentDecl && !ParentDecl->getDeclName()) {
-            AnonStruct = ParentDecl;
-            break;
-          }
-        }
-        if (FunctionOrMethod) {
-          // C++ [class.static.data]p5: A local class shall not have static data
-          // members.
-          Diag(D.getIdentifierLoc(),
-               diag::err_static_data_member_not_allowed_in_local_class)
-              << Name << RD->getDeclName()
-              << llvm::to_underlying(RD->getTagKind());
-        } else if (AnonStruct) {
-          // C++ [class.static.data]p4: Unnamed classes and classes contained
-          // directly or indirectly within unnamed classes shall not contain
-          // static data members.
-          Diag(D.getIdentifierLoc(),
-               diag::err_static_data_member_not_allowed_in_anon_struct)
-              << Name << llvm::to_underlying(AnonStruct->getTagKind());
-          Invalid = true;
-        } else if (RD->isUnion()) {
-          // C++98 [class.union]p1: If a union contains a static data member,
-          // the program is ill-formed. C++11 drops this restriction.
-          Diag(D.getIdentifierLoc(),
-               getLangOpts().CPlusPlus11
-                 ? diag::warn_cxx98_compat_static_data_member_in_union
-                 : diag::ext_static_data_member_in_union) << Name;
-        }
-      }
-    }
-
     // Match up the template parameter lists with the scope specifier, then
     // determine whether we have a template or a template specialization.
-    bool InvalidScope = false;
     TemplateParams = MatchTemplateParametersToScopeSpecifier(
         D.getDeclSpec().getBeginLoc(), D.getIdentifierLoc(),
         D.getCXXScopeSpec(),
@@ -7683,8 +7611,7 @@ NamedDecl *Sema::ActOnVariableDeclarator(
             ? D.getName().TemplateId
             : nullptr,
         TemplateParamLists,
-        /*never a friend*/ false, IsMemberSpecialization, InvalidScope);
-    Invalid |= InvalidScope;
+        /*never a friend*/ false, IsMemberSpecialization, Invalid);
 
     if (TemplateParams) {
       if (!TemplateParams->size() &&
@@ -7727,6 +7654,102 @@ NamedDecl *Sema::ActOnVariableDeclarator(
              "should have a 'template<>' for this decl");
     }
 
+    bool IsExplicitSpecialization =
+        IsVariableTemplateSpecialization && !IsPartialSpecialization;
+
+    // C++ [temp.expl.spec]p2:
+    //   The declaration in an explicit-specialization shall not be an
+    //   export-declaration. An explicit specialization shall not use a
+    //   storage-class-specifier other than thread_local.
+    //
+    // We use the storage-class-specifier from DeclSpec because we may have
+    // added implicit 'extern' for declarations with __declspec(dllimport)!
+    if (SCSpec != DeclSpec::SCS_unspecified &&
+        (IsExplicitSpecialization || IsMemberSpecialization)) {
+      Diag(D.getDeclSpec().getStorageClassSpecLoc(),
+           diag::ext_explicit_specialization_storage_class)
+          << FixItHint::CreateRemoval(D.getDeclSpec().getStorageClassSpecLoc());
+    }
+
+    if (CurContext->isRecord()) {
+      if (SC == SC_Static) {
+        if (const CXXRecordDecl *RD = dyn_cast<CXXRecordDecl>(DC)) {
+          // Walk up the enclosing DeclContexts to check for any that are
+          // incompatible with static data members.
+          const DeclContext *FunctionOrMethod = nullptr;
+          const CXXRecordDecl *AnonStruct = nullptr;
+          for (DeclContext *Ctxt = DC; Ctxt; Ctxt = Ctxt->getParent()) {
+            if (Ctxt->isFunctionOrMethod()) {
+              FunctionOrMethod = Ctxt;
+              break;
+            }
+            const CXXRecordDecl *ParentDecl = dyn_cast<CXXRecordDecl>(Ctxt);
+            if (ParentDecl && !ParentDecl->getDeclName()) {
+              AnonStruct = ParentDecl;
+              break;
+            }
+          }
+          if (FunctionOrMethod) {
+            // C++ [class.static.data]p5: A local class shall not have static
+            // data members.
+            Diag(D.getIdentifierLoc(),
+                 diag::err_static_data_member_not_allowed_in_local_class)
+                << Name << RD->getDeclName()
+                << llvm::to_underlying(RD->getTagKind());
+          } else if (AnonStruct) {
+            // C++ [class.static.data]p4: Unnamed classes and classes contained
+            // directly or indirectly within unnamed classes shall not contain
+            // static data members.
+            Diag(D.getIdentifierLoc(),
+                 diag::err_static_data_member_not_allowed_in_anon_struct)
+                << Name << llvm::to_underlying(AnonStruct->getTagKind());
+            Invalid = true;
+          } else if (RD->isUnion()) {
+            // C++98 [class.union]p1: If a union contains a static data member,
+            // the program is ill-formed. C++11 drops this restriction.
+            Diag(D.getIdentifierLoc(),
+                 getLangOpts().CPlusPlus11
+                     ? diag::warn_cxx98_compat_static_data_member_in_union
+                     : diag::ext_static_data_member_in_union)
+                << Name;
+          }
+        }
+      } else if (IsVariableTemplate || IsPartialSpecialization) {
+        // There is no such thing as a member field template.
+        Diag(D.getIdentifierLoc(), diag::err_template_member)
+            << II << TemplateParams->getSourceRange();
+        // Recover by pretending this is a static data member template.
+        SC = SC_Static;
+      }
+    } else if (DC->isRecord()) {
+      // This is an out-of-line definition of a static data member.
+      switch (SC) {
+      case SC_None:
+        break;
+      case SC_Static:
+        Diag(D.getDeclSpec().getStorageClassSpecLoc(),
+             diag::err_static_out_of_line)
+            << FixItHint::CreateRemoval(
+                   D.getDeclSpec().getStorageClassSpecLoc());
+        break;
+      case SC_Auto:
+      case SC_Register:
+      case SC_Extern:
+        // [dcl.stc] p2: The auto or register specifiers shall be applied only
+        // to names of variables declared in a block or to function parameters.
+        // [dcl.stc] p6: The extern specifier cannot be used in the declaration
+        // of class members
+
+        Diag(D.getDeclSpec().getStorageClassSpecLoc(),
+             diag::err_storage_class_for_static_member)
+            << FixItHint::CreateRemoval(
+                   D.getDeclSpec().getStorageClassSpecLoc());
+        break;
+      case SC_PrivateExtern:
+        llvm_unreachable("C storage class in c++!");
+      }
+    }
+
     if (IsVariableTemplateSpecialization) {
       SourceLocation TemplateKWLoc =
           TemplateParamLists.size() > 0
@@ -7772,8 +7795,6 @@ NamedDecl *Sema::ActOnVariableDeclarator(
     // the variable (matching the scope specifier), store them.
     // An explicit variable template specialization does not own any template
     // parameter lists.
-    bool IsExplicitSpecialization =
-        IsVariableTemplateSpecialization && !IsPartialSpecialization;
     unsigned VDTemplateParamLists =
         (TemplateParams && !IsExplicitSpecialization) ? 1 : 0;
     if (TemplateParamLists.size() > VDTemplateParamLists)
@@ -10203,25 +10224,45 @@ Sema::ActOnFunctionDeclarator(Scope *S, Declarator &D, DeclContext *DC,
       NewFD->setImplicitlyInline(ImplicitInlineCXX20);
     }
 
-    if (SC == SC_Static && isa<CXXMethodDecl>(NewFD) &&
-        !CurContext->isRecord()) {
-      // C++ [class.static]p1:
-      //   A data or function member of a class may be declared static
-      //   in a class definition, in which case it is a static member of
-      //   the class.
+    if (!isFriend && SC != SC_None) {
+      // C++ [temp.expl.spec]p2:
+      //   The declaration in an explicit-specialization shall not be an
+      //   export-declaration. An explicit specialization shall not use a
+      //   storage-class-specifier other than thread_local.
+      //
+      // We diagnose friend declarations with storage-class-specifiers
+      // elsewhere.
+      if (isFunctionTemplateSpecialization || isMemberSpecialization) {
+        Diag(D.getDeclSpec().getStorageClassSpecLoc(),
+             diag::ext_explicit_specialization_storage_class)
+            << FixItHint::CreateRemoval(
+                   D.getDeclSpec().getStorageClassSpecLoc());
+      }
 
-      // Complain about the 'static' specifier if it's on an out-of-line
-      // member function definition.
+      if (SC == SC_Static && !CurContext->isRecord() && DC->isRecord()) {
+        assert(isa<CXXMethodDecl>(NewFD) &&
+               "Out-of-line member function should be a CXXMethodDecl");
+        // C++ [class.static]p1:
+        //   A data or function member of a class may be declared static
+        //   in a class definition, in which case it is a static member of
+        //   the class.
 
-      // MSVC permits the use of a 'static' storage specifier on an out-of-line
-      // member function template declaration and class member template
-      // declaration (MSVC versions before 2015), warn about this.
-      Diag(D.getDeclSpec().getStorageClassSpecLoc(),
-           ((!getLangOpts().isCompatibleWithMSVC(LangOptions::MSVC2015) &&
-             cast<CXXRecordDecl>(DC)->getDescribedClassTemplate()) ||
-           (getLangOpts().MSVCCompat && NewFD->getDescribedFunctionTemplate()))
-           ? diag::ext_static_out_of_line : diag::err_static_out_of_line)
-        << FixItHint::CreateRemoval(D.getDeclSpec().getStorageClassSpecLoc());
+        // Complain about the 'static' specifier if it's on an out-of-line
+        // member function definition.
+
+        // MSVC permits the use of a 'static' storage specifier on an
+        // out-of-line member function template declaration and class member
+        // template declaration (MSVC versions before 2015), warn about this.
+        Diag(D.getDeclSpec().getStorageClassSpecLoc(),
+             ((!getLangOpts().isCompatibleWithMSVC(LangOptions::MSVC2015) &&
+               cast<CXXRecordDecl>(DC)->getDescribedClassTemplate()) ||
+              (getLangOpts().MSVCCompat &&
+               NewFD->getDescribedFunctionTemplate()))
+                 ? diag::ext_static_out_of_line
+                 : diag::err_static_out_of_line)
+            << FixItHint::CreateRemoval(
+                   D.getDeclSpec().getStorageClassSpecLoc());
+      }
     }
 
     // C++11 [except.spec]p15:
@@ -10589,27 +10630,6 @@ Sema::ActOnFunctionDeclarator(Scope *S, Declarator &D, DeclContext *DC,
                                                 Previous))
           NewFD->setInvalidDecl();
       }
-
-      // C++ [dcl.stc]p1:
-      //   A storage-class-specifier shall not be specified in an explicit
-      //   specialization (14.7.3)
-      // FIXME: We should be checking this for dependent specializations.
-      FunctionTemplateSpecializationInfo *Info =
-          NewFD->getTemplateSpecializationInfo();
-      if (Info && SC != SC_None) {
-        if (SC != Info->getTemplate()->getTemplatedDecl()->getStorageClass())
-          Diag(NewFD->getLocation(),
-               diag::err_explicit_specialization_inconsistent_storage_class)
-            << SC
-            << FixItHint::CreateRemoval(
-                                      D.getDeclSpec().getStorageClassSpecLoc());
-
-        else
-          Diag(NewFD->getLocation(),
-               diag::ext_explicit_specialization_storage_class)
-            << FixItHint::CreateRemoval(
-                                      D.getDeclSpec().getStorageClassSpecLoc());
-      }
     } else if (isMemberSpecialization && isa<CXXMethodDecl>(NewFD)) {
       if (CheckMemberSpecialization(NewFD, Previous))
           NewFD->setInvalidDecl();
diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
index 8ab429e2a136e..55284d638efb1 100644
--- a/clang/lib/Sema/SemaDeclCXX.cpp
+++ b/clang/lib/Sema/SemaDeclCXX.cpp
@@ -3489,9 +3489,9 @@ Sema::ActOnCXXMemberDeclarator(Scope *S, AccessSpecifier AS, Declarator &D,
     break;
   }
 
-  bool isInstField = ((DS.getStorageClassSpec() == DeclSpec::SCS_unspecified ||
-                       DS.getStorageClassSpec() == DeclSpec::SCS_mutable) &&
-                      !isFunc);
+  bool isInstField = (DS.getStorageClassSpec() == DeclSpec::SCS_unspecified ||
+                      DS.getStorageClassSpec() == DeclSpec::SCS_mutable) &&
+                     !isFunc && TemplateParameterLists.empty();
 
   if (DS.hasConstexprSpecifier() && isInstField) {
     SemaDiagnosticBuilder B =
@@ -3541,6 +3541,7 @@ Sema::ActOnCXXMemberDeclarator(Scope *S, AccessSpecifier AS, Declarator &D,
 
     IdentifierInfo *II = Name.getAsIdentifierInfo();
 
+#if 0
     // Member field could not be with "template" keyword.
     // So TemplateParameterLists should be empty in this case.
     if (TemplateParameterLists.size()) {
@@ -3561,6 +3562,7 @@ Sema::ActOnCXXMemberDeclarator(Scope *S, AccessSpecifier AS, Declarator &D,
       }
       return nullptr;
     }
+#endif
 
     if (D.getName().getKind() == UnqualifiedIdKind::IK_TemplateId) {
       Diag(D.getIdentifierLoc(), diag::err_member_with_template_arguments)
diff --git a/clang/test/CXX/dcl.dcl/dcl.spec/dcl.stc/p1.cpp b/clang/test/CXX/dcl.dcl/dcl.spec/dcl.stc/p1.cpp
index cbb439ef5fecd..f6b5d2487e73d 100644
--- a/clang/test/CXX/dcl.dcl/dcl.spec/dcl.stc/p1.cpp
+++ b/clang/test/CXX/dcl.dcl/dcl.spec/dcl.stc/p1.cpp
@@ -7,7 +7,7 @@ template<typename T> void f(T) {}
 template<typename T> static void g(T) {}
 
 
-template<> static void f<int>(int); // expected-error{{explicit specialization has extraneous, inconsistent storage class 'static'}}
+template<> static void f<int>(int); // expected-warning{{explicit specialization cannot have a storage class}}
 template static void f<float>(float); // expected-error{{explicit instantiation cannot have a storage class}}
 
 template<> void f<double>(double);
@@ -29,4 +29,5 @@ int X<T>::value = 17;
 
 template static int X<int>::value; // expected-error{{explicit instantiation cannot have a storage class}}
 
-template<> static int X<float>::value; // expected-error{{'static' can only be specified inside the class definition}}
+template<> static int X<float>::value; // expected-warning{{explicit specialization cannot have a storage class}}
+                                       // expected-error@-1{{'static' can only be specified inside the class definition}}
diff --git a/clang/test/CXX/drs/cwg7xx.cpp b/clang/test/CXX/drs/cwg7xx.cpp
index 0300dae08d6d3..6d93e2948dadb 100644
--- a/clang/test/CXX/drs/cwg7xx.cpp
+++ b/clang/test/CXX/drs/cwg7xx.cpp
@@ -80,7 +80,7 @@ namespace cwg727 { // cwg727: partial
 
     template<> struct C<int>;
     template<> void f<int>();
-    template<> static int N<int>;
+    template<> int N<int>;
 
     template<typename T> struct C<T*>;
     template<typename T> static int N<T*>;
@@ -91,7 +91,7 @@ namespace cwg727 { // cwg727: partial
       //   expected-note@#cwg727-C {{explicitly specialized declaration is here}}
       template<> void f<float>();
       // expected-error@-1 {{no function template matches function template specialization 'f'}}
-      template<> static int N<float>;
+      template<> int N<float>;
       // expected-error@-1 {{variable template specialization of 'N' not in class 'A' or an enclosing namespace}}
       //   expected-note@#cwg727-N {{explicitly specialized declaration is here}}
 
@@ -109,7 +109,7 @@ namespace cwg727 { // cwg727: partial
       template<> void A::f<double>();
       // expected-error@-1 {{o function template matches function template specialization 'f'}}
       // expected-error@-2 {{non-friend class member 'f' cannot have a qualified name}}
-      template<> static int A::N<double>;
+      template<> int A::N<double>;
       // expected-error@-1 {{non-friend class member 'N' cannot have a qualified name}}
       // expected-error@-2 {{variable template specialization of 'N' not in class 'A' or an enclosing namespace}}
       //   expected-note@#cwg727-N {{explicitly specialized declaration is here}}
@@ -166,7 +166,7 @@ namespace cwg727 { // cwg727: partial
 
     template<> struct C<int> {};
     template<> void f<int>() {}
-    template<> static const int N<int>;
+    template<> const int N<int>;
 
     template<typename T> struct C<T*> {};
     template<typename T> static const int N<T*>;
@@ -208,18 +208,18 @@ namespace cwg727 { // cwg727: partial
 #if __cplusplus >= 201402L
     template<int> struct B {
       template<int> static const int u = 1;
-      template<> static const i...
[truncated]

Copy link
Collaborator

@erichkeane erichkeane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sensible to me! Definitely needs a release note. Perhaps an alert on discourse about 'potentially breaking' changes.

clang/lib/Sema/SemaDeclCXX.cpp Outdated Show resolved Hide resolved
clang/lib/Sema/SemaDeclCXX.cpp Outdated Show resolved Hide resolved
@sdkrystian sdkrystian force-pushed the class-scope-expl-var-spec branch from e870dc2 to 8ceceab Compare May 30, 2024 21:05
Copy link
Contributor

@mizvekov mizvekov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interestingly, only GCC errors on a case like this:

namespace {
  template<class T> int A = 0;
}
template<> int A<void> = 0;

https://godbolt.org/z/TTjssKxz5

Copy link
Contributor

@Endilll Endilll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DR test changes look good.

@sdkrystian
Copy link
Member Author

sdkrystian commented May 30, 2024

@mizvekov GCC seems to be right; [decl.meaning.general] p3.3 requires that the specialized declaration be nominable (per [basic.scope.scope] p7) in S (which in this case is the global namespace). The target scope of the primary template is not the global namespace, nor is it any member of the inline namespace set thereof, so this should be ill-formed. Making the unnamed namespace declaration inline results in GCC accepting the declaration.

@mizvekov
Copy link
Contributor

@mizvekov GCC seems to be right; [decl.meaning.general] p3.3 requires that the specialized declaration be nominable (per [basic.scope.scope] p7) in S (which in this case is the global namespace). The target scope of the primary template is not the global namespace, nor is it any member of the inline namespace set thereof, so this should be ill-formed. Making the unnamed namespace declaration inline results in GCC accepting the declaration.

I think that's right. The GCC error message has bogus detail of course, there is no nested name specifier that would work here.
At least for clang, we are putting the specialized declaration in the anonymous namespace and giving it internal linkage, so this is not that bad.

I suppose fixing this is outside of the scope of this patch? If so, that would be alright.

@sdkrystian
Copy link
Member Author

I suppose fixing this is outside of the scope of this patch? If so, that would be alright.

@mizvekov Yeah, definitely. I can submit a path to fix this either tomorrow or sometime next week.

Copy link
Contributor

@mizvekov mizvekov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small nit, otherwise LGTM, thanks!

clang/test/CXX/temp/temp.spec/temp.expl.spec/p2-20.cpp Outdated Show resolved Hide resolved
@sdkrystian
Copy link
Member Author

sdkrystian commented Jun 18, 2024

@mizvekov GCC seems to be right; [decl.meaning.general] p3.3 requires that the specialized declaration be nominable (per [basic.scope.scope] p7) in S (which in this case is the global namespace). The target scope of the primary template is not the global namespace, nor is it any member of the inline namespace set thereof, so this should be ill-formed. Making the unnamed namespace declaration inline results in GCC accepting the declaration.

I think that's right. The GCC error message has bogus detail of course, there is no nested name specifier that would work here. At least for clang, we are putting the specialized declaration in the anonymous namespace and giving it internal linkage, so this is not that bad.

I suppose fixing this is outside of the scope of this patch? If so, that would be alright.

@mizvekov Correction: GCC is wrong here. Embarrassingly, I'm the one who implemented clangs current behavior in this patch :P. The requirement for the specialized declaration to be nominable in [decl.meaning.general] p3.3 only applies to function template specializations:

If it declares a class member, the terminal name of the declarator-id is not looked up; otherwise, only those lookup results that are nominable in S are considered when identifying any function template specialization being declared.

@sdkrystian sdkrystian force-pushed the class-scope-expl-var-spec branch from 28be1bb to 4f5cc85 Compare June 18, 2024 13:53
@sdkrystian sdkrystian merged commit 9a88aa0 into llvm:main Jun 18, 2024
6 of 7 checks passed
@bgra8
Copy link
Contributor

bgra8 commented Jun 24, 2024

@sdkrystian we need a way to disable the new check so we can do the code fixes for our large codebase while still using the old compiler.

As the patch description suggests the correct code is rejected by the current clang so we have no path forward here. Can you please add a flag to allow switching to the old behavior?

@alexfh
Copy link
Contributor

alexfh commented Jun 24, 2024

Specifically, the problem is that we can't fix code that corresponds to case # 1 from the description, since it produces code corresponding to pattern # 2, which can't compile with the previous clang version. Shipping fixes together with the compiler is not an option for us (and probably for many other users), so it should be possible to tell the new Clang to accept pattern # 1, i.e. it should be a warning (included into -Wall, probably) rather than a hard error for at least one compiler release.

@dwblaikie
Copy link
Collaborator

Sent a patch to add a warning flag for the warning this patch uses: #96699

With that, we could disable the warning during the compiler migration, decoupling compiler migration from code cleanup.

dwblaikie added a commit that referenced this pull request Jun 27, 2024
…ializations (#96699)

With the recent fix for this situation in class members (#93873) (for
which the fixed code is invalid prior to this patch - making migrating
code difficult as it must be in lock-step with the compiler migration,
if building with -Werror) it'd be really useful to be able to disable
this warning during the compiler migration/decouple the compiler
migration from the source fixes.

In theory this approach will regress the codebase to the previous
non-member cases of this issue that were already being held back by the
warning (as opposed to if we carved out the new cases into a separate
warning from the existing cases) but I think this'll be so rare and the
cleanup so simple, that the extra regressions of disabling the warning
broadly won't be too much of a problem. (but if folks disagree, I'm open
to making the warning more fine-grained)
@alexfh
Copy link
Contributor

alexfh commented Jul 1, 2024

It looks like the presence of static on template variable specializations makes difference in the namespace context: https://gcc.godbolt.org/z/WGsreqbz8

Specifically, the specializations not marked static result in an exported variable. Thus, we have seemingly valid code that generates this diagnostic:

namespace A {
template <unsigned N>
static constexpr unsigned kMaxUnsignedInt = 2 * kMaxUnsignedInt<N - 1> + 1;

template <>
static constexpr unsigned kMaxUnsignedInt<1> = 1;
}

However, if we remove the static from the specialization, we're getting a potential ODR violation (considering this code can be in a header included into multiple translation units).

The right thing for this specific case is inline constexpr, but nevertheless, there seems to be an inconsistency now in how Clang handles the case now.

@alexfh
Copy link
Contributor

alexfh commented Jul 1, 2024

There's more (https://gcc.godbolt.org/z/d39hxf65K):

class D {
template <unsigned N>
static constexpr auto kMaxUnsignedInt = 2 * kMaxUnsignedInt<N - 1> + 1;

template <>
constexpr auto kMaxUnsignedInt<1> = 1;
};

fails to compile with

<source>:14:11: error: 'auto' not allowed in non-static class member
   14 | constexpr auto kMaxUnsignedInt<1> = 1;
      |           ^~~~

@sdkrystian
Copy link
Member Author

sdkrystian commented Jul 2, 2024

There's more (https://gcc.godbolt.org/z/d39hxf65K):

class D {
template <unsigned N>
static constexpr auto kMaxUnsignedInt = 2 * kMaxUnsignedInt<N - 1> + 1;

template <>
constexpr auto kMaxUnsignedInt<1> = 1;
};

fails to compile with

<source>:14:11: error: 'auto' not allowed in non-static class member
   14 | constexpr auto kMaxUnsignedInt<1> = 1;
      |           ^~~~

@alexfh Missed this one :) Opened a PR to fix this here.

@alexfh alexfh requested a review from ilya-biryukov July 2, 2024 16:19
@alexfh
Copy link
Contributor

alexfh commented Jul 2, 2024

Thanks for proposing a fix! I've been hinted though that Clang may be incorrect in accepting explicit specializations of a templated static class data member in the class scope. It looks like with all the restrictions in the standard it may only be allowed to place explicit specializations of a templated static class data member outside of the class. Though IANALL and I'm not sure what the intention of the standard is in this area.

In any case, introducing any restrictions should be done with a clear migration path, so #97425 is likely helpful anyway.

@sdkrystian
Copy link
Member Author

I've been hinted though that Clang may be incorrect in accepting explicit specializations of a templated static class data member in the class scope. It looks like with all the restrictions in the standard it may only be allowed to place explicit specializations of a templated static class data member outside of the class.

@alexfh What makes you say that? I think the resolution of CWG727 quite clearly permits class scope explicit specializations of function templates, class templates, and static data member templates.

@dwblaikie
Copy link
Collaborator

It looks like the presence of static on template variable specializations makes difference in the namespace context: https://gcc.godbolt.org/z/WGsreqbz8

Specifically, the specializations not marked static result in an exported variable. Thus, we have seemingly valid code that generates this diagnostic:

namespace A {
template <unsigned N>
static constexpr unsigned kMaxUnsignedInt = 2 * kMaxUnsignedInt<N - 1> + 1;

template <>
static constexpr unsigned kMaxUnsignedInt<1> = 1;
}

However, if we remove the static from the specialization, we're getting a potential ODR violation (considering this code can be in a header included into multiple translation units).

The right thing for this specific case is inline constexpr, but nevertheless, there seems to be an inconsistency now in how Clang handles the case now.

This seems to be pretty clearly a bug - in clang's specialization handling even prior to this patch.
GCC doesn't do this, it uses the storage class of the generic template for the specialization, which seems correct to me.

@sdkrystian are you interested in/able to fix this ^ ? https://gcc.godbolt.org/z/oPxKvvdeM

sdkrystian added a commit that referenced this pull request Jul 3, 2024
…mplates declared without 'static' as static data members when diagnosing uses of 'auto' (#97425)

After #93873 clang no longer permits declarations of explicit
specializations of static data member templates to use the `auto`
_placeholder-type-specifier_:
```
struct A {
  template<int N>
  static constexpr auto x = 0;

  template<>
  constexpr auto x<1> = 1; // error: 'auto' not allowed in non-static struct member
};
```
This patch fixes the issue.
lravenclaw pushed a commit to lravenclaw/llvm-project that referenced this pull request Jul 3, 2024
…ializations (llvm#96699)

With the recent fix for this situation in class members (llvm#93873) (for
which the fixed code is invalid prior to this patch - making migrating
code difficult as it must be in lock-step with the compiler migration,
if building with -Werror) it'd be really useful to be able to disable
this warning during the compiler migration/decouple the compiler
migration from the source fixes.

In theory this approach will regress the codebase to the previous
non-member cases of this issue that were already being held back by the
warning (as opposed to if we carved out the new cases into a separate
warning from the existing cases) but I think this'll be so rare and the
cleanup so simple, that the extra regressions of disabling the warning
broadly won't be too much of a problem. (but if folks disagree, I'm open
to making the warning more fine-grained)
kbluck pushed a commit to kbluck/llvm-project that referenced this pull request Jul 6, 2024
…mplates declared without 'static' as static data members when diagnosing uses of 'auto' (llvm#97425)

After llvm#93873 clang no longer permits declarations of explicit
specializations of static data member templates to use the `auto`
_placeholder-type-specifier_:
```
struct A {
  template<int N>
  static constexpr auto x = 0;

  template<>
  constexpr auto x<1> = 1; // error: 'auto' not allowed in non-static struct member
};
```
This patch fixes the issue.
AlexisPerry pushed a commit to llvm-project-tlp/llvm-project that referenced this pull request Jul 9, 2024
…h storage-class-specifiers (llvm#93873)

According to [temp.expl.spec] p2:
> The declaration in an _explicit-specialization_ shall not be an
_export-declaration_. An explicit specialization shall not use a
_storage-class-specifier_ other than `thread_local`.

Clang partially implements this, but a number of issues exist:
1. We don't diagnose class scope explicit specializations of variable
templates with _storage-class-specifiers_, e.g.
    ```
    struct A
    {
        template<typename T>
        static constexpr int x = 0;

        template<>
static constexpr int x<void> = 1; // ill-formed, but clang accepts
    };
    ````
2. We incorrectly reject class scope explicit specializations of
variable templates when `static` is not used, e.g.
    ```
    struct A
    {
        template<typename T>
        static constexpr int x = 0;

        template<>
constexpr int x<void> = 1; // error: non-static data member cannot be
constexpr; did you intend to make it static?
    };
    ````
3. We don't diagnose dependent class scope explicit specializations of
function templates with storage class specifiers, e.g.
    ```
    template<typename T>
    struct A
    {
        template<typename U>
        static void f();

        template<>
        static void f<int>(); // ill-formed, but clang accepts
    };
    ````

This patch addresses these issues as follows:
- # 1 is fixed by issuing a diagnostic when an explicit
specialization of a variable template has storage class specifier
- # 2 is fixed by considering any non-function declaration with any
template parameter lists at class scope to be a static data member. This
also allows for better error recovery (it's more likely the user
intended to declare a variable template than a "field template").
- # 3 is fixed by checking whether a function template explicit
specialization has a storage class specifier even when the primary
template is not yet known.

One thing to note is that it would be far simpler to diagnose this when
parsing the _decl-specifier-seq_, but such an implementation would
necessitate a refactor of `ParsedTemplateInfo` which I believe to be
outside the scope of this patch.
AlexisPerry pushed a commit to llvm-project-tlp/llvm-project that referenced this pull request Jul 9, 2024
…ializations (llvm#96699)

With the recent fix for this situation in class members (llvm#93873) (for
which the fixed code is invalid prior to this patch - making migrating
code difficult as it must be in lock-step with the compiler migration,
if building with -Werror) it'd be really useful to be able to disable
this warning during the compiler migration/decouple the compiler
migration from the source fixes.

In theory this approach will regress the codebase to the previous
non-member cases of this issue that were already being held back by the
warning (as opposed to if we carved out the new cases into a separate
warning from the existing cases) but I think this'll be so rare and the
cleanup so simple, that the extra regressions of disabling the warning
broadly won't be too much of a problem. (but if folks disagree, I'm open
to making the warning more fine-grained)
egorzhdan added a commit to swiftlang/swift that referenced this pull request Jul 23, 2024
… template specializations

This fixes a number of test failures in reverse C++ interop.

Clang's behavior was changed in llvm/llvm-project#93873, and it no longer accepts the C++ headers that Swift generates.

rdar://132283247
egorzhdan added a commit to swiftlang/swift that referenced this pull request Jul 23, 2024
… template specializations

This fixes a number of test failures in reverse C++ interop.

Clang's behavior was changed in llvm/llvm-project#93873, and it no longer accepts the C++ headers that Swift generates.

rdar://132283247
egorzhdan added a commit to swiftlang/swift that referenced this pull request Jul 24, 2024
… template specializations

This fixes a number of test failures in reverse C++ interop.

Clang's behavior was changed in llvm/llvm-project#93873, and it no longer accepts the C++ headers that Swift generates.

rdar://132283247
egorzhdan added a commit to swiftlang/swift that referenced this pull request Aug 9, 2024
…specializations

This fixes a number of test failures in reverse C++ interop.

Clang's behavior was changed in llvm/llvm-project#93873, and it no longer accepts the C++ headers that Swift generates.

rdar://132283247
searlmc1 pushed a commit to ROCm/llvm-project that referenced this pull request Sep 23, 2024
…h storage-class-specifiers (llvm#93873)

According to [temp.expl.spec] p2:
> The declaration in an _explicit-specialization_ shall not be an
_export-declaration_. An explicit specialization shall not use a
_storage-class-specifier_ other than `thread_local`.

Clang partially implements this, but a number of issues exist:
1. We don't diagnose class scope explicit specializations of variable
templates with _storage-class-specifiers_, e.g.
    ```
    struct A
    {
        template<typename T>
        static constexpr int x = 0;

        template<>
static constexpr int x<void> = 1; // ill-formed, but clang accepts
    };
    ````
2. We incorrectly reject class scope explicit specializations of
variable templates when `static` is not used, e.g.
    ```
    struct A
    {
        template<typename T>
        static constexpr int x = 0;

        template<>
constexpr int x<void> = 1; // error: non-static data member cannot be
constexpr; did you intend to make it static?
    };
    ````
3. We don't diagnose dependent class scope explicit specializations of
function templates with storage class specifiers, e.g.
    ```
    template<typename T>
    struct A
    {
        template<typename U>
        static void f();

        template<>
        static void f<int>(); // ill-formed, but clang accepts
    };
    ````

This patch addresses these issues as follows:
- # 1 is fixed by issuing a diagnostic when an explicit
specialization of a variable template has storage class specifier
- # 2 is fixed by considering any non-function declaration with any
template parameter lists at class scope to be a static data member. This
also allows for better error recovery (it's more likely the user
intended to declare a variable template than a "field template").
- # 3 is fixed by checking whether a function template explicit
specialization has a storage class specifier even when the primary
template is not yet known.

One thing to note is that it would be far simpler to diagnose this when
parsing the _decl-specifier-seq_, but such an implementation would
necessitate a refactor of `ParsedTemplateInfo` which I believe to be
outside the scope of this patch.

(cherry-picked from commit 9a88aa0)

Change-Id: I83a3158e50af6550db031c7b45d772bc9ceda487
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:modules C++20 modules and Clang Header Modules clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants