[Clang][Parse] Diagnose member template declarations with multiple declarators #78243

sdkrystian · 2024-01-16T09:28:59Z

In a template-declaration, explicit specialization, or explicit instantiation the init-declarator-list in the declaration shall contain at most one declarator.

A member-declaration that is a template-declaration or explicit-specialization contains a declaration, even though it declares a member. This means it will contain an init-declarator-list (not a member-declarator-list), so [temp.pre] p5 applies.

This diagnoses declarations such as:

struct A
{
    template<typename T>
    static const int x = 0, f(); // error: a template declaration can only declare a single entity

    template<typename T>
    static const int g(), y = 0; // error: a template declaration can only declare a single entity
};

The diagnostic messages are the same as those of the equivalent namespace scope declarations.

Note: since we currently do not diagnose declarations with multiple abbreviated function template declarators at namespace scope e.g., void f(auto), g(auto);, I did not add diagnostics for the equivalent member declarations.

sdkrystian · 2024-01-16T09:29:18Z

Ping @erichkeane

llvmbot · 2024-01-16T09:29:27Z

@llvm/pr-subscribers-clang

Author: Krystian Stasiowski (sdkrystian)

Changes

According to [[temp.pre] p5](http://eel.is/c++draft/temp.pre#5):
> In a template-declaration, explicit specialization, or explicit instantiation the init-declarator-list in the declaration shall contain at most one declarator.

A member-declaration that is a template-declaration or explicit-specialization contains a declaration, even though it declares a member. This means it will contain an init-declarator-list (not a member-declarator-list), so [[temp.pre] p5](http://eel.is/c++draft/temp.pre#5) applies.

This diagnoses declarations such as:

struct A
{
    template&lt;typename T&gt;
    static const int x = 0, f(); // error: a template declaration can only declare a single entity

    template&lt;typename T&gt;
    static const int g(), y = 0; // error: a template declaration can only declare a single entity
};

The diagnostic messages are the same as those of the equivalent namespace scope declarations.

Note: since we currently do not diagnose declarations with multiple abbreviated function template declarators at namespace scope e.g., void f(auto), g(auto);, I did not add diagnostics for the equivalent member declarations.

Full diff: https://github.com/llvm/llvm-project/pull/78243.diff

4 Files Affected:

(modified) clang/docs/ReleaseNotes.rst (+1)
(modified) clang/lib/Parse/ParseDeclCXX.cpp (+9)
(modified) clang/test/CXX/temp/p3.cpp (+6)
(modified) clang/test/OpenMP/declare_simd_messages.cpp (+1-2)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index ea57769a4a5795..54debc4d40e945 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -555,6 +555,7 @@ Improvements to Clang's diagnostics
 - Clang now diagnoses unexpanded packs within the template argument lists of function template specializations.
 - Clang now diagnoses attempts to bind a bitfield to an NTTP of a reference type as erroneous
   converted constant expression and not as a reference to subobject.
+- Clang now diagnoses member template declarations with multiple declarators.
 
 
 Improvements to Clang's time-trace
diff --git a/clang/lib/Parse/ParseDeclCXX.cpp b/clang/lib/Parse/ParseDeclCXX.cpp
index 5576be9e717a9b..36a9ef199a9588 100644
--- a/clang/lib/Parse/ParseDeclCXX.cpp
+++ b/clang/lib/Parse/ParseDeclCXX.cpp
@@ -3167,6 +3167,15 @@ Parser::ParseCXXClassMemberDeclaration(AccessSpecifier AS,
 
     DeclaratorInfo.complete(ThisDecl);
 
+    if (TemplateInfo.Kind != ParsedTemplateInfo::NonTemplate) {
+      if (Tok.is(tok::comma)) {
+        Diag(Tok, diag::err_multiple_template_declarators)
+          << (int)TemplateInfo.Kind;
+        SkipUntil(tok::semi, StopBeforeMatch);
+      }
+      break;
+    }
+
     // If we don't have a comma, it is either the end of the list (a ';')
     // or an error, bail out.
     SourceLocation CommaLoc;
diff --git a/clang/test/CXX/temp/p3.cpp b/clang/test/CXX/temp/p3.cpp
index b708c613d352d2..9e561d0b9a83b2 100644
--- a/clang/test/CXX/temp/p3.cpp
+++ b/clang/test/CXX/temp/p3.cpp
@@ -15,3 +15,9 @@ template<typename T> struct B { } f(); // expected-error {{expected ';' after st
 template<typename T> struct C { } // expected-error {{expected ';' after struct}}
 
 A<int> c;
+
+struct D {
+  template<typename T> static const int x = 0, f(); // expected-error {{can only declare a single entity}}
+
+  template<typename T> static const int g(), y = 0; // expected-error {{can only declare a single entity}}
+};
diff --git a/clang/test/OpenMP/declare_simd_messages.cpp b/clang/test/OpenMP/declare_simd_messages.cpp
index dd24322694b69f..fea045400e1faf 100644
--- a/clang/test/OpenMP/declare_simd_messages.cpp
+++ b/clang/test/OpenMP/declare_simd_messages.cpp
@@ -33,10 +33,9 @@ int main();
 int main();
 
 struct A {
-// expected-error@+1 {{function declaration is expected after 'declare simd' directive}}
   #pragma omp declare simd
   template<typename T>
-  T infunc1(T a), infunc2(T a);
+  T infunc1(T a);
 };
 
 // expected-error@+1 {{single declaration is expected after 'declare simd' directive}}

github-actions · 2024-01-16T09:31:18Z

✅ With the latest revision this PR passed the C/C++ code formatter.

cor3ntin · 2024-01-16T13:02:37Z

clang/lib/Parse/ParseDeclCXX.cpp

+    if (TemplateInfo.Kind != ParsedTemplateInfo::NonTemplate) {
+      if (Tok.is(tok::comma)) {
+        Diag(Tok, diag::err_multiple_template_declarators)
+            << (int)TemplateInfo.Kind;
+        SkipUntil(tok::semi, StopBeforeMatch);
+      }
+      break;
+    }


I am not sure it make sense to do that in the parser.
Can we maybe to that in FinalizeDeclaratorGroup?
At that point we should have fully formed Decls and we can check whether they are templates

@cor3ntin We diagnose namespace scope template/explicit specialization/explicit instantiation declarations with multiple declarators in the parser as well... is there a reason not to?

The 'later' we do these sort of diagnostics, the better we are at recovering from them, and the better the diagnostics can be. IMO, if this is possible, putting this in FinalizeDeclaratorGroup makes a lot more sense here.

At least that way, we could recover and pretend one of those are the 'right' template that we intend to declare.

How about issuing the diagnostics in the parser, but instead of skipping to the semicolon we just continue parsing? That way the diagnostics are issued in lexical order when a declaration with multiple errors is encountered.

I added a commit which issues a diagnostic from the parser when a second declarator is encountered for templates, and then have ParseSingleDeclarationAfterTemplate call ParseDeclGroup... let me know if y'all think this approach is worth pursuing

sdkrystian · 2024-01-27T00:43:23Z

@erichkeane I have refactored ParseSingleDeclarationAfterTemplate (now named ParseDeclarationAfterTemplate) to call ParseDeclGroup and return the resultant DeclGroup.

With respect to moving the point of diagnosis to FinalizeDeclaratorGroup: as of now, the diagnostics are still issued from the parser (as it's simpler & results in diagnostics being emitted in lexical order). We can diagnosis this in FinalizeDeclaratorGroup if you still deem it necessary.

sdkrystian · 2024-01-30T16:07:51Z

Ping @cor3ntin

sdkrystian · 2024-02-01T15:38:48Z

Ping @erichkeane

erichkeane

I think this looks fine now... WDYT @cor3ntin ?

…clarators

…clarators (llvm#78243) According to [temp.pre] p5: > In a template-declaration, explicit specialization, or explicit instantiation the init-declarator-list in the declaration shall contain at most one declarator. A member-declaration that is a template-declaration or explicit-specialization contains a declaration, even though it declares a member. This means it _will_ contain an init-declarator-list (not a member-declarator-list), so [temp.pre] p5 applies. This diagnoses declarations such as: ``` struct A { template<typename T> static const int x = 0, f(); // error: a template declaration can only declare a single entity template<typename T> static const int g(), y = 0; // error: a template declaration can only declare a single entity }; ``` The diagnostic messages are the same as those of the equivalent namespace scope declarations. Note: since we currently do not diagnose declarations with multiple abbreviated function template declarators at namespace scope e.g., `void f(auto), g(auto);`, so this patch does not add diagnostics for the equivalent member declarations. This patch also refactors `ParseSingleDeclarationAfterTemplate` (now named `ParseDeclarationAfterTemplate`) to call `ParseDeclGroup` and return the resultant `DeclGroup`.

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:openmp OpenMP related changes to Clang labels Jan 16, 2024

sdkrystian force-pushed the multi-member-declarator branch from 44c582f to b5d814a Compare January 16, 2024 09:32

cor3ntin reviewed Jan 16, 2024

View reviewed changes

sdkrystian force-pushed the multi-member-declarator branch 3 times, most recently from f8ba2f5 to 27ec7b3 Compare January 18, 2024 16:26

sdkrystian force-pushed the multi-member-declarator branch from 27ec7b3 to f9f50a2 Compare January 27, 2024 00:28

sdkrystian force-pushed the multi-member-declarator branch from f9f50a2 to 35be507 Compare January 27, 2024 07:00

sdkrystian requested a review from cor3ntin January 29, 2024 14:49

sdkrystian force-pushed the multi-member-declarator branch from 35be507 to bba768b Compare January 30, 2024 19:13

erichkeane approved these changes Feb 1, 2024

View reviewed changes

cor3ntin approved these changes Feb 1, 2024

View reviewed changes

sdkrystian force-pushed the multi-member-declarator branch from bba768b to 945fdf5 Compare February 1, 2024 16:01

[Clang][Parse] Diagnose member template declarations with multiple de…

07999d2

…clarators

sdkrystian force-pushed the multi-member-declarator branch from 945fdf5 to 07999d2 Compare February 1, 2024 16:02

sdkrystian merged commit c5f4619 into llvm:main Feb 1, 2024

sdkrystian deleted the multi-member-declarator branch February 26, 2024 15:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Clang][Parse] Diagnose member template declarations with multiple declarators #78243

[Clang][Parse] Diagnose member template declarations with multiple declarators #78243

Uh oh!

sdkrystian commented Jan 16, 2024

Uh oh!

sdkrystian commented Jan 16, 2024

Uh oh!

llvmbot commented Jan 16, 2024

Uh oh!

github-actions bot commented Jan 16, 2024 •

edited

Loading

Uh oh!

cor3ntin Jan 16, 2024

Uh oh!

sdkrystian Jan 16, 2024

Uh oh!

erichkeane Jan 16, 2024

Uh oh!

sdkrystian Jan 16, 2024

Uh oh!

sdkrystian Jan 16, 2024 •

edited

Loading

Uh oh!

sdkrystian commented Jan 27, 2024

Uh oh!

sdkrystian commented Jan 30, 2024

Uh oh!

sdkrystian commented Feb 1, 2024

Uh oh!

erichkeane left a comment

Uh oh!

Uh oh!

[Clang][Parse] Diagnose member template declarations with multiple declarators #78243

[Clang][Parse] Diagnose member template declarations with multiple declarators #78243

Uh oh!

Conversation

sdkrystian commented Jan 16, 2024

Uh oh!

sdkrystian commented Jan 16, 2024

Uh oh!

llvmbot commented Jan 16, 2024

Uh oh!

github-actions bot commented Jan 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cor3ntin Jan 16, 2024

Choose a reason for hiding this comment

Uh oh!

sdkrystian Jan 16, 2024

Choose a reason for hiding this comment

Uh oh!

erichkeane Jan 16, 2024

Choose a reason for hiding this comment

Uh oh!

sdkrystian Jan 16, 2024

Choose a reason for hiding this comment

Uh oh!

sdkrystian Jan 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdkrystian commented Jan 27, 2024

Uh oh!

sdkrystian commented Jan 30, 2024

Uh oh!

sdkrystian commented Feb 1, 2024

Uh oh!

erichkeane left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Jan 16, 2024 •

edited

Loading

sdkrystian Jan 16, 2024 •

edited

Loading