[Clang] Implement CWG2518 - static_assert(false)

[cor3ntin]

Summary

This allows static_assert(false) to not be ill-formed
in template definitions.

This change is applied as a DR in all C++ modes.

Of notes, a couple of tests were relying of the eager nature
of static_assert

• test/SemaTemplate/instantiation-dependence.cpp
• test/SemaTemplate/instantiate-var-template.cpp

I don't know if the changes to static_assert
still allow that sort of tests to be expressed.

[cor3ntin]

Forgot to run clang-format

[erichkeane]

clang/lib/Sema/SemaDeclCXX.cpp

16790

Hmm... interesting way to calculate template depth, I wasn't aware of that one. Does this cause problems in 'a template caused another template to instantiate' sorta thing?

Also, is the "CPlusPlus" test here necessary?

clang/test/SemaTemplate/instantiate-var-template.cpp

34

You should be able to instantiate this template later, and probably what we now have to do. Also, 'dependent' is the spelling in this case, 'dependant' is something different :)

clang/test/SemaTemplate/instantiation-dependence.cpp

40–41

This one is probably a bigger pain to test, and I don't have a good idea.

[Endill]

Thank you for the patch.
Any plans to backport this to 16.x branch?

[cor3ntin]

clang/lib/Sema/SemaDeclCXX.cpp

16790

It's what I came up with. Do you think there is a better way?
If you have suggestions for additional tests, I'm all ears!

The CplusPlus tests is just there to avoid unnecessary cycles in C mode. It probably doesn't do much of a difference.

clang/test/SemaTemplate/instantiate-var-template.cpp

34

I'm afraid doing though would defeat the intent of the test - it is after all named "InstantiationDependent"

[cor3ntin]

In D144285#4135775, @Endill wrote:

Thank you for the patch.
Any plans to backport this to 16.x branch?

I think that's something we'd need to discuss with more folks but maybe a bit of time to see how this behaves on real code wouldn't hurt - even if I understand people have a keen interest in getting access to that sooner rather than later.

[erichkeane]

In D144285#4135775, @Endill wrote:

Thank you for the patch.
Any plans to backport this to 16.x branch?

I would not really want us to do that, the breaking change here is concerning, and I'd like this to spend time in trunk 'baking' a while.

clang/lib/Sema/SemaDeclCXX.cpp

16790

For "Zero" I usually would just check if the current decl-context is dependent. I'm not positive if that is 'correct' without more thought, though I would expect it to be? I think we make sure that is set up correctly everywhere. it would at least be a good check to see where we mess that up :)

clang/test/SemaTemplate/instantiate-var-template.cpp

34

Ah, yeah, you're right, it isn't clear what this is trying to test to me unfortunately. It might require some history digging. Same comment as below on the spelling.

[cor3ntin]

• Simplify how we check we are in a dependent context
• Fix typos

[cor3ntin]

clang/lib/Sema/SemaDeclCXX.cpp

16790

Yes, i feel stupid now, that was the obvious solution! Thanks

[erichkeane]

clang/lib/Sema/SemaDeclCXX.cpp

16791

CplusPlus check is now not really beneficial? I'm not sure how much it matters now though that these are both just bit-loads.

[cor3ntin]

clang/lib/Sema/SemaDeclCXX.cpp

16791

isDependentContext still does a bunch of work, recursively. I think we should keep it!

[erichkeane]

clang/lib/Sema/SemaDeclCXX.cpp

16791

Ah, Oh! You're right! It is the Expr class where this is free/just checking a bit. Disregard.

[shafik]

clang/docs/ReleaseNotes.rst

76

I am assuming this will be updated eventually but that version is not the final one and the one that was approved can be found from: https://github.com/cplusplus/papers/issues/1251

and it here: https://cplusplus.github.io/CWG/issues/2518.html

I think a better wording would be would not be ill-formed when evaluated in the context of a template definition

clang/lib/Sema/SemaDeclCXX.cpp

16791

So isDependentContext() strictly means we are in a definition context? I think it makes sense after reading the implementation but not obvious at first. Maybe that would to document someplace?

[erichkeane]

clang/lib/Sema/SemaDeclCXX.cpp

16791

Shafik: (if you mean Template Definition Context), yes more or less. IIRC, that term was added to the standard after the clang Templates were implemented (I remember someone mentioning a big standards 'template' rewrite a few years back), so thats perhaps why it wasn't named that. You can get the same from an expression (isInstantiationDependent, isValueDependent, and isTypeDependent, the latter two having language equivalence, and the former just a catch-all).

Feel free to write up some documentation on this in the internals manual, as long as you properly explain the Template Definition Context/etc means.

[rsmith]

clang/lib/Sema/SemaDeclCXX.cpp

16809–16827

I wonder if it's worth adding a custom diagnostic (eg, "this template cannot be instantiated: %0") for the case where we're in template instantiation and the expression is the bool literal false.

clang/test/SemaCXX/static-assert.cpp

53–59

I think we should also test the case where AlwaysFails is instantiated twice, and that we get one "static assertion failed" error per instantiation, rather than only one in total.

clang/test/SemaTemplate/instantiate-var-template.cpp

34

I think you can preserve the intent of this test by using the old array trick rather than a static assert:

int check[a<sizeof(sizeof(f(T())))> == 0 ? 1 : -1]; // expected-error {{array with a negative size}}

clang/test/SemaTemplate/instantiation-dependence.cpp

40–41

You can use the array trick here too.

[cor3ntin]

• Reword Release note
• Restore dependent instantiation tests using Richard's suggestion
• Add additional test to check that diagnostics for static_assert are emitted once per instantiation.

[cor3ntin]

clang/docs/ReleaseNotes.rst

76

I'll tweak the comment a bit, thanks.
The link points to the yet-to-be-published core issue list so it will automatically point to the right thing eventually.

clang/lib/Sema/SemaDeclCXX.cpp

16809–16827

I'm not sure i see the motivation. Why would we want to special case false? The expression could also be an always false, never dependent expression

[aaron.ballman]

In D144285#4135807, @erichkeane wrote:

In D144285#4135775, @Endill wrote:

Thank you for the patch.
Any plans to backport this to 16.x branch?

I would not really want us to do that, the breaking change here is concerning, and I'd like this to spend time in trunk 'baking' a while.

+1, at this point the only things we should be backporting to 16.x are fixes for regressions or fixes for security concerns.

clang/lib/Sema/SemaDeclCXX.cpp

16809–16827

Richard may have different ideas in mind, but the motivation to me is code like:

template <typename Ty>
struct S {
  static_assert(false, "you have to use one of the valid specializations, not the primary template");
};

template <>
struct S<int> {
};

template <>
struct S<float> {    
};

int main() {
  S<int> s1;
  S<float> s2;
  S<double> s3;
}

Rather than telling the user the static_assert failed because false is not true, having a custom diagnostic might read better for users. GCC doesn't produce a custom diagnostic -- the behavior isn't terrible, but the "false evaluates to false" note is effectively just noise, too: https://godbolt.org/z/456bzWG7c

[cor3ntin]

clang/lib/Sema/SemaDeclCXX.cpp

16809–16827

OH. That makes sense now, thanks. I think I agree.
Interestingly, in gcc immediate calls are really immediate :) https://godbolt.org/z/b3vrzf4sj

[cor3ntin]

I played with different diagnostics, we already specialize the
literal case, and I didn't find that not saying a static_assert
failed improved things.
Instead, printing, in addition of that diagnostic, an instantiation
stack provides additional useful information.
(The stack is only printed for the literal bool/integer case)

[aaron.ballman]

clang/lib/Sema/SemaDeclCXX.cpp

16809–16827

I think you should add this case as a test in dr25xx.cpp to show we get the behavior correct with specializations.

clang/test/CXX/drs/dr25xx.cpp

5–14

What do these tests have to do with this DR?

9

Why do we expect an error on this line in a #if 0 block??

clang/test/SemaCXX/static-assert.cpp

235–236

[cor3ntin]

Address Aaron's comments

[cor3ntin]

clang/test/CXX/drs/dr25xx.cpp

5–14

This dr is wild https://wiki.edg.com/pub/Wg21issaquah2023/StrawPolls/p2796r0.html
CWG merged the static_assert PR in the DR asserting that error should produce a diagnostics - note that there will probably be some follow ups
https://lists.isocpp.org/core/2023/02/13915.php

Here I'm testing a warning is emitted even if the build was already failed.

9

Oups, we don't

[aaron.ballman]

clang/test/CXX/drs/dr25xx.cpp

5–14

Ah, yeah, CWG2700 is sort of included in here.

I wonder if we should back out the #error/#warning tests and handle that explicitly as part of CWG2700 as that's going to supersede the changes from CWG2518.

[cor3ntin]

Remove the #error / #warning tests

[aaron.ballman]

LGTM!

clang/test/SemaTemplate/instantiate-var-template.cpp

36

:: gasps :: spurious whitespace change! :-D

[rupprecht]

Here's one change this patch causes on "real" code (invalid code, but something a user might try to compile): we see is a static_assert in gmock that now fails to report a useful error message: https://godbolt.org/z/sPr1PYT9d

Previously we saw error: static assertion failed due to requirement <...snip...>: This method does not take 2 arguments. Parenthesize all types with unprotected commas.. It still fails to compile, but without the nice error message.

My guess, although I haven't looked too closely, is that it's due to the static_assert(false) here: https://github.com/google/googletest/blob/2d4f208765af7fa376b878860a7677ecc0bc390a/googlemock/include/gmock/gmock-function-mocker.h#L149

[cor3ntin]

@rupprecht

This expands to

template <typename T>
class MyMock {
  static_assert( static_assert( ::testing::tuple_size<typename ::testing::internal::Function< ::testing::internal::identity_t<int(double x, char c, bool cond)> >::ArgumentTuple>::value == 2, "This method does not take " "2" " arguments. Parenthesize all types with unprotected commas."); 
};

(some stuff omitted)

Not getting the error here is the expected outcome of this change.
The committee considered that these scenarios were far less likely than the scenario where people did not want a diagnostic.
Instantiating MyMock<int> T; produces the warning.

I think this code can be changed to something like that https://godbolt.org/z/6bcrYfdTf - to partially recover the previous diagnostics.
If however we find this change to disruptive, we should inform WG21.

[rupprecht]

In D144285#4163004, @cor3ntin wrote:

If however we find this change to disruptive, we should inform WG21.

Thanks for the explanation, I think I understand the issue now. I got totally thrown off by the title -- it's not about literally writing static_assert(false), it's about deferring static_assert evaluation to template instantiations. Being able to write static_assert(false) (or any falsy constant expression) is just the common use case for this.

So possibly the most trivial example of something that used to break, but now builds:

template <typename>
void Fail() {
    static_assert(false, "my assertion failed");
}

... but will still fail as soon as you invoke Fail<any_type>() somewhere.

It doesn't look like there's a lot of impact from this, and the breakages are corner cases like this. It might be worth mentioning this one case to WG21 but I'm not sure what I would change about the wording.

clang/www/cxx_dr_status.html

14915

Is it possible to make this link point to https://cplusplus.github.io/CWG/issues/2518.html? This link is inaccessible to anyone not on ISO.

[cor3ntin]

In D144285#4176850, @rupprecht wrote:

In D144285#4163004, @cor3ntin wrote:

If however we find this change to disruptive, we should inform WG21.

Thanks for the explanation, I think I understand the issue now. I got totally thrown off by the title -- it's not about literally writing static_assert(false), it's about deferring static_assert evaluation to template instantiations. Being able to write static_assert(false) (or any falsy constant expression) is just the common use case for this.

So possibly the most trivial example of something that used to break, but now builds:

template <typename>
void Fail() {
    static_assert(false, "my assertion failed");
}

... but will still fail as soon as you invoke Fail<any_type>() somewhere.

It doesn't look like there's a lot of impact from this, and the breakages are corner cases like this. It might be worth mentioning this one case to WG21 but I'm not sure what I would change about the wording.

Will do !

clang/www/cxx_dr_status.html

14915

It will get updated whenever a new core issue list gets published, which will hopefully be soon-ish.
This file is generated automatically so changing the link might actually not be trivial

clang/docs/ReleaseNotes.rst

 infrastructure are described first, followed by language-specific
 sections with improvements to Clang's support for those languages.
 
 C++ Language Changes
 --------------------
 - Improved ``-O0`` code generation for calls to ``std::forward_like``. Similarly to
   ``std::move, std::forward`` et al. it is now treated as a compiler builtin and implemented
   directly rather than instantiating the definition from the standard library.
+- Implemented `CWG2518 <https://wg21.link/CWG2518>`_ which allows ``static_assert(false)``
+  to not be ill-formed when its condition is evaluated in the context of a template definition.
 
 C++20 Feature Support
 ^^^^^^^^^^^^^^^^^^^^^
 
 C++2b Feature Support
 ^^^^^^^^^^^^^^^^^^^^^
 
 Resolutions to C++ Defect Reports

clang/lib/Sema/SemaDeclCXX.cpp

if (!AssertExpr->isTypeDependent() && !AssertExpr->isValueDependent() &&
     }
 
     if (!Failed && VerifyIntegerConstantExpression(
                        BaseExpr, &Cond,
                        diag::err_static_assert_expression_is_not_constant,
                        FoldKind).isInvalid())
       Failed = true;
 
-    if (!Failed && !Cond) {
+    // CWG2518
+    // [dcl.pre]/p10  If [...] the expression is evaluated in the context of a
+    // template definition, the declaration has no effect.
+    bool InTemplateDefinition =
+        getLangOpts().CPlusPlus && CurContext->isDependentContext();
+
+    if (!Failed && !Cond && !InTemplateDefinition) {
+
       SmallString<256> MsgBuffer;
       llvm::raw_svector_ostream Msg(MsgBuffer);
       if (AssertMessage) {
         const auto *MsgStr = cast<StringLiteral>(AssertMessage);
         if (MsgStr->isOrdinary())
           Msg << MsgStr->getString();
         else
           MsgStr->printPretty(Msg, nullptr, getPrintingPolicy());
       }
 
       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)
           << !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)
             << !AssertMessage << Msg.str() << AssertExpr->getSourceRange();
+        PrintContextStack();
       }
       Failed = true;
     }
   } else {
     ExprResult FullAssertExpr = ActOnFinishFullExpr(AssertExpr, StaticAssertLoc,
                                                     /*DiscardedValue*/false,
                                                     /*IsConstexpr*/true);
     if (FullAssertExpr.isInvalid())
       Failed = true;

clang/test/CXX/drs/dr25xx.cpp

 // RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-unknown %s -verify
 
+namespace dr2518 { // dr2518: 17 review
+
+template <class T>
+void f(T t) {
+  if constexpr (sizeof(T) != sizeof(int)) {
+    static_assert(false, "must be int-sized"); // expected-error {{must be int-size}}
+  }
+}
+
+void g(char c) {
+  f(0);
+  f(c); // expected-note {{requested here}}
+}
+
+template <typename Ty>
+struct S {
+  static_assert(false); // expected-error {{static assertion failed}}
+};
+
+template <>
+struct S<int> {};
+
+template <>
+struct S<float> {};
+
+int test_specialization() {
+  S<int> s1;
+  S<float> s2;
+  S<double> s3; // expected-note {{in instantiation of template class 'dr2518::S<double>' requested here}}
+}
+
+}
+
+
 namespace dr2565 { // dr2565: 16 open
   template<typename T>
     concept C = requires (typename T::type x) {
       x + 1;
     };
   static_assert(!C<int>);
 
   // Variant of this as reported in GH57487.

clang/test/SemaCXX/access-base-class.cpp

 
 namespace T8 {
 template <int>
 struct flag {
   static constexpr bool value = true;
 };
 
 template <class T>
-struct trait : flag<sizeof(T)> {};
+struct trait : flag<sizeof(T)> {}; // expected-note 2{{here}}
 
-template <class T, bool Inferred = trait<T>::value>
+template <class T, bool Inferred = trait<T>::value> // expected-note {{here}}
 struct a {};
 
 template <class T>
 class b {
-  a<T> x;
+  a<T> x; // expected-note {{here}}
   using U = a<T>;
 };
 
 template <int>
 struct Impossible {
   static_assert(false, ""); // expected-error {{static assertion failed}}
 };
 
 // verify "no member named 'value'" bogus diagnostic is not emitted.
-trait<b<Impossible<0>>>::value;
+trait<b<Impossible<0>>>::value; // expected-note {{here}}
 } // namespace T8

clang/test/SemaCXX/coroutines.cpp

struct DepTestType {
   static CoroMemberTag test_static_template(const char *volatile &, unsigned) {
     auto TC = co_yield 0;
     using TCT = decltype(TC);
     static_assert(TCT::MatchesArgs<const char *volatile &, unsigned>, "");
     static_assert(!TCT::MatchesArgs<DepTestType &, const char *volatile &, unsigned>, "");
   }
 };
 
-template struct DepTestType<int>; // expected-note {{requested here}}
+template struct DepTestType<int>; // expected-note 2{{requested here}}
 template CoroMemberTag DepTestType<int>::test_member_template(long, const char *) const &&;
 
 template CoroMemberTag DepTestType<int>::test_static_template<void>(const char *volatile &, unsigned);
 
 struct bad_promise_deleted_constructor {
   // expected-note@+1 {{'bad_promise_deleted_constructor' has been explicitly marked deleted here}}
   bad_promise_deleted_constructor() = delete;
   coro<bad_promise_deleted_constructor> get_return_object();

clang/test/SemaCXX/static-assert.cpp

 // expected-error@-1 {{static assertion failed: <U+0080><U+0081><U+0082><U+0083><U+0099><U+009A><U+009B><U+009C><U+009D><U+009E><U+009F>}}
 
 //! Contains RTL/LTR marks
 static_assert(false, "\u200Eabc\u200Fdef\u200Fgh"); // expected-error {{static assertion failed: ‎abc‏def‏gh}}
 
 //! Contains ZWJ/regional indicators
 static_assert(false, "🏳️‍🌈 🏴󠁧󠁢󠁥󠁮󠁧󠁿 🇪🇺"); // expected-error {{static assertion failed: 🏳️‍🌈 🏴󠁧󠁢󠁥󠁮󠁧󠁿 🇪🇺}}
 
 template<typename T> struct AlwaysFails {
   // Only give one error here.
-  static_assert(false, ""); // expected-error {{static assertion failed}}
+  static_assert(false, ""); // expected-error 2{{static assertion failed}}
 };
-AlwaysFails<int> alwaysFails;
+AlwaysFails<int> alwaysFails; // expected-note {{instantiation}}
+AlwaysFails<double> alwaysFails2; // expected-note {{instantiation}}
+
 
 template<typename T> struct StaticAssertProtected {
   static_assert(__is_literal(T), ""); // expected-error {{static assertion failed}}
   static constexpr T t = {}; // no error here
 };
 struct X { ~X(); };
 StaticAssertProtected<int> sap1;
 StaticAssertProtected<X> sap2; // expected-note {{instantiation}}

 struct NotBool {
 } notBool;
 constexpr NotBool constexprNotBool;
 static_assert(notBool, "message");          // expected-error {{value of type 'struct NotBool' is not contextually convertible to 'bool'}}
 static_assert(constexprNotBool, "message"); // expected-error {{value of type 'const NotBool' is not contextually convertible to 'bool'}}
 
 static_assert(1 , "") // expected-error {{expected ';' after 'static_assert'}}
 
+namespace DependentAlwaysFalse {
+template <typename Ty>
+struct S {
+  static_assert(false); // expected-error{{static assertion failed}} \
+                        // expected-warning {{C++17 extension}}
+};
+
+template <typename Ty>
+struct T {
+  static_assert(false, "test"); // expected-error{{static assertion failed: test}}
+};
+
+int f() {
+  S<double> s; //expected-note {{in instantiation of template class 'DependentAlwaysFalse::S<double>' requested here}}
+  T<double> t; //expected-note {{in instantiation of template class 'DependentAlwaysFalse::T<double>' requested here}}
+}
+}
 
 namespace Diagnostics {
   /// No notes for literals.
   static_assert(false, ""); // expected-error {{failed}}
   static_assert(1.0 > 2.0, ""); // expected-error {{failed}}
   static_assert('c' == 'd', ""); // expected-error {{failed}}
   static_assert(1 == 2, ""); // expected-error {{failed}}
 

clang/test/SemaTemplate/instantiate-var-template.cpp

namespace InstantiationDependent {
 
   template<int> constexpr int a = 1;
   template<typename T> constexpr T b = a<sizeof(sizeof(f(T())))>; // expected-error {{invalid application of 'sizeof' to an incomplete type 'void'}}
 
   static_assert(b<int> == 1, "");
   static_assert(b<char> == 1, ""); // expected-note {{in instantiation of}}
 
   template<typename T> void f() {
-    static_assert(a<sizeof(sizeof(f(T())))> == 0, ""); // expected-error {{static assertion failed due to requirement 'a<sizeof (sizeof (f(type-parameter-0-0())))> == 0'}} \
-                                                       // expected-note {{evaluates to '1 == 0'}}
+    int check[a<sizeof(sizeof(f(T())))> == 0 ? 1 : -1]; // expected-error {{array with a negative size}}
   }
 }
 
 namespace PR24483 {
   template<typename> struct A;
   template<typename... T> A<T...> models;
   template<> struct B models<>; // expected-error {{incomplete type 'struct B'}} expected-note {{forward declaration}}
 }
 
 namespace InvalidInsertPos {
   template<typename T, int N> T v;
   template<int N> decltype(v<int, N-1>) v<int, N>;
   template<> int v<int, 0>;
   int k = v<int, 500>;
 }

clang/test/SemaTemplate/instantiation-dependence.cpp

namespace PR33655 {
 
   int &test1 = func<One>();
   float &test2 = func<Two>();
 
   template<class ...Args> struct indirect_void_t_imp { using type = void; };
   template<class ...Args> using indirect_void_t = typename indirect_void_t_imp<Args...>::type;
 
   template<class T> void foo() {
-    static_assert(!__is_void(indirect_void_t<T>)); // "ok", dependent
-    static_assert(!__is_void(void_t<T>)); // expected-error {{failed}}
+    int check1[__is_void(indirect_void_t<T>) == 0 ? 1 : -1]; // "ok", dependent
+    int check2[__is_void(void_t<T>) == 0 ? 1 : -1]; // expected-error {{array with a negative size}}
   }
 }
 
 namespace PR46791 { // also PR45782
   template<typename T, typename = void>
   struct trait {
     static constexpr int specialization = 0;
   };

clang/www/cxx_dr_status.html

   </tr>
   <tr class="open" id="2517">
     <td><a href="https://wg21.link/cwg2517">2517</a></td>
     <td>open</td>
     <td>Useless restriction on use of parameter in <I>constraint-expression</I></td>
     <td align="center">Not resolved</td>
   </tr>
   <tr class="open" id="2518">
     <td><a href="https://wg21.link/cwg2518">2518</a></td>
     <td>review</td>
     <td>Conformance requirements and <TT>#error</TT>/<TT>#warning</TT></td>
-    <td align="center">Not resolved</td>
+    <td class="unreleased" align="center">Clang 17</td>
   </tr>
   <tr class="open" id="2519">
     <td><a href="https://wg21.link/cwg2519">2519</a></td>
     <td>drafting</td>
     <td>Object representation of a bit-field</td>
     <td align="center">Not resolved</td>
   </tr>
   <tr class="open" id="2520">