[PATCH] D93110: [analyzer] Implement a first version of suppressions via attributes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D93110#2448587 <https://reviews.llvm.org/D93110#2448587>, @vsavchenko wrote:

> Right now, I reused existing `suppress` attribute.  However I did it in a rather unconventional manner.  I allowed 0 arguments in one spelling and >1 in another, which seems odd.
> I can see a couple other possible solutions here:
>
> - Choose a "keyword" that would be used to suppress all warnings (e.g. "suppress(all)")
> - Introduce two different suppress attributes (maybe even rename existing attribute to be GSLSuppress)

I don't think it's a problem that we use the same semantic attribute but with different syntax requirements for the arguments. The semantics for GSL suppression and Clang suppression are the same so it makes sense to me to use the same semantic attribute so we don't wind up checking `isa<ThisAttr, ThatAttr>(D)` everywhere. However, if it does start to get awkward, I think splitting the attributes into two different semantic attributes is preferable to using a keyword.

One usability concern I have with adding `[[clang::suppress]]` is that users are going to want to use it to suppress all the various kinds of diagnostics and not just clang static analyzer warnings. We get away with `[[gsl::suppress]]` because that's very specific to the C++ core guidelines, but we can't really hand-wave away the problem with `[[clang::suppress]]`. Have you explored how this attribute will work with clang frontend diagnostics or clang-tidy diagnostics? (This ignores some of the even harder diagnostics like ones that percolate back up from the backend, but we should probably keep those in mind as well.)



================
Comment at: clang/include/clang/Basic/Attr.td:2366
   let Args = [VariadicStringArgument<"DiagnosticIdentifiers">];
   let Documentation = [SuppressDocs];
 }
----------------
The documentation will need to be updated for this. I have no idea if that may be a bit awkward because we're documenting two different suppression mechanisms (that do roughly the same thing).


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4567
 static void handleSuppressAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
-  if (!checkAttributeAtLeastNumArgs(S, AL, 1))
+  if (AL.isCXX11Attribute() && !checkAttributeAtLeastNumArgs(S, AL, 1))
     return;
----------------
The behavior here will be that `[[gsl::suppress]]` in C++11 mode and `[[clang::suppress]]` in C++11 mode require at least one argument, while `[[clang::supress]]` in C2x mode and `__attribute__((suppress))` in all language modes allow any number of arguments (including zero). That doesn't seem like what you want. I think you need something more like:
```
// The [[gsl::suppress]] spelling requires at least one argument, but the GNU
// and [[clang::suppress]] spellings do not require any arguments.
if (AL.hasScope() && AL.getScopeName()->isStr("gsl") &&
    !checkAttributeAtLeastNumArgs(S, AL, 1))
  return;
```


================
Comment at: clang/lib/Sema/SemaStmtAttr.cpp:56
                                 SourceRange Range) {
-  if (A.getNumArgs() < 1) {
+  if (A.isCXX11Attribute() && A.getNumArgs() < 1) {
     S.Diag(A.getLoc(), diag::err_attribute_too_few_arguments) << A << 1;
----------------
Same issue here.

(In other news, it looks like this attribute could stand to be updated to be a `DeclOrStmtAttr` once D92800 lands -- that's not your problem to solve, more just an observation.)


================
Comment at: clang/lib/Sema/SemaStmtAttr.cpp:69
     // FIXME: Warn if the rule name is unknown. This is tricky because only
-    // clang-tidy knows about available rules.
+    // clang-tidy and static analyzer know about available rules.
     DiagnosticIdentifiers.push_back(RuleName);
----------------



================
Comment at: clang/lib/StaticAnalyzer/Core/BugReporter.cpp:2885
+
+  if (It != Attrs.end()) {
+    return cast<SuppressAttr>(*It);
----------------
The coding standard has us elide braces on single-line ifs (same comment applies elsewhere).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93110