[Sema] Improve static_assert diagnostics.

[courbet]

Summary

In our codebase, static_assert(std::some_type_trait<Ts...>::value, "msg")
(where some_type_trait is an std type_trait and Ts... is the
appropriate template parameters) account for 11.2% of the static_asserts.

In these cases, the Ts are typically not spelled out explicitly, e.g.
static_assert(std::is_same<SomeT::TypeT, typename SomeDependentT::value_type>::value, "message");

The diagnostic when the assert fails is typically not very useful, e.g.
static_assert failed due to requirement 'std::is_same<SomeT::TypeT, typename SomeDependentT::value_type>::value' "message"

This change makes the diagnostic spell out the types explicitly , e.g.
static_assert failed due to requirement 'std::is_same<int, float>::value' "message"

See tests for more examples.

After this is submitted, I intend to handle
static_assert(!std::some_type_trait<Ts...>::value, "msg"),
which is another 6.6% of static_asserts.

[courbet]

Handle all std type traits.

[aaron.ballman]

lib/Sema/SemaTemplate.cpp

3070

No need for the pointer itself to be const qualified -- drop the top-level const qualifier (here and elsewhere).

3073–3085

I would drop all of this logic and replace it (see below).

3096

Please don't use auto here.

3115

You can also check Var->isInStdNamespace() here to simplify the logic above.

[• Quuxplusone]

test/SemaCXX/static-assert.cpp

111

I would like to see some more realistic test cases. I suggest this test case for example:

struct BI_tag {};
struct RAI_tag : BI_tag {};
struct MyIterator {
    using tag = BI_tag;
};
struct MyContainer {
    using iterator = MyIterator;
};
template<class Container>
void foo() {
    static_assert(std::is_base_of_v<RAI_tag, typename Container::iterator::tag>);
}

This is an example where as a programmer I would not want to see only failed due to requirement std::is_base_of_v<RAI_tag, BI_tag> — that doesn't help me solve the issue. OTOH, since every diagnostic includes a cursor to the exact text of the static_assert already, I think it's fair to say that the current diagnostic message is redundant, and therefore it's okay to replace it (as you propose to do) with something that is not redundant.

[courbet]

Address Aaron's comments.

[courbet]

Thanks for the comments.

lib/Sema/SemaTemplate.cpp

3070

constness prevents me from accidentally reassigning the variable. But I won't fight over it :)

3115

Thanks for the pointer ! I was looking for something like this :)
I still have to check this on the qualifier and not the variable though, but that does make the logic a lot simpler.

test/SemaCXX/static-assert.cpp

111

I think it's fair to say that the current diagnostic message is redundant, and therefore it's okay to replace it (as you propose to do) with something that is not redundant.

Yes, the proposal here might not be the *best* possible diagnostic for all cases, but it's already a huge improvement on the existing one, and solves a significant proportion of use cases.

Here, the programmer will see:

test.cc:13:5: error: static_assert failed due to requirement 'std::is_base_of<RAI_tag, BI_tag>::value'
    static_assert(std::is_base_of<RAI_tag, typename Container::iterator::tag>::value);
    ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

which I think is a reasonable help for debugging.

[aaron.ballman]

lib/Sema/SemaTemplate.cpp

3076

Formatting is incorrect here; you should run the patch through clang-format.

Can getAsRecordDecl() return null even when looking for a type out of a NNS? If so, you should assert/test for that.

3115

Ah, good point!

test/SemaCXX/static-assert.cpp

111

@Quuxplusone, do you have recommendations for what you'd prefer to see instead?

FWIW, I think this is a good incremental improvement. If there's more information we could display easily as part of this patch, we should consider it, but I'm also fine with saying this is progress.

[courbet]

clang-formant patch

[courbet]

harden against null getAsRecordDecl().

[courbet]

lib/Sema/SemaTemplate.cpp

3076

Good point, I've added a test.

[courbet]

fix variable name in comment

[• Quuxplusone]

test/SemaCXX/static-assert.cpp

111

@Quuxplusone, do you have recommendations for what you'd prefer to see instead?

On the diagnostic itself, no, this looks good and I was just thinking out loud.

On the test cases, yes, I suggest that there should be at least one test case where a static_assert appears inside a template and uses something template-dependent.

[courbet]

test/SemaCXX/static-assert.cpp

111

SG, I'll add such an example (with std::is_same if you don't mind to avoid having to duplicate the whole <type_traits> in the test :) )

[• Quuxplusone]

lib/Sema/SemaTemplate.cpp

3061

Why do you bother to check a whitelist of "known" type trait names? It seems to me that if you replaced this function body with return true;, the diagnostic would automatically be able to handle cases such as

static_assert(std::is_trivially_move_constructible<T>::value);  // not a builtin
static_assert(folly::IsRelocatable<T>::value);  // not in namespace std

What would go wrong if you replaced this entire function body with return true;?

3109

Again, why do you bother to check this? I would not expect the compiler to treat differently

struct Trait { static constexpr bool value = true; }; static_assert(Trait::value);
struct Trait { static constexpr bool wodget = true; }; static_assert(Trait::wodget);

[courbet]

Add unit test with dependent types.

[courbet]

Replace custom printing code with clang helper.

[courbet]

expand types in all qualifiers, not only type traits.

[courbet]

lib/Sema/SemaTemplate.cpp

3061

That's a very good question. I guess because I was mainly interested in this. But now that I think of it, there is no reason to limit to "known" type traits, or even to type traits at all. Any qualified DeclRefExpr shoul see its qualifiers be spelled out with explicit types, e.g.

ns::S1<T>::S2<int, typename U::value_type>::var

should be diagnosed as something like:

ns::S1<double>::S2<int, long int>::var

I've done that and added unit tests. Tell me what you think.

[• Quuxplusone]

lib/Sema/SemaTemplate.cpp

3064

I don't understand this code enough to say whether this "qualified name" business is needed. Is this what currently prevents you from expanding e.g. is_base_of_v<T, U>, as opposed to is_base_of<T, U>::value? What would go wrong if you removed this condition?

test/SemaCXX/static-assert.cpp

123

Incidentally, you can put // expected-error@-1{{...}} on the following line, to avoid such long lines.

141

Nice!
I would also like to see at least one test where one or more of the types are hard-to-name; e.g.

template<class T>
void foo(T t) {
    static_assert(std::is_integral<T>::value);
    static_assert(std::is_integral<decltype(t)>::value);
}
void test() { foo([](){}); }

Another interesting case would be where one of the types is non-existent — which I expect would not hit your codepath at all, right?

template<class T>
void foo(T t) {
    static_assert(std::is_integral<typename T::iterator>::value);
    static_assert(std::is_integral<decltype(*t)>::value);
}
void test() { foo(42); }

And should there be any test for a static_assert with no "message" element? or are you confident that that'll just work, and you want to keep this test file building in C++11 mode?

[courbet]

• add more tests
• handle c++17 constructs
• add c++17 tests in static-assert-cxx17.cpp

[courbet]

Thanks

lib/Sema/SemaTemplate.cpp

3064

Expr does not have getQualifier(), DeclRefExpr is the one that does.

is_base_of_v is also a DeclRefExpr, so that works as expected.

test/SemaCXX/static-assert.cpp

123

Thanks for the tip.

141

I would also like to see at least one test where one or more of the types are hard-to-name; e.g.

SG, done.

Another interesting case would be where one of the types is non-existent — which I expect would not hit your codepath at all, right?

Right. Done.

And should there be any test for a static_assert with no "message" element? or are you confident that that'll just work, and you want to keep this test file building in C++11 mode?

This test file has negative tests for c++17 message-less static asserts, so I've added the c++17 tests in a separate file.

[aaron.ballman]

lib/AST/NestedNameSpecifier.cpp

308–310 ↗

(On Diff #175855)

I'd remove the getAsRecordDecl() from the first if and instead use dyn_cast_or_null in the second if. Probably something like:

const auto *Record = dyn_cast_or_null<ClassTemplateSpecializationDecl>(getAsRecordDecl());
if (ResolveTemplateArguments && Record) {
}

332 ↗

(On Diff #175855)

This looks to be a spurious formatting change.

[• Quuxplusone]

Looks fine to me; please don't let me block this any further. :) Someone else, e.g. @aaron.ballman, should be the one to accept it, though.

include/clang/AST/NestedNameSpecifier.h

220 ↗

(On Diff #175855)

Peanut gallery says: Should ResolveTemplateArguments really be here, or should it be a property of the PrintingPolicy the same way e.g. ConstantsAsWritten is a property of the PrintingPolicy? I don't know what a PrintingPolicy is, really, but I know that I hate defaulted boolean function parameters with a passion. ;)

Furthermore, I note that the doc-comment for ConstantsAsWritten, at line ~207 of include/clang/AST/PrettyPrinter.h, is nonsensical and maybe someone should give it some love. (That is totally not your problem, though.)

lib/Sema/SemaTemplate.cpp

3071

Checking my understanding: Am I correct that this code currently does not pretty-print

static_assert(std::is_same<T, int>(), "");

(creating an object of the trait type and then using its constexpr operator bool to convert it to bool)? This is a rare idiom and doesn't need to be supported AFAIC.

test/SemaCXX/static-assert.cpp

143

Looking at this example, I thought of one more case to test.

static_assert(std::is_same_v<T[sizeof(T)], int[4]>, "");

That is, I'd like to see a test that verifies whether T[sizeof(T)] pretty-prints as float[sizeof(float)], or float[4], or something else. (I don't really care what it prints as, as long as it's not completely crazy, and as long as it doesn't regress in future releases.)

164

I guess I meant more like

struct S {};
void bar() {
    foo(S{});
}

error: no member named 'iterator' in 'S'
    static_assert(std::is_same_v<typename T::iterator, int>);
                                          ~~~^

but as neither version goes through your codepath anyway, what you've got is OK.

[aaron.ballman]

include/clang/AST/NestedNameSpecifier.h

220 ↗

(On Diff #175855)

I think this feels like a printing policy decision and should live there.

lib/Sema/SemaTemplate.cpp

3071

I'm fine worrying about that situation for a follow-up patch if it isn't currently supported.

[courbet]

lib/AST/NestedNameSpecifier.cpp

308–310 ↗

(On Diff #175855)

SG, thanks for the pointer to dyn_cast_or_null.

lib/Sema/SemaTemplate.cpp

3071

This is not supported indeed. There are a bunch of other expr types that we could support.
This is the most frequent one (11.2% of our codebase), the next one I plan to address (in a follow-up patch) is static_assert(!std;:type_trait<stuff>::value) with 6.6%.
The one you're mentioning here accounts for 2.8%.

[courbet]

• Fix spurious formating changes
• Add tests
• Improve readability.

[aaron.ballman]

lib/Sema/SemaTemplate.cpp

3055

Comment is a bit out of date as this is no longer specific to static_assert.

It looks like this may also change the behavior of enable_if diagnostic reporting. Do you need to update any of those tests from this change? If not, can you devise some tests for that case as well to show that this isn't just a static_assert behavior? (Do a search for findFailedBooleanCondition to find where the behavior has changed.)

[courbet]

lib/Sema/SemaTemplate.cpp

3055

Nope, unfortunately the code that calls this is actually untested...
I could not find a test that actually enters the:

if (TypeAliasTemplateDecl *AliasTemplate =
          dyn_cast<TypeAliasTemplateDecl>(Template))

in Sema::CheckTemplateIdType.

I stopped at the following insanity:

// RUN: %clang_cc1 -std=c++11 -verify %s

template <typename T, typename U>
struct is_same {
  enum { value = 0 };
};

template <typename T> struct is_same<T, T> {
  enum { value = 1 };
};

struct Dispatch {
  template <typename T> using SameAs = is_same<int, T>;
};

template <typename DispatchT>
struct S {
  template <typename T> using same = typename DispatchT::template SameAs<T>;

  template <typename T>
  static void foo() __attribute__((enable_if(same<T>::value, "")));

};

void runFoo() {
  // S<Dispatch>::foo<int>();
  S<FailedDispatch>::foo<float>();

}

This one exercises the code up to if (CanonType.isNull()) {, but I'm not sure how to get a null type here.

[aaron.ballman]

lib/Sema/SemaTemplate.cpp

3055

How about this:

namespace boost {
  template<bool, typename = void> struct enable_if {};
  template<typename T> struct enable_if<true, T> { typedef T type; };
}

template<typename T> struct NonTemplateFunction {
  typename boost::enable_if<sizeof(T) == 4, int>::type f();
};

template <typename T>
struct Bobble {
  using type = T;
};

struct Frobble {
  using type = char;
};
NonTemplateFunction<Bobble<typename Frobble::type>::type> NTFC;

That should trigger the path in Sema::CheckTypenameType(), I believe.

[courbet]

lib/Sema/SemaTemplate.cpp

3055

This triggers:

else if (ClassTemplateDecl *ClassTemplate
               = dyn_cast<ClassTemplateDecl>(Template))

Modifying it to:

namespace boost {
  template<bool, typename = void> struct enable_if {};
  template<typename T> struct enable_if<true, T> { typedef T type; };
}

template<typename T> struct NonTemplateFunction {
  template <typename U> using toto = typename boost::enable_if<sizeof(U) == 4, int>::type;
  toto<T> f();
};

template <typename T>
struct Bobble {
  using type = T;
};

struct Frobble {
  using type = char;
};
NonTemplateFunction<Bobble<typename Frobble::type>::type> NTFC;

triggers the same path as my previous example, but not the IsNull() test.

[aaron.ballman]

LGTM!

lib/Sema/SemaTemplate.cpp

3055

Aw crud, well, we tried.

[courbet]

Thank you both for the review !

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;
 }
 
+// Returns true if `Name` is the name of an std type trait.
+static bool isTypeTraitName(const llvm::StringRef RecordName) {
+  return
+#define TYPE_TRAIT_1(Spelling, Name, Key) RecordName == (#Spelling + 2) ||
+#define TYPE_TRAIT_2(Spelling, Name, Key) RecordName == (#Spelling + 2) ||
+#define TYPE_TRAIT_N(Spelling, Name, Key) RecordName == (#Spelling + 2) ||
+#include "clang/Basic/TokenKinds.def"
+#undef TYPE_TRAIT_1
+#undef TYPE_TRAIT_2
+#undef TYPE_TRAIT_N
+      false;
+}
+
+// If `Record` is a type trait, pretty prints the condition with actual types
+// and returns true.
+static bool prettyPrintTypeTrait(const NestedNameSpecifier *const NNS,
+                                 llvm::raw_string_ostream &OS,
+                                 const PrintingPolicy &PrintPolicy) {
+  // We are looking for a type.
+  if (NNS == nullptr || NNS->getKind() != NestedNameSpecifier::TypeSpec)
+    return false;
+  // In namespace "::std".
+  const NestedNameSpecifier *const Parent = NNS->getPrefix();
+  if (Parent == nullptr ||
+      Parent->getKind() != NestedNameSpecifier::Namespace ||
+      Parent->getAsNamespace()->getName() != "std")
+    return false;
+  const NestedNameSpecifier *const GrandParent = Parent->getPrefix();
+  if (!(GrandParent == nullptr ||
+        GrandParent->getKind() == NestedNameSpecifier::Global))
+    return false;
+  // Now check whether the record name is one of the known type traits.
+  const RecordDecl *const Record = NNS->getAsRecordDecl();
+  if (!isTypeTraitName(Record->getName()))
+    return false;
+  // Print the type trait with resolved template parameters.
+  const auto *const TmplDecl =
+      dyn_cast<ClassTemplateSpecializationDecl>(Record);
+  assert(TmplDecl &&
+         "std type_traits should be ClassTemplateSpecializationDecl");
+  OS << "std::" << Record->getName() << "<";
+  auto Args = TmplDecl->getTemplateArgs().asArray();
+  Args.front().print(PrintPolicy, OS);
+  for (const auto &Arg : Args.drop_front()) {
+    OS << ", ";
+    Arg.print(PrintPolicy, OS);
+  }
+  OS << ">";
+  return true;
+}
+
+// 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 *const FailedCond,
+                                  const PrintingPolicy &PrintPolicy) {
+  if (const auto *const DR = dyn_cast<DeclRefExpr>(FailedCond)) {
+    const auto *const Var = dyn_cast<VarDecl>(DR->getDecl());
+    // This might be `std::some_type_trait<U,V>::value`.
+    if (Var && Var->isStaticDataMember() && Var->getName() == "value" &&
+        prettyPrintTypeTrait(DR->getQualifier(), OS, PrintPolicy)) {
+      OS << "::value";
+      return;
+    }
+  }
+  FailedCond->printPretty(OS, nullptr, PrintPolicy);
+}
+
 std::pair<Expr *, std::string>
 Sema::findFailedBooleanCondition(Expr *Cond, bool AllowTopLevelCond) {
   Cond = lookThroughRangesV3Condition(PP, Cond);
 
   // Separate out all of the terms in a conjunction.
   SmallVector<Expr *, 4> Terms;
   collectConjunctionTerms(Cond, Terms);
 
if (!AllowTopLevelCond)
       return { nullptr, "" };
 
     FailedCond = Cond->IgnoreParenImpCasts();
   }
 
   std::string Description;
   {
     llvm::raw_string_ostream Out(Description);
-    FailedCond->printPretty(Out, nullptr, getPrintingPolicy());
+    prettyPrintFailedBooleanCondition(Out, FailedCond, getPrintingPolicy());
   }
   return { FailedCond, Description };
 }
 
 QualType Sema::CheckTemplateIdType(TemplateName Name,
                                    SourceLocation TemplateLoc,
                                    TemplateArgumentListInfo &TemplateArgs) {
   DependentTemplateName *DTN

test/SemaCXX/static-assert.cpp

struct first_trait<X> {
   static const bool value = true;
 };
 
 template<typename T> struct second_trait {
   static const bool value = false;
 };
 
 static_assert(first_trait<X>::value && second_trait<X>::value, "message"); // expected-error{{static_assert failed due to requirement 'second_trait<X>::value' "message"}}
+
+namespace std {
+
+template <class Tp, Tp v>
+struct integral_constant {
+  static const Tp value = v;
+  typedef Tp value_type;
+  typedef integral_constant type;
+};
+
+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 <class Tp>
+struct is_const : public false_type {};
+template <class Tp>
+struct is_const<Tp const> : public true_type {};
+
+// We do not define is_same in terms of integral_constant to check that both implementations are supported.
+template <typename T, typename U>
+struct is_same {
+  static const bool value = false;
+};
+
+template <typename T>
+struct is_same<T, T> {
+  static const bool value = true;
+};
+
+} // namespace std
+
+struct ExampleTypes {
+  using T = int;
+  using U = float;
+};
+
+static_assert(std::is_same<ExampleTypes::T, ExampleTypes::U>::value, "message"); // expected-error{{static_assert failed due to requirement 'std::is_same<int, float>::value' "message"}}
+static_assert(std::is_const<ExampleTypes::T>::value, "message");                 // expected-error{{static_assert failed due to requirement 'std::is_const<int>::value' "message"}}