[Parser] Improve diagnostic and error recovery when C++ keywords are used as identifiers.

[hokein]

Summary

Previously, clang emitted a less-usefull diagnostic and didnt recover
well when the keywords is used as identifier in function paramter.

void foo(int case, int x); // previously we drop all parameters after
`int case`.

[sammccall]

clang/lib/Parse/ParseDecl.cpp

6872

As I understand it, we're trying to catch keywords in place of the identifier in a declarator, and in particular the common case where:

• the identifier comes at the end of the declarator
• if the identifier is dropped, the declarator is valid but anonymous (no identifier)
• so declarator parsing succeeds, and then we have a trailing keyword, which is never valid in a param list (need , or ... or ) )

And we can't handle this in ParseDeclarator because in general keywords may be allowed to follow the declarator. (And in some cases there'd be better recovery like inserting punctuation).
Our recovery is just treating this as an anonymous parameter, and fortunately we've already almost done that.

So all of this would be good to capture in the comment :-)

We should restrict to these cases as much as possible: the type should be present, and the identifier should be null[1].

I don't know how confident we can be all the time that our interpretation (the keyword was meant to be a variable) is right, and the diagnostic might be confusing in that case.
e.g. void foo(int const) will trigger this diagnostic, and the user may intend anonymous void foo(const int). using keyword const as an identifier is not permitted may be less clear than e.g. the more specific invalid parameter name: 'const' is a keyword.

(Note that because of the [1] case, as the patch stands void foo(int x const) will trigger using keyword const as an identifier..., which is bad!)

clang/test/Parser/cxx-keyword-identifiers.cpp

12

this seems nice but harder, I think the grammar is more permissive here. I guess we'd be able to whitelist the keywords that can legitimately follow?

[hokein]

address review comments

• refine the diagnostic messages
• add more testcases
• more restrict to the diagnosed case

[hokein]

clang/lib/Parse/ParseDecl.cpp

6872

yes, exactly, added comments. I first tried to handle it in ParseDeclarator, but failed with that.

And void foo(int const) would not trigger the diagnostic -- this is because it is a valid declarator, the identifier is dropped, case[2].

by restricting the case (only emit the diagnostic when the type is known, and identifier is null), void foo(int x const) will not trigger the diagnostic -- the type and identifier are known.

clang/test/Parser/cxx-keyword-identifiers.cpp

12

yeah, I think at least we could append a message to the expected-unqualified-id when the next Tok is a keyword.

18

unfortunately, the error recovery doesn't handle well for this case, we emit a spurious diagnostic, but I think it is a fair tradeoff.

[sammccall]

This is fairly visible, but seems reasonably safe to me. Let's see if anyone complains :-)

clang/test/Parser/cxx-keyword-identifiers.cpp

17

can you add a comment like FIXME: bad recovery?

(So future readers/modifiers can know which tests are establishing desirable behavior vs documenting current accidental behavior)

Actually I think this is better than the old behaviour (but ideally we'd suppress the followon diagnostic)

[hokein]

add FIXME.

[jdoerfert]

This is cool, thx!

One case I stumbled upon recently is:
void explicit() {}

Which results in these rather confusing messages:

/tmp/test.cpp:1:14: warning: explicit(bool) is a C++20 extension [-Wc++20-extensions]
void explicit() { }
             ^
/tmp/test.cpp:1:15: error: expected expression
void explicit() { }
              ^
/tmp/test.cpp:1:17: error: expected unqualified-id
void explicit() { }
                ^
1 warning and 2 errors generated.

Just in case you want to continue this ;)

clang/include/clang/Basic/DiagnosticParseKinds.td

def ext_for_range_init_stmt : ExtWarn<
   "range-based for loop initialization statements are a C++20 extension">,
   InGroup<CXX20>;
 def warn_cxx17_compat_for_range_init_stmt : Warning<
   "range-based for loop initialization statements are incompatible with "
   "C++ standards before C++20">, DefaultIgnore, InGroup<CXXPre20Compat>;
 def warn_empty_init_statement : Warning<
   "empty initialization statement of '%select{if|switch|range-based for}0' "
   "has no effect">, InGroup<EmptyInitStatement>, DefaultIgnore;
+def err_keyword_as_ident : Error <
+  "using keyword '%0' as an identifier is not permitted">;
 
 // C++ derived classes
 def err_dup_virtual : Error<"duplicate 'virtual' in base specifier">;
 
 // C++ operator overloading
 def err_literal_operator_string_prefix : Error<
   "string literal after 'operator' cannot have an encoding prefix">;
 def err_literal_operator_string_not_empty : Error<

clang/lib/Parse/ParseDecl.cpp

 //===--- ParseDecl.cpp - Declaration Parsing --------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
if (DS.isEmpty() && ParmDeclarator.getIdentifier() == nullptr &&
           } else {
             // Inform the actions module about the default argument
             Actions.ActOnParamDefaultArgument(Param, EqualLoc,
                                               DefArgResult.get());
           }
         }
       }
 
+      // Recovery if a keyword is used as an identifier.
+      if (Tok.getIdentifierInfo() &&
+          Tok.getIdentifierInfo()->isKeyword(getLangOpts())) {
+        Diag(Tok, diag::err_keyword_as_ident) << PP.getSpelling(Tok);
+        // Consume the keyword.
+        ConsumeToken();
+      }
       ParamInfo.push_back(DeclaratorChunk::ParamInfo(ParmII,
                                           ParmDeclarator.getIdentifierLoc(),
                                           Param, std::move(DefArgToks)));
     }
 
     if (TryConsumeToken(tok::ellipsis, EllipsisLoc)) {
       if (!getLangOpts().CPlusPlus) {
         // We have ellipsis without a preceding ',', which is ill-formed

clang/test/Parser/cxx-keyword-identifiers.cpp

+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -verify %s
+
+
+int foo(int case, int throw, int y) { // expected-error {{using keyword 'case' as an identifier is not permitted}} \
+                                         expected-error {{using keyword 'throw' as}}
+  // Trailing parameters should be recovered.
+  y = 1;
+}
+
+void test() {
+  // FIXME: we shoud improve the dianostics for the following cases.
+  int case; // expected-error {{expected unqualified-id}}
+  struct X {
+    int case; // expected-error {{expected member name or ';'}}
+  };
+}