[Clang][Parser] Add a warning to ambiguous uses of T...[N] types #116332
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ck expansion prior to C++2c
zyn0217
added
the
clang:frontend
|
@llvm/pr-subscribers-clang Author: Younan Zhang (zyn0217) ChangesThis patch makes cases Fixes #115222 Full diff: https://github.com/llvm/llvm-project/pull/116332.diff 5 Files Affected:
diff --git a/clang/include/clang/Basic/DiagnosticParseKinds.td b/clang/include/clang/Basic/DiagnosticParseKinds.td
index 0da509280068ad..7b408f3eed5ba5 100644
--- a/clang/include/clang/Basic/DiagnosticParseKinds.td
+++ b/clang/include/clang/Basic/DiagnosticParseKinds.td
@@ -718,6 +718,11 @@ def warn_empty_init_statement : Warning<
"has no effect">, InGroup<EmptyInitStatement>, DefaultIgnore;
def err_keyword_as_parameter : Error <
"invalid parameter name: '%0' is a keyword">;
+def warn_pre_cxx26_ambiguous_pack_indexing_type : Warning<
+ "parameter packs without specifying a name would become a pack indexing "
+ "declaration in C++2c onwards">, InGroup<CXXPre26Compat>;
+def note_add_a_name_to_pre_cxx26_parameter_packs : Note<
+ "add a name to disambiguate">;
// C++ derived classes
def err_dup_virtual : Error<"duplicate 'virtual' in base specifier">;
diff --git a/clang/lib/Parse/ParseExprCXX.cpp b/clang/lib/Parse/ParseExprCXX.cpp
index ce3624f366a2a1..c596785b267b5b 100644
--- a/clang/lib/Parse/ParseExprCXX.cpp
+++ b/clang/lib/Parse/ParseExprCXX.cpp
@@ -242,7 +242,25 @@ bool Parser::ParseOptionalCXXScopeSpecifier(
SourceLocation Start = Tok.getLocation();
DeclSpec DS(AttrFactory);
SourceLocation CCLoc;
+ TentativeParsingAction MaybePackIndexing(*this, /*Unannotated=*/true);
SourceLocation EndLoc = ParsePackIndexingType(DS);
+ // C++ [cpp23.dcl.dcl-2]:
+ // Previously, T...[n] would declare a pack of function parameters.
+ // T...[n] is now a pack-index-specifier. [...] Valid C++ 2023 code that
+ // declares a pack of parameters without specifying a declarator-id
+ // becomes ill-formed.
+ if (!Tok.is(tok::coloncolon) && !getLangOpts().CPlusPlus26 &&
+ getCurScope()->isFunctionDeclarationScope()) {
+ Diag(DS.getEllipsisLoc(),
+ diag::warn_pre_cxx26_ambiguous_pack_indexing_type);
+ Diag(DS.getEllipsisLoc(),
+ diag::note_add_a_name_to_pre_cxx26_parameter_packs);
+ MaybePackIndexing.Revert();
+ return false;
+ }
+
+ MaybePackIndexing.Commit();
+
if (DS.getTypeSpecType() == DeclSpec::TST_error)
return false;
diff --git a/clang/test/CXX/dcl.decl/dcl.meaning/dcl.fct/p13.cpp b/clang/test/CXX/dcl.decl/dcl.meaning/dcl.fct/p13.cpp
index dbb6e60d9b93d7..0ae08da280b152 100644
--- a/clang/test/CXX/dcl.decl/dcl.meaning/dcl.fct/p13.cpp
+++ b/clang/test/CXX/dcl.decl/dcl.meaning/dcl.fct/p13.cpp
@@ -58,8 +58,9 @@ void b(T[] ...);
template<typename T>
void c(T ... []); // expected-error {{expected expression}} \
- // expected-error {{'T' does not refer to the name of a parameter pack}} \
- // expected-warning {{pack indexing is a C++2c extension}}
+ // expected-error {{type 'T[]' of function parameter pack does not contain any unexpanded parameter packs}} \
+ // expected-warning {{would become a pack indexing declaration in C++2c onwards}} \
+ // expected-note {{add a name to disambiguate}}
template<typename T>
void d(T ... x[]); // expected-error{{type 'T[]' of function parameter pack does not contain any unexpanded parameter packs}}
diff --git a/clang/test/Parser/cxx0x-decl.cpp b/clang/test/Parser/cxx0x-decl.cpp
index a0b3266c738ff5..91b97df459df92 100644
--- a/clang/test/Parser/cxx0x-decl.cpp
+++ b/clang/test/Parser/cxx0x-decl.cpp
@@ -214,12 +214,8 @@ struct MemberComponentOrder : Base {
void NoMissingSemicolonHere(struct S
[3]);
template<int ...N> void NoMissingSemicolonHereEither(struct S... [N]);
-// expected-error@-1 {{'S' does not refer to the name of a parameter pack}} \
-// expected-error@-1 {{declaration of anonymous struct must be a definition}} \
-// expected-error@-1 {{expected parameter declarator}} \
-// expected-error@-1 {{pack indexing is a C++2c extension}} \
-
-
+// expected-warning@-1 {{parameter packs without specifying a name would become a pack indexing declaration in C++2c onwards}} \
+// expected-note@-1 {{add a name to disambiguate}}
// This must be at the end of the file; we used to look ahead past the EOF token here.
// expected-error@+1 {{expected unqualified-id}} expected-error@+1{{expected ';'}}
diff --git a/clang/test/SemaCXX/cxx2c-pack-indexing-ext-diags.cpp b/clang/test/SemaCXX/cxx2c-pack-indexing-ext-diags.cpp
index 80dfeb9b6a8bf5..f4dbd5ca539635 100644
--- a/clang/test/SemaCXX/cxx2c-pack-indexing-ext-diags.cpp
+++ b/clang/test/SemaCXX/cxx2c-pack-indexing-ext-diags.cpp
@@ -19,3 +19,27 @@ void f(T... t) {
// cxx11-warning@+1 {{pack indexing is a C++2c extension}}
T...[0] c;
}
+
+template <typename... T>
+void g(T... [1]); // cxx11-warning {{parameter packs without specifying a name would become a pack indexing declaration in C++2c onwards}} \
+ // cxx11-note {{add a name to disambiguate}} \
+ // cxx26-warning {{pack indexing is incompatible with C++ standards before C++2c}} \
+ // cxx26-note {{candidate function template not viable}}
+
+template <typename... T>
+void h(T... param[1]);
+
+template <class T>
+struct S {
+ using type = T;
+};
+
+template <typename... T>
+void h(typename T... [1]::type); // cxx11-warning {{pack indexing is a C++2c extension}} \
+ // cxx26-warning {{pack indexing is incompatible with C++ standards before C++2c}}
+
+void call() {
+ g<int, double>(nullptr, nullptr); // cxx26-error {{no matching function for call to 'g'}}
+ h<int, double>(nullptr, nullptr);
+ h<S<int>, S<const char *>>("hello");
+}
|
|
I am not sure we want to keep supporting the old syntax, even in older language modes (notably, it isn't supported by other compilers and is at best missleading), but I'd like more people to chime in on that. I'm all for improving diagnostics though. |
|
I did think about this more, and given we made the feature an extension we should not have 2 different meaning in different contexts, so I think improving the diagnostic is really the only thing we should consider here |
|
We basically have the following options here:
(2) and (3) are basically removing the pack expansion parse from the C++23 grammar. Maybe we should ask the committee for a DR number to explicitly bless this approach. |
erichkeane
reviewed
Nov 18, 2024
| @@ -65,7 +64,7 @@ int main() { | |||
| } | |||
|
|
|||
|
|
|||
| namespace GH11460 { | |||
| namespace GH111460 { | |||
There was a problem hiding this comment.
That was a typo. The patch - 1ad5f31#diff-64f7b2e488768fb71e127d736dc81d9d817ec14b759a8ec23fab7416d70a31aeR68 - was actually fixing 111460
I think the standard has already decided on 3, that this construct is ill-formed in C++23. I think we should do a warning in C++98->C++20 that this going to break in the future, do an Error in C++23, and do the 'new' behavior in C++26. |
|
@erichkeane Nah, it's technically valid in C++23. There was no deprecate/remove period because how unused/useless that feature is. (just spell it T*) |
Ah, oof! Thank you for clarifying. That is really unfortunate. I don't have a great idea on how to handle the transition. Are we finding that we have uses of this? If so, there is perhaps value to a warning diagnostic that it is changing (and re-enabling the old behavior?). Else, I wonder if we are OK being non-conforming in Pre-C++26 mode and just leaving this as an error, despite being valid code. A diagnostic to say "we know we aren't conforming here, but this changes meaning, so give this an identifier" is perhaps acceptable there? I don't have a great idea here as far as what appetite we have for accepting/disallowing the older meaning. |
I think we should just remove it (maybe make it an error that can be downgraded to a warning for now and then a hard error in the next release? not sure that’s even necessary tho), because, candidly, it just seems categorically useless (and every other compiler seems to agree considering that they don’t support it to begin with). |
+1; if we get someone dinging us on the lack of C++23 conformance over this, then we'll have a better idea of what the actual use case(s) are we need to support. I suspect this will hit a few people, but not many and I'm pretty doubtful it will be in system headers. |
Yeah, I can live with this. I think a reasonable diagnostic series for C++23 and below (warning-as-default-error for now, hard error later?) is preferential, else lgtm. |
Unlikely
All of which factored into WG21 introducing this breaking change |
|
Thank you all for the feedback. So I'm going to keep it invalid and just add a warning for clarity. |
zyn0217
changed the title
|
Friendly ping |
cor3ntin
reviewed
Dec 12, 2024
clang/lib/Parse/ParseExprCXX.cpp
Outdated
Comment on lines
263
to
265
| // However, we still avoid parsing them as pack expansions because this is a | ||
| // rare use case of packs, despite being partway non-conforming, to ensure | ||
| // semantic consistency given that we have backported this feature. |
There was a problem hiding this comment.
This comment look inconsistent with what you are doing
cor3ntin
approved these changes
Dec 12, 2024
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/30/builds/12200 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/59/builds/9751 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/190/builds/11288 Here is the relevant piece of the build log for the reference |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
void f(T... [N])is no longer treated as a function with a parameter of pack expansion type after the implementation of the pack indexing feature. This patch introduces a warning to clarify such cases while maintaining it as a pack indexing type in all language modes.Fixes #115222