[clang][AST] Invalidate DecompositionDecl if it has invalid initializer.

[hokein]

Fix #67495, #72198

We build ill-formed AST nodes for invalid structured binding. For case int [_, b] = {0, 0};, the DecompositionDecl is valid, and its children BindingDecls are valid but with a NULL type, this breaks clang invariants in many places, and using these BindingDecls can lead to crashes. This patch fixes them by marking the DecompositionDecl and its children invalid.

CC @ADKaster, @ecnelises

[llvmbot]

@llvm/pr-subscribers-clang

Author: Haojian Wu (hokein)

Changes

Fix #67495, #72198

CC @ADKaster, @ecnelises

---

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

2 Files Affected:

• (modified) clang/lib/Sema/SemaDecl.cpp (+9)
• (modified) clang/test/AST/ast-dump-invalid-initialized.cpp (+14-1)

diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 3876eb501083acb..b89031425e4b5ac 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -13540,6 +13540,15 @@ void Sema::AddInitializerToDecl(Decl *RealDecl, Expr *Init, bool DirectInit) {
           CreateRecoveryExpr(Init->getBeginLoc(), Init->getEndLoc(), Args);
       if (RecoveryExpr.get())
         VDecl->setInit(RecoveryExpr.get());
+      // In general, for error recovery purposes, the initalizer doesn't play
+      // part in the valid bit of the declaration. There are a few exceptions:
+      //  1) if the var decl has a deduced auto type, and the type cannot be
+      //     deduced by an invalid initializer;
+      //  2) if the var decl is decompsition decl with a concrete type (e.g.
+      //    `int [a, b] = 1;`), and the initializer is invalid;
+      // Case 1) is already handled earlier in this function.
+      if (llvm::isa<DecompositionDecl>(VDecl)) // Case 2)
+        VDecl->setInvalidDecl();
       return;
     }
 
diff --git a/clang/test/AST/ast-dump-invalid-initialized.cpp b/clang/test/AST/ast-dump-invalid-initialized.cpp
index 1c374ae716a9db5..a71a02f0f60039e 100644
--- a/clang/test/AST/ast-dump-invalid-initialized.cpp
+++ b/clang/test/AST/ast-dump-invalid-initialized.cpp
@@ -24,4 +24,17 @@ void test() {
   auto b4 = A(1);
   // CHECK: `-VarDecl {{.*}} invalid b5 'auto'
   auto b5 = A{1};
-}
\ No newline at end of file
+}
+
+void pr72198() {
+  // CHECK: DecompositionDecl {{.*}} invalid 'int'
+  int [_, b] = {0, 0};
+  [b]{};
+}
+
+int get_point();
+void pr67495() {
+  // CHECK: DecompositionDecl {{.*}} invalid 'int &'
+  auto& [x, y] = get_point();
+  [x, y] {};
+}

[hnrklssn]

What differentiates the DecompositionDecl such that we need to mark the decl invalid when there's an error in the RHS, while for other decls we don't? It seems inconsistent.

[cor3ntin]

Maybe we should move the 1) part of the comment line 13496, or just remove it entirely.

[shafik]

We should have the comment in both places. This is very helpful to understanding the code.

I agree that having the comment far away from the code that it is commenting on is less useful. The code could easily be moved in a refactor or in other ways and become completely disconnected.

[hokein]

I think this comment is helpful (especially the context is not obvious here). I'd like to keep it.

The code handling the case 1) is https://github.com/llvm/llvm-project/blob/1c05fe350064aa3a1784bb09829a07d501842d97/clang/lib/Sema/SemaDecl.cpp#L13363C3-L13384, and it is easier to understand the purpose by reading the code. I'd avoid having two similar comments in the codebase.

[hokein]

What differentiates the DecompositionDecl such that we need to mark the decl invalid when there's an error in the RHS, while for other decls we don't? It seems inconsistent.

DecompositionDecl (aka structure binding) is special here, a legal DecompositionDecl must be declared with a deduced auto type, so the its initializer should play part of the decl's invalid bit. For the error case where a DecompositionDecl with a non-deduced type, clang still builds the Decl AST node (with invalid bit) for better recovery. There are some oversight cases. This patch is fixing those.

[hnrklssn]

What differentiates the DecompositionDecl such that we need to mark the decl invalid when there's an error in the RHS, while for other decls we don't? It seems inconsistent.

DecompositionDecl (aka structure binding) is special here, a legal DecompositionDecl must be declared with a deduced auto type, so the its initializer should play part of the decl's invalid bit. For the error case where a DecompositionDecl with a non-deduced type, clang still builds the Decl AST node (with invalid bit) for better recovery. There are some oversight cases. This patch is fixing those.

Ah that makes sense then. I didn't realise it had to be auto.

[hokein]

Friendly ping.

[shafik]

We need a release note and please add a more detailed summary. A description of the problem being solved and the solution to the fix provides.

[hokein]

We need a release note and please add a more detailed summary. A description of the problem being solved and the solution to the fix provides.

Done.

[hokein]

Friendly ping.

[AaronBallman]

LGTM with a tiny nit.