This is an archive of the discontinued LLVM Phabricator instance.

[Sema] Improve static_assert diagnostics.
ClosedPublic

Authored by courbet on Nov 26 2018, 8:30 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

courbet created this revision.Nov 26 2018, 8:30 AM
courbet updated this revision to Diff 175465.Nov 27 2018, 5:46 AM

Handle all std type traits.

courbet edited the summary of this revision. (Show Details)Nov 27 2018, 5:47 AM
courbet added a subscriber: pilki.
courbet retitled this revision from [WIP][Sema] Improve static_assert diagnostics. to [Sema] Improve static_assert diagnostics..Nov 27 2018, 5:49 AM
aaron.ballman added inline comments.
lib/Sema/SemaTemplate.cpp
3070 ↗(On Diff #175465)

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

3073–3085 ↗(On Diff #175465)

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

3096 ↗(On Diff #175465)

Please don't use auto here.

3115 ↗(On Diff #175465)

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

Quuxplusone added inline comments.
test/SemaCXX/static-assert.cpp
111 ↗(On Diff #175465)

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 updated this revision to Diff 175638.Nov 28 2018, 1:12 AM
courbet marked 5 inline comments as done.
courbet edited the summary of this revision. (Show Details)

Address Aaron's comments.

courbet marked an inline comment as done.Nov 28 2018, 1:21 AM

Thanks for the comments.

lib/Sema/SemaTemplate.cpp
3070 ↗(On Diff #175465)

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

3115 ↗(On Diff #175465)

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 ↗(On Diff #175465)

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 added inline comments.Nov 28 2018, 5:01 AM
lib/Sema/SemaTemplate.cpp
3076 ↗(On Diff #175638)

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 ↗(On Diff #175465)

Ah, good point!

test/SemaCXX/static-assert.cpp
111 ↗(On Diff #175465)

@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 updated this revision to Diff 175667.Nov 28 2018, 5:06 AM

clang-formant patch

courbet updated this revision to Diff 175668.Nov 28 2018, 5:09 AM
courbet marked 2 inline comments as done.

harden against null getAsRecordDecl().

courbet added inline comments.Nov 28 2018, 5:09 AM
lib/Sema/SemaTemplate.cpp
3076 ↗(On Diff #175638)

Good point, I've added a test.

courbet updated this revision to Diff 175669.Nov 28 2018, 5:10 AM

fix variable name in comment

test/SemaCXX/static-assert.cpp
111 ↗(On Diff #175465)

@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 marked an inline comment as done.Nov 28 2018, 5:34 AM
courbet added inline comments.
test/SemaCXX/static-assert.cpp
111 ↗(On Diff #175465)

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 :) )

lib/Sema/SemaTemplate.cpp
3061 ↗(On Diff #175669)

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 ↗(On Diff #175669)

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 updated this revision to Diff 175673.Nov 28 2018, 5:40 AM

Add unit test with dependent types.

courbet updated this revision to Diff 175679.Nov 28 2018, 6:29 AM

Replace custom printing code with clang helper.

courbet updated this revision to Diff 175710.Nov 28 2018, 9:30 AM
courbet marked 2 inline comments as done.

expand types in all qualifiers, not only type traits.

courbet marked 2 inline comments as done.Nov 28 2018, 9:31 AM
courbet added inline comments.
lib/Sema/SemaTemplate.cpp
3061 ↗(On Diff #175669)

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.

lib/Sema/SemaTemplate.cpp
3064 ↗(On Diff #175710)

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 ↗(On Diff #175710)

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

141 ↗(On Diff #175710)

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 updated this revision to Diff 175855.Nov 29 2018, 5:13 AM
courbet marked 5 inline comments as done.
  • add more tests
  • handle c++17 constructs
  • add c++17 tests in static-assert-cxx17.cpp

Thanks

lib/Sema/SemaTemplate.cpp
3064 ↗(On Diff #175710)

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 ↗(On Diff #175710)

Thanks for the tip.

141 ↗(On Diff #175710)

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 added inline comments.Nov 30 2018, 6:26 AM
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.

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 ↗(On Diff #175855)

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 ↗(On Diff #175855)

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 ↗(On Diff #175855)

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 added inline comments.Dec 1 2018, 9:50 AM
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 ↗(On Diff #175855)

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

courbet marked 4 inline comments as done.Dec 3 2018, 1:39 AM
courbet added inline comments.
lib/AST/NestedNameSpecifier.cpp
308–310 ↗(On Diff #175855)

SG, thanks for the pointer to dyn_cast_or_null.

lib/Sema/SemaTemplate.cpp
3071 ↗(On Diff #175855)

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 updated this revision to Diff 176320.Dec 3 2018, 1:40 AM
  • Fix spurious formating changes
  • Add tests
  • Improve readability.
aaron.ballman added inline comments.Dec 3 2018, 4:45 AM
lib/Sema/SemaTemplate.cpp
3055 ↗(On Diff #176320)

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 marked an inline comment as done.Dec 3 2018, 5:44 AM
courbet added inline comments.
lib/Sema/SemaTemplate.cpp
3055 ↗(On Diff #176320)

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 added inline comments.Dec 3 2018, 6:34 AM
lib/Sema/SemaTemplate.cpp
3055 ↗(On Diff #176320)

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 marked an inline comment as done.Dec 3 2018, 7:00 AM
courbet added inline comments.
lib/Sema/SemaTemplate.cpp
3055 ↗(On Diff #176320)

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 accepted this revision.Dec 3 2018, 7:09 AM

LGTM!

lib/Sema/SemaTemplate.cpp
3055 ↗(On Diff #176320)

Aw crud, well, we tried.

This revision is now accepted and ready to land.Dec 3 2018, 7:09 AM

Thank you both for the review !

This revision was automatically updated to reflect the committed changes.