[Sema][Cxx17] Error message for C++17 static_assert(pred) without string literal

[jkorous]

Summary

C++17 adds form of static_assert without string literal.

static_assert ( bool_constexpr )

Currently clang produces diagnostics like this:

assert.cpp:1:1: error: static_assert failed
static_assert(false);
^             ~~~~~
1 error generated.

This patch adds assert expression to error message in case string literal is missing:

/Users/jkorous/assert.cpp:1:1: error: static_assert failed "false"
static_assert(false);
^             ~~~~~
1 error generated.

Reasons why printing assert expression in absence of string literal is useful are:

1. Serialized diagnostics (--serialize-diagnostics) don't contain code snippet and thus error message lack context.
2. No all tools scraping clang diagnostics use caret snippet provided by -f-caret-diagnostics.

This patch doesn't address message length limit as currently clang doesn't truncate messages anyway but it might be subject of another patch.

rdar://problem/38931241

[rsmith]

This would violate our guidelines for diagnostic messages; see https://clang.llvm.org/docs/InternalsManual.html#the-format-string

In particular: "Take advantage of location information. The user will be able to see the line and location of the caret, so you don’t need to tell them that the problem is with the 4th argument to the function: just point to it."

Reasons why printing assert expression in absence of string literal is useful are:

1. Serialized diagnostics (--serialize-diagnostics) don't contain code snippet and thus error message lack context.
2. Not all tools scraping clang diagnostics use caret snippet provided by -fcaret-diagnostics.

This seems like an excellent reason to fix those tools -- per our documentation, Clang's diagnostics are not designed to be used without the caret and snippet.

If you want to change our diagnostic policy from "the diagnostic text plus the line and location of the caret should be enough to identify the problem; do not repeat information in the diagnostic that the caret portion shows" to "the diagnostic text alone should be sufficient", that's certainly something we can discuss, but you'll need to make the case for why that's a good change of policy.

[dexonsmith]

The code looks correct to me, although I have a few suggestions inline. When you resubmit, please include full context (e.g., git diff -U999999).

Jan and I discussed this ahead of time and I agree with adding the assertion as the message. However, I'd rather hear from others before signing off.

lib/Sema/SemaDeclCXX.cpp

13739–13740

Is this extra nesting necessary?

13744–13746

Inserting to the beginning of the buffer seems a bit weird, especially since you're jumping between Msg and MsgBuffer.

This feels more natural to me:

Msg << '"';
AssertExpr->printPretty(Msg, nullptr, getPrintingPolicy());
Msg << '"';

13761

Is there always a message now? If so, can you just assert that before the if statement and simplify this code?

[dexonsmith]

(Somehow I missed Richard's comment when I replied last night, even though it preceded mine by 5 hours...)

In D46834#1098727, @rsmith wrote:

This would violate our guidelines for diagnostic messages; see https://clang.llvm.org/docs/InternalsManual.html#the-format-string

In particular: "Take advantage of location information. The user will be able to see the line and location of the caret, so you don’t need to tell them that the problem is with the 4th argument to the function: just point to it."

Reasons why printing assert expression in absence of string literal is useful are:

1. Serialized diagnostics (--serialize-diagnostics) don't contain code snippet and thus error message lack context.
2. Not all tools scraping clang diagnostics use caret snippet provided by -fcaret-diagnostics.

This seems like an excellent reason to fix those tools -- per our documentation, Clang's diagnostics are not designed to be used without the caret and snippet.

If you want to change our diagnostic policy from "the diagnostic text plus the line and location of the caret should be enough to identify the problem; do not repeat information in the diagnostic that the caret portion shows" to "the diagnostic text alone should be sufficient", that's certainly something we can discuss, but you'll need to make the case for why that's a good change of policy.

I wasn't aware of the policy. I can't convince myself that static_assert should be an exception to it.

It's ironic, though. What I'm concerned about are interfaces like a Vim location list, a condensed view of diagnostics that won't show the source until you select a particular diagnostic. The status quo may lead some users to leverage a macro like this to hack the location list view:

#define MY_STATIC_ASSERT(X) static_assert(X, #X)

getting rid of which was the motivation for the single-argument static_assert in the first place.

[jkorous]

We reconsidered this in light of the policy - thanks for pointing that out Richard!
Just to be sure that I understand it - the policy is meant for CLI and not serialized diagnostics, right?

[rsmith]

In D46834#1101586, @jkorous wrote:

We reconsidered this in light of the policy - thanks for pointing that out Richard!
Just to be sure that I understand it - the policy is meant for CLI and not serialized diagnostics, right?

The policy certainly seems designed around the CLI use case. For serialized diagnostics, it would make sense to either serialize the snippet or enough information that the snippet can be reconstructed. And if that can't be done, or fails to satisfy some other use case, then we should discuss how we proceed -- for instance, we could consider having different diagnostic messages for the case where we have a snippet and for the case where we do not.

[dexonsmith]

In D46834#1102391, @rsmith wrote:

In D46834#1101586, @jkorous wrote:

We reconsidered this in light of the policy - thanks for pointing that out Richard!
Just to be sure that I understand it - the policy is meant for CLI and not serialized diagnostics, right?

The policy certainly seems designed around the CLI use case. For serialized diagnostics, it would make sense to either serialize the snippet or enough information that the snippet can be reconstructed. And if that can't be done, or fails to satisfy some other use case, then we should discuss how we proceed -- for instance, we could consider having different diagnostic messages for the case where we have a snippet and for the case where we do not.

Right. There are places in the IDE where there is a condensed view of all diagnostics (like a Vim location list), and others where the diagnostics are shown inline with the sources. I think what we want is an optional auxiliary record/field in a diagnostic with that contains context for when the source context is missing, and then the IDE can choose which to display. It's optional because most diagnostics are good enough as is for location lists.

[rsmith]

In D46834#1102395, @dexonsmith wrote:

In D46834#1102391, @rsmith wrote:

The policy certainly seems designed around the CLI use case. For serialized diagnostics, it would make sense to either serialize the snippet or enough information that the snippet can be reconstructed. And if that can't be done, or fails to satisfy some other use case, then we should discuss how we proceed -- for instance, we could consider having different diagnostic messages for the case where we have a snippet and for the case where we do not.

Right. There are places in the IDE where there is a condensed view of all diagnostics (like a Vim location list), and others where the diagnostics are shown inline with the sources. I think what we want is an optional auxiliary record/field in a diagnostic with that contains context for when the source context is missing, and then the IDE can choose which to display. It's optional because most diagnostics are good enough as is for location lists.

That sounds good to me. I think it would also make sense to use the alternate form for the CLI case if the user is using -fno-caret-diagnostics for some reason.

[dexonsmith]

In D46834#1102407, @rsmith wrote:

In D46834#1102395, @dexonsmith wrote:

In D46834#1102391, @rsmith wrote:

The policy certainly seems designed around the CLI use case. For serialized diagnostics, it would make sense to either serialize the snippet or enough information that the snippet can be reconstructed. And if that can't be done, or fails to satisfy some other use case, then we should discuss how we proceed -- for instance, we could consider having different diagnostic messages for the case where we have a snippet and for the case where we do not.

Right. There are places in the IDE where there is a condensed view of all diagnostics (like a Vim location list), and others where the diagnostics are shown inline with the sources. I think what we want is an optional auxiliary record/field in a diagnostic with that contains context for when the source context is missing, and then the IDE can choose which to display. It's optional because most diagnostics are good enough as is for location lists.

That sounds good to me. I think it would also make sense to use the alternate form for the CLI case if the user is using -fno-caret-diagnostics for some reason.

Yes that sounds right.

lib/Parse/ParseDeclCXX.cpp

 
 /// ParseStaticAssertDeclaration - Parse C++0x or C11 static_assert-declaration.
 ///
-/// [C++0x] static_assert-declaration:
+/// [C++17] static_assert-declaration:
+///           static_assert ( constant-expression  ) ;
+///
+/// [C++11] static_assert-declaration:
 ///           static_assert ( constant-expression  ,  string-literal  ) ;
 ///
 /// [C11]   static_assert-declaration:

lib/Sema/SemaDeclCXX.cpp

 
     if (!Failed && !Cond) {
       SmallString<256> MsgBuffer;
-      llvm::raw_svector_ostream Msg(MsgBuffer);
-      if (AssertMessage)
-        AssertMessage->printPretty(Msg, nullptr, getPrintingPolicy());
+      {
+        llvm::raw_svector_ostream Msg(MsgBuffer);
+        if (AssertMessage)
+          AssertMessage->printPretty(Msg, nullptr, getPrintingPolicy());
+        else {
+          AssertExpr->printPretty(Msg, nullptr, getPrintingPolicy());
+          MsgBuffer.insert(MsgBuffer.begin(), '\"');
+          MsgBuffer.append(1, '\"');
+        }
+      }
 
       Expr *InnerCond = nullptr;
       std::string InnerCondDescription;

                                    /*AllowTopLevelCond=*/false);
       if (InnerCond) {
         Diag(StaticAssertLoc, diag::err_static_assert_requirement_failed)
-          << InnerCondDescription << !AssertMessage
-          << Msg.str() << InnerCond->getSourceRange();
+          << InnerCondDescription << MsgBuffer.empty() << MsgBuffer.str()
+          << InnerCond->getSourceRange();
       } else {
         Diag(StaticAssertLoc, diag::err_static_assert_failed)
-          << !AssertMessage << Msg.str() << AssertExpr->getSourceRange();
+          << MsgBuffer.empty() << MsgBuffer.str()
+          << AssertExpr->getSourceRange();
       }
       Failed = true;
     }

test/SemaCXX/static-assert-cxx17.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++17 %s
+
+static_assert(false, "foo"); // expected-error {{static_assert failed "foo"}}
+static_assert(false == true); // expected-error {{static_assert failed "false == true"}}
+static_assert(false, "foo \"bar\" baz"); // expected-error {{static_assert failed "foo \"bar\" baz"}}

test/SemaCXX/static-assert.cpp

 StaticAssertProtected<X> sap2; // expected-note {{instantiation}}
 
 static_assert(true); // expected-warning {{C++17 extension}}
-static_assert(false); // expected-error-re {{failed{{$}}}} expected-warning {{extension}}
+static_assert(false); // expected-error {{static_assert failed "false"}} expected-warning {{C++17 extension}}
 
 
 // Diagnostics for static_assert with multiple conditions