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

16823–16824

16830–16833

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

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

[sousajo]

clang/test/SemaCXX/static-assert.cpp

302

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

221

[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/docs/ReleaseNotes.rst

- Clang now checks for completeness of the second and third arguments in the
   conditional operator.
   (`#59718 <https://github.com/llvm/llvm-project/issues/59718>`_)
 - There were some cases in which the diagnostic for the unavailable attribute
   might not be issued, this fixes those cases.
   (`61815 <https://github.com/llvm/llvm-project/issues/61815>`_)
 - Clang now avoids unnecessary diagnostic warnings for obvious expressions in
   the case of binary operators with logical OR operations.
   (`#57906 <https://github.com/llvm/llvm-project/issues/57906>`_)
+- Clang's "static assertion failed" diagnostic now points to the static assertion
+  expression instead of pointing to the ``static_assert`` token.
+  (`#61951 <https://github.com/llvm/llvm-project/issues/61951>`_)
 
 Bug Fixes in This Version
 -------------------------
 
 - Fix crash when diagnosing incorrect usage of ``_Nullable`` involving alias
   templates.
   (`#60344 <https://github.com/llvm/llvm-project/issues/60344>`_)
 - Fix confusing warning message when ``/clang:-x`` is passed in ``clang-cl``

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->getBeginLoc(), 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();
+        Diag(InnerCond->getBeginLoc(),
+             diag::err_static_assert_requirement_failed)
+            << InnerCondDescription << !AssertMessage << Msg.str()
+            << InnerCond->getSourceRange();
         DiagnoseStaticAssertDetails(InnerCond);
       } else {
-        Diag(StaticAssertLoc, diag::err_static_assert_failed)
+        Diag(AssertExpr->getBeginLoc(), diag::err_static_assert_failed)
             << !AssertMessage << Msg.str() << AssertExpr->getSourceRange();
         PrintContextStack();
       }
       Failed = true;
     }
   } else {
     ExprResult FullAssertExpr = ActOnFinishFullExpr(AssertExpr, StaticAssertLoc,
                                                     /*DiscardedValue*/false,

clang/test/SemaCXX/static-assert.cpp

 #define A_IS_B (a == b)
   static_assert(CHECK_4(5), ""); // expected-error {{failed}}
 
   constexpr int a = 4;
   constexpr int b = 5;
   static_assert(CHECK_4(a) && A_IS_B, ""); // expected-error {{failed}} \
                                            // expected-note {{evaluates to '4 == 5'}}
 
+  static_assert(
+    false, // expected-error {{static assertion failed}}
+    ""
+  );
+
+  static_assert(
+    true && false, // expected-error {{static assertion failed due to requirement 'true && false'}}
+    ""
+  );
+
+  static_assert(
+    // with a comment here
+    true && false, // expected-error {{static assertion failed due to requirement 'true && false'}}
+    ""
+  );
+
+  static_assert(
+    // with a comment here
+    (true && // expected-error {{static assertion failed due to requirement '(true && false) || false'}}
+    false)
+    || false,
+    ""
+  );
 
 }