[clang] [Clang] Don't treat a declaration that declares nothing as an invalid statement (PR #224682)

[Aaron Ballman via cfe-commits]

================
@@ -231,6 +231,11 @@ StmtResult Parser::ParseStatementOrDeclarationAfterAttributes(
                                    GNUAttrs.Range.getBegin());
       } else if (GNUAttrs.Range.getBegin().isValid())
         DeclStart = GNUAttrs.Range.getBegin();
+      // A declaration that declares nothing (`int;`) yields no Decl but still
+      // occupies the statement position; unlike a pragma, ParseStatement() must
+      // not skip it.
+      if (!Decl)
+        return Actions.ActOnNullStmt(PrevTokLocation);
----------------
AaronBallman wrote:

I'm not certain an empty `DeclStmt` will cover all of the scenarios because I don't think we create a `DeclStmt` for a field declaration, but that's another case we have to care about. I think this is a full list of the situations we need to handle if we want to retain an AST node for something which declares nothing: https://godbolt.org/z/qrfdffaY5

Personally, I think there are two reasonable ways forward:

1) Continue to not retain any AST node for these (note, the empty anon struct in C makes a `RecordDecl` and in C++ makes both a `CXXRecordDecl` and `VarDecl` already) and fix the issue in CodeGen for the narrow case in #215454
2) Create an empty `DeclStmt`, `FieldDecl`, etc in the AST and change all the fallout.

My preference is for (1) because I don't think the amount of changes for (2) are worth it for such a nominal language extension. In fact, someday I'd prefer for us to stop supporting this as an extension. In C++ mode it's pretty easy to do I think (GCC already covers this only under `-fpermissive` which Clang will never support, so we can already justify turning this into a warning which defaults to an error in C++). I suspect it's harder to do in C, but I also question how often this extension is used intentionally. Some searches show it does get turned off by a few hundred projects:

https://sourcegraph.com/search?q=context:global+-file:.*clang.*+-file:.*test.*+lang:Makefile+-Wno-missing-declarations&patternType=keyword&sm=0
https://sourcegraph.com/search?q=context:global+-file:.*clang.*+-file:.*test.*+lang:CMake+-Wno-missing-declarations&patternType=keyword&sm=0

but how much of that is cargo cult and the diagnostic never actually triggers vs intentionally creating declarations which declare nothing.

All that said, I'm not certain @efriedma-quic or others have a different take on the situation. The one thing I'm not comfortable with is switching these from no node to a `NullStmt` node; I think that's heading in the wrong direction because there is no `NullStmt` there. If we had a `RecoveryDecl` similar to `RecoveryExpr`, that might be more reasonable, but that's basically the same amount of churn as (2) I believe (if not more).

https://github.com/llvm/llvm-project/pull/224682