Make 'static assertion failed' diagnostics point to the static assertion expression

[sousajo]

Summary

"static assertion failed" pointed to the static_assert token and
then underlined the static assertion expression:

<source>:3:1: error: static assertion failed
    static_assert(false);
    ^             ~~~~~
    1 error generated.

See Godbolt: https://godbolt.org/z/r38booz59

Now one points and highlights the assertion expression.

Fixes https://github.com/llvm/llvm-project/issues/61951

[tbaeder]

The patch does not apply, so the pre-commit CI does not run.

[sousajo]

just rebased

[sousajo]

This comment was removed by sousajo.

[sousajo]

rebased

[tbaeder]

I think the other Diag call should be changed as well:

./a.cpp:4:15: error: static assertion failed
static_assert(false);
              ^~~~~
./a.cpp:6:1: error: static assertion failed due to requirement 'true && false'
static_assert(true && false);
^             ~~~~~~~~~~~~~

[tbaeder]

Would also be nice to add a release note for this.

Also, let's ping @aaron.ballman and @cjdb, the former might want a test added. :)

[sousajo]

In D147745#4252761, @tbaeder wrote:

I think the other Diag call should be changed as well:

./a.cpp:4:15: error: static assertion failed
static_assert(false);
              ^~~~~
./a.cpp:6:1: error: static assertion failed due to requirement 'true && false'
static_assert(true && false);
^             ~~~~~~~~~~~~~

ah sure! just did locally, will update the patch, when it has also the release note and (possibly) the test, as to not waste CI resources :)

Would also be nice to add a release note for this.

Also, let's ping @aaron.ballman and @cjdb, the former might want a test added. :)

Okidoki, would also be nice to get a pointer to a similar test, so I can take a look into it, and meanwhile Ill try to find some example for the release note.

[sousajo]

Added two test cases, and the release note.

[sousajo]

Just realized that in the case:

static_assert(
true && 
false);

We will now point to:

test.cpp:5:1: error: static assertion failed due to requirement 'true && false'
true && 
^~~~~~~

Which seems a bit off. gcc points to the conditional operator instead:

https://godbolt.org/z/MjsEs7eW6

[tbaeder]

I think this is generally a shortcoming in clang, we never emit more than one line of code, even if we should.

[tbaeder]

Ah, there is -fcared-diagnostics-max-lines as a cc1 option: https://godbolt.org/z/Y4dW35v7M

[sousajo]

In D147745#4253763, @tbaeder wrote:

Ah, there is -fcared-diagnostics-max-lines as a cc1 option: https://godbolt.org/z/Y4dW35v7M

Ah oki, using that option:

test.cpp:5:1: error: static assertion failed due to requirement 'true && false || false'
true && 
^~~~~~~
false
~~~~~
|| false);
~~~~~~~~

Shows the whole expression :)

[tbaeder]

clang/lib/Sema/SemaDeclCXX.cpp

16822–16824

16832

16835–16836

[sousajo]

• change AssertExpr->getSourceRange().getBegin() calls to AssertExpr->getBeginLoc()
• same for InnerCond->getSourceRange().getBegin()
• add a couple more tests

[sousajo]

Does it look fine now ?

[tbaeder]

I think so, I'm just waiting for @aaron.ballman or @cjdb do chime in. Also, with https://reviews.llvm.org/D147875 in the mix, it might be useful to point to the static_assert token in some way? But it's not important I think, part of the expression is probably always on the same line as the static_assert token.

[cjdb]

LGTM from a diagnostics perspective. Having some pointer to the static_assert would be nice in situations where the expression is far removed from the start of the static_assert keyword (probably due to a long comment), but that would result in a note, and that isn't really worth it IMO.

clang/test/SemaCXX/static-assert.cpp

302 ↗

(On Diff #512027)

It's worth being mindful of D146376, which might impact this message.

[sousajo]

clang/test/SemaCXX/static-assert.cpp

302 ↗

(On Diff #512027)

Ill rebase to be sure we have it

[sousajo]

Also agree, imho this is better now and will be fine most of time

[aaron.ballman]

LGTM with a minor grammar change in the release note (feel free to take it or leave it as you see fit).

clang/docs/ReleaseNotes.rst

219 ↗

(On Diff #512027)

[sousajo]

In D147745#4262198, @aaron.ballman wrote:

LGTM with a minor grammar change in the release note (feel free to take it or leave it as you see fit).

Sure I can add it later today. Then one of you would have to land it for me ^^

[aaron.ballman]

In D147745#4262223, @sousajo wrote:

In D147745#4262198, @aaron.ballman wrote:

LGTM with a minor grammar change in the release note (feel free to take it or leave it as you see fit).

Sure I can add it later today. Then one of you would have to land it for me ^^

Thank you for letting us know -- what name and email address would you like us to use for patch attribution?

[sousajo]

In D147745#4262386, @aaron.ballman wrote:

In D147745#4262223, @sousajo wrote:

In D147745#4262198, @aaron.ballman wrote:

LGTM with a minor grammar change in the release note (feel free to take it or leave it as you see fit).

Sure I can add it later today. Then one of you would have to land it for me ^^

Thank you for letting us know -- what name and email address would you like us to use for patch attribution?

Name: Jorge Pinto Sousa
Email: <[e-mail]>

^^

[sousajo]

Rebased and fix wording of the release note

[sousajo]

@aaron.ballman should be ok now ^^

[sousajo]

rebased

[sousajo]

windows checks are failing, but looks like it is unrelated to this change

clang/lib/Sema/SemaDeclCXX.cpp

if (!Failed && !Cond && !InTemplateDefinition) {
 
       Expr *InnerCond = nullptr;
       std::string InnerCondDescription;
       std::tie(InnerCond, InnerCondDescription) =
         findFailedBooleanCondition(Converted.get());
       if (InnerCond && isa<ConceptSpecializationExpr>(InnerCond)) {
         // Drill down into concept specialization expressions to see why they
         // weren't satisfied.
-        Diag(StaticAssertLoc, diag::err_static_assert_failed)
+        Diag(AssertExpr->getSourceRange().getBegin(),
+             diag::err_static_assert_failed)
             << !AssertMessage << Msg.str() << AssertExpr->getSourceRange();
         ConstraintSatisfaction Satisfaction;
         if (!CheckConstraintSatisfaction(InnerCond, Satisfaction))
           DiagnoseUnsatisfiedConstraint(Satisfaction);
       } else if (InnerCond && !isa<CXXBoolLiteralExpr>(InnerCond)
                            && !isa<IntegerLiteral>(InnerCond)) {
         Diag(StaticAssertLoc, diag::err_static_assert_requirement_failed)
           << InnerCondDescription << !AssertMessage
           << Msg.str() << InnerCond->getSourceRange();
         DiagnoseStaticAssertDetails(InnerCond);
       } else {
-        Diag(StaticAssertLoc, diag::err_static_assert_failed)
+        Diag(AssertExpr->getSourceRange().getBegin(),
+             diag::err_static_assert_failed)
             << !AssertMessage << Msg.str() << AssertExpr->getSourceRange();
         PrintContextStack();
       }
       Failed = true;
     }
   } else {
     ExprResult FullAssertExpr = ActOnFinishFullExpr(AssertExpr, StaticAssertLoc,
                                                     /*DiscardedValue*/false,