[Sema] Better static assert diagnostics for expressions involving temporaries.

[courbet]

Summary

Handles expressions such as:

• std::is_const<T>()
• std::is_const<T>()();
• std::is_same(decltype(U()), V>::value;

[courbet]

cosmetics

[• Quuxplusone]

lib/Sema/SemaTemplate.cpp

3089

It might be more maintainer-proof to write this as

std::pair<const char *, const char *> Braces;
if (Node->isStdInitListInitialization())
  Braces = std::make_pair("", "");
else if (Node->isListInitialization())
  Braces = std::make_pair("{", "}");
else
  Braces = std::make_pair("(", ")");
OS << Braces.first;
for (...) { ... }
OS << Braces.second;
return true;

test/SemaCXX/static-assert.cpp

136

Conspicuously missing any test for lines 3081–3106 above. IIUC, those lines would trigger on things like

template<class T> struct X { int i=0, j=0; constexpr operator bool() const { return false; } };
template<class T> void test() {
    static_assert(X<T>{});
    static_assert(X<T>{1,2});
    static_assert(T{0});
    static_assert(T(0));
}
template void test<int>();

But I guess I don't see why extra code is needed to handle those; shouldn't the pretty-printer handle them already? What do the current diagnostics look like for my example?

[courbet]

test/SemaCXX/static-assert.cpp

136

Note: Your examples are CXXFunctionalCastExpr, and CXXUnresolvedConstructExpr respectively, not CXXTemporaryObjectExpr, so they are not handled by this change.

They are correctly printed before this change with T==int.

The issue comes when adding an extra layer of indirection:

struct ExampleTypes {
  explicit ExampleTypes(int);
  using T = int;
  using U = float;
};

template<class T> struct X {
  int i=0;
  int j=0;
  constexpr operator bool() const { return false; }
};

template<class T> void foo6() {
    static_assert(X<typename T::T>());
    static_assert(X<typename T::T>{});
    static_assert(X<typename T::T>{1,2});
    static_assert(X<typename T::T>({1,2}));
    static_assert(typename T::T{0});
    static_assert(typename T::T(0));
}
template void foo6<ExampleTypes>();

The errors before the change are:

File /usr/local/google/home/courbet/llvm/llvm/tools/clang/test/SemaCXX/static-assert-cxx17.cpp Line 71: static_assert failed due to requirement 'X<typename ExampleTypes::T>()'
  File /usr/local/google/home/courbet/llvm/llvm/tools/clang/test/SemaCXX/static-assert-cxx17.cpp Line 72: static_assert failed due to requirement 'X<typename ExampleTypes::T>{}'
  File /usr/local/google/home/courbet/llvm/llvm/tools/clang/test/SemaCXX/static-assert-cxx17.cpp Line 73: static_assert failed due to requirement 'X<typename ExampleTypes::T>{1, 2}'
  File /usr/local/google/home/courbet/llvm/llvm/tools/clang/test/SemaCXX/static-assert-cxx17.cpp Line 74: static_assert failed due to requirement 'X<typename ExampleTypes::T>({1, 2})'
  File /usr/local/google/home/courbet/llvm/llvm/tools/clang/test/SemaCXX/static-assert-cxx17.cpp Line 75: static_assert failed due to requirement 'typename ExampleTypes::T{0}'
  File /usr/local/google/home/courbet/llvm/llvm/tools/clang/test/SemaCXX/static-assert-cxx17.cpp Line 76: static_assert failed due to requirement 'typename ExampleTypes::T(0)'

I must admit that I did not think of handling these, and now I think it would actually be better to tap into the pretty printer to avoid code duplication.

[courbet]

Handle remaining static_assert categories inside the prettyprinter.

[courbet]

clang-format patch

[courbet]

revert now unneeded changes.

[aaron.ballman]

include/clang/AST/Stmt.h

847 ↗

(On Diff #178012)

I'm pretty wary of long lists of parameters that include booleans with default values. Why is PrintCanonicalTypes not part of the printing policy? I think that's where we should keep all the knobs relating to how the printing happens.

[courbet]

move PrintQualifiedTypes to PrintingPolicy

include/clang/AST/Stmt.h

847 ↗

(On Diff #178012)

Good point, done.

[aaron.ballman]

lib/AST/StmtPrinter.cpp

78 ↗

(On Diff #178882)

Spurious formatting changes -- can revert this file.

lib/AST/TypePrinter.cpp

165 ↗

(On Diff #178882)

t doesn't meet the usual naming requirements; how about QT?

lib/Sema/SemaTemplate.cpp

3062–3064

Why are you dropping the explicit here?

3154

Don't use auto here, please.

[courbet]

lib/AST/TypePrinter.cpp

165 ↗

(On Diff #178882)

I was keeping it for consistency with TypePrinter::print(QualType t,, but done.

lib/Sema/SemaTemplate.cpp

3062–3064

Sorry, this is a revert of an approach where it had two parameters.

[courbet]

address review comments

[courbet]

clang-format diff

[aaron.ballman]

LGTM, but you should wait a few days before committing in case @Quuxplusone has comments.

[• Quuxplusone]

LGTM. Tiny style suggestions, which I won't mind if you ignore.

include/clang/AST/Type.h

995 ↗

(On Diff #178888)

Nit: could this function have been left in-line, and just changed split() to splitAccordingToPolicy(this, Policy)?
Even simpler, could splitAccordingToPolicy be made a member function of QualType, so that most of these diffs could be simply s/split()/splitAccordingToPolicy(Policy)/ without introducing any new temporary variables or anything? I.e.

void getAsStringInternal(std::string &Str,
                          const PrintingPolicy &Policy) const {
    return getAsStringInternal(splitAccordingToPolicy(Policy), Str, Policy);
}

But if that would make the code harder to read instead of easier, then don't mind me.

998 ↗

(On Diff #178888)

Nit: this function's body hasn't changed, so personally I would have left it alone (not out-of-lined it).

[courbet]

address review comments

[courbet]

Thanks for the comments !

include/clang/AST/Type.h

995 ↗

(On Diff #178888)

I'd rather avoid polluting the QualType API with splitAccordingToPolicy() which is really only useful for this use case.

without introducing any new temporary variables or anything? i.e.

Actually this is independent of the above change, done.

lib/Sema/SemaTemplate.cpp

 
 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 &P)
-      : Policy(P) {}
+  FailedBooleanConditionPrinterHelper(const ASTContext &Context,
+                                      const PrintingPolicy &P)
+      : Context(Context), Policy(P) {}
 
   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 true;
     }
+    if (auto *Node = dyn_cast<CXXTemporaryObjectExpr>(E)) {
+      Node->getType().getCanonicalType().print(OS, Policy);
+      if (Node->isStdInitListInitialization())
+        // Nothing to do; braces are part of creating the std::initializer_list.
+        ;
+      else if (Node->isListInitialization())
+        OS << "{";
+      else
+        OS << "(";
+      for (CXXTemporaryObjectExpr::arg_iterator Arg = Node->arg_begin(),
+                                                ArgEnd = Node->arg_end();
+           Arg != ArgEnd; ++Arg) {
+        if ((*Arg)->isDefaultArgument())
+          break;
+        if (Arg != Node->arg_begin())
+          OS << ", ";
+        (*Arg)->printPretty(OS, this, Policy);
+      }
+      if (Node->isStdInitListInitialization())
+        /* See above. */;
+      else if (Node->isListInitialization())
+        OS << "}";
+      else
+        OS << ")";
+      return true;
+    }
     return false;
   }
 
 private:
+  const ASTContext &Context;
   const PrintingPolicy Policy;
 };
 
 } // end anonymous namespace
 
 std::pair<Expr *, std::string>
 Sema::findFailedBooleanCondition(Expr *Cond) {
   Cond = lookThroughRangesV3Condition(PP, Cond);
for (Expr *Term : Terms) {
     }
   }
   if (!FailedCond)
     FailedCond = Cond->IgnoreParenImpCasts();
 
   std::string Description;
   {
     llvm::raw_string_ostream Out(Description);
-    FailedBooleanConditionPrinterHelper Helper(getPrintingPolicy());
+    FailedBooleanConditionPrinterHelper Helper(Context, getPrintingPolicy());
     FailedCond->printPretty(Out, &Helper, getPrintingPolicy());
   }
   return { FailedCond, Description };
 }
 
 QualType Sema::CheckTemplateIdType(TemplateName Name,
                                    SourceLocation TemplateLoc,
                                    TemplateArgumentListInfo &TemplateArgs) {
   DependentTemplateName *DTN

test/SemaCXX/static-assert.cpp

 
 namespace std {
 
 template <class Tp, Tp v>
 struct integral_constant {
   static const Tp value = v;
   typedef Tp value_type;
   typedef integral_constant type;
+  constexpr operator value_type() const noexcept { return value; }
+  constexpr value_type operator()() const noexcept { return value; }
 };
 
 template <class Tp, Tp v>
 const Tp integral_constant<Tp, v>::value;
 
 typedef integral_constant<bool, true> true_type;
 typedef integral_constant<bool, false> false_type;
 

 template <typename T>
 struct is_same<T, T> {
   static const bool value = true;
 };
 
 } // namespace std
 
 struct ExampleTypes {
+  explicit ExampleTypes(int);
   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"}}
+static_assert(std::is_const<ExampleTypes::T>(), "message");
+// expected-error@-1{{static_assert failed due to requirement 'std::is_const<int>()' "message"}}
+static_assert(!(std::is_const<const ExampleTypes::T>()()), "message");
+// expected-error@-1{{static_assert failed due to requirement '!(std::is_const<const int>()())' "message"}}
+static_assert(std::is_same<decltype(std::is_const<const ExampleTypes::T>()), int>::value, "message");
+// expected-error@-1{{static_assert failed due to requirement 'std::is_same<std::is_const<const int>, int>::value' "message"}}
+static_assert(std::is_const<decltype(ExampleTypes::T(3))>::value, "message");
+// expected-error@-1{{static_assert failed due to requirement 'std::is_const<int>::value' "message"}}
+static_assert(std::is_const<decltype(ExampleTypes::T())>::value, "message");
+// expected-error@-1{{static_assert failed due to requirement 'std::is_const<int>::value' "message"}}
+static_assert(std::is_const<decltype(ExampleTypes(3))>::value, "message");
+// expected-error@-1{{static_assert failed due to requirement 'std::is_const<ExampleTypes>::value' "message"}}
 
 struct BI_tag {};
 struct RAI_tag : BI_tag {};
 struct MyIterator {
   using tag = BI_tag;
 };
 struct MyContainer {
   using iterator = MyIterator;