Update static_assert message for redundant cases

[Krishna-13-cyber]

Summary

There are some simple messages where an expansion isn't particularly helpful or needed. We aim to eliminate expansions that don't add much value for user (this will give us slightly more room or prioritise to have longer diagnostics elsewhere).

The test case for which it has been tested:

constexpr auto is_gitlab = false;
constexpr auto is_weekend = false;
static_assert(is_gitlab or is_weekend);

Previous warning/error message:

<source>:4:1: error: static assertion failed due to requirement 'is_gitlab || is_weekend'
static_assert(is_gitlab or is_weekend);
^             ~~~~~~~~~~~~~~~~~~~~~~~
<source>:4:25: note: expression evaluates to 'false || false'
static_assert(is_gitlab or is_weekend);
              ~~~~~~~~~~^~~~~~~~~~~~~

Currrent warning/error message:

<source>:4:1: error: static assertion failed due to requirement 'is_gitlab'
static_assert(is_gitlab and is_weekend);
^             ~~~~~~~~~

This patch aims to remove some redundant cases of static assert messages where the expansions are particularly unhelpful. In this particular patch, we have ignored the printing of diagnostic warnings for binary operators with logical OR operations.
This is done to prioritise and prefer to emit longer diagnostic warnings for more important concerns elsewhere.

[tbaeder]

Looks like you're just removing the output altogether, so that won't work.

Try running ninja check-clang-semacxx and see all the test failures you (should) get. For a proper patch, it would also be good if you add a test case for the case you're fixing.

[cjdb]

Thanks for working on this, it's much appreciated!

<source>:4:1: error: static assertion failed due to requirement 'is_gitlab'
static_assert(is_gitlab and is_weekend);
^             ~~~~~~~~~

The arrow seems a bit off here.

[Krishna-13-cyber]

I have updated the patch without removing the whole of the previous block.I have added a condition to find the case where we don't need a diagnostic message to be printed or warned.

It passes all test cases without any failure or regression!

[tbaeder]

Since we only handle BinaryOperators in the toplevel assertion expression, I think it should just be safe to never diagnose for them if it's an BO_LOr opcode, so you should be able to just check for that in DiagnoseStaticAssertDetails().

[Krishna-13-cyber]

Have worked on the top level with Boolean literals unlike the previous diffs.I have updated the test case as well with this new upcoming change.

Thanks!

[cjdb]

clang/test/SemaCXX/static-assert.cpp

261–265

We should also have a test for conjunctions, disjunctions, and negations.

[tbaeder]

I think just checking that the toplevel binary operator is a logical or should be enough? Since that only fails if both operands evaluate to false, so we don't need to print the "false or false" diagnostic.

clang/test/SemaCXX/static-assert.cpp

262

This diagnostic should be kept. From looking at the condition, it is not obvious what the two functions evaluate to.

[Krishna-13-cyber]

The above Binary operator test case says that there are two Boolean expressions,UsefulToPrintExpr says we can avoid to print these expressions as they are don't need a note and are understandable.

So if we go by this we will have to remove the note.By this we are removing note for boolean literals as well as expressions.It will be nice to make it generic rather than specifically target cases of false || false , false && false. As the warning note print out the boolean values which can be avoided giving preference to the other diagnostics in terms of boolean literals and expressions.

Yes, I making a new diff for test cases of conjunction and disjunction as well.

clang/test/SemaCXX/static-assert.cpp

261–265

Yes, I making a new diff for test cases of conjunction and disjunction as well.

262

The above Binary operator test case says that there are two Boolean expressions,UsefulToPrintExpr says we can avoid to print these expressions as they are don't need a note and are understandable.

So if we go by this we will have to remove the note.By this we are removing note for boolean literals as well as expressions.It will be nice to make it generic rather than specifically target cases of false || false , false && false. As the warning note print out the boolean values which can be avoided giving preference to the other diagnostics in terms of boolean literals and expressions.

[tbaeder]

clang/test/SemaCXX/static-assert.cpp

262

I think you are confusing boolean expressions with boolean literals.

What is the problem with fixing this issue by simply ignoring toplevel BO_LOr binary operators?

[Krishna-13-cyber]

Updated with the new test cases and implemented the logic suggested for ignoring the BO_LOr operators at the top level. My sincere apologies @tbaeder that I could not place this logic at the first instance.

Thanks!

[tbaeder]

Remember to upload you patches with context.

clang/lib/Sema/SemaDeclCXX.cpp

16732

You can just drop the comment and move this into the if statement above, e..g if (...; BO && BO->getOpcode() != BO_Lor).

clang/test/SemaCXX/static-assert.cpp

262

That comment doesn't make a lot of sense?

263

Remove the backslash

269

Neither this one.

270

Remove the backslash

[Krishna-13-cyber]

I have removed the redundant comments and have moved the if condition above with the ongoing conditional statement (to avoid printing obvious expressions).

• Updated static_assert warning message
• Added Test cases

[tbaeder]

Make sure to include context in the patch you upload: https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

clang/lib/Sema/SemaDeclCXX.cpp

16732

I was talking about the one that checks that E is a BinaryOperator.

[Krishna-13-cyber]

This patch aims to remove some redundant cases of static assert messages where the expansions are particularly unhelpful. In this particular patch, we have ignored the printing of diagnostic warnings for binary operators with logical OR operations.
This is done to prioritise and prefer to emit longer diagnostic warnings for more important concerns elsewhere.

[aaron.ballman]

clang/test/SemaCXX/static-assert.cpp

262

+1 -- please spell out more of the diagnostic wording (I see you're following a pattern elsewhere in the file, but we should have enough diagnostic wording to know what the diagnostic is.)

It's hard for me to tell from this review because the patch was not uploaded with context (next time, please use -U9999 when making the patch diff so that this context exists), but is there test coverage showing what happens when the operators are chained together? e.g.,

static_assert(inverted(true) || invert(true) || false, "");
static_assert((true && invert(true)) || false, "");
static_assert(true && inverted(false) && inverted(true), "");

[Krishna-13-cyber]

• Updated patch
• Added testcases for chained operators

[tbaeder]

clang/lib/Sema/SemaDeclCXX.cpp

16718

This line is over 80 colums now, remember to format it: https://llvm.org/docs/Contributing.html#format-patches

[aaron.ballman]

clang/test/SemaCXX/static-assert.cpp

266–268

We really need to see more of the diagnostic to know whether the behavior is reasonable or not.

I was thinking we'd have given a note as to why the failure occurred, but I see now that the current behavior doesn't give a note either. Not that I think this is related to your patch, but the current behavior seems a bit surprising: https://godbolt.org/z/erPGdsanK Note how the last diagnostic points to exactly where the failure comes from but the first two use the entire expression as the failure.

llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn

26

This seems like an unrelated change.

[Krishna-13-cyber]

• Updated with clang-formatting
• Removed the redundant from the diff

Actually I had done that at the first place clang-format -i file.cpp but that seems to make a lot of changes and the diff becomes really big. I was a bit uncertain on that due to which I did not use it.
I have renewed the diff.
Thanks!

[tbaeder]

Does git clang-format HEAD~1 not work on your system? It should only format the changed parts, not everything.

[Krishna-13-cyber]

• Updated diff with git clang-format HEAD~1

[aaron.ballman]

The changes need a release note and please do add more text to the expected-error comments so we can see what the diagnostic actually produces (instead of just {{failed}}).

[Krishna-13-cyber]

• Updated with release note
• I had tried adding more text to the expected-error but it already gives a diagnostic of static assertion failed due to requirement currently. If I try additions to {{failed}} then we get error diagnostics expected but not seen.

[aaron.ballman]

In D146376#4244355, @Krishna-13-cyber wrote:

• Updated with release note
• I had tried adding more text to the expected-error but it already gives a diagnostic of static assertion failed due to requirement currently. If I try additions to {{failed}} then we get error diagnostics expected but not seen.

Ah, sorry for being unclear! We didn't mean add an additional expected-error comment, but to modify the ones you have. e.g.,

Currently:

static_assert(true && false, ""); // expected-error {{failed}}

Changes to:

static_assert(true && false, ""); // expected-error {{static assertion failed due to requirement 'true && false'}}

(or whatever the actual expected full text of the diagnostic is.)

This helps the reviewers to make sure that the diagnostic behavior isn't regressing in surprising ways but pass all our tests because we're only looking for the word failed and considering that passing.

[Krishna-13-cyber]

• Updated with the diagnostic messages (comments for test cases)

[aaron.ballman]

LGTM aside from a small nit with the release notes. Do you need someone to land this on your behalf? If so, please rebase the patch so it applies cleanly to trunk and upload it again, and let me know what name and email address you would like used for patch attribution.

clang/docs/ReleaseNotes.rst

183 ↗

(On Diff #511181)

[Krishna-13-cyber]

Thanks a lot everyone for helping me out with my first patch. I have uploaded the patch again after rebase.
Yes, It would be great if you could land it on my behalf.
Name: Krishna Narayanan
Email: <[e-mail]>

Thanks again!

[xbolva00]

The patch description mixes "or" and "and" in examples, please fix it.

SO probably you meant

<source>:4:1: error: static assertion failed due to requirement 'is_gitlab'
static_assert(is_gitlab or is_weekend);
^             ~~~~~~~~~

[aaron.ballman]

In D146376#4250814, @xbolva00 wrote:

The patch description mixes "or" and "and" in examples, please fix it.

SO probably you meant

<source>:4:1: error: static assertion failed due to requirement 'is_gitlab'
static_assert(is_gitlab or is_weekend);
^             ~~~~~~~~~

Good catch, I'll fix that when landing.

clang/lib/Sema/SemaDeclCXX.cpp

if (isa<BinaryOperator>(E))
     return false;
 
   return true;
 }
 
 /// Try to print more useful information about a failed static_assert
 /// with expression \E
 void Sema::DiagnoseStaticAssertDetails(const Expr *E) {
-  if (const auto *Op = dyn_cast<BinaryOperator>(E)) {
+  if (const auto *Op = dyn_cast<BinaryOperator>(E);Op && Op->getOpcode() != BO_LOr) {
     const Expr *LHS = Op->getLHS()->IgnoreParenImpCasts();
     const Expr *RHS = Op->getRHS()->IgnoreParenImpCasts();
 
     // Ignore comparisons of boolean expressions with a boolean literal.
     if ((isa<CXXBoolLiteralExpr>(LHS) && RHS->getType()->isBooleanType()) ||
         (isa<CXXBoolLiteralExpr>(RHS) && LHS->getType()->isBooleanType()))
       return;
 
     // Don't print obvious expressions.
     if (!UsefulToPrintExpr(LHS) && !UsefulToPrintExpr(RHS))
       return;
 
     struct {
       const clang::Expr *Cond;
       Expr::EvalResult Result;
       SmallString<12> ValueString;
       bool Print;
     } DiagSide[2] = {{LHS, Expr::EvalResult(), {}, false},
                      {RHS, Expr::EvalResult(), {}, false}};
     for (unsigned I = 0; I < 2; I++) {
       const Expr *Side = DiagSide[I].Cond;
 

clang/test/SemaCXX/static-assert.cpp

namespace Diagnostics {
   }
   static_assert(getChar() == 'a', ""); // expected-error {{failed}} \
                                        // expected-note {{evaluates to ''c' == 'a''}}
 
   /// Bools are printed as bools.
   constexpr bool invert(bool b) {
     return !b;
   }
+
+  static_assert(invert(true) || invert(true), ""); // expected-error {{failed}}
   static_assert(invert(true) == invert(false), ""); // expected-error {{failed}} \
                                                     // expected-note {{evaluates to 'false == true'}}
+  static_assert(true && false, ""); // expected-error {{failed}}
+  static_assert(invert(true) || invert(true) || false, ""); // expected-error {{failed}}
+  static_assert((true && invert(true)) || false, ""); // expected-error {{failed}}
+  static_assert(true && invert(false) && invert(true), ""); // expected-error {{failed}}
 
   /// No notes here since we compare a bool expression with a bool literal.
   static_assert(invert(true) == true, ""); // expected-error {{failed}}
 
 #pragma clang diagnostic push
 #pragma clang diagnostic ignored "-Wc99-extensions"
   constexpr _Complex float com = {5,6};
   constexpr _Complex float com2 = {1, 9};
   static_assert(com == com2, ""); // expected-error {{failed}} \
                                   // expected-note {{evaluates to '(5 + 6i) == (1 + 9i)'}}
 #pragma clang diagnostic pop

llvm/utils/gn/secondary/clang-tools-extra/clangd/unittests/BUILD.gn

deps = [
     "//clang/lib/AST",
     "//clang/lib/Basic",
     "//clang/lib/Format",
     "//clang/lib/Frontend",
     "//clang/lib/Index",
     "//clang/lib/Lex",
     "//clang/lib/Sema",
     "//clang/lib/Serialization",
+    "//clang/lib/Testing",
     "//clang/lib/Tooling",
     "//clang/lib/Tooling/Core",
     "//clang/lib/Tooling/Inclusions",
     "//clang/lib/Tooling/Syntax",
     "//llvm/include/llvm/Config:llvm-config",
     "//llvm/lib/Support",
     "//llvm/lib/Testing/Annotations",
     "//llvm/lib/Testing/Support",