[Clang] Implement CWG2518 - static_assert(false) #9200

compnerd · 2024-08-31T00:46:05Z

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.

Reviewed By: #clang-language-wg, erichkeane, aaron.ballman

Differential Revision: https://reviews.llvm.org/D144285

(cherry picked from commit 00e2098)

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. Reviewed By: #clang-language-wg, erichkeane, aaron.ballman Differential Revision: https://reviews.llvm.org/D144285 (cherry picked from commit 00e2098)

compnerd · 2024-08-31T00:46:12Z

@swift-ci please test

ahatanaka · 2024-09-04T03:40:43Z

It looks like this commit broke the following clang tests:

CXX/drs/dr25xx.cpp
SemaCXX/static-assert-cxx26.cpp
SemaCXX/static-assert.cpp

@compnerd, could you fix them?

compnerd · 2024-09-04T17:20:44Z

@ahatanaka sure - do you have a link to the failure? I'm mostly building on Windows. It seems that there might be local changes to the tests as this was a direct cherry-pick from upstream.

ahatanaka · 2024-09-04T18:38:53Z

Unfortunately, this is an internal CI failure, so we can't share the link with you. You can reproduce the failure by checking out this branch and running the tests: https://github.com/swiftlang/llvm-project/tree/stable/20230725.

ahatanaka · 2024-09-04T18:52:31Z

It looks like clang fails to compile the code you added when the target C++ standard is old.

error: 'error' diagnostics seen but not expected:
  File clang/test/CXX/drs/dr25xx.cpp Line 88: expected '(' after 'if'
  File clang/test/CXX/drs/dr25xx.cpp Line 89: expected directive cannot follow 'expected-no-diagnostics' directive
  File clang/test/CXX/drs/dr25xx.cpp Line 95: expected directive cannot follow 'expected-no-diagnostics' directive
  File clang/test/CXX/drs/dr25xx.cpp Line 100: expected parameter declarator
  File clang/test/CXX/drs/dr25xx.cpp Line 100: expected ')'
  File clang/test/CXX/drs/dr25xx.cpp Line 100: a type specifier is required for all declarations
  File clang/test/CXX/drs/dr25xx.cpp Line 100: expected directive cannot follow 'expected-no-diagnostics' directive
  File clang/test/CXX/drs/dr25xx.cpp Line 112: expected directive cannot follow 'expected-no-diagnostics' directive
error: 'note' diagnostics seen but not expected:
  File clang/test/CXX/drs/dr25xx.cpp Line 100: to match this '('
9 errors generated.

Note that 00e2098 only tests -std=c++20.

drodriguez · 2024-09-06T00:37:36Z

Got the same kind of errors in our own internal CI.

I don't understand why this needs to be cherry-picked, though. This branch is supposed to be branched on 2023-07-23, and the commit was landed in LLVM upstream 2023-02-28. In fact it is part of the branch already in 00e2098.

If one compare the diffs of this PR and 00e2098, probably because cherry-picking this diff overwrites some other commits since the original, instead of reapplying it cleanly.

Can we revert this?

compnerd · 2024-09-06T00:44:27Z

The change to clang/lib/Sema/SemaDeclCXX.cpp seems to be missing - the early check does result in a failure of static_assert(false) for an unevaluated template. The diff shows this matching up the behaviour there.

drodriguez

This should be reverted ASAP. I can prepare the PR, but I cannot merge myself, so if someone can do it quicker, please do.

A simple @swift-ci please test in changes of the llvm-project is normally not enough to catch things. I recommend @swift-ci please llvm test but it is normally broken for other causes, so it cannot be used as a barrier for merging.

drodriguez · 2024-09-06T00:49:31Z

clang/lib/Sema/SemaDeclCXX.cpp

-    // If the static_assert passes, only verify that
-    // the message is grammatically valid without evaluating it.
-    if (!Failed && AssertMessage && Cond.getBoolValue()) {
-      std::string Str;
-      EvaluateStaticAssertMessageAsString(AssertMessage, Str, Context,
-                                          /*ErrorOnInvalidMessage=*/false);
-    }
-


This bit is added in 47ccfd7 (https://reviews.llvm.org/D154290) on 2023-07-19, after the commit that is being double cherry-picked added the chunk of code below.

drodriguez · 2024-09-06T00:54:05Z

clang/lib/Sema/SemaDeclCXX.cpp

-        Diag(AssertExpr->getBeginLoc(), diag::err_static_assert_failed)
-            << !HasMessage << Msg.str() << AssertExpr->getSourceRange();
+        Diag(StaticAssertLoc, diag::err_static_assert_failed)
+            << !AssertMessage << Msg.str() << AssertExpr->getSourceRange();


This two lines are undoing the changes in:

66202d8 (https://reviews.llvm.org/D147745) on 2023-04-13, which modifies the code around the one the originally commit also modified before.

47ccfd7 (https://reviews.llvm.org/D154290) on 2023-07-19, which modifies the code introduced by the original commit.

drodriguez · 2024-09-06T00:56:25Z

clang/test/CXX/drs/dr25xx.cpp

+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}}
+}
+
+}
+
+


The same namespace exist in line 24 above, but that code includes modifications to the original test that are not part of this repeated section, like the ones introduced in 5ce5e98 which should avoid one of the failures we are experiencing in the tests.

This reverts commit 4857f75, reversing changes made to fe0f467. This reverts PR #9200. The cherry-pick that's being tried in that PR is already part of the branch (00e2098), and the commit removes code added after the original commit, and duplicates some testing code, producing errors in several tests. See the comments of #9200 for more details.

Revert "Merge pull request #9200 from compnerd/static_assert_false"

compnerd merged commit 4857f75 into swiftlang:stable/20230725 Sep 1, 2024
3 checks passed

compnerd deleted the static_assert_false branch September 1, 2024 04:40

drodriguez reviewed Sep 6, 2024

View reviewed changes

drodriguez mentioned this pull request Sep 6, 2024

Revert "Merge pull request #9200 from compnerd/static_assert_false" #9215

Merged

drodriguez added a commit that referenced this pull request Sep 6, 2024

Merge pull request #9215 from drodriguez/undo-9200

6f0a61d

Revert "Merge pull request #9200 from compnerd/static_assert_false"

etcwilde mentioned this pull request Feb 26, 2025

Swift on Windows from Winget is apparently too old for Visual Studio swiftlang/swift#79439

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Clang] Implement CWG2518 - static_assert(false) #9200

[Clang] Implement CWG2518 - static_assert(false) #9200

Uh oh!

compnerd commented Aug 31, 2024

Uh oh!

compnerd commented Aug 31, 2024

Uh oh!

Uh oh!

ahatanaka commented Sep 4, 2024

Uh oh!

compnerd commented Sep 4, 2024

Uh oh!

ahatanaka commented Sep 4, 2024

Uh oh!

ahatanaka commented Sep 4, 2024

Uh oh!

drodriguez commented Sep 6, 2024

Uh oh!

compnerd commented Sep 6, 2024

Uh oh!

drodriguez left a comment

Uh oh!

drodriguez Sep 6, 2024

Uh oh!

drodriguez Sep 6, 2024

Uh oh!

drodriguez Sep 6, 2024

Uh oh!

Uh oh!

[Clang] Implement CWG2518 - static_assert(false) #9200

[Clang] Implement CWG2518 - static_assert(false) #9200

Uh oh!

Conversation

compnerd commented Aug 31, 2024

Uh oh!

compnerd commented Aug 31, 2024

Uh oh!

Uh oh!

ahatanaka commented Sep 4, 2024

Uh oh!

compnerd commented Sep 4, 2024

Uh oh!

ahatanaka commented Sep 4, 2024

Uh oh!

ahatanaka commented Sep 4, 2024

Uh oh!

drodriguez commented Sep 6, 2024

Uh oh!

compnerd commented Sep 6, 2024

Uh oh!

drodriguez left a comment

Choose a reason for hiding this comment

Uh oh!

drodriguez Sep 6, 2024

Choose a reason for hiding this comment

Uh oh!

drodriguez Sep 6, 2024

Choose a reason for hiding this comment

Uh oh!

drodriguez Sep 6, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!