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

[Haojian Wu via cfe-commits]

Author: Haojian Wu
Date: 2020-04-08T15:15:33+02:00
New Revision: 625acd8f6847a156b236b8f11b4b02b11cac3766

URL: https://github.com/llvm/llvm-project/commit/625acd8f6847a156b236b8f11b4b02b11cac3766
DIFF: https://github.com/llvm/llvm-project/commit/625acd8f6847a156b236b8f11b4b02b11cac3766.diff

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

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`.
```

Reviewers: sammccall

Reviewed By: sammccall

Subscribers: cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D77633

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

Modified: 
    clang/include/clang/Basic/DiagnosticParseKinds.td
    clang/lib/Parse/ParseDecl.cpp

Removed: 
    


################################################################################
diff  --git a/clang/include/clang/Basic/DiagnosticParseKinds.td b/clang/include/clang/Basic/DiagnosticParseKinds.td
index a1311d7776c6..29497f9c8296 100644
--- a/clang/include/clang/Basic/DiagnosticParseKinds.td
+++ b/clang/include/clang/Basic/DiagnosticParseKinds.td
@@ -591,6 +591,8 @@ def warn_cxx17_compat_for_range_init_stmt : Warning<
 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">;

diff  --git a/clang/lib/Parse/ParseDecl.cpp b/clang/lib/Parse/ParseDecl.cpp
index e60d1f68fba5..8bd7571f1242 100644
--- a/clang/lib/Parse/ParseDecl.cpp
+++ b/clang/lib/Parse/ParseDecl.cpp
@@ -6815,6 +6815,31 @@ void Parser::ParseParameterDeclarationClause(
           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);

diff  --git a/clang/test/Parser/cxx-keyword-identifiers.cpp b/clang/test/Parser/cxx-keyword-identifiers.cpp
new file mode 100644
index 000000000000..4a60ce9233f5
--- /dev/null
+++ b/clang/test/Parser/cxx-keyword-identifiers.cpp
@@ -0,0 +1,27 @@
+// 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 ';'}}
+  };
+}