Page MenuHomePhabricator

[Sema] Further improvements to to static_assert diagnostics.
ClosedPublic

Authored by courbet on Tue, Dec 4, 7:16 AM.

Details

Diff Detail

Repository
rL LLVM

Event Timeline

courbet created this revision.Tue, Dec 4, 7:16 AM
aaron.ballman added inline comments.Tue, Dec 4, 2:24 PM
lib/Sema/SemaTemplate.cpp
3065 ↗(On Diff #176634)

Is this definition necessary?

Quuxplusone added inline comments.Tue, Dec 4, 2:31 PM
lib/Sema/SemaTemplate.cpp
3062 ↗(On Diff #176634)

explicit

3065 ↗(On Diff #176634)

Nit: I don't know if it is LLVM style to provide explicitly written-out overrides for all virtual destructors. In my own code, if a destructor would be redundant, I wouldn't write it.

3089 ↗(On Diff #176634)

} // end anonymous namespace

test/SemaCXX/static-assert.cpp
117 ↗(On Diff #176634)

Please also add a test case for the is_const_v inline-variable-template version.

template<class T>
inline constexpr bool is_const_v = is_const<T>::value;
static_assert(is_const_v<ExampleTypes::T>, "message");  // if this test case was missing from the previous patch
static_assert(!is_const_v<ExampleTypes::ConstT>, "message");  // exercise the same codepath for this new feature

Also, does using the PrinterHelper mean that you get a bunch of other cases for free? Like, does this work now too?

static_assert(is_const<T>::value == false, "message");
courbet marked 7 inline comments as done.Wed, Dec 5, 1:11 AM
courbet added inline comments.
lib/Sema/SemaTemplate.cpp
3065 ↗(On Diff #176634)

I don't think the style guide says anything; removed.

test/SemaCXX/static-assert.cpp
117 ↗(On Diff #176634)

Please also add a test case for the is_const_v inline-variable-template version.

done (in c++17 tests)

Also, does using the PrinterHelper mean that you get a bunch of other cases for free? Like, does this work now too?

Exactly. While I'm at it I've added tests for your use case and removed the no longer needed AllowTopLevel parameter.

courbet updated this revision to Diff 176771.Wed, Dec 5, 1:11 AM
courbet marked 2 inline comments as done.
  • Address comments
  • Add more tests
  • Remove AllowTopLevel and handle cases like sizeof(T) (-> sizeof(int))
courbet updated this revision to Diff 176773.Wed, Dec 5, 1:21 AM
  • Update PHC and C11 tests.
aaron.ballman added inline comments.Thu, Dec 6, 9:24 AM
test/PCH/cxx-static_assert.cpp
17 ↗(On Diff #176773)

I'm not certain how I feel about now printing the failure condition when there's an explicit message provided. From what I understand, a fair amount of code in the wild does static_assert(some_condition, "some_condition") because of older language modes where the message was not optional. I worry we're going to start seeing a lot of diagnostics like: static_assert failed due to requirement '1 == 2' "N == 2", which seems a bit low-quality. See DFAPacketizer::DFAPacketizer() in LLVM as an example of something similar.

Given that the user entered a message, do we still want to show the requirement? Do we feel the same way if the requirement is fairly large?

courbet marked an inline comment as done.Fri, Dec 7, 1:09 AM
courbet added inline comments.
test/PCH/cxx-static_assert.cpp
17 ↗(On Diff #176773)

The issue is that "N == 2" is a useless error message. Actually, since the error message has to be a string literal, there is no way for the user to print a debuggable output. So I really think we should print the failed condition.

Quuxplusone added inline comments.Fri, Dec 7, 7:07 AM
test/PCH/cxx-static_assert.cpp
17 ↗(On Diff #176773)

FWIW, I also don't agree with Aaron's concern.

I do think there is a lot of code in the wild whose string literal was "phoned in." After all, this is why C++17 allows us to omit the string literal: to avoid boilerplate like this.

static_assert(some-condition, "some-condition");
static_assert(some-condition, "some-condition was not satisfied");
static_assert(some-condition, "some-condition must be satisfied");
static_assert(some-condition, "");

But should Clang go _out of its way_ to suppress such "redundant" string literals? First of all, such suppression would be C++14-and-earlier: if a C++17-native program contains a string literal, we should maybe assume it's on purpose. Second, it's not clear how a machine could detect "redundant" literals with high fidelity.

static_assert(std::is_integral<T>, "std::is_integral<T>");
    // clearly redundant
static_assert(std::is_integral<T>, "T must be integral");
    // redundant, but unlikely to be machine-detectable as such

static_assert((DFA_MAX_RESTERMS * DFA_MAX_RESOURCES) <= (8 * sizeof(DFAInput)),
    "(DFA_MAX_RESTERMS * DFA_MAX_RESOURCES) > (8 * sizeof(DFAInput))");
    // redundant, but unlikely to be machine-detectable as such
    // thanks to the substitution of > for <=

static_assert((DFA_MAX_RESTERMS * DFA_MAX_RESOURCES) <= (8 * sizeof(DFAInput)),
    "(DFA_MAX_RESTERMS * DFA_MAX_RESOURCES) too big for DFAInput");
    // redundant, but unlikely to be machine-detectable as such

In any event, I agree with @courbet that Clang should print the expansion of the failed condition in any case.

Also: One might argue that if the static_assert fits completely on one source line, then we could omit the string-literal from our error message because the string literal will be shown anyway as part of the offending source line — but I believe IDE-users would complain about that, so, we shouldn't do that. And even then, we'd still want to print the failed condition!

Checking my understanding: The static_assert above (taken from DFAPacketizer::DFAPacketizer() in LLVM) would be unchanged by @courbet's patches, because none of its subexpressions are template-dependent. Right?

aaron.ballman added inline comments.Fri, Dec 7, 7:28 AM
test/PCH/cxx-static_assert.cpp
17 ↗(On Diff #176773)

But should Clang go _out of its way_ to suppress such "redundant" string literals?

I wasn't suggesting it should; I was suggesting that Clang should be conservative and suppress printing the conditional when a message is present, not when they look to be redundant enough.

if a C++17-native program contains a string literal, we should maybe assume it's on purpose.

This is the exact scenario I was envisioning.

It's a relatively weak preference, but I kind of prefer not displaying the conditional information in the presence of a message (at least in C++17 and above), especially as the conditional can be huge. I'm thinking of scenarios where the user does something like:

static_assert(condition1 && condition2 && (condition3 || condition4), "Simple explanation");

except that condition is replaced by std::some_type_trait<Blah> in various interesting ways.

Quuxplusone added inline comments.
test/PCH/cxx-static_assert.cpp
17 ↗(On Diff #176773)

I'm thinking of scenarios where the user does something like:

static_assert(condition1 && condition2 && (condition3 || condition4), "Simple explanation");

except that condition is replaced by std::some_type_trait<Blah> in various interesting ways.

I think the risk is extremely high that "Simple explanation" will not be actually useful to the person compiling the code, and they'll want to know exactly what failed and why (which is the direction Clement's patches are taking us). I also see the same trend happening with C++2a Concepts in @saar.raz's fork: concept diagnostics are quite verbose compared to type-trait diagnostics because it's useful to tell the user "hey, Regular<T> failed because Copyable<T> failed because MoveAssignable<T> failed because..." and eventually get all the way down to the _real_ problem, which ends up being something like "T was deduced as int&". An additional "Simple explanation" can be helpful, but is rarely as useful as the full story.

aaron.ballman accepted this revision.Sun, Dec 9, 11:47 AM

LGTM!

test/PCH/cxx-static_assert.cpp
17 ↗(On Diff #176773)

Okay, I find that to be a compelling argument, thank you @Quuxplusone!

This revision is now accepted and ready to land.Sun, Dec 9, 11:47 AM
This revision was automatically updated to reflect the committed changes.

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

I agree. I'll have a look at it.