[clang][Sema] Preserve the initializer of invalid VarDecls

[HighCommander4]

Fixes clangd/clangd#1821

[mizvekov]

Thanks for the improvement!

Can you also add a test case?

[HighCommander4]

Thanks for the improvement!

Can you also add a test case?

Thanks for the feedback!

I added a new test case. However, the patch currently fails two existing test cases:

Clang :: Parser/cxx-class.cpp
Clang :: SemaCXX/cxx1z-init-statement.cpp

I will leave the PR as a Draft while I investigate these failures.

[HighCommander4]

I added a new test case. However, the patch currently fails two existing test cases:

Clang :: Parser/cxx-class.cpp
Clang :: SemaCXX/cxx1z-init-statement.cpp

I will leave the PR as a Draft while I investigate these failures.

The latest version of the patch fixes these failures.

I'm slightly uncertain about the change to JumpDiagnostics.cpp, whose purpose is to fix the cxx1z-init-statement.cpp test failure.

The affected code looks like this:

  switch (int x, y = 0; y) { // expected-note 2 {{previous definition is here}}
    case 0:
      int x = 0; // expected-error {{redefinition of 'x'}}
    case 1:
      int y = 0; // expected-error {{redefinition of 'y'}}
  }

and without the JumpDiagnostics.cpp change, we get an extra diagnostic:

error: cannot jump from switch statement to this case label
    5 |     case 1:
      |     ^
note: jump bypasses variable initialization
    4 |       int x = 0; // expected-error {{redefinition of 'x'}}
      |

but maybe that's actually fine and we should just update the test to expect this diagnostic instead?

[llvmbot]

@llvm/pr-subscribers-clang

Author: Nathan Ridge (HighCommander4)

Changes

Fixes clangd/clangd#1821

---

Full diff: https://github.com/llvm/llvm-project/pull/88645.diff

3 Files Affected:

• (modified) clang/lib/Sema/JumpDiagnostics.cpp (+2-1)
• (modified) clang/lib/Sema/SemaDecl.cpp (+16-5)
• (modified) clang/test/AST/ast-dump-recovery.cpp (+8)

diff --git a/clang/lib/Sema/JumpDiagnostics.cpp b/clang/lib/Sema/JumpDiagnostics.cpp
index ec3892e92f3c3b..27d5284fdb67e7 100644
--- a/clang/lib/Sema/JumpDiagnostics.cpp
+++ b/clang/lib/Sema/JumpDiagnostics.cpp
@@ -179,7 +179,8 @@ static ScopePair GetDiagForGotoScopeDecl(Sema &S, const Decl *D) {
     }
 
     const Expr *Init = VD->getInit();
-    if (S.Context.getLangOpts().CPlusPlus && VD->hasLocalStorage() && Init) {
+    if (S.Context.getLangOpts().CPlusPlus && VD->hasLocalStorage() && Init &&
+        !Init->containsErrors()) {
       // C++11 [stmt.dcl]p3:
       //   A program that jumps from a point where a variable with automatic
       //   storage duration is not in scope to a point where it is in scope
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 9fdd8eb236d1ee..6075624722b1cc 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -13435,16 +13435,18 @@ void Sema::checkNonTrivialCUnion(QualType QT, SourceLocation Loc,
 void Sema::AddInitializerToDecl(Decl *RealDecl, Expr *Init, bool DirectInit) {
   // If there is no declaration, there was an error parsing it.  Just ignore
   // the initializer.
-  if (!RealDecl || RealDecl->isInvalidDecl()) {
+  if (!RealDecl) {
     CorrectDelayedTyposInExpr(Init, dyn_cast_or_null<VarDecl>(RealDecl));
     return;
   }
 
   if (CXXMethodDecl *Method = dyn_cast<CXXMethodDecl>(RealDecl)) {
-    // Pure-specifiers are handled in ActOnPureSpecifier.
-    Diag(Method->getLocation(), diag::err_member_function_initialization)
-      << Method->getDeclName() << Init->getSourceRange();
-    Method->setInvalidDecl();
+    if (!Method->isInvalidDecl()) {
+      // Pure-specifiers are handled in ActOnPureSpecifier.
+      Diag(Method->getLocation(), diag::err_member_function_initialization)
+        << Method->getDeclName() << Init->getSourceRange();
+      Method->setInvalidDecl();
+    }
     return;
   }
 
@@ -13456,6 +13458,15 @@ void Sema::AddInitializerToDecl(Decl *RealDecl, Expr *Init, bool DirectInit) {
     return;
   }
 
+  if (VDecl->isInvalidDecl()) {
+    CorrectDelayedTyposInExpr(Init, VDecl);
+    ExprResult Recovery =
+        CreateRecoveryExpr(Init->getBeginLoc(), Init->getEndLoc(), {Init});
+    if (Expr *E = Recovery.get())
+      VDecl->setInit(E);
+    return;
+  }
+
   // WebAssembly tables can't be used to initialise a variable.
   if (Init && !Init->getType().isNull() &&
       Init->getType()->isWebAssemblyTableType()) {
diff --git a/clang/test/AST/ast-dump-recovery.cpp b/clang/test/AST/ast-dump-recovery.cpp
index cfb013585ad744..77527743fe8577 100644
--- a/clang/test/AST/ast-dump-recovery.cpp
+++ b/clang/test/AST/ast-dump-recovery.cpp
@@ -413,6 +413,14 @@ void RecoveryExprForInvalidDecls(Unknown InvalidDecl) {
   // CHECK-NEXT: `-RecoveryExpr {{.*}} '<dependent type>'
 }
 
+void InitializerOfInvalidDecl() {
+  int ValidDecl;
+  Unkown InvalidDecl = ValidDecl;
+  // CHECK:      VarDecl {{.*}} invalid InvalidDecl
+  // CHECK-NEXT: `-RecoveryExpr {{.*}} '<dependent type>' contains-errors
+  // CHECK-NEXT:   `-DeclRefExpr {{.*}} 'int' lvalue Var {{.*}} 'ValidDecl'
+}
+
 void RecoverToAnInvalidDecl() {
   Unknown* foo; // invalid decl
   goo; // the typo was correct to the invalid foo.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[HighCommander4]

@mizvekov does the updated patch look reasonable to you?

[mizvekov]

LGTM, Thanks!