[Sema] Do not print default template parameters.

[courbet]

Summary

As a followup to D55270.

[• Quuxplusone]

lib/AST/TypePrinter.cpp

173

FWIW, I don't understand this comment's terminology. "Whether default template parameters were actually written": should that say something like "whether a template parameter came from a default argument"?
And why are the Pointer, VariableArray, etc. cases here? What goes wrong if you remove them?

test/SemaCXX/static-assert-cxx17.cpp

99

(1) This cosmetic change is produced by the case IncompleteArray: that I questioned above, right? I'm surprised that the pretty-printed type changes from "east const" to "west const." But that's fine.

(2) Sorry I didn't notice before: surely where the message currently says [0] it should say [] instead! That's an existing bug in the pretty-printer, going back super far: https://godbolt.org/z/y9KzEq So I guess it's not your responsibility to fix. But I hope there's a bug open about it somewhere?

116

Please keep the old test case

static_assert(constexpr_return_false<typename T::T, typename T::U>());
// expected-error@-1{{static_assert failed due to requirement 'constexpr_return_false<int, float>()'}}

and then add this new test case. My understanding is that the old test case should still pass, even after your change.

120

None of these new test cases actually involve the default template argument.

I'd like to see two test cases explicitly mimicking vector. (Warning: I didn't run this code!)

template<class> struct A {};
template<class, class = A<T>> struct V {};
template<class C> void testx() {
    static_assert(std::is_same<C*, void>::value, "");
    // expected-error@-1{{due to requirement 'std::is_same<V<int>*, void>::value'}}
}
template testx<V<int>>();
template<class> struct PMRA {};
template<class T> using PMRV = V<T, PMRA<T>>;
template<class C> void test() {
    static_assert(std::is_same<C*, void>::value, "");
    // expected-error@-1{{due to requirement 'std::is_same<V<int, PMRA<int>>*, void>::value'}}
    // expected-error@-2{{due to requirement 'std::is_same<PMRV<int>*, void>::value'}}
}
template testy<PMRV<int>>();

The expected-error@-2 above is my fantasy world. I don't think it's possible to achieve in real life. :)

test/SemaCXX/static-assert.cpp

130

Why did this output change?

[courbet]

lib/AST/TypePrinter.cpp

173

I've expanded the comments.

test/SemaCXX/static-assert-cxx17.cpp

99

(1) This cosmetic change is produced by the case IncompleteArray: that I questioned above, right? I'm surprised that the pretty-printed type changes from "east const" to "west const." But that's fine.

Indeed. There's some logic in the printer to see where the const should go.

(2) Sorry I didn't notice before: surely where the message currently says [0] it should say [] instead! That's an existing bug in the pretty-printer, going back super far: https://godbolt.org/z/y9KzEq So I guess it's not your responsibility to fix. But I hope there's a bug open about it somewhere?

Is it actually a bug ? In the godbolt example I actually thing putting the zero there actually clarifies the semantics of the expression for the reader, so I would'nt say it hurts.

116

The old one is in foo7().

120

None of these new test cases actually involve the default template argument.

This one is to check that we actually do print when specified explicitly. foo6 above tests the default template arguments (notice the change from template <class T> struct X to template <class T, class U = double> struct X` above). I've renamed foo6 and foo7 to make that clear.

Before this change, static_asserts in foo6 and foo7 printed the same thing. Now they don't.

I'd like to see two test cases explicitly mimicking vector.

OK, I think I misunderstood what you wanted here. I don't think what you want is actually doable, because by the time you're in test(), C is just a type without any way of knowing whether the user explicitly provided the template parameter or relied on the default.

What we could do though is always erase template parameters that are the same as default ones. But that would mean printing due to requirement 'std::is_same<V<int>*, void>::value' even when the user wrote template testx<V<int, A<int>>>();.
WDYT ?

test/SemaCXX/static-assert.cpp

130

This changed in the parent diff D55933.

[• Quuxplusone]

test/SemaCXX/static-assert-cxx17.cpp

120

Here's what I wrote over on D55270.

In D55270#1327704, @Quuxplusone wrote:

@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>!)

So does this patch (D55933) actually change the message printed by Peter's https://godbolt.org/z/TM0UHc ? (I think it does not.)
Does it change the message printed by https://godbolt.org/z/Q0AD70 ? (I think it does.)

I think most of your new test cases in foo7 are redundant and don't really need to be there. Contrariwise, I would like to see one new test case isomorphic to https://godbolt.org/z/Q0AD70 , because that strikes me as a very realistic case that we want to protect against regressions.

[courbet]

Add more tests as suggested in the comments.

test/SemaCXX/static-assert-cxx17.cpp

120

So does this patch (D55933) actually change the message printed by Peter's https://godbolt.org/z/TM0UHc ? (I think it does not.)

This is correct, it does not.

Does it change the message printed by https://godbolt.org/z/Q0AD70 ? (I think it does.)

It does.

I think most of your new test cases in foo7 are redundant and don't really need to be there.

Sound good, but I've still kept a few cases for coverage.

Contrariwise, I would like to see one new test case isomorphic to https://godbolt.org/z/Q0AD70 , because that strikes me as a very realistic case that we want to protect against regressions.

Done.

[• Quuxplusone]

lib/AST/TypePrinter.cpp

179

Nit: there's an extra ` on this line.

I think I vaguely understand the purpose of this switch now. It feels ugly, but I have no concrete suggestion for improvement. Do you not need a case here to delay canonicalization of X<Some::Class>& also?

[courbet]

Handle {L,R}Value references.

lib/AST/TypePrinter.cpp

179

Indeed. Fixed + added tests.

[• Quuxplusone]

lib/AST/TypePrinter.cpp

179

And void(*)(X<Some::Class>)? And possibly int[X<Some::Class>::value], but I think that might be what Type::DependentSizedArray is doing in there. And void (^)(X<Some::Class>) in Objective-C++.

Surely there should be a list somewhere of all the "compound types" that need to go here. And then this starts feeling a lot like the visitor pattern. I still don't know how it _should_ look, but this switch is feeling ickier and ickier.

include/clang/AST/NestedNameSpecifier.h

public:
   /// Whether this nested name specifier involves a template
   /// parameter.
   bool isInstantiationDependent() const;
 
   /// Whether this nested-name-specifier contains an unexpanded
   /// parameter pack (for C++11 variadic templates).
   bool containsUnexpandedParameterPack() const;
 
-  /// Print this nested name specifier to the given output stream. If
-  /// `ResolveTemplateArguments` is true, we'll print actual types, e.g.
-  /// `ns::SomeTemplate<int, MyClass>` instead of
-  /// `ns::SomeTemplate<Container::value_type, T>`.
-  void print(raw_ostream &OS, const PrintingPolicy &Policy,
-             bool ResolveTemplateArguments = false) const;
+  /// Print this nested name specifier to the given output stream.
+  void print(raw_ostream &OS, const PrintingPolicy &Policy) const;
 
   void Profile(llvm::FoldingSetNodeID &ID) const {
     ID.AddPointer(Prefix.getOpaqueValue());
     ID.AddPointer(Specifier);
   }
 
   /// Dump the nested name specifier to standard output to aid
   /// in debugging.

lib/AST/NestedNameSpecifier.cpp

case TypeSpecWithTemplate:
     return getAsType()->containsUnexpandedParameterPack();
   }
 
   llvm_unreachable("Invalid NNS Kind!");
 }
 
 /// Print this nested name specifier to the given output
 /// stream.
-void NestedNameSpecifier::print(raw_ostream &OS, const PrintingPolicy &Policy,
-                                bool ResolveTemplateArguments) const {
+void NestedNameSpecifier::print(raw_ostream &OS, const PrintingPolicy &Policy) const {
   if (getPrefix())
     getPrefix()->print(OS, Policy);
 
   switch (getKind()) {
   case Identifier:
     OS << getAsIdentifier()->getName();
     break;
 
case Super:
     break;
 
   case TypeSpecWithTemplate:
     OS << "template ";
     // Fall through to print the type.
     LLVM_FALLTHROUGH;
 
   case TypeSpec: {
-    const auto *Record =
-            dyn_cast_or_null<ClassTemplateSpecializationDecl>(getAsRecordDecl());
-    if (ResolveTemplateArguments && Record) {
-        // Print the type trait with resolved template parameters.
-        Record->printName(OS);
-        printTemplateArgumentList(OS, Record->getTemplateArgs().asArray(),
-                                  Policy);
-        break;
-    }
     const Type *T = getAsType();
 
     PrintingPolicy InnerPolicy(Policy);
     InnerPolicy.SuppressScope = true;
 
     // Nested-name-specifiers are intended to contain minimally-qualified
     // types. An actual ElaboratedType will not occur, since we'll store
     // just the type that is referred to in the nested-name-specifier (e.g.,

lib/AST/TypePrinter.cpp

 
 void TypePrinter::spaceBeforePlaceHolder(raw_ostream &OS) {
   if (!HasEmptyPlaceHolder)
     OS << ' ';
 }
 
 static SplitQualType splitAccordingToPolicy(QualType QT,
                                             const PrintingPolicy &Policy) {
-  if (Policy.PrintCanonicalTypes)
-    QT = QT.getCanonicalType();
+  if (!Policy.PrintCanonicalTypes)
     return QT.split();
+  if (QT.isNull())
+    return QT.split();
+  // We do not canonicalize a few type classes because this would remove the
+  // information about whether a template parameter came from a default
+  // argument.
+  // In the following comments, consider:
+  //   `template struct X<typename T, typename U = int> {};`, and
+  //   `struct Some { using Class = float; };`
+  switch (QT.getTypePtr()->getTypeClass()) {
+    // `X<Some::Class>` gets canonicalized to `X<float, int>`. We disable
+    // canonicalization so that `printTag()`` can see the template parameters as
+    // written.
+    case Type::TemplateSpecialization:
+    // `X<Some::Class>*` gets canonicalized to `X<float, int>*`. Disabling
+    // canonicalization of the pointer/array will canonicalize the pointee when
+    // it is is processed, resulting in `X<float>*`.
+    case Type::Pointer:
+    case Type::ConstantArray:
+    case Type::IncompleteArray:
+    case Type::VariableArray:
+    case Type::DependentSizedArray:
+      return QT.split();
+    default:
+      return QT.getCanonicalType().split();
+  }
 }
 
 void TypePrinter::print(QualType t, raw_ostream &OS, StringRef PlaceHolder) {
   SplitQualType split = splitAccordingToPolicy(t, Policy);
   print(split.Ty, split.Quals, OS, PlaceHolder);
 }
 
 void TypePrinter::print(const Type *T, Qualifiers Quals, raw_ostream &OS,

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;
 }
 
-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);
 
Sema::findFailedBooleanCondition(Expr *Cond) {
   if (!FailedCond)
     FailedCond = Cond->IgnoreParenImpCasts();
 
   std::string Description;
   {
     llvm::raw_string_ostream Out(Description);
     PrintingPolicy Policy = getPrintingPolicy();
     Policy.PrintCanonicalTypes = true;
-    FailedBooleanConditionPrinterHelper Helper(Policy);
-    FailedCond->printPretty(Out, &Helper, Policy, 0, "\n", nullptr);
+    FailedCond->printPretty(Out, nullptr, Policy, 0, "\n", nullptr);
   }
   return { FailedCond, Description };
 }
 
 QualType Sema::CheckTemplateIdType(TemplateName Name,
                                    SourceLocation TemplateLoc,
                                    TemplateArgumentListInfo &TemplateArgs) {
   DependentTemplateName *DTN

test/SemaCXX/static-assert-cxx17.cpp

 // RUN: %clang_cc1 -fsyntax-only -verify %s -std=c++1z -triple=x86_64-linux-gnu
 
-template <typename U, typename V>
+template <typename U, typename V = U>
 struct S1 {
   static constexpr const bool value = false;
 };
 
 template <typename U, typename V>
 inline constexpr bool global_inline_var = S1<U, V>::value;
 
 template <typename T>
 struct S2 {
   template <typename U, typename V>
   static inline constexpr bool var = global_inline_var<U, V>;
 };
 
-template <typename U, typename V>
+template <typename U, typename V = double>
 inline constexpr bool constexpr_return_false() {
   return false;
 }
 
 template <typename U, typename V>
 void foo() {
   static_assert(S1<U, V>::value);
   // expected-error@-1{{static_assert failed due to requirement 'S1<int, float>::value'}}
+  static_assert(S1<U>::value);
+  // expected-error@-1{{static_assert failed due to requirement 'S1<int>::value'}}
 }
 template void foo<int, float>();
 // expected-note@-1{{in instantiation of function template specialization 'foo<int, float>' requested here}}
 
 template <typename U, typename V>
 void foo2() {
   static_assert(global_inline_var<U, V>);
   // expected-error@-1{{static_assert failed due to requirement 'global_inline_var<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>
+template <class T, class U = double>
 struct X {
   int i = 0;
   int j = 0;
   constexpr operator bool() const { return false; }
 };
 
 template <class T>
-void foo6() {
+void test_template_parameter_from_default() {
   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'}}
+  // expected-error@-1{{static_assert failed due to requirement '(const X<int> [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'}}
+  static_assert(constexpr_return_false<typename T::T>());
+  // expected-error@-1{{static_assert failed due to requirement 'constexpr_return_false<int>()'}}
+}
+template void test_template_parameter_from_default<ExampleTypes>();
+// expected-note@-1{{in instantiation of function template specialization 'test_template_parameter_from_default<ExampleTypes>' requested here}}
+
+template <class T, class U>
+void test_template_parameter_as_written() {
+  static_assert(X<typename T::T, U>());
+  // expected-error@-1{{static_assert failed due to requirement 'X<int, float>()'}}
+  static_assert(sizeof(X<typename T::T, U>) == 0);
+  // expected-error@-1{{static_assert failed due to requirement 'sizeof(X<int, float>) == 0'}}
   static_assert(constexpr_return_false<typename T::T, typename T::U>());
   // expected-error@-1{{static_assert failed due to requirement 'constexpr_return_false<int, float>()'}}
 }
-template void foo6<ExampleTypes>();
-// expected-note@-1{{in instantiation of function template specialization 'foo6<ExampleTypes>' requested here}}
+template void test_template_parameter_as_written<ExampleTypes, float>();
+// expected-note@-1{{in instantiation of function template specialization 'test_template_parameter_as_written<ExampleTypes, float>' requested here}}
+
+
+template<class T, class = decltype(sizeof(0))>
+struct is_pointerable { static constexpr bool value = false; };
+
+template<class T>
+struct is_pointerable<T, decltype(sizeof(T*))> { static constexpr bool value = true; };
+
+template<class T>
+void test_is_pointerable()
+{
+    static_assert(is_pointerable<T>::value);
+    // expected-error@-1{{due to requirement 'is_pointerable<int &>::value'}}
+    static_assert(not is_pointerable<T>::value);
+    // expected-error@-1{{due to requirement '!is_pointerable<int>::value'}}
+}
+template void test_is_pointerable<int&>();
+// expected-note@-1{{in instantiation of function template specialization 'test_is_pointerable<int &>' requested here}}
+template void test_is_pointerable<int>();
+// expected-note@-1{{in instantiation of function template specialization 'test_is_pointerable<int>' requested here}}
+
+
+// This test emulates std::vector with a default allocator.
+// FIXME: Ideally we would like to avoid printing the default here (print 'S1<V<int> *, void>::value')
+template<class> struct A {};
+template<class T, class = A<T>> struct V {};
+template<class C> void test_std_vector_like_from_default() {
+    static_assert(S1<C*, void>::value);
+    // expected-error@-1{{due to requirement 'S1<V<int, A<int> > *, void>::value'}}
+}
+template void test_std_vector_like_from_default<V<int>>();
+// expected-note@-1{{in instantiation of function template specialization 'test_std_vector_like_from_default<V<int, A<int> > >' requested here}}

test/SemaCXX/static-assert.cpp

 // 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"}}
+// expected-error@-1{{static_assert failed due to requirement 'std::is_same<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 {};