[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

6897

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

13

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

6897

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

13

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_parameter : Error <
+  "invalid parameter name: '%0' is a keyword">;
 
 // 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

if (DS.isEmpty() && ParmDeclarator.getIdentifier() == nullptr &&
       // parameter pack declaration.
       if (Tok.is(tok::ellipsis) &&
           (NextToken().isNot(tok::r_paren) ||
            (!ParmDeclarator.getEllipsisLoc().isValid() &&
             !Actions.isUnexpandedParameterPackPermitted())) &&
           Actions.containsUnexpandedParameterPacks(ParmDeclarator))
         DiagnoseMisplacedEllipsisInDeclarator(ConsumeToken(), ParmDeclarator);
 
+      // Now we are at the point where declarator parsing is finished.
+      //
+      // Try to catch keywords in place of the identifier in a declarator, and
+      // in particular the common case where:
+      //   1 identifier comes at the end of the declarator
+      //   2 if the identifier is dropped, the declarator is valid but anonymous
+      //     (no identifier)
+      //   3 declarator parsing succeeds, and then we have a trailing keyword,
+      //     which is never valid in a param list (e.g. missing a ',')
+      // 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). ParseDeclarator is just
+      // treating this as an anonymous parameter, and fortunately at this point
+      // we've already almost done that.
+      //
+      // We care about case 1) where the declarator type should be known, and
+      // the identifier should be null.
+      if (!ParmDeclarator.isInvalidType() && !ParmDeclarator.hasName()) {
+        if (Tok.getIdentifierInfo() &&
+            Tok.getIdentifierInfo()->isKeyword(getLangOpts())) {
+          Diag(Tok, diag::err_keyword_as_parameter) << PP.getSpelling(Tok);
+          // Consume the keyword.
+          ConsumeToken();
+        }
+      }
       // Inform the actions module about the parameter declarator, so it gets
       // added to the current scope.
       Decl *Param = Actions.ActOnParamDeclarator(getCurScope(), ParmDeclarator);
       // Parse the default argument, if any. We parse the default
       // arguments in all dialects; the semantic analysis in
       // ActOnParamDefaultArgument will reject the default argument in
       // C.
       if (Tok.is(tok::equal)) {
if (DS.isEmpty() && ParmDeclarator.getIdentifier() == nullptr &&
           } else {
             // Inform the actions module about the default argument
             Actions.ActOnParamDefaultArgument(Param, EqualLoc,
                                               DefArgResult.get());
           }
         }
       }
 
       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
         // in C. Complain and provide the fix.

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

+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -verify %s
+
+
+int foo1(int case, int throw, int y) { // expected-error {{invalid parameter name: 'case' is a keyword}} \
+                                          expected-error {{invalid}}
+  // Trailing parameters should be recovered.
+  y = 1;
+}
+
+int foo2(int case = 1); // expected-error {{invalid parameter}}
+int foo3(int const); // ok: without parameter name.
+// ok: override has special meaning when used after method functions. it can be
+// used as name.
+int foo4(int override);
+int foo5(int x const); // expected-error {{expected ')'}} expected-note {{to match this '('}}
+// FIXME: bad recovery on the case below, "invalid parameter" is desired, the
+// followon diagnostics should be suppressed.
+int foo6(int case __attribute((weak))); // expected-error {{invalid parameter}}  \
+                                        // expected-error {{expected ')'}} expected-note {{to match this '('}}
+
+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 ';'}}
+  };
+}