Stop double-diagnosing explicit convert operator in switch condition

[erichkeane]

Note this also likely fixes a bunch of other cases. We were double-diagnosting in a template because we were generating the expression anyway, so any attempts to instantiate the function would instantiate the expression, thus re-diagnosing it.

This patch replaces it with a RecoveryExpr. Additionally, VerifyIntegerConstantExpression couldn't handle the RecoveryExpr, as it requires a non-dependent expression result (which a RecoveryExpr is dependent). Additionally, callers of it use the return value to decide that VerifyIntegerConstantExpression succeeded, so it fails if it sees a RecoveryExpr.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Erich Keane (erichkeane)

Changes

Note this also likely fixes a bunch of other cases. We were double-diagnosting in a template because we were generating the expression anyway, so any attempts to instantiate the function would instantiate the expression, thus re-diagnosing it.

This patch replaces it with a RecoveryExpr. Additionally, VerifyIntegerConstantExpression couldn't handle the RecoveryExpr, as it requires a non-dependent expression result (which a RecoveryExpr is dependent). Additionally, callers of it use the return value to decide that VerifyIntegerConstantExpression succeeded, so it fails if it sees a RecoveryExpr.

---

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

3 Files Affected:

• (modified) clang/lib/Sema/SemaExpr.cpp (+6)
• (modified) clang/lib/Sema/SemaOverload.cpp (+8-5)
• (modified) clang/test/SemaCXX/explicit.cpp (+15)

diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index d2c77ad61644f0..c7e1d1daafea99 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -17522,6 +17522,12 @@ Sema::VerifyIntegerConstantExpression(Expr *E, llvm::APSInt *Result,
     if (Converted.isInvalid())
       return Converted;
     E = Converted.get();
+    // The 'explicit' case causes us to get a RecoveryExpr.  Give up here so we
+    // don't try to evaluate it later. We also don't want to return the
+    // RecoveryExpr here, as it results in this call succeeding, thus callers of
+    // this function will attempt to use 'Value'.
+    if (isa<RecoveryExpr>(E))
+      return ExprError();
     if (!E->getType()->isIntegralOrUnscopedEnumerationType())
       return ExprError();
   } else if (!E->getType()->isIntegralOrUnscopedEnumerationType()) {
diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index adc319e97b7625..48d6264029e9bf 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -6563,11 +6563,14 @@ diagnoseNoViableConversion(Sema &SemaRef, SourceLocation Loc, Expr *&From,
                                                        HadMultipleCandidates);
     if (Result.isInvalid())
       return true;
-    // Record usage of conversion in an implicit cast.
-    From = ImplicitCastExpr::Create(SemaRef.Context, Result.get()->getType(),
-                                    CK_UserDefinedConversion, Result.get(),
-                                    nullptr, Result.get()->getValueKind(),
-                                    SemaRef.CurFPFeatureOverrides());
+
+    // Replace the conversion with a RecoveryExpr, so we don't try to
+    // instantiate it later, but can further diagnose here.
+    Result = SemaRef.CreateRecoveryExpr(From->getBeginLoc(), From->getEndLoc(),
+                                        From, Result.get()->getType());
+    if (Result.isInvalid())
+      return true;
+    From = Result.get();
   }
   return false;
 }
diff --git a/clang/test/SemaCXX/explicit.cpp b/clang/test/SemaCXX/explicit.cpp
index ba2c36d99daf62..3bb04a4d62e6c8 100644
--- a/clang/test/SemaCXX/explicit.cpp
+++ b/clang/test/SemaCXX/explicit.cpp
@@ -266,3 +266,18 @@ namespace PR18777 {
   struct S { explicit operator bool() const; } s;
   int *p = new int(s); // expected-error {{no viable conversion}}
 }
+
+namespace DoubleDiags {
+  struct ExplicitConvert{
+    explicit operator int();//#DOUBLE_DIAG_OP_INT
+  } EC;
+  template<typename T>
+  void Template(){
+      // expected-error@+2{{switch condition type 'struct ExplicitConvert' requires explicit conversion to 'int'}}
+      // expected-note@#DOUBLE_DIAG_OP_INT{{conversion to integral type 'int'}}
+      switch(EC){}
+  };
+  void Inst() {
+    Template<int>();
+  }
+}

[Endilll]

I'm having second thoughts about leveraging recovery expressions as a side-band mechanism to de-duplicate diagnostics, because not modeling things properly in AST might backfire in the future. But I don't have anything better on my mind, so I don't want to block the progress of this PR.

[cor3ntin]

Did you try to use ExprDependence::Error?

[erichkeane]

I'm having second thoughts about leveraging recovery expressions as a side-band mechanism to de-duplicate diagnostics, because not modeling things properly in AST might backfire in the future. But I don't have anything better on my mind, so I don't want to block the progress of this PR.

This is a case where we previously would use ExprError. Typically, we don't include 'incorrect' expressions in the AST, as it results in us trying to instantiate them (like we see here). The duplicate diagnostics is a side-effect of doing that wrong.

The RecoveryExpr is, IMO, a half decent way to actually model things properly in the AST despite them having an error.

[erichkeane]

Did you try to use ExprDependence::Error?

I've only ever seen that used in regards to the RecoveryExpr, so I think I am? Also, RecoveryExpr doesn't really have a way of changing its dependence to anything but what it is.

[Sirraide]

I've only ever seen that used in regards to the RecoveryExpr, so I think I am?

It’s used in a few other places from what I can tell (e.g. see InitListExpr::markError()); I think the result is the same either way (i.e. containsErrors() ends up returning true).

I was looking around a bit and also just found this comment here:

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

Lines 3734 to 3739 in 215eee6

	// Clang AST consumers assume a DeclRefExpr refers to a valid decl. We
	// wrap a DeclRefExpr referring to an invalid decl with a dependent-type
	// RecoveryExpr to avoid follow-up semantic analysis (thus prevent bogus
	// diagnostics).
	if (VD->isInvalidDecl() && E)
	return CreateRecoveryExpr(E->getBeginLoc(), E->getEndLoc(), {E});

So I suppose there is at least some prededent for using RecoveryExpr to suppress diagnostics.

I don’t have too much experience with either, but at least intuitively, ExprDependence::Error sounds like it would make sense here, because if not for cases like this one then what is it actually for I wonder?

[erichkeane]

I've only ever seen that used in regards to the RecoveryExpr, so I think I am?

It’s used in a few other places from what I can tell (e.g. see InitListExpr::markError()); I think the result is the same either way (i.e. containsErrors() ends up returning true).

I was looking around a bit and also just found this comment here:

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

Lines 3734 to 3739 in 215eee6

	// Clang AST consumers assume a DeclRefExpr refers to a valid decl. We
	// wrap a DeclRefExpr referring to an invalid decl with a dependent-type
	// RecoveryExpr to avoid follow-up semantic analysis (thus prevent bogus
	// diagnostics).
	if (VD->isInvalidDecl() && E)
	return CreateRecoveryExpr(E->getBeginLoc(), E->getEndLoc(), {E});

So I suppose there is at least some prededent for using RecoveryExpr to suppress diagnostics.

I don’t have too much experience with either, but at least intuitively, ExprDependence::Error sounds like it would make sense here, because if not for cases like this one then what is it actually for I wonder?

I was looking into that, it doesn't seem to prevent being used in instantiation like RecoveryExpr does. I DO note that RecoveryExpr DOES contain the original AST nodes, so it isn't like we're losing anything.

[AaronBallman]

The changes should come with a release note, but this LGTM otherwise

[cor3ntin]

LGTM