[PATCH] D95396: Improve static_assert/_Static_assert diagnostics

Wed Feb 17 13:58:20 PST 2021

rsmith added inline comments.

================
Comment at: clang/lib/Parse/ParseDeclCXX.cpp:874-876
+    if (!getLangOpts().CPlusPlus)
+      Diag(Tok, diag::warn_cxx_static_assert_in_c)
+          << FixItHint::CreateReplacement(Tok.getLocation(), "_Static_assert");
----------------
aaron.ballman wrote:
> rsmith wrote:
> > aaron.ballman wrote:
> > > aaron.ballman wrote:
> > > > rsmith wrote:
> > > > > 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).
> > > > 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?
> > > 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?
> > Well, it's good to see that they've made progress, but it [looks like](https://godbolt.org/z/YfEhGW) 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.
> >  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.
> 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.

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

https://reviews.llvm.org/D95396