[Sema] Further improvements to to static_assert diagnostics.

[courbet]

Summary

We're now handling cases like static_assert(!expr) and
static_assert(!(expr))`.

[aaron.ballman]

lib/Sema/SemaTemplate.cpp

3065

Is this definition necessary?

[• Quuxplusone]

lib/Sema/SemaTemplate.cpp

3062

explicit

3065

Nit: I don't know if it is LLVM style to provide explicitly written-out overrides for all virtual destructors. In my own code, if a destructor would be redundant, I wouldn't write it.

3087

} // end anonymous namespace

test/SemaCXX/static-assert.cpp

117

Please also add a test case for the is_const_v inline-variable-template version.

template<class T>
inline constexpr bool is_const_v = is_const<T>::value;
static_assert(is_const_v<ExampleTypes::T>, "message");  // if this test case was missing from the previous patch
static_assert(!is_const_v<ExampleTypes::ConstT>, "message");  // exercise the same codepath for this new feature

Also, does using the PrinterHelper mean that you get a bunch of other cases for free? Like, does this work now too?

static_assert(is_const<T>::value == false, "message");

[courbet]

lib/Sema/SemaTemplate.cpp

3065

I don't think the style guide says anything; removed.

test/SemaCXX/static-assert.cpp

117

Please also add a test case for the is_const_v inline-variable-template version.

done (in c++17 tests)

Also, does using the PrinterHelper mean that you get a bunch of other cases for free? Like, does this work now too?

Exactly. While I'm at it I've added tests for your use case and removed the no longer needed AllowTopLevel parameter.

[courbet]

• Address comments
• Add more tests
• Remove AllowTopLevel and handle cases like sizeof(T) (-> sizeof(int))

[courbet]

• Update PHC and C11 tests.

[aaron.ballman]

test/PCH/cxx-static_assert.cpp

17 ↗

(On Diff #176773)

I'm not certain how I feel about now printing the failure condition when there's an explicit message provided. From what I understand, a fair amount of code in the wild does static_assert(some_condition, "some_condition") because of older language modes where the message was not optional. I worry we're going to start seeing a lot of diagnostics like: static_assert failed due to requirement '1 == 2' "N == 2", which seems a bit low-quality. See DFAPacketizer::DFAPacketizer() in LLVM as an example of something similar.

Given that the user entered a message, do we still want to show the requirement? Do we feel the same way if the requirement is fairly large?

[courbet]

test/PCH/cxx-static_assert.cpp

17 ↗

(On Diff #176773)

The issue is that "N == 2" is a useless error message. Actually, since the error message has to be a string literal, there is no way for the user to print a debuggable output. So I really think we should print the failed condition.

[• Quuxplusone]

test/PCH/cxx-static_assert.cpp

17 ↗

(On Diff #176773)

FWIW, I also don't agree with Aaron's concern.

I do think there is a lot of code in the wild whose string literal was "phoned in." After all, this is why C++17 allows us to omit the string literal: to avoid boilerplate like this.

static_assert(some-condition, "some-condition");
static_assert(some-condition, "some-condition was not satisfied");
static_assert(some-condition, "some-condition must be satisfied");
static_assert(some-condition, "");

But should Clang go _out of its way_ to suppress such "redundant" string literals? First of all, such suppression would be C++14-and-earlier: if a C++17-native program contains a string literal, we should maybe assume it's on purpose. Second, it's not clear how a machine could detect "redundant" literals with high fidelity.

static_assert(std::is_integral<T>, "std::is_integral<T>");
    // clearly redundant
static_assert(std::is_integral<T>, "T must be integral");
    // redundant, but unlikely to be machine-detectable as such

static_assert((DFA_MAX_RESTERMS * DFA_MAX_RESOURCES) <= (8 * sizeof(DFAInput)),
    "(DFA_MAX_RESTERMS * DFA_MAX_RESOURCES) > (8 * sizeof(DFAInput))");
    // redundant, but unlikely to be machine-detectable as such
    // thanks to the substitution of > for <=

static_assert((DFA_MAX_RESTERMS * DFA_MAX_RESOURCES) <= (8 * sizeof(DFAInput)),
    "(DFA_MAX_RESTERMS * DFA_MAX_RESOURCES) too big for DFAInput");
    // redundant, but unlikely to be machine-detectable as such

In any event, I agree with @courbet that Clang should print the expansion of the failed condition in any case.

Also: One might argue that if the static_assert fits completely on one source line, then we could omit the string-literal from our error message because the string literal will be shown anyway as part of the offending source line — but I believe IDE-users would complain about that, so, we shouldn't do that. And even then, we'd still want to print the failed condition!

Checking my understanding: The static_assert above (taken from DFAPacketizer::DFAPacketizer() in LLVM) would be unchanged by @courbet's patches, because none of its subexpressions are template-dependent. Right?

[aaron.ballman]

test/PCH/cxx-static_assert.cpp

17 ↗

(On Diff #176773)

But should Clang go _out of its way_ to suppress such "redundant" string literals?

I wasn't suggesting it should; I was suggesting that Clang should be conservative and suppress printing the conditional when a message is present, not when they look to be redundant enough.

if a C++17-native program contains a string literal, we should maybe assume it's on purpose.

This is the exact scenario I was envisioning.

It's a relatively weak preference, but I kind of prefer not displaying the conditional information in the presence of a message (at least in C++17 and above), especially as the conditional can be huge. I'm thinking of scenarios where the user does something like:

static_assert(condition1 && condition2 && (condition3 || condition4), "Simple explanation");

except that condition is replaced by std::some_type_trait<Blah> in various interesting ways.

[• Quuxplusone]

test/PCH/cxx-static_assert.cpp

17 ↗

(On Diff #176773)

I'm thinking of scenarios where the user does something like:

static_assert(condition1 && condition2 && (condition3 || condition4), "Simple explanation");

except that condition is replaced by std::some_type_trait<Blah> in various interesting ways.

I think the risk is extremely high that "Simple explanation" will not be actually useful to the person compiling the code, and they'll want to know exactly what failed and why (which is the direction Clement's patches are taking us). I also see the same trend happening with C++2a Concepts in @saar.raz's fork: concept diagnostics are quite verbose compared to type-trait diagnostics because it's useful to tell the user "hey, Regular<T> failed because Copyable<T> failed because MoveAssignable<T> failed because..." and eventually get all the way down to the _real_ problem, which ends up being something like "T was deduced as int&". An additional "Simple explanation" can be helpful, but is rarely as useful as the full story.

[aaron.ballman]

LGTM!

test/PCH/cxx-static_assert.cpp

17 ↗

(On Diff #176773)

Okay, I find that to be a compelling argument, thank you @Quuxplusone!

[courbet]

Thanks.

[• Quuxplusone]

@courbet: On the cpplang Slack, Peter Feichtinger mentioned that defaulted template arguments should perhaps be treated differently. Is there any way to suppress the , std::allocator<int> part of this diagnostic? https://godbolt.org/z/TM0UHc

Before your patches:

<source>:11:5: error: static_assert failed due to requirement 'std::is_integral_v<typename S::t>'
    static_assert(std::is_integral_v<typename T::t>);
    ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

After your patches:

<source>:11:5: error: static_assert failed due to requirement 'std::is_integral_v<std::vector<int, std::allocator<int> > >'
    static_assert(std::is_integral_v<typename T::t>);
    ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I don't think the new behavior is worse; but I don't think it's optimal, either.

I think Clang already has a feature to suppress printing these parameters; you would just have to figure out where it is and try to hook it into your thing. (And if you do, I'm going to ask for a test case where T::t is std::pmr::vector<int>!)

[courbet]

I agree. I'll have a look at it.

include/clang/Sema/Sema.h

public:
 
   /// Check the enable_if expressions on the given function. Returns the first
   /// failing attribute, or NULL if they were all successful.
   EnableIfAttr *CheckEnableIf(FunctionDecl *Function, ArrayRef<Expr *> Args,
                               bool MissingImplicitThis = false);
 
   /// Find the failed Boolean condition within a given Boolean
   /// constant expression, and describe it with a string.
-  ///
-  /// \param AllowTopLevelCond Whether to allow the result to be the
-  /// complete top-level condition.
-  std::pair<Expr *, std::string>
-  findFailedBooleanCondition(Expr *Cond, bool AllowTopLevelCond);
+  std::pair<Expr *, std::string> findFailedBooleanCondition(Expr *Cond);
 
   /// Emit diagnostics for the diagnose_if attributes on Function, ignoring any
   /// non-ArgDependent DiagnoseIfAttrs.
   ///
   /// Argument-dependent diagnose_if attributes should be checked each time a
   /// function is used as a direct callee of a function call.
   ///
   /// Returns true if any errors were emitted.

lib/Sema/SemaDeclCXX.cpp

if (!Failed && !Cond) {
       SmallString<256> MsgBuffer;
       llvm::raw_svector_ostream Msg(MsgBuffer);
       if (AssertMessage)
         AssertMessage->printPretty(Msg, nullptr, getPrintingPolicy());
 
       Expr *InnerCond = nullptr;
       std::string InnerCondDescription;
       std::tie(InnerCond, InnerCondDescription) =
-        findFailedBooleanCondition(Converted.get(),
-                                   /*AllowTopLevelCond=*/false);
+        findFailedBooleanCondition(Converted.get());
       if (InnerCond) {
         Diag(StaticAssertLoc, diag::err_static_assert_requirement_failed)
           << InnerCondDescription << !AssertMessage
           << Msg.str() << InnerCond->getSourceRange();
       } else {
         Diag(StaticAssertLoc, diag::err_static_assert_failed)
           << !AssertMessage << Msg.str() << AssertExpr->getSourceRange();
       }

lib/Sema/SemaTemplate.cpp

static Expr *lookThroughRangesV3Condition(Preprocessor &PP, Expr *Cond) {
 
   StringRef MacroName = PP.getImmediateMacroName(Loc);
   if (MacroName == "CONCEPT_REQUIRES" || MacroName == "CONCEPT_REQUIRES_")
     return BinOp->getRHS();
 
   return Cond;
 }
 
-// Print a diagnostic for the failing static_assert expression. Defaults to
-// pretty-printing the expression.
-static void prettyPrintFailedBooleanCondition(llvm::raw_string_ostream &OS,
-                                              const Expr *FailedCond,
-                                              const PrintingPolicy &Policy) {
-  const auto *DR = dyn_cast<DeclRefExpr>(FailedCond);
+namespace {
+
+// A PrinterHelper that prints more helpful diagnostics for some sub-expressions
+// within failing boolean expression, such as substituting template parameters
+// for actual types.
+class FailedBooleanConditionPrinterHelper : public PrinterHelper {
+public:
+  explicit FailedBooleanConditionPrinterHelper(const PrintingPolicy &Policy)
+      : Policy(Policy) {}
+
+  bool handledStmt(Stmt *E, raw_ostream &OS) override {
+    const auto *DR = dyn_cast<DeclRefExpr>(E);
     if (DR && DR->getQualifier()) {
       // If this is a qualified name, expand the template arguments in nested
       // qualifiers.
       DR->getQualifier()->print(OS, Policy, true);
       // Then print the decl itself.
       const ValueDecl *VD = DR->getDecl();
       OS << VD->getName();
       if (const auto *IV = dyn_cast<VarTemplateSpecializationDecl>(VD)) {
         // This is a template variable, print the expanded template arguments.
         printTemplateArgumentList(OS, IV->getTemplateArgs().asArray(), Policy);
       }
-    return;
+      return true;
     }
-  FailedCond->printPretty(OS, nullptr, Policy);
+    return false;
   }
 
+private:
+  const PrintingPolicy &Policy;
+};
+
+} // end anonymous namespace
+
 std::pair<Expr *, std::string>
-Sema::findFailedBooleanCondition(Expr *Cond, bool AllowTopLevelCond) {
+Sema::findFailedBooleanCondition(Expr *Cond) {
   Cond = lookThroughRangesV3Condition(PP, Cond);
 
   // Separate out all of the terms in a conjunction.
   SmallVector<Expr *, 4> Terms;
   collectConjunctionTerms(Cond, Terms);
 
   // Determine which term failed.
   Expr *FailedCond = nullptr;
   for (Expr *Term : Terms) {
     Expr *TermAsWritten = Term->IgnoreParenImpCasts();
 
-    // Literals are uninteresting.
-    if (isa<CXXBoolLiteralExpr>(TermAsWritten) ||
-        isa<IntegerLiteral>(TermAsWritten))
-      continue;
-
     // The initialization of the parameter from the argument is
     // a constant-evaluated context.
     EnterExpressionEvaluationContext ConstantEvaluated(
       *this, Sema::ExpressionEvaluationContext::ConstantEvaluated);
 
     bool Succeeded;
     if (Term->EvaluateAsBooleanCondition(Succeeded, Context) &&
         !Succeeded) {
       FailedCond = TermAsWritten;
       break;
     }
   }
+  if (!FailedCond)
+    FailedCond = Cond->IgnoreParenImpCasts();
 
-  if (!FailedCond) {
-    if (!AllowTopLevelCond)
-      return { nullptr, "" };
+  // Literals are uninteresting.
+  if (isa<CXXBoolLiteralExpr>(FailedCond) || isa<IntegerLiteral>(FailedCond))
+    return {nullptr, ""};
 
-    FailedCond = Cond->IgnoreParenImpCasts();
-  }
-
   std::string Description;
   {
     llvm::raw_string_ostream Out(Description);
-    prettyPrintFailedBooleanCondition(Out, FailedCond, getPrintingPolicy());
+    FailedBooleanConditionPrinterHelper Helper(getPrintingPolicy());
+    FailedCond->printPretty(Out, &Helper, getPrintingPolicy());
   }
   return { FailedCond, Description };
 }
 
 QualType Sema::CheckTemplateIdType(TemplateName Name,
                                    SourceLocation TemplateLoc,
                                    TemplateArgumentListInfo &TemplateArgs) {
   DependentTemplateName *DTN
if (CanonType.isNull()) {
               (*DeductionInfo)->hasSFINAEDiagnostic() &&
               (*DeductionInfo)->peekSFINAEDiagnostic().second.getDiagID() ==
                 diag::err_typename_nested_not_found_enable_if &&
               TemplateArgs[0].getArgument().getKind()
                 == TemplateArgument::Expression) {
             Expr *FailedCond;
             std::string FailedDescription;
             std::tie(FailedCond, FailedDescription) =
-              findFailedBooleanCondition(
-                TemplateArgs[0].getSourceExpression(),
-                /*AllowTopLevelCond=*/true);
+              findFailedBooleanCondition(TemplateArgs[0].getSourceExpression());
 
             // Remove the old SFINAE diagnostic.
             PartialDiagnosticAt OldDiag =
               {SourceLocation(), PartialDiagnostic::NullDiagnostic()};
             (*DeductionInfo)->takeSFINAEDiagnostic(OldDiag);
 
             // Add a new SFINAE diagnostic specifying which condition
             // failed.
case LookupResult::NotFound: {
     Expr *Cond = nullptr;
     if (isEnableIf(QualifierLoc, II, CondRange, Cond)) {
       // If we have a condition, narrow it down to the specific failed
       // condition.
       if (Cond) {
         Expr *FailedCond;
         std::string FailedDescription;
         std::tie(FailedCond, FailedDescription) =
-          findFailedBooleanCondition(Cond, /*AllowTopLevelCond=*/true);
+          findFailedBooleanCondition(Cond);
 
         Diag(FailedCond->getExprLoc(),
              diag::err_typename_nested_not_found_requirement)
           << FailedDescription
           << FailedCond->getSourceRange();
         return QualType();
       }
 

test/SemaCXX/static-assert-cxx17.cpp

 
 template <typename T>
 void foo4() {
   static_assert(S1<T[sizeof(T)], int[4]>::value, "");
   // expected-error@-1{{static_assert failed due to requirement 'S1<float [4], int [4]>::value'}}
 };
 template void foo4<float>();
 // expected-note@-1{{in instantiation of function template specialization 'foo4<float>' requested here}}
+
+
+template <typename U, typename V>
+void foo5() {
+  static_assert(!!(global_inline_var<U, V>));
+  // expected-error@-1{{static_assert failed due to requirement '!!(global_inline_var<int, float>)'}}
+}
+template void foo5<int, float>();
+// expected-note@-1{{in instantiation of function template specialization 'foo5<int, float>' requested here}}

test/SemaCXX/static-assert.cpp

void g() {
     static_assert(false, "false is false"); // expected-error {{static_assert failed "false is false"}}
 }
 
 class C {
     static_assert(false, "false is false"); // expected-error {{static_assert failed "false is false"}}
 };
 
 template<int N> struct T {
-    static_assert(N == 2, "N is not 2!"); // expected-error {{static_assert failed "N is not 2!"}}
+    static_assert(N == 2, "N is not 2!"); // expected-error {{static_assert failed due to requirement '1 == 2' "N is not 2!"}}
 };
 
 T<1> t1; // expected-note {{in instantiation of template class 'T<1>' requested here}}
 T<2> t2;
 
 template<typename T> struct S {
-    static_assert(sizeof(T) > sizeof(char), "Type not big enough!"); // expected-error {{static_assert failed "Type not big enough!"}}
+    static_assert(sizeof(T) > sizeof(char), "Type not big enough!"); // expected-error {{static_assert failed due to requirement 'sizeof(char) > sizeof(char)' "Type not big enough!"}}
 };
 
 S<char> s1; // expected-note {{in instantiation of template class 'S<char>' requested here}}
 S<int> s2;
 
 static_assert(false, L"\xFFFFFFFF"); // expected-error {{static_assert failed L"\xFFFFFFFF"}}
 static_assert(false, u"\U000317FF"); // expected-error {{static_assert failed u"\U000317FF"}}
 // FIXME: render this as u8"\u03A9"
struct ExampleTypes {
   using T = int;
   using U = float;
 };
 
 static_assert(std::is_same<ExampleTypes::T, ExampleTypes::U>::value, "message");
 // expected-error@-1{{static_assert failed due to requirement 'std::is_same<int, float>::value' "message"}}
 static_assert(std::is_const<ExampleTypes::T>::value, "message");
 // expected-error@-1{{static_assert failed due to requirement 'std::is_const<int>::value' "message"}}
+static_assert(!std::is_const<const ExampleTypes::T>::value, "message");
+// expected-error@-1{{static_assert failed due to requirement '!std::is_const<const int>::value' "message"}}
+static_assert(!(std::is_const<const ExampleTypes::T>::value), "message");
+// expected-error@-1{{static_assert failed due to requirement '!(std::is_const<const int>::value)' "message"}}
+static_assert(std::is_const<const ExampleTypes::T>::value == false, "message");
+// expected-error@-1{{static_assert failed due to requirement 'std::is_const<const int>::value == false' "message"}}
+static_assert(!(std::is_const<const ExampleTypes::T>::value == true), "message");
+// expected-error@-1{{static_assert failed due to requirement '!(std::is_const<const int>::value == true)' "message"}}
 
 struct BI_tag {};
 struct RAI_tag : BI_tag {};
 struct MyIterator {
   using tag = BI_tag;
 };
 struct MyContainer {
   using iterator = MyIterator;