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

[Haojian Wu via Phabricator via cfe-commits]

hokein marked 2 inline comments as done.
hokein added inline comments.


================
Comment at: clang/lib/Parse/ParseDecl.cpp:6872
 
+      // Recovery if a keyword is used as an identifier.
+      if (Tok.getIdentifierInfo() &&
----------------
sammccall wrote:
> 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!)
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. 



================
Comment at: clang/test/Parser/cxx-keyword-identifiers.cpp:12
+  // FIXME: we shoud improve the dianostics for the following cases.
+  int case; // expected-error {{expected unqualified-id}}
+  struct X {
----------------
sammccall wrote:
> 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?
yeah, I think at least we could append a message to the `expected-unqualified-id` when the next Tok is a keyword.


================
Comment at: clang/test/Parser/cxx-keyword-identifiers.cpp:17
+int foo6(int case __attribute((weak))); // expected-error {{invalid parameter}}  \
+                                        // expected-error {{expected ')'}} expected-note {{to match this '('}}
+
----------------
unfortunately, the error recovery doesn't handle well for this case, we emit a spurious diagnostic, but I think it is a fair tradeoff.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77633/new/

https://reviews.llvm.org/D77633