[PATCH] D93110: [analyzer] Implement fine-grained suppressions via attributes

Tue Feb 16 11:12:44 PST 2021

aaron.ballman added a comment.

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

> In D93110#2534690 <https://reviews.llvm.org/D93110#2534690>, @aaron.ballman wrote:
>
>> In D93110#2529917 <https://reviews.llvm.org/D93110#2529917>, @NoQ wrote:
>>
>>>> What I want to avoid is having a static analyzer-specific suppression attribute, and a different one for clang-tidy, and a third one for the frontend.
>>>
>>> Given that there are already multiple suppression mechanisms circulating around (clang-tidy's `// NOLINT`, frontend pragmas, static analyzer's `#ifdef __clang_analyzer__`, now also attribute), i guess a path forward would be to eventually add support for multiple mechanisms everywhere.  This may be tedious to implement but benefits may be well worth it. Consistency across tools is important when tools are used together; for instance, when clang-tidy integrates static analyzer, static analyzer automatically starts supporting `// NOLINT` through clang-tidy's diagnostic consumer magic. This would also be amazing for discoverability: users can keep using their preferred mechanism and it'd just work out of the box when they add a new tool to their arsenal.
>>
>> To be clear, I'm fine with multiple mechanisms, but not fine with multiple of the same mechanisms. e.g., I don't think it's an issue that we can suppress via the command line, pragmas, NOLINT comments, and attribute usage, but I do think it's a problem if there's one attribute used by clang-tidy and a different attribute used by the static analyzer, and a third attribute for clang itself. Using different attributes causes users to have to consider what basically amount to implementation details of their toolchain and makes it harder to do things like integrate clang-tidy and the clang static analyzer together.
>
> We can integrate these solutions together for clang-tidy and for clang static analyzer, it's not a problem at all.  I would've used the existing Suppress attribute if it was without `gsl` part.

Sorry, I failed at being clear. Let me take another stab at it.

<ideally>
I don't want multiple unrelated semantic attributes to handle this concept. We can have a single semantic attribute with multiple ways of spelling it (e.g., `[[gsl::suppress]]` and `[[clang::suppress]]`) easily enough. I want to avoid code that looks like `if (const auto *A = D->getAttr<SuppressAttr>()) { ... } else if (const auto *A = D->getAttr<SomeOtherSuppressAttr>()) { ... }` because this causes maintenance problems where someone invariably forgets the less-used suppression attribute somewhere. (I would be fine if we had a common base class used by multiple suppression semantic attributes, if that was necessary to avoid this problem. )

I also don't want multiple attribute spellings for different parts of the toolchain. e.g., a user should not have to write `[[clang::suppress(...)]]` for a frontend suppression and `[[tidy::suppress(...)]]` for a tidy suppression, and `[[csa::suppress(...)]]` for a static analyzer suppression. This leaks implementation details out to users that they're not likely to have insights into anyway and it causes problems when we shift a diagnostic around or combine tools (like the static analyzer and tidy being able to run one another's checks).

I don't think it's an issue to have multiple attribute spellings to support different coding style guidelines or to be compatible with another compiler because users will be able to reason about those annotations more easily than which part of the toolchain a diagnostic comes from. So I don't think it's an issue to have `[[gsl::suppress(...)]]` and `[[clang::suppress(...)]]` as spellings so long as we wind up with a single semantic attribute interface under the hood.
</ideally>

> As I mentioned in the summary, at the moment, we want to use it ONLY inside functions.  I can see that `SuppressAttr` is not used anywhere in the whole, but I can't be sure that it's not used somewhere externally.  So, I couldn't just reduce the scope of it.

We have the notion of attribute accessors to help with this sort of thing. It's reasonable to add an accessor to see whether the attribute was spelled with the gsl spelling, and if so, make alternative diagnostic decisions from it. e.g. https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/Attr.td#L650

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93110