[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

3088

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

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

lib/Sema/SemaTemplate.cpp

3063

Why are you dropping the explicit here?

3125–3128

Don't use auto here, please.

[courbet]

lib/AST/TypePrinter.cpp

165

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

lib/Sema/SemaTemplate.cpp

3063

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

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

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

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.

include/clang/AST/PrettyPrinter.h

PrintingPolicy(const LangOptions &LO)
         SuppressLifetimeQualifiers(false),
         SuppressTemplateArgsInCXXConstructors(false), Bool(LO.Bool),
         Restrict(LO.C99), Alignof(LO.CPlusPlus11), UnderscoreAlignof(LO.C11),
         UseVoidForZeroParams(!LO.CPlusPlus), TerseOutput(false),
         PolishForDeclaration(false), Half(LO.Half),
         MSWChar(LO.MicrosoftExt && !LO.WChar), IncludeNewlines(true),
         MSVCFormatting(false), ConstantsAsWritten(false),
         SuppressImplicitBase(false), FullyQualifiedName(false),
-        RemapFilePaths(false) {}
+        RemapFilePaths(false), PrintCanonicalTypes(false) {}
 
   /// Adjust this printing policy for cases where it's known that we're
   /// printing C++ code (for instance, if AST dumping reaches a C++-only
   /// construct). This should not be used if a real LangOptions object is
   /// available.
   void adjustForCPlusPlus() {
     SuppressTagKeyword = true;
     Bool = true;
struct PrintingPolicy {
 
   /// When true, print the fully qualified name of function declarations.
   /// This is the opposite of SuppressScope and thus overrules it.
   unsigned FullyQualifiedName : 1;
 
   /// Whether to apply -fdebug-prefix-map to any file paths.
   unsigned RemapFilePaths : 1;
 
+  /// Whether to print types as written or canonically.
+  unsigned PrintCanonicalTypes : 1;
+
   /// When RemapFilePaths is true, this function performs the action.
   std::function<std::string(StringRef)> remapPath;
 };
 
 } // end namespace clang
 
 #endif

include/clang/AST/Type.h

public:
   static std::string getAsString(const Type *ty, Qualifiers qs,
                                  const PrintingPolicy &Policy);
 
   std::string getAsString() const;
   std::string getAsString(const PrintingPolicy &Policy) const;
 
   void print(raw_ostream &OS, const PrintingPolicy &Policy,
              const Twine &PlaceHolder = Twine(),
-             unsigned Indentation = 0) const {
-    print(split(), OS, Policy, PlaceHolder, Indentation);
-  }
+             unsigned Indentation = 0) const;
 
   static void print(SplitQualType split, raw_ostream &OS,
                     const PrintingPolicy &policy, const Twine &PlaceHolder,
-                    unsigned Indentation = 0) {
-    return print(split.Ty, split.Quals, OS, policy, PlaceHolder, Indentation);
-  }
+                    unsigned Indentation = 0);
 
   static void print(const Type *ty, Qualifiers qs,
                     raw_ostream &OS, const PrintingPolicy &policy,
                     const Twine &PlaceHolder,
                     unsigned Indentation = 0);
 
   void getAsStringInternal(std::string &Str,
-                           const PrintingPolicy &Policy) const {
-    return getAsStringInternal(split(), Str, Policy);
-  }
+                           const PrintingPolicy &Policy) const;
 
   static void getAsStringInternal(SplitQualType split, std::string &out,
-                                  const PrintingPolicy &policy) {
-    return getAsStringInternal(split.Ty, split.Quals, out, policy);
-  }
+                                  const PrintingPolicy &policy);
 
   static void getAsStringInternal(const Type *ty, Qualifiers qs,
                                   std::string &out,
                                   const PrintingPolicy &policy);
 
   class StreamedQualTypeHelper {
     const QualType &T;
     const PrintingPolicy &Policy;

lib/AST/TypePrinter.cpp

public:
     void print(const Type *ty, Qualifiers qs, raw_ostream &OS,
                StringRef PlaceHolder);
     void print(QualType T, raw_ostream &OS, StringRef PlaceHolder);
 
     static bool canPrefixQualifiers(const Type *T, bool &NeedARCStrongQualifier);
     void spaceBeforePlaceHolder(raw_ostream &OS);
     void printTypeSpec(NamedDecl *D, raw_ostream &OS);
 
-    void printBefore(const Type *ty, Qualifiers qs, raw_ostream &OS);
     void printBefore(QualType T, raw_ostream &OS);
-    void printAfter(const Type *ty, Qualifiers qs, raw_ostream &OS);
     void printAfter(QualType T, raw_ostream &OS);
     void AppendScope(DeclContext *DC, raw_ostream &OS);
     void printTag(TagDecl *T, raw_ostream &OS);
     void printFunctionAfter(const FunctionType::ExtInfo &Info, raw_ostream &OS);
 #define ABSTRACT_TYPE(CLASS, PARENT)
 #define TYPE(CLASS, PARENT) \
     void print##CLASS##Before(const CLASS##Type *T, raw_ostream &OS); \
     void print##CLASS##After(const CLASS##Type *T, raw_ostream &OS);
 #include "clang/AST/TypeNodes.def"
+
+  private:
+    void printBefore(const Type *ty, Qualifiers qs, raw_ostream &OS);
+    void printAfter(const Type *ty, Qualifiers qs, raw_ostream &OS);
   };
 
 } // namespace
 
 static void AppendTypeQualList(raw_ostream &OS, unsigned TypeQuals,
                                bool HasRestrictKeyword) {
   bool appendSpace = false;
   if (TypeQuals & Qualifiers::Const) {
static void AppendTypeQualList(raw_ostream &OS, unsigned TypeQuals,
   }
 }
 
 void TypePrinter::spaceBeforePlaceHolder(raw_ostream &OS) {
   if (!HasEmptyPlaceHolder)
     OS << ' ';
 }
 
+static SplitQualType splitAccordingToPolicy(QualType QT,
+                                            const PrintingPolicy &Policy) {
+  if (Policy.PrintCanonicalTypes)
+    QT = QT.getCanonicalType();
+  return QT.split();
+}
+
 void TypePrinter::print(QualType t, raw_ostream &OS, StringRef PlaceHolder) {
-  SplitQualType split = t.split();
+  SplitQualType split = splitAccordingToPolicy(t, Policy);
   print(split.Ty, split.Quals, OS, PlaceHolder);
 }
 
 void TypePrinter::print(const Type *T, Qualifiers Quals, raw_ostream &OS,
                         StringRef PlaceHolder) {
   if (!T) {
     OS << "NULL TYPE";
     return;
case Type::SubstTemplateTypeParm:
       CanPrefixQualifiers = false;
       break;
   }
 
   return CanPrefixQualifiers;
 }
 
 void TypePrinter::printBefore(QualType T, raw_ostream &OS) {
-  SplitQualType Split = T.split();
+  SplitQualType Split = splitAccordingToPolicy(T, Policy);
 
   // If we have cv1 T, where T is substituted for cv2 U, only print cv1 - cv2
   // at this level.
   Qualifiers Quals = Split.Quals;
   if (const auto *Subst = dyn_cast<SubstTemplateTypeParmType>(Split.Ty))
     Quals -= QualType(Subst, 0).getQualifiers();
 
   printBefore(Split.Ty, Quals, OS);
if (NeedARCStrongQualifier) {
       Quals.print(OS, Policy, /*appendSpaceIfNonEmpty=*/!PrevPHIsEmpty.get());
     } else {
       Quals.print(OS, Policy, /*appendSpaceIfNonEmpty=*/!PrevPHIsEmpty.get());
     }
   }
 }
 
 void TypePrinter::printAfter(QualType t, raw_ostream &OS) {
-  SplitQualType split = t.split();
+  SplitQualType split = splitAccordingToPolicy(t, Policy);
   printAfter(split.Ty, split.Quals, OS);
 }
 
 /// Prints the part of the type string after an identifier, e.g. for
 /// "int foo[10]" it prints "[10]".
 void TypePrinter::printAfter(const Type *T, Qualifiers Quals, raw_ostream &OS) {
   switch (T->getTypeClass()) {
 #define ABSTRACT_TYPE(CLASS, PARENT)

 
 std::string QualType::getAsString(const Type *ty, Qualifiers qs,
                                   const PrintingPolicy &Policy) {
   std::string buffer;
   getAsStringInternal(ty, qs, buffer, Policy);
   return buffer;
 }
 
+void QualType::print(raw_ostream &OS, const PrintingPolicy &Policy,
+                     const Twine &PlaceHolder, unsigned Indentation) const {
+  SplitQualType split = splitAccordingToPolicy(*this, Policy);
+  print(split, OS, Policy, PlaceHolder, Indentation);
+}
+
+void QualType::print(SplitQualType split, raw_ostream &OS,
+                     const PrintingPolicy &policy, const Twine &PlaceHolder,
+                     unsigned Indentation) {
+  return print(split.Ty, split.Quals, OS, policy, PlaceHolder, Indentation);
+}
+
 void QualType::print(const Type *ty, Qualifiers qs,
                      raw_ostream &OS, const PrintingPolicy &policy,
                      const Twine &PlaceHolder, unsigned Indentation) {
   SmallString<128> PHBuf;
   StringRef PH = PlaceHolder.toStringRef(PHBuf);
 
   TypePrinter(policy, Indentation).print(ty, qs, OS, PH);
 }
 
+void QualType::getAsStringInternal(std::string &Str,
+                                   const PrintingPolicy &Policy) const {
+  SplitQualType split = splitAccordingToPolicy(*this, Policy);
+  return getAsStringInternal(split, Str, Policy);
+}
+
+void QualType::getAsStringInternal(SplitQualType split, std::string &out,
+                                   const PrintingPolicy &policy) {
+  return getAsStringInternal(split.Ty, split.Quals, out, policy);
+}
+
 void QualType::getAsStringInternal(const Type *ty, Qualifiers qs,
                                    std::string &buffer,
                                    const PrintingPolicy &policy) {
   SmallString<256> Buf;
   llvm::raw_svector_ostream StrOS(Buf);
   TypePrinter(policy).print(ty, qs, StrOS, buffer);
   std::string str = StrOS.str();
   buffer.swap(str);
 }

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) {}
 
   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;
     }
     return false;
   }
 
 private:
   const PrintingPolicy Policy;
 };
 
 } // end anonymous namespace
 
 std::pair<Expr *, std::string>
 Sema::findFailedBooleanCondition(Expr *Cond) {
   Cond = lookThroughRangesV3Condition(PP, Cond);
 
   // Separate out all of the terms in a conjunction.
   SmallVector<Expr *, 4> Terms;
   collectConjunctionTerms(Cond, Terms);
 
for (Expr *Term : Terms) {
     }
   }
   if (!FailedCond)
     FailedCond = Cond->IgnoreParenImpCasts();
 
   std::string Description;
   {
     llvm::raw_string_ostream Out(Description);
-    FailedBooleanConditionPrinterHelper Helper(getPrintingPolicy());
-    FailedCond->printPretty(Out, &Helper, getPrintingPolicy());
+    PrintingPolicy Policy = getPrintingPolicy();
+    Policy.PrintCanonicalTypes = true;
+    FailedBooleanConditionPrinterHelper Helper(Policy);
+    FailedCond->printPretty(Out, &Helper, Policy, 0, "\n", nullptr);
   }
   return { FailedCond, Description };
 }
 
 QualType Sema::CheckTemplateIdType(TemplateName Name,
                                    SourceLocation TemplateLoc,
                                    TemplateArgumentListInfo &TemplateArgs) {
   DependentTemplateName *DTN

test/SemaCXX/static-assert-cxx17.cpp

 
 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}}
+
+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>());
+  // expected-error@-1{{static_assert failed due to requirement 'X<int>()'}}
+  static_assert(X<typename T::T>{});
+  // expected-error@-1{{static_assert failed due to requirement 'X<int>{}'}}
+  static_assert(X<typename T::T>{1, 2});
+  // expected-error@-1{{static_assert failed due to requirement 'X<int>{1, 2}'}}
+  static_assert(X<typename T::T>({1, 2}));
+  // expected-error@-1{{static_assert failed due to requirement 'X<int>({1, 2})'}}
+  static_assert(typename T::T{0});
+  // expected-error@-1{{static_assert failed due to requirement 'int{0}'}}
+  static_assert(typename T::T(0));
+  // expected-error@-1{{static_assert failed due to requirement 'int(0)'}}
+  static_assert(sizeof(X<typename T::T>) == 0);
+  // expected-error@-1{{static_assert failed due to requirement 'sizeof(X<int>) == 0'}}
+  static_assert((const X<typename T::T> *)nullptr);
+  // expected-error@-1{{static_assert failed due to requirement '(const X<int> *)nullptr'}}
+  static_assert(static_cast<const X<typename T::T> *>(nullptr));
+  // expected-error@-1{{static_assert failed due to requirement 'static_cast<const X<int> *>(nullptr)'}}
+  static_assert((const X<typename T::T>[]){} == nullptr);
+  // expected-error@-1{{static_assert failed due to requirement '(X<int> const[0]){} == nullptr'}}
+  static_assert(sizeof(X<decltype(X<typename T::T>().X<typename T::T>::~X())>) == 0);
+  // expected-error@-1{{static_assert failed due to requirement 'sizeof(X<void>) == 0'}}
+}
+template void foo6<ExampleTypes>();
+// expected-note@-1{{in instantiation of function template specialization 'foo6<ExampleTypes>' requested here}}

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;