Going forward with [[clang::suppress]]

I’m trying to land Valeriy’s old patch: ⚙ D93110 [analyzer] Implement fine-grained suppressions via attributes so I’m curious what do you all think of it! And of potential future directions. The patch introduces an attribute [[clang::suppress(...)]] to suppress arbitrary static analysis false positives.

It’s like #ifndef __clang_analyzer__ but not ugly. It’s like // NOLINT but first-class citizen. It’s like [[gsl::suppress]] but universal. It’s like Rust #[allow(...)] but in C and C++.

Because it’s a statement attribute, you can put it on arbitrary individual statements, or on a compound statement, or on a control flow construct, or even on a NullStmt if you like. It also respects uniquing locations, so you can suppress leak warnings by annotating either the leak point or the allocation site.

The patch adds support for the attribute in the static analyzer but there’s no fundamental reason we can’t teach clang-tidy to use it; and I’ll probably attempt to do that in a follow-up patch. Initially we also considered teaching it about clang frontend warnings, but I no longer think it’s a good idea; the problem is that it won’t be able to suppress warnings that are emitted before the attribute object is constructed and the attributed statement is fully parsed, which is just inconsistent user experience. So with frontend warnings it’s probably better to stick to #pragma clang diagnostic.

Conversely, it may be a good idea to introduce this attribute “in pragma form” in order to suppress warnings in ranges that span multiple functions. (I don’t think #pragma clang attribute is going to give us that for free; it probably doesn’t work well with statement attributes, so this’ll need some manual work.)

Related to that, currently the attribute can’t be placed on functions or classes, because back when the patch was written we hadn’t made up our mind whether it should suppress warnings in things outside of its immediate scope, such as redeclarations of the function, or out-of-line methods of the class. These days we’re thinking that it should stick to the lexical scope, potentially with an alternative spelling to auto-propagate more aggressively. This feature is quite desirable for AST-based checks that may emit warnings against non-statements, esp. considering the clang-tidy use case where such checkers are much more popular.

Finally, the elephant in the room is checker names. For now the attribute doesn’t allow you to specify which classes of warnings are suppressed. I just don’t like our static analyzer checker names; they aren’t great, separations into packages is arbitrary and a lot of checkers are misplaced, checkers that emit warnings from inside core should have never been placed there. We want to fix checker names, but this means that we can’t promise that they’ll be stable, so we don’t want people to use them.

In the past I have floated an idea to replace checker names with a system of “hashtags” (“keywords”) that each individual diagnostic is described with, so that diagnostics could be turned on or off based on these hashtags. Multiple hashtags can be attached to the same diagnostic, which means we have little incentive to break them.

While this won’t happen overnight, I think for the purposes of the [[clang::suppress]] mechanism we could make the first step in this direction: introduce a notion of the “warning hashtag” and make the suppression attribute react to the hashtag attached to the warning. We could automatically promote existing checker names as well as their respective package names to hashtags (both in the static analyzer, eg. [#unix #unix.Malloc], and in clang-tidy, eg. [#bugprone #bugprone-assert-side-effect]) so that they could be used for suppression if the user is already familiar with them, but in the static analyzer we don’t want people to rely on them being stable, so in any case we’ll continue to incrementally introduce more and more “better” hashtags to cover specific checkers.

In the specification of gsl::suppress we have the following form:

[[gsl::suppress(tag, justification: "message")]]

The justification field can be tremendously useful. In its simplest form, it is documentation to describe why the author thought the warning was a false positive. But more importantly, static analysis tools could potentially include suppressed warnings in the output like a Sarif file along with the suppression justification. E.g., some organizations have requirements that certain checks that have virtually no false positives cannot be suppressed without an approval from a security researcher. In this workflow, the justification field can contain the approval number, and some CI jobs could check if all of the suppressed warnings from a certain check have correct approval numbers.

I think whatever we settle on, we might want to consider having a similar optional justification field. What do you think?

Could you elaborate on this? In what way would [[clang::suppress(...)]] be more universal? In case the shortcomings of [[gsl::suppress]] is specific to Clang’s implementation, maybe we could look into fixing those issues instead?

For example, in MSVC, [[gsl::suppress]] is both a statement attribute and a declaration attribute. I think suppressing warnings is already a really fragmented experience across the different toolchains, so I’d like to know more about the reasons to introduce one more way to suppress warnings before we contribute to the fragmentation.

These are really good points. I don’t have a strong feeling as long as it is documented.

Huge +1 on from me. I share the same vision and genuinely hope something like this will be picked up by multiple toolchains. In an optimal scenario, I’d even hope to be able to assign tags to compiler warnings (although that could make things more confusing if compiler warning suppression would not work with the proposed attributes).

I just wanted to say I read this, and makes sense to me.
I don’t have a strong feeling about it given that Sonar is just another vendor, having something else for suppressing issues, so it’s not going to interfere with us. :smiley:

I think this is great to have. I don’t think we have “keyword arguments” for attributes in clang, and it probably won’t work without them. Though I’d definitely love to have “keyword arguments” for a few other reasons as well. So I think it’s a very good future direction.

Also the justification argument could be simulated if a macro is used for the attribute:

#define SUPPRESS(justification, checkers...) [[clang::suppress(checkers...)]]

I was only trying to say that users no longer need to know anything about GSL in order to use the attribute. Instead of something that looks ecosystem-specific, they get something that “fits in” anywhere.

Technically they aren’t even different attributes under the hood, just different spellings of the same attribute. We think gsl::suppress is the step in the right direction, we just want to give it a better name and actually implement support for it in the tools.

The ultimate goal is to have something that doesn’t occupy 3-4 lines of code like #ifndef __clang_analyzer__ or #pragma clang diagnostic. And ideally also not as informal and ephemeral as // NOLINT comments.

@erichkeane @AaronBallman I’m curious if you have any blocking concerns on this! Otherwise I’ll try to land the phabricator patch this week and continue to improve it on github if that’s ok.

Ah, I see. That makes sense. Should have looked at the patch first :smiley: Sorry for the misunderstanding.

Aaron is out of contact until the end of the year, we should perhaps hold off and wait for his feedback.

I DID do a quick review of the CFE bits though, and as long as someone in the SA has done a sufficient review, I think the patch linked is fine to push.

Aha I’ll try to land it then! Aaron did approve it earlier, and I’ll do my best to course-correct if a new perspective comes up.

The SA part looks good to me (as someone who didn’t write the code^^), I have a few things I want to change (mostly to make it reusable in clang-tidy) but I’ll make follow-up patches on github about this, as it’s going to be backwards-compatible.

@dkrupp, would your team find this suppression attribute useful? I vaguely remember that you supported it but I couldn’t immediately find the discussion. And your team is one of the major “shareholders” of the static analyzer so I really care about your opinion!