Page MenuHomePhabricator

[clang] Print more information about failed static assertions
ClosedPublic

Authored by tbaeder on Aug 1 2022, 5:19 AM.

Details

Summary

For

static constexpr m = 10;
static_assert(m == 11);

the output is before:

./test.cpp:2:1: error: static_assert failed due to requirement 'm == 11'
static_assert(m == 11);
^             ~~~~~~~
1 error generated.

and after:

./test.cpp:2:1: error: static assertion failed due to requirement 'm == 11'
static_assert(m == 11);
^             ~~~~~~~
./test.cpp:2:15: note: left-hand side of operator '==' evaluates to '10'
static_assert(m == 11);
              ^
1 error generated.

The patch adds a new function Sema::DiagnoseStaticAssertDetails(), which currently only handles binary operators with an integer literal on one side and a non-integer literal on the other side. This is intentionally kept simple, but can be improved incrementally over time of course.

Diff Detail

Event Timeline

tbaeder created this revision.Aug 1 2022, 5:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 1 2022, 5:19 AM
tbaeder requested review of this revision.Aug 1 2022, 5:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 1 2022, 5:19 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
tbaeder added inline comments.Aug 1 2022, 5:24 AM
clang/lib/Sema/SemaDeclCXX.cpp
16743

Oops, this needs to go.

tbaeder updated this revision to Diff 449199.Aug 2 2022, 12:13 AM
tbaeder edited the summary of this revision. (Show Details)Aug 2 2022, 1:20 AM
tbaeder updated this revision to Diff 449226.Aug 2 2022, 1:49 AM

Oh, I noticed that g++ already does this:

test.cpp:27:21: error: static assertion failed
   27 | static_assert(foo() < 4);
      |               ~~~~~~^~~
test.cpp:27:21: note: the comparison reduces to ‘(10 < 4)’

Wow, I *really* like this improvement, thank you for working on it! This is going to be especially helpful for things like static_assert(foo() == 12, "oh no, foo wasn't 12!");. Can you also add a release note for this improvement?

clang/include/clang/Basic/DiagnosticSemaKinds.td
1536

Should probably add at least one test that shows the "right-hand" version of the diagnostic.

clang/lib/Sema/SemaDeclCXX.cpp
16564–16571

I think we want a little bit more smarts here -- -2 is not an IntegerLiteral, it's a unary expression whose subexpression is an integer literal. I think we need to handle unary expressions because it'd be weird for foo() == 12 to get helpful information but foo() == -12 not to.

16579–16580

Similar issue here for unary expressions; we probably don't want 1 == -12 to get past this check.

16587–16590

This same logic seems correct for float, fixed point, and complex (though this one may need some extra work because of the real and imaginary parts). Any reason not to handle those as well?

tbaeder updated this revision to Diff 449589.Aug 3 2022, 1:49 AM

I just took the plunge and made DiagnoseStaticAssertDetails() take into account all static assertions, regardless of integer literals. It now also prints integers, booleans, chars and floats correctly.

For example:

test.cpp:30:1: error: static assertion failed due to requirement 'inv(true) == inv(false)'
static_assert(inv(true) == inv(false));
^             ~~~~~~~~~~~~~~~~~~~~~~~
test.cpp:30:15: note: left-hand side of operator '==' evaluates to 'false'
static_assert(inv(true) == inv(false));
              ^~~~~~~~~
test.cpp:30:28: note: right-hand side of operator '==' evaluates to 'true'
static_assert(inv(true) == inv(false));
                           ^~~~~~~~~~

the output when both sides are printed is rather clumsy to me, and some things are not handled at all (e.g. failed static assertions when the toplevel expression is not a BinaryOpertator, e.g. static_assert(!k) - the user doesn't know what value k is, which is important in case k is of an integer type).

But again, this kind of begs people to suggest more and more improvements, but I think this is a good enough start :)

tbaeder added inline comments.Aug 3 2022, 1:55 AM
clang/lib/Sema/SemaDeclCXX.cpp
16574

I really hope there's a better way to do this that I just don't know about :)

tbaeder updated this revision to Diff 449625.Aug 3 2022, 3:48 AM

Whatever, I had time and you're still asleep, so I updated the output:

test.cpp:30:1: error: static assertion failed due to requirement 'inv(true) == inv(false)'
static_assert(inv(true) == inv(false));
^             ~~~~~~~~~~~~~~~~~~~~~~~
test.cpp:30:25: note: expression evaluates to false == true
static_assert(inv(true) == inv(false));
              ~~~~~~~~~~^~~~~~~~~~~~~
1 error generated.
tbaeder marked 3 inline comments as done and an inline comment as not done.Aug 3 2022, 3:49 AM
tbaeder updated this revision to Diff 449637.Aug 3 2022, 5:06 AM

Also print nullptr expressions.

tbaeder updated this revision to Diff 449639.Aug 3 2022, 5:23 AM

FWIW, complex numbers are already covered it seems:

test.cpp:35:1: error: static assertion failed due to requirement '__real c == __imag c'
static_assert(__real c == __imag c);
^             ~~~~~~~~~~~~~~~~~~~~
test.cpp:35:24: note: expression evaluates to 5 == 6
static_assert(__real c == __imag c);
              ~~~~~~~~~^~~~~~~~~~~
1 error generated.
tbaeder updated this revision to Diff 449669.Aug 3 2022, 8:04 AM

Ignore what I said, they are supported now though.

FWIW, complex numbers are already covered it seems:

test.cpp:35:1: error: static assertion failed due to requirement '__real c == __imag c'
static_assert(__real c == __imag c);
^             ~~~~~~~~~~~~~~~~~~~~
test.cpp:35:24: note: expression evaluates to 5 == 6
static_assert(__real c == __imag c);
              ~~~~~~~~~^~~~~~~~~~~
1 error generated.

Use ‘5 ==6’ ? So add quotes ..

Use ‘5 ==6’ ? So add quotes ..

+1 to the suggestion to use quotes for a bit of visual distinction between the diagnostic message and the code embedded within it.

clang/lib/Sema/SemaDeclCXX.cpp
16573–16584

Thankfully, there is a better way:

llvm::raw_svector_ostream OS(Str);
OS << (BoolValue ? "true" : "false");

which applies to the other cases in this function as well.

16647–16649

FixedPointLiteral? ImaginaryLiteral?

16658
16672
tbaeder marked 4 inline comments as done.Aug 3 2022, 9:58 PM

+1 to the suggestion to use quotes for a bit of visual distinction between the diagnostic message and the code embedded within it.

One problem is that both the integer value 0 and the character constant '0' are printed as '0' (same for all other single-digit numbers). gcc's output doesn't have that problem because it prints chars as integers, but I don't really like that.

tbaeder updated this revision to Diff 449874.Aug 3 2022, 9:58 PM
tbaeder updated this revision to Diff 449885.Aug 3 2022, 11:32 PM

+1 to the suggestion to use quotes for a bit of visual distinction between the diagnostic message and the code embedded within it.

One problem is that both the integer value 0 and the character constant '0' are printed as '0' (same for all other single-digit numbers). gcc's output doesn't have that problem because it prints chars as integers, but I don't really like that.

Agreed on not printing chars as integers, but would printing ''0'' in that case to make it more "clear" that it's a character constant? I think we'd want that for something like static_assert('a' < 0, "what?") so it's not printed as 'a < 0'.

IMO it's clear enough...

test.cpp:24:1: error: static assertion failed due to requirement 'c != c'
static_assert(c != c);
^             ~~~~~~
test.cpp:24:17: note: expression evaluates to 'a != a'
static_assert(c != c);
              ~~^~~~
test.cpp:25:1: error: static assertion failed due to requirement 'c < 'a''
static_assert(c < 'a');
^             ~~~~~~~
test.cpp:25:15: note: left-hand side of operator '<' evaluates to 'a'
static_assert(c < 'a');
              ^
2 errors generated.

Printing the first note as ''a' != 'a'' doesn't make it clearer. But for

test.cpp:24:1: error: static assertion failed due to requirement 'c != c'
static_assert(c != c);
^             ~~~~~~
test.cpp:24:17: note: expression evaluates to '0 != 0'
static_assert(c != c);
              ~~^~~~
test.cpp:25:1: error: static assertion failed due to requirement 'c > 'a''
static_assert(c > 'a');
^             ~~~~~~~
test.cpp:25:15: note: left-hand side of operator '>' evaluates to '0'
static_assert(c > 'a');
              ^

where c is a char variable, the output looks kind of weird. "0 != 0" looks like we're comparing two integers.

We might also go the g++ route and use evaluates to ('0' != '0') for the full expression, but that looks weird for just one side: evaluates to (6). But we could also always print the full expression so we only have one case.

personally I dont see huge value to print “ left-hand side of operator '==' evaluates to …”

I would like to see the full expression.

+1 to just copy gcc’s approach

tbaeder updated this revision to Diff 449942.Aug 4 2022, 5:45 AM
tbaeder marked an inline comment as done.Aug 4 2022, 6:13 AM

For the record, the output now looks something like this:

test.cpp:24:1: error: static assertion failed due to requirement 'c != c'
static_assert(c != c);
^             ~~~~~~
test.cpp:24:17: note: expression evaluates to '('0' != '0')'
static_assert(c != c);
              ~~^~~~
test.cpp:25:1: error: static assertion failed due to requirement 'c > 'a''
static_assert(c > 'a');
^             ~~~~~~~
test.cpp:25:17: note: expression evaluates to '('0' > 'a')'
static_assert(c > 'a');
              ~~^~~~~
2 errors generated.
aaron.ballman added reviewers: erichkeane, cjdb, Restricted Project, jyknight.Aug 5 2022, 5:34 AM

For the record, the output now looks something like this:

test.cpp:24:1: error: static assertion failed due to requirement 'c != c'
static_assert(c != c);
^             ~~~~~~
test.cpp:24:17: note: expression evaluates to '('0' != '0')'
static_assert(c != c);
              ~~^~~~

This looks hard to read to me...

test.cpp:25:1: error: static assertion failed due to requirement 'c > 'a''
static_assert(c > 'a');
^ ~~~~~~~
test.cpp:25:17: note: expression evaluates to '('0' > 'a')'

Same confusion here -- it took me a while to realize what's happening is that we're quoting the part outside of the parens and that's throwing me for a loop. We typically quote things that are syntax but in this case, the parens are not part of the syntax and so the surrounding quotes are catching me. Given that parens are being used as a visual delimiter and a single quote serves the same purpose, I would expect something more like:

test.cpp:25:17: note: expression evaluates to ''0' > 'a''
test.cpp:26:17: note: expression evaluates to '0 > 'a''
test.cpp:27:17: note: expression evaluates to '14 > 5'
test.cpp:28:17: note: expression evaluates to 'true == false'
test.cpp:29:17: note: expression evaluates to 'nullptr != nullptr'

Adding a few more folks as reviewers to see if there is a consensus position on how to proceed.

or

test.cpp:25:17: note: evaluated expression: '0' > 'a'

I don't know much about fixit hints, would they be appropriate to display it below the line itself, e.g.

test.cpp:24:17: note: expression evaluates to
static_assert(c != c);
            '0' != '0'
              ~~^~~~

Or would it be even better to just inline the evaluated expression into the static_assert ala

test.cpp:24:17: note: expression evaluates to
static_assert('0' != '0');
                ~~^~~~

I don't know much about fixit hints, would they be appropriate to display it below the line itself, e.g.

test.cpp:24:17: note: expression evaluates to
static_assert(c != c);
            '0' != '0'
              ~~^~~~

Nope, that would be a bad thing -- Clang has the ability to automatically apply fix-it hints to the source, so this would modify the static assertion to continue to be false which isn't much of a fix.

Or would it be even better to just inline the evaluated expression into the static_assert ala

test.cpp:24:17: note: expression evaluates to
static_assert('0' != '0');
                ~~^~~~

Making sure I'm following along, you mean that we'd display the error for the static assertion, and then the note would show static_assert(<actual values in the reconstituted expression>)?

I think this might work for very simple static asserts, but what about more complicated ones like static_assert(foo() && (bar() == 12 || baz()) && 'a' != quux(4000));?

Any tests with macros?

cjdb added a comment.Aug 5 2022, 8:43 AM

Thank you so much for working on this! It's been on my todo list for a while and just haven't gotten around to it.

For the record, the output now looks something like this:

test.cpp:24:1: error: static assertion failed due to requirement 'c != c'
static_assert(c != c);
^             ~~~~~~
test.cpp:24:17: note: expression evaluates to '('0' != '0')'
static_assert(c != c);
              ~~^~~~

This looks hard to read to me...

test.cpp:25:1: error: static assertion failed due to requirement 'c > 'a''
static_assert(c > 'a');
^ ~~~~~~~
test.cpp:25:17: note: expression evaluates to '('0' > 'a')'

Same confusion here -- it took me a while to realize what's happening is that we're quoting the part outside of the parens and that's throwing me for a loop. We typically quote things that are syntax but in this case, the parens are not part of the syntax and so the surrounding quotes are catching me. Given that parens are being used as a visual delimiter and a single quote serves the same purpose, I would expect something more like:

test.cpp:25:17: note: expression evaluates to ''0' > 'a''
test.cpp:26:17: note: expression evaluates to '0 > 'a''
test.cpp:27:17: note: expression evaluates to '14 > 5'
test.cpp:28:17: note: expression evaluates to 'true == false'
test.cpp:29:17: note: expression evaluates to 'nullptr != nullptr'

Adding a few more folks as reviewers to see if there is a consensus position on how to proceed.

Agreed on confusion, but ''0' > 'a'' is painful to read. Sadly, short of changing quotes to backticks (something that'd be good for non-terminal-based consumers of SARIF), I'm not sure I can suggest anything better in one line. If we can span multiple lines, which I think would improve readability:

test.cpp:25:17: note: expression evaluates with:
  LHS: '0'
  RHS: 'a'
test.cpp:26:17: note: expression evaluates with:
  LHS: 0
  RHS: 'a'
test.cpp:27:17: note: expression evaluates with:
  LHS: 14
  RHS: 5
test.cpp:28:17: note: expression evaluates with:
  LHS: true
  RHS: false
test.cpp:29:17: note: expression evaluates with:
  LHS: nullptr
  RHS: nullptr

My assertion library follows what Catch2 and simple-test do, and uses Actual and Expected, but I'm not sure hardcoding those terms into Clang is a good idea.

I don't really want to bike-shed about the presentation for too long... I'm fine with just removing the parens, since we present it like that in the error message as well anyway:

./assert.cpp:6:1: error: static assertion failed due to requirement ''c' == 'a''
static_assert('c' == 'a')

Any tests with macros?

I can add some, but they should be handled transparently, with the usual "expanded from macro" note.

I don't really want to bike-shed about the presentation for too long...

I understand the desire, but at the same time, the whole goal of this patch is to improve the presentation of the diagnostic. You can't invite us all to a bikeshed painting party and then ask us not to use our brushes, that's just cruel! :-D

I'm fine with just removing the parens, since we present it like that in the error message as well anyway:

./assert.cpp:6:1: error: static assertion failed due to requirement ''c' == 'a''
static_assert('c' == 'a')

That's still my preference as well. Splitting across multiple lines with hard line breaks has some issues for some IDEs that want to display diagnostic information in a listbox (IIRC), so I'd rather we avoid ad hoc use of line breaks in diagnostics for the moment (it'd be easier/more appropriate if we were making a diagnostic policy for using multiple lines, but that'd involve an RFC and is more effort than I think is needed for this patch).

Any tests with macros?

I can add some, but they should be handled transparently, with the usual "expanded from macro" note.

I'm fine either way.

tbaeder updated this revision to Diff 451143.Aug 9 2022, 7:26 AM
cjdb accepted this revision.Aug 9 2022, 9:46 AM

I don't really want to bike-shed about the presentation for too long...

I understand the desire, but at the same time, the whole goal of this patch is to improve the presentation of the diagnostic. You can't invite us all to a bikeshed painting party and then ask us not to use our brushes, that's just cruel! :-D

Agreed. Getting the presentation of diagnostics right is critical.

I'm fine with just removing the parens, since we present it like that in the error message as well anyway:

./assert.cpp:6:1: error: static assertion failed due to requirement ''c' == 'a''
static_assert('c' == 'a')

That's still my preference as well. Splitting across multiple lines with hard line breaks has some issues for some IDEs that want to display diagnostic information in a listbox (IIRC), so I'd rather we avoid ad hoc use of line breaks in diagnostics for the moment (it'd be easier/more appropriate if we were making a diagnostic policy for using multiple lines, but that'd involve an RFC and is more effort than I think is needed for this patch).

Fair enough, no further objections on my part.

Any tests with macros?

I can add some, but they should be handled transparently, with the usual "expanded from macro" note.

I'm fine either way.

This revision is now accepted and ready to land.Aug 9 2022, 9:46 AM
aaron.ballman accepted this revision.Aug 9 2022, 10:04 AM

LGTM, thank you for working on this, it's a great usability enhancement!

This broke building with GCC (also noted by buildbot mails):

../tools/clang/lib/Sema/SemaDeclCXX.cpp: In member function ‘void clang::Sema::DiagnoseStaticAssertDetails(const clang::Expr*)’:
../tools/clang/lib/Sema/SemaDeclCXX.cpp:16666:19: error: declaration of ‘const clang::Expr* clang::Sema::DiagnoseStaticAssertDetails(const clang::Expr*)::<unnamed struct>::Expr’ changes meaning of ‘Expr’ [-fpermissive]
16666 |       const Expr *Expr;
      |                   ^~~~
In file included from ../tools/clang/include/clang/AST/DeclCXX.h:22,
                 from ../tools/clang/include/clang/AST/ASTLambda.h:18,
                 from ../tools/clang/lib/Sema/SemaDeclCXX.cpp:15:
../tools/clang/include/clang/AST/Expr.h:109:7: note: ‘Expr’ declared here as ‘class clang::Expr’
  109 | class Expr : public ValueStmt {
      |       ^~~~

This broke building with GCC (also noted by buildbot mails):

../tools/clang/lib/Sema/SemaDeclCXX.cpp: In member function ‘void clang::Sema::DiagnoseStaticAssertDetails(const clang::Expr*)’:
../tools/clang/lib/Sema/SemaDeclCXX.cpp:16666:19: error: declaration of ‘const clang::Expr* clang::Sema::DiagnoseStaticAssertDetails(const clang::Expr*)::<unnamed struct>::Expr’ changes meaning of ‘Expr’ [-fpermissive]
16666 |       const Expr *Expr;
      |                   ^~~~
In file included from ../tools/clang/include/clang/AST/DeclCXX.h:22,
                 from ../tools/clang/include/clang/AST/ASTLambda.h:18,
                 from ../tools/clang/lib/Sema/SemaDeclCXX.cpp:15:
../tools/clang/include/clang/AST/Expr.h:109:7: note: ‘Expr’ declared here as ‘class clang::Expr’
  109 | class Expr : public ValueStmt {
      |       ^~~~

It is kinda bad that GCC throws an error and Clang does not even print a warning.

How is it even possible? Clang does not implement strict(er) rules ? @aaron.ballman

This broke building with GCC (also noted by buildbot mails):

../tools/clang/lib/Sema/SemaDeclCXX.cpp: In member function ‘void clang::Sema::DiagnoseStaticAssertDetails(const clang::Expr*)’:
../tools/clang/lib/Sema/SemaDeclCXX.cpp:16666:19: error: declaration of ‘const clang::Expr* clang::Sema::DiagnoseStaticAssertDetails(const clang::Expr*)::<unnamed struct>::Expr’ changes meaning of ‘Expr’ [-fpermissive]
16666 |       const Expr *Expr;
      |                   ^~~~
In file included from ../tools/clang/include/clang/AST/DeclCXX.h:22,
                 from ../tools/clang/include/clang/AST/ASTLambda.h:18,
                 from ../tools/clang/lib/Sema/SemaDeclCXX.cpp:15:
../tools/clang/include/clang/AST/Expr.h:109:7: note: ‘Expr’ declared here as ‘class clang::Expr’
  109 | class Expr : public ValueStmt {
      |       ^~~~

It is kinda bad that GCC throws an error and Clang does not even print a warning.

How is it even possible? Clang does not implement strict(er) rules ? @aaron.ballman

I pushed up a change to "fix" this, but I believe GCC is wrong to give an error here. At least, nobody else gives one and GCC is inconsistent about what it diagnoses: https://godbolt.org/z/hTqM8MPch

This broke building with GCC (also noted by buildbot mails):

../tools/clang/lib/Sema/SemaDeclCXX.cpp: In member function ‘void clang::Sema::DiagnoseStaticAssertDetails(const clang::Expr*)’:
../tools/clang/lib/Sema/SemaDeclCXX.cpp:16666:19: error: declaration of ‘const clang::Expr* clang::Sema::DiagnoseStaticAssertDetails(const clang::Expr*)::<unnamed struct>::Expr’ changes meaning of ‘Expr’ [-fpermissive]
16666 |       const Expr *Expr;
      |                   ^~~~
In file included from ../tools/clang/include/clang/AST/DeclCXX.h:22,
                 from ../tools/clang/include/clang/AST/ASTLambda.h:18,
                 from ../tools/clang/lib/Sema/SemaDeclCXX.cpp:15:
../tools/clang/include/clang/AST/Expr.h:109:7: note: ‘Expr’ declared here as ‘class clang::Expr’
  109 | class Expr : public ValueStmt {
      |       ^~~~

It is kinda bad that GCC throws an error and Clang does not even print a warning.

How is it even possible? Clang does not implement strict(er) rules ? @aaron.ballman

I pushed up a change to "fix" this

FWIW, the same issue actually already was fixed by ebe0674acb8bb3404d0e2a6b689d5e3cd02bb0b6 - although your commit made it even clearer.

This broke building with GCC (also noted by buildbot mails):

../tools/clang/lib/Sema/SemaDeclCXX.cpp: In member function ‘void clang::Sema::DiagnoseStaticAssertDetails(const clang::Expr*)’:
../tools/clang/lib/Sema/SemaDeclCXX.cpp:16666:19: error: declaration of ‘const clang::Expr* clang::Sema::DiagnoseStaticAssertDetails(const clang::Expr*)::<unnamed struct>::Expr’ changes meaning of ‘Expr’ [-fpermissive]
16666 |       const Expr *Expr;
      |                   ^~~~
In file included from ../tools/clang/include/clang/AST/DeclCXX.h:22,
                 from ../tools/clang/include/clang/AST/ASTLambda.h:18,
                 from ../tools/clang/lib/Sema/SemaDeclCXX.cpp:15:
../tools/clang/include/clang/AST/Expr.h:109:7: note: ‘Expr’ declared here as ‘class clang::Expr’
  109 | class Expr : public ValueStmt {
      |       ^~~~

It is kinda bad that GCC throws an error and Clang does not even print a warning.

How is it even possible? Clang does not implement strict(er) rules ? @aaron.ballman

I pushed up a change to "fix" this

FWIW, the same issue actually already was fixed by ebe0674acb8bb3404d0e2a6b689d5e3cd02bb0b6 - although your commit made it even clearer.

Oh neat, thanks for pointing that out! I reflexively rename declarations when they share the name of a type because I have run into similar issues to this before, but it's usually things like IDE "go to definition" behavior that winds up breaking, not the build. (I run into this all the time with people doing const Attr *Attr)

Yup, that paper is currently closed in the WG21 paper tracker FWIW.