[PATCH] D154290: [Clang] Implement P2741R3 - user-generated static_assert messages

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added subscribers: erichkeane, tahonermann, ldionne, hubert.reinterpretcast.
aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:1553-1558
+def err_static_assert_invalid_size : Error<
+  "the message in a static assertion must have a 'size()' member "
+  "function returning an object convertible to 'std::size_t'">;
+def err_static_assert_invalid_data : Error<
+  "the message in a static assertion must have a 'data()' member "
+  "function returning an object convertible to 'const char*'">;
----------------
How about we combine these into one diagnostic given how similar and related they are?


================
Comment at: clang/lib/AST/ExprConstant.cpp:16417-16418
+  }
+  if (!Scope.destroy())
+    return false;
+
----------------
cor3ntin wrote:
> aaron.ballman wrote:
> > Rather than use an RAII object and destroy it manually, let's use `{}` to scope the RAII object appropriately.
> This seems to be the way to use this interface - mostly because destroy can fail and we want to detect that
Oh, what a strange thing to call `RAII`! :-D


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16888-16899
+  SourceLocation Loc = Message->getBeginLoc();
+  assert(Message);
+  assert(!Message->isTypeDependent() && !Message->isValueDependent() &&
+         "can't evaluate a dependant static assert message");
+
+  if (const auto *SL = dyn_cast<StringLiteral>(Message)) {
+    assert(SL->isUnevaluated() && "expected an unevaluated string");
----------------



================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16902
+  if (!RD) {
+    Diag(Message->getBeginLoc(), diag::err_static_assert_invalid_message);
+    return false;
----------------



================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16942-16943
+  if (SizeNotFound || DataNotFound) {
+    Diag(Message->getBeginLoc(),
+         diag::err_static_assert_missing_member_function)
+        << ((SizeNotFound && DataNotFound) ? 2
----------------



================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16960
+    ExprResult Res = BuildMemberReferenceExpr(
+        Message, Message->getType(), Message->getBeginLoc(), false,
+        CXXScopeSpec(), SourceLocation(), nullptr, LR, nullptr, nullptr);
----------------
Can be re-flowed to 80 columns, and I'll stop pointing out changes for this one -- just be sure to replace everything with `Loc` that you can.


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16999-17000
+
+  if (CheckOnly)
+    return true;
+
----------------
Errrr, this seems like a pretty big surprise because evaluating that char range can fail. This means:
```
constexpr struct {
  size_t size() const;
  const char *data() const;
} d;
static_assert(false, d); // Fails, not because of the static assertion but because d is not valid
static_assert(true, d); // Succeeds, despite d not being valid
```
(Regardless of what we land on for a direction, I think this is a good test case to add.)

I don't know that this is reasonable QoI. Most static assertions pass rather than fail, so I think this will hide bugs from users in unexpected ways. I understand there may be concerns about compile time overhead, but those concerns seem misplaced in the absence of evidence: the extra overhead is something the user is opting into explicitly by using constexpr features and constexpr evaluation is well known to slow down compile times. Further, given that many (most?) static assert messages are relatively short (e.g., not kilobytes or megabytes of text), that extra overhead is likely negligible to begin with in this case, at least on an individual assert basis. The downside is, because most static assertions pass, that evaluation is also unnecessary in most cases, and it could add up if there's a lot of static assertions. However, given that users add static assertions to tell them when code is incorrect (assumptions invalidated, etc), it seems pretty counter-productive to hide bugs within the static assertion itself.

What do others think? @hubert.reinterpretcast @tahonermann @erichkeane @ldionne @shafik

If there's some agreement that we'd like to see diagnostics here, my initial preference is for an error diagnostic because a static assertion is only useful if both operands are valid constant expressions, but I could probably live with a warning. However, I think that an error would require filing a DR with WG21 otherwise there are conformance issues.

In the meantime, to keep this review moving without waiting for committee deliberations, I think we can evaluate the constant expression to determine it's validity and report issues as a warning (potentially one that defaults to an error) if others agree that a diagnostic is warranted here.


================
Comment at: clang/test/SemaCXX/static-assert-cxx26.cpp:88-98
+struct ConvertibleToInt {
+    operator int();
+};
+struct ConvertibleToCharPtr {
+    operator const char*();
+};
+struct MessageFromConvertible {
----------------
Use of `true` here means that we missed the fact that `MessageFromConvertible()` is invalid; not only are the `size` and `data` methods not `constexpr`, the `ConvertibleToInt` and `ConvertibleToCharPtr` classes do not have constexpr conversion operators. That could be intentional, but we should definitely have a `false` test that tests the conversion operators are properly called and work as expected when they're correct.


================
Comment at: clang/test/SemaCXX/static-assert-cxx26.cpp:110-111
+
+static_assert(false, Leaks{}); //expected-error {{the message in a static assertion must be produced by a constant expression}} \
+                              // expected-error {{static assertion failed: ub}}
+
----------------
This diagnostic combination is a little bit jarring to me. The first one is telling the user that the message is incorrect and the second one tells the user precisely what that message was, so they're almost contradictory in some ways. However, I'm not certain if others think this is jarring as well.

If we think that, perhaps this sort of circumstance should just say the static assertion failed without the message part, even though we could figure out something as the message in this particular instance.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154290/new/

https://reviews.llvm.org/D154290