[Clang] Handle ?: operator in fold expression

[azmat-y]

Emit diagnostic when the fold operand produces a RecoveryExpr. This solves #162198 @shafik

[llvmbot]

@llvm/pr-subscribers-clang

Author: Azmat Yusuf (azmat-y)

Changes

Emit diagnostic when the fold operand produces a RecoveryExpr. This solves #162198 @shafik

---

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

3 Files Affected:

• (modified) clang/lib/Sema/SemaExpr.cpp (+3)
• (modified) clang/lib/Sema/SemaTemplateVariadic.cpp (+1-1)
• (modified) clang/test/SemaCXX/fold_expr_typo.cpp (+5)

diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index a50c27610dc96..5089a64e85316 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -8966,6 +8966,9 @@ ExprResult Sema::ActOnConditionalOp(SourceLocation QuestionLoc,
       commonExpr = MatExpr.get();
     }
 
+    if (commonExpr->getDependence() & ExprDependence::UnexpandedPack)
+      return ExprError();
+
     opaqueValue = new (Context) OpaqueValueExpr(commonExpr->getExprLoc(),
                                                 commonExpr->getType(),
                                                 commonExpr->getValueKind(),
diff --git a/clang/lib/Sema/SemaTemplateVariadic.cpp b/clang/lib/Sema/SemaTemplateVariadic.cpp
index 0f72d6a13ae06..7f9f85e1a3c32 100644
--- a/clang/lib/Sema/SemaTemplateVariadic.cpp
+++ b/clang/lib/Sema/SemaTemplateVariadic.cpp
@@ -1510,7 +1510,7 @@ static void CheckFoldOperand(Sema &S, Expr *E) {
   E = E->IgnoreImpCasts();
   auto *OCE = dyn_cast<CXXOperatorCallExpr>(E);
   if ((OCE && OCE->isInfixBinaryOp()) || isa<BinaryOperator>(E) ||
-      isa<AbstractConditionalOperator>(E)) {
+      isa<AbstractConditionalOperator>(E) || isa<RecoveryExpr>(E)) {
     S.Diag(E->getExprLoc(), diag::err_fold_expression_bad_operand)
         << E->getSourceRange()
         << FixItHint::CreateInsertion(E->getBeginLoc(), "(")
diff --git a/clang/test/SemaCXX/fold_expr_typo.cpp b/clang/test/SemaCXX/fold_expr_typo.cpp
index 0ef9c15b59476..2359d37740056 100644
--- a/clang/test/SemaCXX/fold_expr_typo.cpp
+++ b/clang/test/SemaCXX/fold_expr_typo.cpp
@@ -12,3 +12,8 @@ template <typename... U> struct A {
     foo<T>((... + static_cast<U>(1))); // expected-error {{expression contains unexpanded parameter pack 'T'}}
   }
 };
+
+template <typename ... T>
+void foo(T... Params) {
+  (Params ?: 1, ...); // expected-error {{expression not permitted as operand of fold expression}}
+}

[Sirraide]

I’m not convinced this is the right approach; what if we have e.g.

template <typename ... T>
void foo(T... Params) {
  ([&]{ Params ?: 1; }(), ...);
}

We still crash on this and I’m sure commonExpr would still be pack-dependent in this case, but we don’t want this to become invalid.

I wonder if we should just delete the assertion in computeDependence(OpaqueValueExpr*).

[zyn0217]

I’m not convinced this is the right approach; what if we have e.g.

template <typename ... T>
void foo(T... Params) {
  ([&]{ Params ?: 1; }(), ...);
}

We still crash on this and I’m sure commonExpr would still be pack-dependent in this case, but we don’t want this to become invalid.

I wonder if we should just delete the assertion in computeDependence(OpaqueValueExpr*).

I think the lambda expression contains unexpanded pack (we check the enclosing lambda every time we encounter a pack) and that flag would pop up to the CallExpr, and thus the commonExpr

[erichkeane]

This would need a release note AFAICT.

[erichkeane]

This feels like it is in desperate need of notes or better source locations.

[azmat-y]

Sorry for the late response, I've been busy with college and such. Do you mean the test itself? Or the diagnostic? g++ diagnostic shows the caret on the conditional operator, should we do something similar here?

[erichkeane]

You can't really do this I think? RecoveryExpr isn't always generated/always valid.

It seems to me that we shouldn't have created the RecoveryExpr here (or done the return ExprError) unless we had diagnosed

[azmat-y]

IIUC the frontend produces a RecoveryExpr on semantic errors that prevent forming well formed statements. In that case should it be always generated in case of semantic errors? Can you point an example where it is not always generated.

[Sirraide]

IIUC the frontend produces a RecoveryExpr on semantic errors that prevent forming well formed statements. In that case should it be always generated in case of semantic errors? Can you point an example where it is not always generated.

RecoveryExprs are only created if they’re enabled by some flag (-frecovery-ast iirc), otherwise, you just get ExprError():

llvm-project/clang/lib/Sema/SemaExpr.cpp

Lines 21591 to 21594 in f42e58f

	ExprResult Sema::CreateRecoveryExpr(SourceLocation Begin, SourceLocation End,
	ArrayRef<Expr *> SubExprs, QualType T) {
	if (!Context.getLangOpts().RecoveryAST)
	return ExprError();

[azmat-y]

I’m not convinced this is the right approach; what if we have e.g.

template <typename ... T>
void foo(T... Params) {
  ([&]{ Params ?: 1; }(), ...);
}

We still crash on this and I’m sure commonExpr would still be pack-dependent in this case, but we don’t want this to become invalid.

I wonder if we should just delete the assertion in computeDependence(OpaqueValueExpr*).

Sorry for late response. Yeah that should stay valid commonExpr does remain pack dependent , though something weird is happening with my build. It compiles your example without any errors even though commonExpr is pack dependent, the control flow moves to ExprError() but after continuing with the execution no error is raised.

Could something else suppress ExprError()?

[Sirraide]

Could something else suppress ExprError()?

Returning ExprError() isn’t the same as issuing an error; we only consider compilation to have failed if an actual error diagnostic is emitted.

[azmat-y]

I’m not convinced this is the right approach; what if we have e.g.

template <typename ... T>
void foo(T... Params) {
  ([&]{ Params ?: 1; }(), ...);
}

will look into handling it in ActOnCXXFoldExpr instead and see if that helps
We still crash on this and I’m sure commonExpr would still be pack-dependent in this case, but we don’t want this to become invalid.

I wonder if we should just delete the assertion in computeDependence(OpaqueValueExpr*).

Commenting out the assert doesn't affect the test suite (check-clang), the tests still pass though I am unaware about any other implications of removing the assert.

I thought of handling it through ActOnCXXFoldExpr earlier but the simpler case provided in the issue is not a fold at all, yet we still crash. I'll update when I think of another approach.