[Clang] Improve support for expression messages in `static_assert` #73234

cor3ntin · 2023-11-23T11:57:26Z

Support non-member functions and callable objects for size and data(). We previously tried to (badly) pick the best overload ourselves, in a way that would only support member functions. We now leave clang construct an unresolved member expression and call that, properly performing overload resolution with callable objects and static functions, consistent with the logic for get calls for structured bindings.
Support UDLs as message expression.
Add tests and mark CWG2798 as resolved

llvmbot · 2023-11-23T11:57:54Z

@llvm/pr-subscribers-clang

Author: cor3ntin (cor3ntin)

Changes

Support non-member functions and callable objects for size and data(). We previously tried to (badly) pick the best overload ourselves, in a way that would only support member functions. We now leave clamg construct an unresolved member expression and call that, properly performing overload resolution with callable objects and static functions, cojnsistent with the logic for get calls for structured bindings.
Support UDLs as message expression.
Add tests and mark CWG2798 as resolved

Full diff: https://github.com/llvm/llvm-project/pull/73234.diff

6 Files Affected:

(modified) clang/docs/ReleaseNotes.rst (+3)
(modified) clang/lib/Parse/ParseDeclCXX.cpp (+1-1)
(modified) clang/lib/Sema/SemaDeclCXX.cpp (+3-21)
(added) clang/test/CXX/drs/dr27xx.cpp (+30)
(modified) clang/test/SemaCXX/static-assert-cxx26.cpp (+44-10)
(modified) clang/www/cxx_dr_status.html (+1-1)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 7e2c990d03e7f27..5a8e63be1258a28 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -617,6 +617,9 @@ Bug Fixes in This Version
 - Fix crash during code generation of C++ coroutine initial suspend when the return
   type of await_resume is not trivially destructible.
   Fixes (`#63803 <https://github.com/llvm/llvm-project/issues/63803>`_)
+- Fix crash when the object used as a ``static_asssert`` message has ``size`` or ``data`` members
+  which are not member functions.
+- Support UDLs in ``static_asssert`` message.
 
 Bug Fixes to Compiler Builtins
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/Parse/ParseDeclCXX.cpp b/clang/lib/Parse/ParseDeclCXX.cpp
index d9125955fda2783..910112ecae964cc 100644
--- a/clang/lib/Parse/ParseDeclCXX.cpp
+++ b/clang/lib/Parse/ParseDeclCXX.cpp
@@ -1023,7 +1023,7 @@ Decl *Parser::ParseStaticAssertDeclaration(SourceLocation &DeclEnd) {
         const Token &T = GetLookAheadToken(I);
         if (T.is(tok::r_paren))
           break;
-        if (!tokenIsLikeStringLiteral(T, getLangOpts())) {
+        if (!tokenIsLikeStringLiteral(T, getLangOpts()) || T.hasUDSuffix()) {
           ParseAsExpression = true;
           break;
         }
diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
index 3688192e6cbe5c5..01e2be3a45be85a 100644
--- a/clang/lib/Sema/SemaDeclCXX.cpp
+++ b/clang/lib/Sema/SemaDeclCXX.cpp
@@ -17291,33 +17291,15 @@ bool Sema::EvaluateStaticAssertMessageAsString(Expr *Message,
 
   auto FindMember = [&](StringRef Member, bool &Empty,
                         bool Diag = false) -> std::optional<LookupResult> {
-    QualType ObjectType = Message->getType();
-    Expr::Classification ObjectClassification =
-        Message->Classify(getASTContext());
-
     DeclarationName DN = PP.getIdentifierInfo(Member);
     LookupResult MemberLookup(*this, DN, Loc, Sema::LookupMemberName);
     LookupQualifiedName(MemberLookup, RD);
     Empty = MemberLookup.empty();
     OverloadCandidateSet Candidates(MemberLookup.getNameLoc(),
                                     OverloadCandidateSet::CSK_Normal);
-    for (NamedDecl *D : MemberLookup) {
-      AddMethodCandidate(DeclAccessPair::make(D, D->getAccess()), ObjectType,
-                         ObjectClassification, /*Args=*/{}, Candidates);
-    }
-    OverloadCandidateSet::iterator Best;
-    switch (Candidates.BestViableFunction(*this, Loc, Best)) {
-    case OR_Success:
-      return std::move(MemberLookup);
-    default:
-      if (Diag)
-        Candidates.NoteCandidates(
-            PartialDiagnosticAt(
-                Loc, PDiag(diag::err_static_assert_invalid_mem_fn_ret_ty)
-                         << (Member == "data")),
-            *this, OCD_AllCandidates, /*Args=*/{});
-    }
-    return std::nullopt;
+    if(MemberLookup.empty())
+      return std::nullopt;
+    return MemberLookup;
   };
 
   bool SizeNotFound, DataNotFound;
diff --git a/clang/test/CXX/drs/dr27xx.cpp b/clang/test/CXX/drs/dr27xx.cpp
new file mode 100644
index 000000000000000..f17726eb045f933
--- /dev/null
+++ b/clang/test/CXX/drs/dr27xx.cpp
@@ -0,0 +1,30 @@
+// RUN: %clang_cc1 -std=c++2c -verify %s
+
+namespace dr2798 { // dr2798: 17 drafting
+#if __cpp_static_assert >= 202306
+struct string {
+    constexpr string() {
+        data_ = new char[6]();
+        __builtin_memcpy(data_, "Hello", 5);
+        data_[5] = 0;
+    }
+    constexpr ~string() {
+        delete[] data_;
+    }
+    constexpr unsigned long size() const {
+        return 5;
+    };
+    constexpr const char* data() const {
+        return data_;
+    }
+
+    char* data_;
+};
+struct X {
+    string s;
+};
+consteval X f() { return {}; }
+
+static_assert(false, f().s); // expected-error {{static assertion failed: Hello}}
+#endif
+}
diff --git a/clang/test/SemaCXX/static-assert-cxx26.cpp b/clang/test/SemaCXX/static-assert-cxx26.cpp
index b19aac6cabd1324..f4ede74f9214a44 100644
--- a/clang/test/SemaCXX/static-assert-cxx26.cpp
+++ b/clang/test/SemaCXX/static-assert-cxx26.cpp
@@ -179,18 +179,20 @@ static_assert(false, Message{}); // expected-error {{static assertion failed: He
 }
 
 struct MessageInvalidSize {
-    constexpr auto size(int) const; // expected-note {{candidate function not viable: requires 1 argument, but 0 were provided}}
-    constexpr auto data() const;
+    constexpr unsigned long size(int) const; // expected-note {{'size' declared here}}
+    constexpr const char* data() const;
 };
 struct MessageInvalidData {
-    constexpr auto size() const;
-    constexpr auto data(int) const; // expected-note {{candidate function not viable: requires 1 argument, but 0 were provided}}
+    constexpr unsigned long size() const;
+    constexpr const char* data(int) const; // expected-note {{'data' declared here}}
 };
 
 static_assert(false, MessageInvalidSize{});  // expected-error {{static assertion failed}} \
-                                             // expected-error {{the message in a static assertion must have a 'size()' member function returning an object convertible to 'std::size_t'}}
+                                             // expected-error {{the message in a static assertion must have a 'size()' member function returning an object convertible to 'std::size_t'}} \
+                                             // expected-error {{too few arguments to function call, expected 1, have 0}}
 static_assert(false, MessageInvalidData{});  // expected-error {{static assertion failed}} \
-                                             // expected-error {{the message in a static assertion must have a 'data()' member function returning an object convertible to 'const char *'}}
+                                             // expected-error {{the message in a static assertion must have a 'data()' member function returning an object convertible to 'const char *'}} \
+                                             // expected-error {{too few arguments to function call, expected 1, have 0}}
 
 struct NonConstMembers {
     constexpr int size() {
@@ -227,14 +229,14 @@ static_assert(false, Variadic{}); // expected-error {{static assertion failed: O
 
 template <typename T>
 struct DeleteAndRequires {
-    constexpr int size() = delete; // expected-note {{candidate function has been explicitly deleted}}
-    constexpr const char* data() requires false; // expected-note {{candidate function not viable: constraints not satisfied}} \
-                                                 // expected-note {{because 'false' evaluated to false}}
+    constexpr int size() = delete; // expected-note {{'size' has been explicitly marked deleted here}}
+    constexpr const char* data() requires false; // expected-note {{because 'false' evaluated to false}}
 };
 static_assert(false, DeleteAndRequires<void>{});
 // expected-error@-1 {{static assertion failed}} \
 // expected-error@-1 {{the message in a static assertion must have a 'size()' member function returning an object convertible to 'std::size_t'}}\
-// expected-error@-1 {{the message in a static assertion must have a 'data()' member function returning an object convertible to 'const char *'}}
+// expected-error@-1 {{invalid reference to function 'data': constraints not satisfied}} \
+// expected-error@-1 {{attempt to use a deleted function}}
 
 class Private {
     constexpr int size(int i = 0) { // expected-note {{implicitly declared private here}}
@@ -294,6 +296,9 @@ struct Frobble {
   constexpr const char *data() const { return "hello"; }
 };
 
+constexpr Frobble operator ""_myd (const char *, unsigned long) { return Frobble{}; }
+static_assert (false, "foo"_myd); // expected-error {{static assertion failed: hello}}
+
 Good<Frobble> a; // expected-note {{in instantiation}}
 Bad<int> b; // expected-note {{in instantiation}}
 
@@ -307,3 +312,32 @@ static_assert((char8_t)-128 == (char8_t)-123, ""); // expected-error {{failed}}
 static_assert((char16_t)0xFEFF == (char16_t)0xDB93, ""); // expected-error {{failed}} \
                                                          // expected-note {{evaluates to 'u'' (0xFEFF, 65279) == u'\xDB93' (0xDB93, 56211)'}}
 }
+
+struct Static {
+  static constexpr int size() { return 5; }
+  static constexpr const char *data() { return "hello"; }
+};
+static_assert(false, Static{}); // expected-error {{static assertion failed: hello}}
+
+struct Data {
+    unsigned long size = 0;
+    const char* data = "hello";
+};
+static_assert(false, Data{}); // expected-error {{called object type 'unsigned long' is not a function or function pointer}} \
+                              // expected-error {{called object type 'const char *' is not a function or function pointer}} \
+                              // expected-error {{the message in a static assertion must have a 'size()' member function returning an object convertible to 'std::size_t'}} \
+                              // expected-error {{static assertion failed}}
+
+struct Callable {
+    struct {
+        constexpr auto operator()() const {
+            return 5;
+        };
+    } size;
+    struct {
+        constexpr auto operator()() const {
+            return "hello";
+        };
+    } data;
+};
+static_assert(false, Callable{}); // expected-error {{static assertion failed: hello}}
diff --git a/clang/www/cxx_dr_status.html b/clang/www/cxx_dr_status.html
index e711b2173d191c1..266891a2e373090 100755
--- a/clang/www/cxx_dr_status.html
+++ b/clang/www/cxx_dr_status.html
@@ -7145,7 +7145,7 @@ <h2 id="cxxdr">C++ defect report implementation status</h2>
     <td><a href="https://cplusplus.github.io/CWG/issues/1223.html">1223</a></td>
     <td>drafting</td>
     <td>Syntactic disambiguation and <I>trailing-return-type</I>s</td>
-    <td class="unreleased" align="center">Clang 17</td>
+    <td class="full" align="center">Clang 17</td>
   </tr>
   <tr id="1224">
     <td><a href="https://cplusplus.github.io/CWG/issues/1224.html">1224</a></td>

github-actions · 2023-11-23T11:59:56Z

✅ With the latest revision this PR passed the C/C++ code formatter.

oliverscherf-tomtom · 2023-11-23T12:31:20Z

clang/docs/ReleaseNotes.rst

+- Fix crash when the object used as a ``static_asssert`` message has ``size`` or ``data`` members
+  which are not member functions.
+- Support UDLs in ``static_asssert`` message.


Suggested change

- Fix crash when the object used as a ``static_asssert`` message has ``size`` or ``data`` members

which are not member functions.

- Support UDLs in ``static_asssert`` message.

- Fix crash when the object used as a ``static_assert`` message has ``size`` or ``data`` members

which are not member functions.

- Support UDLs in ``static_assert`` message.

Endilll

DR testing part looks good, save for a couple of minor comments.

Endilll · 2023-11-23T18:14:18Z

clang/test/CXX/drs/dr27xx.cpp

+};
+consteval X f() { return {}; }
+
+static_assert(false, f().s); // expected-error {{static assertion failed: Hello}}


Would be nice to convert this to // expected-error@-1 style.

FWIW, I have a slight preference for the current form (and always do). -1 or +1 isn't the end of the world, but having the diagnostic on the line which it fires requires the least amount of thinking when reading the code.

I find -1 form to work better for longer messages (removing incentive to shorten them).
More importantly, it explains by example how to use @ in expected directives, making it more accessible for people to list notes below the error (which matches compiler's console output), instead of simply putting expected-note wherever it appears.

A PR I took a look at after this one illustrates the point above: #73493 (comment)

I think it's a subjective decision whether to go on the same line or a nearby line; we have no requirements either way. But I don't think we should write tests as a way to show examples of how to use various diagnostic verifier features; documentation should be written instead and the tests should use whatever is most clear to folks reading them.

(FWIW, what I find easiest to read tends to be:

void foo(); // #decl ... int foo; // #note1 ... foo(); // expected-error {{something is wrong; did you mean to do it the right way?}} \ expected-note@#note1 {{note text here}} \ expected-note@#decl {{declared here}}

but I realize that not everyone will agree with that.)

clang/test/CXX/drs/dr27xx.cpp

AaronBallman · 2023-11-27T15:10:02Z

clang/test/CXX/drs/dr27xx.cpp

+};
+consteval X f() { return {}; }
+
+static_assert(false, f().s); // expected-error {{static assertion failed: Hello}}


FWIW, I have a slight preference for the current form (and always do). -1 or +1 isn't the end of the world, but having the diagnostic on the line which it fires requires the least amount of thinking when reading the code.

AaronBallman · 2023-11-27T15:13:52Z

clang/test/SemaCXX/static-assert-cxx26.cpp

 };

 static_assert(false, MessageInvalidSize{});  // expected-error {{static assertion failed}} \
-                                             // expected-error {{the message in a static assertion must have a 'size()' member function returning an object convertible to 'std::size_t'}}
+                                             // expected-error {{the message in a static assertion must have a 'size()' member function returning an object convertible to 'std::size_t'}} \


The diagnostic is actually slightly worse than it was before -- this does have a size() member function that returns an object convertible to std::size_t, the issue is that it's not viable to call because it has required parameters. The previous notes were a bit better at showing this because some users use () to mean "this is the name of a function not a variable" in prose and not "this is a function call accepting no arguments" -- e.g., memcpy() vs errno Same issue applies to the data() diagnostic below.

Can we do anything about this (without a ton of effort)?

The only thing that change is that the note previously attached to the declaration of size() is now an error attached to the call site (L195) and the declaration gets a 'declared here' note.

I don't see how we could preserve the previous diagnostics, they all come from deep in overload resolution.

Okay, I didn't think it would be easy to retain the previous behavior, but I still had to ask just in case.

- Support non-member functions and callable objects for size and data(). We previously tried to (badly) pick the best overload ourselves, in a way that would only support member functions. We now leave clamg construct an unresolved member expression and call that, properly performing overload resolution with callable objects and static functions, cojnsistent with the logic for `get` calls for structured bindings. - Support UDLs as message expression. - Add tests and mark CWG2798 as resolved

AaronBallman

LGTM!

sylvestre · 2023-11-28T09:31:24Z

Seems it caused:
#73628

cor3ntin · 2023-11-28T11:01:25Z

@sylvestre Thanks. It seems like GCC 7.5 does not select the appropriate constructor https://compiler-explorer.com/z/oc315aYnT - I will commit a fix in a bit.

sylvestre · 2023-11-28T11:08:51Z

Merci à toi :)

cor3ntin requested a review from Endilll as a code owner November 23, 2023 11:57

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Nov 23, 2023

cor3ntin added c++26 and removed clang Clang issues not falling into any other category labels Nov 23, 2023

cor3ntin force-pushed the corentin/fix_static_assert branch from c8f8e6d to c274d62 Compare November 23, 2023 12:06

llvmbot added the clang Clang issues not falling into any other category label Nov 23, 2023

oliverscherf-tomtom reviewed Nov 23, 2023

View reviewed changes

Endilll approved these changes Nov 23, 2023

View reviewed changes

AaronBallman reviewed Nov 27, 2023

View reviewed changes

cor3ntin added 2 commits November 27, 2023 16:51

Fix typos

c8a881a

cor3ntin force-pushed the corentin/fix_static_assert branch from 10433d3 to 1ff9247 Compare November 27, 2023 15:56

AaronBallman approved these changes Nov 27, 2023

View reviewed changes

Format

5125714

cor3ntin force-pushed the corentin/fix_static_assert branch 2 times, most recently from 3bbe1db to 5125714 Compare November 27, 2023 20:20

cor3ntin merged commit fdefe88 into llvm:main Nov 28, 2023

[Clang] Improve support for expression messages in static_assert #73234

[Clang] Improve support for expression messages in static_assert #73234

Uh oh!

Conversation

cor3ntin commented Nov 23, 2023 • edited by Endilll Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Nov 23, 2023

Uh oh!

github-actions bot commented Nov 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Endilll left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

sylvestre commented Nov 28, 2023

Uh oh!

cor3ntin commented Nov 28, 2023

Uh oh!

sylvestre commented Nov 28, 2023

Uh oh!

Uh oh!

[Clang] Improve support for expression messages in `static_assert` #73234

[Clang] Improve support for expression messages in `static_assert` #73234

cor3ntin commented Nov 23, 2023 •

edited by Endilll

Loading

github-actions bot commented Nov 23, 2023 •

edited

Loading