[clang][parser] Fix namespace dropping after malformed declarations

[alejandro-alvarez-sonarsource]

Summary

• After a malformed top-level declaration
• After a malformed templated class method declaration

In both cases, when there is a malformed declaration, any following namespace is dropped from the AST.
This can trigger a cascade of confusing diagnostics that may hide the original error. An example:

// Start #include "SomeFile.h"
template <class T>
void Foo<T>::Bar(void* aRawPtr) {
    (void)(aRawPtr);
}
// End #include "SomeFile.h"

#include <iostream>

int main() {}

We get the original error, plus 19 others from the standard library.
With this patch, we only get the original error.

clangd can also benefit from this patch, as namespaces following the malformed declaration is now preserved. i.e.

#pragma once

MACRO_FROM_MISSING_INCLUDE("X")

namespace my_namespace {
    //...
}

Before this patch, my_namespace is not visible for auto-completion.

[aaron.ballman]

Thank you for working on this! FWIW, I verified that the precommit CI failures are unrelated to this patch.

Can you add a release note to clang/docs/ReleaseNotes.rst about the fix?

clang/lib/Parse/ParseDecl.cpp

2178–2179

It'd help to update the comment below since it doesn't mention why namespaces are handled specially.

2308–2311

Changes for our coding style.

There seems to be some unfortunate interplay here though. Consider:

void bar() namespace foo { int i; }

int main() {
  foo::i = 12;
}

Calling SkipMalformedDecl() changes the behavior for this test case because we don't recover as well. With your patch applied, this gives us two diagnostics:

C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.cpp:1:11: error: expected ';' after top level declarator
void bar() namespace foo { int i; }
          ^
          ;
C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.cpp:4:3: error: use of undeclared identifier 'foo'
  foo::i = 12;
  ^
2 errors generated.

If the namespace keyword is on its own line, then we recover gracefully and don't emit the "use of undeclared identifier" warning.

While this is technically a regression in behavior for this test case, I think the overall changes are still an improvement. I suspect not a whole lot of code puts namespace somewhere other than the start of a line (same for inline namespace which has the same behavior with your patch).

clang/lib/Parse/ParseTemplate.cpp

278

We'll have the same sort of recovery issues here unless the namespace is on its own line, but that also seems like a highly unlikely scenario.

clang/test/Parser/cxx-namespace-after-missing-semicolon.cpp

2

clang/test/Parser/cxx-template-recovery.cpp

2

[alejandro-alvarez-sonarsource]

Applied suggested changes.

clang/lib/Parse/ParseDecl.cpp

2308–2311

Calling SkipMalformedDecl() changes the behavior for this test case because we don't recover as well. With your patch applied, this gives us two diagnostics:

True. This can also be recovered by removing Tok.isAtStartOfLine() in line 2050. However, this has been around for a long time and would change the behavior of
two tests inside test/Parser/recovery.cpp, although only because the broken comments contain the namespace keyword.

Either case seems unlikely to me, so I think I'd lean toward not modifying SkipMalformedDecl. What do you think?

[aaron.ballman]

LGTM! Do you need me to commit on your behalf? If so, what name and email address would you like used for patch attribution?

clang/lib/Parse/ParseDecl.cpp

2308–2311

I also lean towards not modifying SkipMalformedDecl(); I think it's recovery strategy is sensible.

[alejandro-alvarez-sonarsource]

In D150258#4337880, @aaron.ballman wrote:

LGTM! Do you need me to commit on your behalf? If so, what name and email address would you like used for patch attribution?

Thanks a lot! And yes, I would need you to commit. You can use

Name: Alejandro Álvarez Ayllón
Email: [e-mail]

clang/docs/ReleaseNotes.rst

- Fix handling of constexpr dynamic memory allocations in template
   arguments. (`#62462 <https://github.com/llvm/llvm-project/issues/62462>`_)
 - Some predefined expressions are now treated as string literals in MSVC
   compatibility mode.
   (`#114 <https://github.com/llvm/llvm-project/issues/114>`_)
 
 Bug Fixes to AST Handling
 ^^^^^^^^^^^^^^^^^^^^^^^^^
 
+- Preserve ``namespace`` definitions that follow malformed declarations.
+
 Miscellaneous Bug Fixes
 ^^^^^^^^^^^^^^^^^^^^^^^
 
 Miscellaneous Clang Crashes Fixed
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 - Dumping the AST to JSON no longer causes a failed assertion when targetting
   the Microsoft ABI and the AST to be dumped contains dependent names that

clang/lib/Parse/ParseDecl.cpp

if (!isDeclarationAfterDeclarator()) {
             // Recover by treating the 'typedef' as spurious.
             DS.ClearStorageClassSpecs();
           }
 
           Decl *TheDecl = ParseFunctionDefinition(D, ParsedTemplateInfo(),
                                                   &LateParsedAttrs);
           return Actions.ConvertDeclToDeclGroup(TheDecl);
         }
 
-        if (isDeclarationSpecifier(ImplicitTypenameContext::No)) {
-          // If there is an invalid declaration specifier right after the
-          // function prototype, then we must be in a missing semicolon case
-          // where this isn't actually a body.  Just fall through into the code
-          // that handles it as a prototype, and let the top-level code handle
-          // the erroneous declspec where it would otherwise expect a comma or
-          // semicolon.
+        if (isDeclarationSpecifier(ImplicitTypenameContext::No) ||
+            Tok.is(tok::kw_namespace)) {
+          // If there is an invalid declaration specifier or a namespace
+          // definition right after the function prototype, then we must be in a
+          // missing semicolon case where this isn't actually a body.  Just fall
+          // through into the code that handles it as a prototype, and let the
+          // top-level code handle the erroneous declspec where it would
+          // otherwise expect a comma or semicolon. Note that
+          // isDeclarationSpecifier already covers 'inline namespace', since
+          // 'inline' can be a declaration specifier.
         } else {
           Diag(Tok, diag::err_expected_fn_body);
           SkipUntil(tok::semi);
           return nullptr;
         }
       } else {
         if (Tok.is(tok::l_brace)) {
           Diag(Tok, diag::err_function_definition_not_allowed);
if (DeclEnd)
     *DeclEnd = Tok.getLocation();
 
   if (ExpectSemi && ExpectAndConsumeSemi(
                         Context == DeclaratorContext::File
                             ? diag::err_invalid_token_after_toplevel_declarator
                             : diag::err_expected_semi_declaration)) {
     // Okay, there was no semicolon and one was expected.  If we see a
     // declaration specifier, just assume it was missing and continue parsing.
     // Otherwise things are very confused and we skip to recover.
-    if (!isDeclarationSpecifier(ImplicitTypenameContext::No)) {
-      SkipUntil(tok::r_brace, StopAtSemi | StopBeforeMatch);
-      TryConsumeToken(tok::semi);
-    }
+    if (!isDeclarationSpecifier(ImplicitTypenameContext::No))
+      SkipMalformedDecl();
   }
 
   return Actions.FinalizeDeclaratorGroup(getCurScope(), DS, DeclsInGroup);
 }
 
 /// Parse an optional simple-asm-expr and attributes, and attach them to a
 /// declarator. Returns true on an error.
 bool Parser::ParseAsmAttributesAfterDeclarator(Declarator &D) {
   // If a simple-asm-expr is present, parse it.

clang/lib/Parse/ParseTemplate.cpp

Decl *Parser::ParseSingleDeclarationAfterTemplate(
 
   ParseDeclarator(DeclaratorInfo);
 
   if (IsTemplateSpecOrInst)
     SAC.done();
 
   // Error parsing the declarator?
   if (!DeclaratorInfo.hasName()) {
-    // If so, skip until the semi-colon or a }.
-    SkipUntil(tok::r_brace, StopAtSemi | StopBeforeMatch);
-    if (Tok.is(tok::semi))
-      ConsumeToken();
+    SkipMalformedDecl();
     return nullptr;
   }
 
   LateParsedAttrList LateParsedAttrs(true);
   if (DeclaratorInfo.isFunctionDeclarator()) {
     if (Tok.is(tok::kw_requires)) {
       CXXScopeSpec &ScopeSpec = DeclaratorInfo.getCXXScopeSpec();
       DeclaratorScopeObj DeclScopeObj(*this, ScopeSpec);

clang/test/Parser/cxx-namespace-after-missing-semicolon.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+UNKNOWN_MACRO_1("z", 1) // expected-error {{a type specifier is required for all declarations}}
+// expected-error@-1 {{expected ';' after top level declarator}}
+
+namespace foo {
+  class bar {};
+}
+
+int variable = 0; // ok
+foo::bar something; // ok
+
+UNKNOWN_MACRO_2(void) // expected-error {{a type specifier is required for all declarations}}
+// expected-error@-1 {{expected ';' after top level declarator}}
+
+namespace baz {
+  using Bool = bool;
+}
+
+int variable2 = 2; // ok
+const baz::Bool flag = false;  // ok

clang/test/Parser/cxx-template-recovery.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+template <class T>
+void Foo<T>::Bar(void* aRawPtr) { // expected-error {{no template named 'Foo'}}
+    (void)(aRawPtr);
+}
+
+namespace baz {
+  class klass {};
+}
+
+int *variable = 0; // ok
+const baz::klass object; // ok