[clang] New static_assert message is sometimes redundant

[cjdb]

The new static_assert message is great, there are some simple messages where an expansion isn't particularly helpful.

For example

constexpr auto is_gitlab = false;
constexpr auto is_weekend = false;

static_assert(is_gitlab or is_weekend);

will give us

<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);
              ~~~~~~~~~~^~~~~~~~~~~~~

The note isn't necessary, since the error implies that the expression will be false or false. I noticed that if we change the expression to a conjunction, we get a simpler message, so it looks like the machinery might already be present.

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

We should---on a best effort basis---aim to eliminate expansions that don't add value to minimise user frustration (this will give us slightly more room to have longer diagnostics elsewhere).

[llvmbot]

@llvm/issue-subscribers-good-first-issue

[llvmbot]

@llvm/issue-subscribers-clang-frontend

[tbaederr]

The code already tries to minimize unnecessary notes, but this seems to be missing.

The relevant function is Sema::DiagnoseStaticAssertDetails() in clang/lib/Sema/SemaDeclCXX.cpp.

[Fznamznon]

I poked around this one little bit and the fact that the note is not emitted for conjunction seems to be a coincidence. In case of conjunction DiagnoseStaticAssertDetails gets not a binary expression as an input but just a DeclRefExpr referencing on of the operands and early exits. I guess it is just the way findFailedBooleanCondition works whose output is then passed to DiagnoseStaticAssertDetails , it searches for a failed condition of a conjunction so in case of other boolean expressions like or, xor and etc are passed to findFailedBooleanCondition it returns the whole expression.
I'm thinking we could try check if operands of the binary expression is constexpr variables that are initialized by boolean literals. WDYT?

[Krishna-13-cyber]

Hello @tbaederr @cjdb ,
I was just working on this issue, there is a specific condition which is mentioned in the DiagnoseStaticAssertDetails so if we remove that case or just return nothing, it wont give the note that is currently being emitted/displayed.

Please let me know if this is the correct approach!

[tbaederr]

I think that is the output @cjdb expected, but we can't judge if your approach is the right one without seeing a patch.

[Unique-Usman]

@Krishna-13-cyber , are you still working on this?

[Krishna-13-cyber]

Yes, I have solved this and has been merged recently!

[tbaederr]

Closing this then.