Assertion !isNull() && "Cannot retrieve a NULL type pointer" with expansion statement

[akash-manna-sky]

Fixes #212630

The parser applied MaybeCreateExprWithCleanups to the range-init of a template for in both forms. For an expansion-init-list that's wrong: {g(1), g(2), g(3)} is a purely syntactic InitListExpr with no type, so when an element needed cleanups (here a temporary bound to const int&; a non-trivially destructible temporary does it too) the list got wrapped in a typeless ExprWithCleanups. ActOnCXXExpansionStmtPattern then didn't see an InitListExpr anymore, went down the non-enumerating path and hit the !isNull() assertion. g being a function parameter is incidental.

The elements are only ever evaluated as the initializer of the expansion variable in each expansion, where they're rebuilt and get their own ExprWithCleanups, so the parser now discards those cleanups instead of wrapping the list. BuildCXXExpansionSelectExpr does the same when it creates the dependent select expression, otherwise the same thing happens during instantiation of a pattern with a pack ({h(1), ts...}): the rebuilt elements set the cleanup flag and AddInitializerToDecl wraps the select expression, which HasDependentSize casts directly. The test covers both, plus function pointer/reference, member and overloaded calls.

LLM tools were used for this contribution. I've reviewed, built, and tested the change myself before pushing to GitHub.

[llvmorg-github-actions]

@llvm/pr-subscribers-clang

Author: Akash Manna (akash-manna-sky)

Changes

Fixes #212630

The parser applied MaybeCreateExprWithCleanups to the range-init of a template for in both forms. For an expansion-init-list that's wrong: {g(1), g(2), g(3)} is a purely syntactic InitListExpr with no type, so when an element needed cleanups (here a temporary bound to const int&; a non-trivially destructible temporary does it too) the list got wrapped in a typeless ExprWithCleanups. ActOnCXXExpansionStmtPattern then didn't see an InitListExpr anymore, went down the non-enumerating path and hit the !isNull() assertion. g being a function parameter is incidental.

The elements are only ever evaluated as the initializer of the expansion variable in each expansion, where they're rebuilt and get their own ExprWithCleanups, so the parser now discards those cleanups instead of wrapping the list. BuildCXXExpansionSelectExpr does the same when it creates the dependent select expression, otherwise the same thing happens during instantiation of a pattern with a pack ({h(1), ts...}): the rebuilt elements set the cleanup flag and AddInitializerToDecl wraps the select expression, which HasDependentSize casts directly. The test covers both, plus function pointer/reference, member and overloaded calls.

LLM tools were used for this contribution. I've reviewed, built, and tested the change myself before pushing to GitHub.

---

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

4 Files Affected:

• (modified) clang/docs/ReleaseNotes.md (+6)
• (modified) clang/lib/Parse/ParseStmt.cpp (+8-3)
• (modified) clang/lib/Sema/SemaExpand.cpp (+5-1)
• (added) clang/test/SemaTemplate/GH212630.cpp (+49)

diff --git a/clang/docs/ReleaseNotes.md b/clang/docs/ReleaseNotes.md
index 3c6694f510952..c49428a2a95d0 100644
--- a/clang/docs/ReleaseNotes.md
+++ b/clang/docs/ReleaseNotes.md
@@ -531,6 +531,12 @@ features cannot lower the translation-unit ABI level;
   parameter that follows a parameter pack (e.g.
   `template <typename... T> S::S(T..., int = 10) {}`).  (#GH216211)
 
+- Fixed an assertion failure in an enumerating expansion statement
+  (`template for`) when an element of the expansion-init-list needed cleanups,
+  e.g. a temporary bound to a reference parameter such as `{g(1), g(2)}` with
+  `int g(const int&)`, or a temporary of a type with a non-trivial destructor.
+  (#GH212630)
+
 #### Bug Fixes to AST Handling
 
 - Fixed a non-deterministic ordering of unused local typedefs that made
diff --git a/clang/lib/Parse/ParseStmt.cpp b/clang/lib/Parse/ParseStmt.cpp
index 219bcd980e860..9f5a37e840c1b 100644
--- a/clang/lib/Parse/ParseStmt.cpp
+++ b/clang/lib/Parse/ParseStmt.cpp
@@ -1965,9 +1965,14 @@ void Parser::ParseForRangeInitializerAfterColon(ForRangeInit &FRI,
     assert(Actions.CurContext->isExpansionStmt());
     Sema::ContextRAII CtxGuard(Actions, Actions.CurContext->getParent(),
                                /*NewThis=*/false);
-    FRI.RangeExpr =
-        Tok.is(tok::l_brace) ? ParseExpansionInitList() : ParseExpression();
-    FRI.RangeExpr = Actions.MaybeCreateExprWithCleanups(FRI.RangeExpr);
+    if (Tok.is(tok::l_brace)) {
+      // The elements are only evaluated as the initializer of the expansion
+      // variable in each expansion, so their cleanups belong there.
+      FRI.RangeExpr = ParseExpansionInitList();
+      Actions.DiscardCleanupsInEvaluationContext();
+    } else {
+      FRI.RangeExpr = Actions.MaybeCreateExprWithCleanups(ParseExpression());
+    }
   } else if (Tok.is(tok::l_brace)) {
     FRI.RangeExpr = ParseBraceInitializer();
   } else {
diff --git a/clang/lib/Sema/SemaExpand.cpp b/clang/lib/Sema/SemaExpand.cpp
index 779b7add08344..77e7cb282c5a0 100644
--- a/clang/lib/Sema/SemaExpand.cpp
+++ b/clang/lib/Sema/SemaExpand.cpp
@@ -589,8 +589,12 @@ StmtResult Sema::FinishCXXExpansionStmt(Stmt *Exp, Stmt *Body) {
 }
 
 ExprResult Sema::BuildCXXExpansionSelectExpr(InitListExpr *Range, Expr *Idx) {
-  if (Idx->isValueDependent() || InitListContainsPack(Range))
+  if (Idx->isValueDependent() || InitListContainsPack(Range)) {
+    // The elements are only evaluated by the expansion that selects them, so
+    // their cleanups must not wrap this expression.
+    DiscardCleanupsInEvaluationContext();
     return new (Context) CXXExpansionSelectExpr(Context, Range, Idx);
+  }
 
   // The index is a DRE to a template parameter; we should never
   // fail to evaluate it.
diff --git a/clang/test/SemaTemplate/GH212630.cpp b/clang/test/SemaTemplate/GH212630.cpp
new file mode 100644
index 0000000000000..aab9d9d7fd5ad
--- /dev/null
+++ b/clang/test/SemaTemplate/GH212630.cpp
@@ -0,0 +1,49 @@
+// RUN: %clang_cc1 -std=c++26 -fsyntax-only -verify %s
+// expected-no-diagnostics
+
+namespace GH212630 {
+
+void f(int g(const int&)) {
+  template for (auto x : {g(1), g(2), g(3)})
+    g(0);
+}
+
+struct M {
+  int m(const int &x) const { return x; }
+};
+
+int overloaded(const int &);
+long overloaded(const long &);
+
+void related(int (*fp)(const int &), int (&fr)(const int &), M m) {
+  template for (auto x : {fp(1), fr(2), m.m(3), overloaded(4), overloaded(5L)}) {}
+}
+
+constexpr int h(const int &x) { return x * 2; }
+
+struct S {
+  int v;
+  constexpr S(int v) : v(v) {}
+  constexpr ~S() {}
+};
+
+constexpr int direct() {
+  int sum = 0;
+  template for (auto x : {h(1), h(2), h(3)}) { sum += x; }
+  template for (constexpr auto x : {h(1), h(2), h(3)}) { sum += x; }
+  template for (auto s : {S(1), S(2)}) { sum += s.v; }
+  return sum;
+}
+static_assert(direct() == 27);
+
+// With a pack, the elements are rebuilt when the template is instantiated.
+template <typename... Ts>
+constexpr int pack(Ts... ts) {
+  int sum = 0;
+  template for (auto x : {h(1), h(ts)...}) { sum += x; }
+  template for (auto s : {S(ts)...}) { sum += s.v; }
+  return sum;
+}
+static_assert(pack(2, 3) == 17);
+
+} // namespace GH212630

[Sirraide]

So this is fixing a symptom, not the underlying problem. The actual issue here is that we need to apply lifetime extension to each element in the expansion-init-list individually. This is possible, but it requires quite a bit of refactoring to thread that through sema and template instantiation.

From what I can tell, once we support lifetime extension in enumerating expansion statements, we should never end up with any pending cleanups after a call to ParseExpansionInitList()