Improve static_assert/_Static_assert diagnostics

[aaron.ballman]

Summary

I noticed that our diagnostics relating to static assertions are a bit confused. For instance, when in MS compatibility mode in C (where we accept static_assert even without including <assert.h>), we would fail to warn the user that they were using the wrong spelling (even in pedantic mode), we were missing a compatibility warning about using _Static_assert in earlier standards modes, diagnostics for the optional message were not reflected in C as they were in C++, etc.

This patch improves the diagnostic functionality in both C and C++ mode. It adds -Wc89-c99-c11-c17-compat and -Wc89-c99-c11-c17-compat-pedantic diagnostics groups and adds new C-specific diagnostics for a static assertion without a diagnostic message.

[aaron.ballman]

Rebasing.

[rsmith]

clang/lib/Parse/ParseDeclCXX.cpp

875–877

I don't think this diagnostic is useful as-is: on Windows, including <assert.h> doesn't help because it doesn't #define static_assert. And people hitting this also can't switch to using _Static_assert, because MSVC doesn't provide it, only static_assert.

If we want to warn here, we could perhaps check whether <assert.h> has been included, but getting that check correct across PCH / modules is not straightforward. (If we knew what include guard the CRT's assert.h used (if any), I guess we could check whether that's defined, but that'd be a bit of a hack.) But I'm somewhat inclined to think we don't have a good way to distinguish between the good cases and the bad ones, so we shouldn't warn. Hopefully MS will fix their CRT at some point and we can stop providing this compatibility hack entirely (or start warning on it by default).

[aaron.ballman]

clang/lib/Parse/ParseDeclCXX.cpp

875–877

Are you sure they don't support _Static_assert yet? I seem to be able to use it fine: https://godbolt.org/z/vG47he

That said, this does appear to be only available in newer versions of MSVC, so perhaps you're correct about the diagnostic being a bit unhelpful. My primary concern is that use of static_assert in C is a nonconforming extension and we default to -fms-compatibility on Windows when Clang is built by MSVC. So it's not difficult to accidentally run into this, but the only warning we give on it with -Weverything -pedantic is how it's not compatible with C++98.

WDYT?

[aaron.ballman]

clang/lib/Parse/ParseDeclCXX.cpp

875–877

I suppose one option would be to look at what version of MSVC we're trying to be compatible with to see if that's a version that supports /std:c11 and only emit this diagnostic in that case, but tbh, that feels like it'll lead to confusing diagnostic behavior (esp given that we default to ms compatibility mode silently when you build Clang with MSVC on Windows).

Given that MSVC does support _Static_assert when you enable C11 or later language mode, I'm inclined to warn on this construct by default.

WDYT?

[aaron.ballman]

Ping.

[rsmith]

FYI, https://reviews.llvm.org/D17444 captures some of the history here.

clang/lib/Parse/ParseDeclCXX.cpp

875–877

Well, it's good to see that they've made progress, but it looks like their <assert.h> still doesn't #define static_assert, so I think we still don't have an actionable warning we can produce here. We can't reasonably tell people to include <assert.h> (as this patch does) because that doesn't work. And it doesn't seem reasonable to tell people to use _Static_assert instead, if they actually have included <assert.h>. (I don't think we want to encourage people to use _Static_assert instead of <assert.h> + static_assert.)

So I don't think MSVC adding support for _Static_assert really changes anything here -- until their <assert.h> works, or we find some good way to detect whether it was properly included, this warning will fire on both correct code and incorrect code, which doesn't seem all that useful.

[aaron.ballman]

clang/lib/Parse/ParseDeclCXX.cpp

875–877

And it doesn't seem reasonable to tell people to use _Static_assert instead, if they actually have included <assert.h>. (I don't think we want to encourage people to use _Static_assert instead of <assert.h> + static_assert.)

Ideally, yes. But this isn't ideal -- we produce no diagnostic for this nonconforming extension and that's causing pain in practice. As an example of where I ran into this: I had a header file that was shared between C and (mostly) C++ code and added a static_assert to it but forgot to add #include <assert.h>. This compiled great in MSVC and clang-cl, but when compiled with clang on CI is when I finally found the issue. e.g., like this: https://godbolt.org/z/cs8YGb

If I had to pick between behaviors, I think I'd prefer pushing people towards using _Static_assert even if assert.h is included over silently accepting a nonconforming extension in pedantic mode.

Rather than trying to see what header guard was used by <assert.h>, couldn't we assume that if assert is defined as a macro then <assert.h> must have been included (or the user triggered UB and gets what they get)? So the logic could be: only diagnose use of the static_assert (keyword) in C mode if assert is not defined?

We can't reasonably tell people to include <assert.h> (as this patch does) because that doesn't work.

But it does (as far as the user is concerned)? In MS compatibility mode, static_assert is always accepted, so the include isn't strictly necessary but isn't harmful either. Outside of MS compatibility mode, the include is required to spell it static_assert because we won't treat it as a keyword. If we go with the "only diagnose when assert is not defined as a macro" approach, it would give us the desired behavior here.

[rsmith]

clang/lib/Parse/ParseDeclCXX.cpp

875–877

we produce no diagnostic for this nonconforming extension

Well, I'd argue that it's not a non-conforming extension so much as it's a different language mode that has different rules. We don't accept static_assert if -fms-compatibility is disabled. I don't think conformance to ISO C has any bearing here, because you're not compiling the input in ISO C mode.

So the logic could be: only diagnose use of the static_assert (keyword) in C mode if assert is not defined?

Interesting, I'd not considered that. I think that could work well enough.

This approach wouldn't work when compiling preprocessed output, though perhaps we could fix that too, by making the preprocessor implicitly inject a #define static_assert _Static_assert if it sees a #define of assert in MS compatibility mode. We'd need to keep the static_assert keyword around for compatibility with code that uses it without a suitable #include, but we could then warn on it unconditionally, as this patch does.

We can't reasonably tell people to include <assert.h> (as this patch does) because that doesn't work.

But it does (as far as the user is concerned)?

As far as the user is concerned, if their source file already includes <assert.h> and they get a warning that says "you need to include <assert.h>", they would reasonably conclude that that's a compiler bug. I think we need to avoid that possibility.

[aaron.ballman]

Updated based on review feedback.

[rsmith]

@rnk / @thakis Can you take a look at this and see if you're happy with this "defining assert implicitly defines static_assert" approach?

clang/include/clang/Basic/DiagnosticParseKinds.td

427–434

Perhaps:

def ext_ms_static_assert : ExtWarn<
  "use of 'static_assert' without inclusion of <assert.h> is a Microsoft extension">,
  "<assert.h>">, InGroup<MicrosoftStaticAssert>;

I think MicrosoftStaticAssert should also be a subgroup of the Microsoft (-Wmicrosoft) warning group.

clang/lib/Lex/PPDirectives.cpp

2884 ↗

(On Diff #324707)

Do we need to give the expansion token a source location? What do diagnostics pointing at this token look like?

clang/lib/Parse/ParseDeclCXX.cpp

882

I don't think we need the isMacroDefined check here; we will have a static_assert macro in that case, so we won't see a kw_static_assert token. Checking for assert being defined means we won't warn on this, and we should:

#include <assert.h>
#undef static_assert
static_assert(1, "");

[aaron.ballman]

Updating based on review feedback.

clang/lib/Lex/PPDirectives.cpp

2884 ↗

(On Diff #324707)

Given:

#define assert

// This is a comment
int i = 0;
static_assert(0, "test");

I get:

F:\llvm-project>llvm\out\build\x64-Debug\bin\clang.exe -fsyntax-only "F:\Aaron B
allman\Desktop\test.c"
F:\Aaron Ballman\Desktop\test.c:5:1: error: static_assert failed "test"
static_assert(0, "test");
^             ~
F:\Aaron Ballman\Desktop\test.c:5:1: note: expanded from macro 'static_assert'
static_assert(0, "test");
^
1 error generated.

which is a bit funky due to the note. However, I'm not certain what source location to give it. If I give it the location of the assert identifier, then I get output like:

F:\llvm-project>llvm\out\build\x64-Debug\bin\clang.exe -fsyntax-only "F:\Aaron B
allman\Desktop\test.c"
F:\Aaron Ballman\Desktop\test.c:5:1: error: static_assert failed "test"
static_assert(0, "test");
^             ~
F:\Aaron Ballman\Desktop\test.c:1:9: note: expanded from macro 'static_assert'
#define assert
        ^
1 error generated.

which seems even more mysterious.

[aaron.ballman]

Rebasing on ToT.

[aaron.ballman]

Ping.

[rnk]

I guess my only concern is, what happens if MSVC fixes assert.h? Do we need to make the implicit #define static_assert _Static_assert conditional on the absence of any static_assert definition?

[aaron.ballman]

Address review comments from @rnk

Only define the static_assert macro if one is not already defined and test the expected behavior.

[rnk]

lgtm, but please make sure that Richard is happy

[rsmith]

Looks like test/FixIt/fixit-static-assert.cpp is failing in Phabricator's pre-merge checks: B91556. Please take a look at that before landing this; I think there's a decent chance that it's indicative of a real problem.

[aaron.ballman]

In D95396#2598932, @rsmith wrote:

Looks like test/FixIt/fixit-static-assert.cpp is failing in Phabricator's pre-merge checks: B91556. Please take a look at that before landing this; I think there's a decent chance that it's indicative of a real problem.

The issue was that I was using Extension instead of ExtWarn for the 'static_assert' with no message warnings -- I changed it to use ExtWarn and fixed up the new tests. If you prefer I use Extension instead, I'm happy to do that and change the fixit test instead.

I've commit in 8da090381d567d0ec555840f6b2a651d2997e4b3, thank you for the reviews!

clang/include/clang/Basic/DiagnosticGroups.td

 
 def InvalidIOSDeploymentTarget : DiagGroup<"invalid-ios-deployment-target">;
 
 def CXX17CompatMangling : DiagGroup<"c++17-compat-mangling">;
 def : DiagGroup<"c++1z-compat-mangling", [CXX17CompatMangling]>;
 // Name of this warning in GCC.
 def NoexceptType : DiagGroup<"noexcept-type", [CXX17CompatMangling]>;
 
+// Warnings for C2x code which is not compatible with prior C standards.
+def CPre2xCompat : DiagGroup<"c89-c99-c11-c17-compat">;
+def CPre2xCompatPedantic : DiagGroup<"c89-c99-c11-c17-compat-pedantic",
+                                     [CPre2xCompat]>;
+
 // Warnings for C++1y code which is not compatible with prior C++ standards.
 def CXXPre14Compat : DiagGroup<"c++98-c++11-compat">;
 def CXXPre14CompatPedantic : DiagGroup<"c++98-c++11-compat-pedantic",
                                        [CXXPre14Compat,
                                         CXXPre14CompatBinaryLiteral]>;
 def CXXPre17Compat : DiagGroup<"c++98-c++11-c++14-compat">;
 def CXXPre17CompatPedantic : DiagGroup<"c++98-c++11-c++14-compat-pedantic",
                                        [CXXPre17Compat]>;

clang/include/clang/Basic/DiagnosticParseKinds.td

def err_unexpected_colon_in_nested_name_spec : Error<
   "unexpected ':' in nested name specifier; did you mean '::'?">;
 def err_unexpected_token_in_nested_name_spec : Error<
   "'%0' cannot be a part of nested name specifier; did you mean ':'?">;
 def err_bool_redeclaration : Error<
   "redeclaration of C++ built-in type 'bool'">;
 def warn_cxx98_compat_static_assert : Warning<
   "static_assert declarations are incompatible with C++98">,
   InGroup<CXX98Compat>, DefaultIgnore;
-def ext_static_assert_no_message : ExtWarn<
-  "static_assert with no message is a C++17 extension">, InGroup<CXX17>;
+def warn_cxx_static_assert_in_c : Warning<
+  "'static_assert' is spelled '_Static_assert' in C; consider including "
+  "<assert.h>">, InGroup<DiagGroup<"static-assert-extension">>;
+def ext_cxx_static_assert_no_message : Extension<
+  "'static_assert' with no message is a C++17 extension">, InGroup<CXX17>;
+def ext_c_static_assert_no_message : Extension<
+  "'_Static_assert' with no message is a C2x extension">, InGroup<C2x>;
 def warn_cxx14_compat_static_assert_no_message : Warning<
-  "static_assert with no message is incompatible with C++ standards before "
+  "'static_assert' with no message is incompatible with C++ standards before "
   "C++17">,
   DefaultIgnore, InGroup<CXXPre17Compat>;
+def warn_c17_compat_static_assert_no_message : Warning<
+  "'_Static_assert' with no message is incompatible with C standards before "
+  "C2x">,
+  DefaultIgnore, InGroup<CPre2xCompat>;
 def err_function_definition_not_allowed : Error<
   "function definition is not allowed here">;
 def err_expected_end_of_enumerator : Error<
   "expected '= constant-expression' or end of enumerator definition">;
 def err_expected_coloncolon_after_super : Error<
   "expected '::' after '__super'">;
 
 def ext_decomp_decl_empty : ExtWarn<

clang/lib/Parse/ParseDeclCXX.cpp

 ///           _Static_assert ( constant-expression  ,  string-literal  ) ;
 ///
 Decl *Parser::ParseStaticAssertDeclaration(SourceLocation &DeclEnd){
   assert(Tok.isOneOf(tok::kw_static_assert, tok::kw__Static_assert) &&
          "Not a static_assert declaration");
 
   if (Tok.is(tok::kw__Static_assert) && !getLangOpts().C11)
     Diag(Tok, diag::ext_c11_feature) << Tok.getName();
-  if (Tok.is(tok::kw_static_assert))
+  if (Tok.is(tok::kw_static_assert)) {
+    if (!getLangOpts().CPlusPlus)
+      Diag(Tok, diag::warn_cxx_static_assert_in_c)
+          << FixItHint::CreateReplacement(Tok.getLocation(), "_Static_assert");
+    else
       Diag(Tok, diag::warn_cxx98_compat_static_assert);
+  }
 
   SourceLocation StaticAssertLoc = ConsumeToken();
 
   BalancedDelimiterTracker T(*this, tok::l_paren);
   if (T.consumeOpen()) {
     Diag(Tok, diag::err_expected) << tok::l_paren;
     SkipMalformedDecl();
     return nullptr;
   }
 
   EnterExpressionEvaluationContext ConstantEvaluated(
       Actions, Sema::ExpressionEvaluationContext::ConstantEvaluated);
   ExprResult AssertExpr(ParseConstantExpressionInExprEvalContext());
   if (AssertExpr.isInvalid()) {
     SkipMalformedDecl();
     return nullptr;
   }
 
   ExprResult AssertMessage;
   if (Tok.is(tok::r_paren)) {
-    Diag(Tok, getLangOpts().CPlusPlus17
-                  ? diag::warn_cxx14_compat_static_assert_no_message
-                  : diag::ext_static_assert_no_message)
-      << (getLangOpts().CPlusPlus17
+    unsigned DiagVal;
+    if (getLangOpts().CPlusPlus17)
+      DiagVal = diag::warn_cxx14_compat_static_assert_no_message;
+    else if (getLangOpts().CPlusPlus)
+      DiagVal = diag::ext_cxx_static_assert_no_message;
+    else if (getLangOpts().C2x)
+      DiagVal = diag::warn_c17_compat_static_assert_no_message;
+    else
+      DiagVal = diag::ext_c_static_assert_no_message;
+    Diag(Tok, DiagVal) << ((getLangOpts().CPlusPlus17 || getLangOpts().C2x)
                                ? FixItHint()
-              : FixItHint::CreateInsertion(Tok.getLocation(), ", \"\""));
+                               : FixItHint::CreateInsertion(Tok.getLocation(),
+                                                            ", \"\""));
   } else {
     if (ExpectAndConsume(tok::comma)) {
       SkipUntil(tok::semi);
       return nullptr;
     }
 
     if (!isTokenStringLiteral()) {
       Diag(Tok, diag::err_expected_string_literal)

clang/test/Parser/static_assert.c

+// RUN: %clang_cc1 -fsyntax-only -std=c2x -DTEST_SPELLING -verify=c2x %s
+// RUN: %clang_cc1 -fsyntax-only -std=c2x -DTEST_SPELLING -fms-compatibility -verify=c2x-ms %s
+// RUN: %clang_cc1 -fsyntax-only -std=c2x -Wc89-c99-c11-c17-compat -verify=c2x-compat %s
+// RUN: %clang_cc1 -fsyntax-only -std=c99 -verify=c99 %s
+// RUN: %clang_cc1 -fsyntax-only -std=c99 -pedantic -verify=c99-pedantic %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++17 -verify=cxx17 -x c++ %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++17 -pedantic -verify=cxx17-pedantic -x c++ %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++98 -verify=cxx98 -x c++ %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++98 -pedantic -verify=cxx98-pedantic -x c++ %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++17 -Wc++98-c++11-c++14-compat -verify=cxx17-compat -x c++ %s
+
+// c99-no-diagnostics
+// cxx17-no-diagnostics
+// cxx98-no-diagnostics
+
+#ifdef TEST_SPELLING
+// Only test the C++ spelling in C mode in some of the tests, to reduce the
+// amount of diagnostics to have to check. This spelling is allowed in MS-
+// compatibility mode in C, but otherwise produces errors.
+static_assert(1, ""); // c2x-error {{expected parameter declarator}} \
+                      // c2x-error {{expected ')'}} \
+                      // c2x-note {{to match this '('}} \
+                      // c2x-warning {{type specifier missing, defaults to 'int'}} \
+                      // c2x-ms-warning {{'static_assert' is spelled '_Static_assert' in C; consider including <assert.h>}}
+#endif
+
+// We support _Static_assert as an extension in older C modes and in all C++
+// modes, but only as a pedantic warning.
+_Static_assert(1, ""); // c99-pedantic-warning {{'_Static_assert' is a C11 extension}} \
+                       // cxx17-pedantic-warning {{'_Static_assert' is a C11 extension}} \
+                       // cxx98-pedantic-warning {{'_Static_assert' is a C11 extension}}
+
+// _Static_assert without a message has more complex diagnostic logic:
+//   * In C++17 or C2x mode, it's supported by default.
+//   * But there is a special compat warning flag to warn about portability to
+//     older standards.
+//   * In older standard pedantic modes, warn about supporting without a
+//     message as an extension.
+_Static_assert(1); // c99-pedantic-warning {{'_Static_assert' with no message is a C2x extension}} \
+                   // cxx98-pedantic-warning {{'static_assert' with no message is a C++17 extension}} \
+                   // c2x-compat-warning {{'_Static_assert' with no message is incompatible with C standards before C2x}} \
+                   // cxx17-compat-warning {{'static_assert' with no message is incompatible with C++ standards before C++17}} \
+                   // c99-pedantic-warning {{'_Static_assert' is a C11 extension}} \
+                   // cxx17-pedantic-warning {{'_Static_assert' is a C11 extension}} \
+                   // cxx98-pedantic-warning {{'_Static_assert' is a C11 extension}}

clang/test/SemaCXX/static-assert.cpp

-// RUN: %clang_cc1 -fsyntax-only -verify %s -std=c++11 -triple=x86_64-linux-gnu
+// RUN: %clang_cc1 -fsyntax-only -verify %s -std=c++11 -pedantic -triple=x86_64-linux-gnu
 
 int f(); // expected-note {{declared here}}
 
 static_assert(f(), "f"); // expected-error {{static_assert expression is not an integral constant expression}} expected-note {{non-constexpr function 'f' cannot be used in a constant expression}}
 static_assert(true, "true is not false");
 static_assert(false, "false is false"); // expected-error {{static_assert failed "false is false"}}
 
 void g() {