[cfe-dev] [analyzer][RFC] Attribute(s) to enhance/configure the analysis

[Jan Korous via cfe-dev]

Hi all,

I'm sorry I'm a little late to the party!

I have couple thoughts first and a proposal at the end.

> On Oct 20, 2020, at 1:06 PM, Gábor Horváth via cfe-dev <cfe-dev at lists.llvm.org> wrote:

> I think an ideal suppress annotation would:
> * be applicable to a wide range of scopes, not just function scope
> * have clear documentation indicating this is the last resort and refer to some guidelines how to suppress false positives by improving the code
> * come with an easy mechanism to check whether the annotation makes any difference, so we can easily get rid of redundant ones
> * come with an easy mechanism to check how many issues are suppressed by the same annotation (or even expect the user to specify this number as an argument and warn if that does not match the reality?)


We must consider projects that need to support multiple versions of clang and by proxy multiple versions of Static Analyzer. Since there can be both false positives and false negatives in Analyzer we need to have a good story for situations when an attribute doesn't make sense for a particular Analyzer version (for example older one) but the maintainers still want to keep it in source. Maybe we could produce just lower severity diagnostics than warning?
(In case someone asks - I would strongly prefer to not introduce versioning of these attributes.)

> On Oct 20, 2020, at 11:24 AM, Valeriy Savchenko via cfe-dev <cfe-dev at lists.llvm.org> wrote:
> However, the hardest choice here is the string argument for the attribute.  Again, it would be perfect if we can avoid weird identifiers like “java.com <http://java.com/>.warnings.alpha.whatever.2020”, it would be good if that string will make it clear for people who don’t use the analyzer what’s going on here.
> Checker names that we use in command line are a bit too verbose, error messages are prone to changes, bug categories are too broad and abstract (the intention is pretty vague if you suppress “Logic error”).  
> 
> My suggestion would be using so-called “Bug types” (we already show them in the summary table in index.html).  Each diagnostic has one, it’s unique and more-or-less descriptive.  If we revise all the existing bug types and try to make them concise and clear, I think it can work!

Just to make sure we're on the same page - I'll use one of the WebKit checkers that I need to have a suppression for as an example:

checker name:
	webkit.UncountedLambdaCapturesChecker
error messages (depend on the context):
	warning: Captured raw-pointer 'ref_countable' to uncounted type is unsafe [webkit.UncountedLambdaCapturesChecker]
	warning: Implicitly captured raw-pointer 'ref_countable' to uncounted type is unsafe [webkit.UncountedLambdaCapturesChecker]
bug category:
	WebKit coding guidelines
bug type:
	Uncounted call argument for a raw pointer/reference parameter

Assuming I got it right from all the above I have a clear preference which is the checker name. In a way it would be analogous to pragmas for cc1 diagnostics suppression - the same identifier is used in CLI and in the source for suppression.
My second choice would be to introduce another string identifier as I have a hard time imagining any other of the existing strings being introduce to codebases.

> On Oct 20, 2020, at 1:11 PM, Valeriy Savchenko via cfe-dev <cfe-dev at lists.llvm.org> wrote:
> 
> I just want to add something that I think was not very clear from my previous email.  The existing “suppress” attribute is a statement attribute, which gives it a very fine level of granularity.

I have couple use-cases for declaration attributes (like ParmVarDecl, FiledDecl, VarDecl) but I assume that no matter what mechanism we devise it should be applicable for declaration attributes too.
One thing I feel we need is "kind safety". While it's trivial for single-purpose attribute to define what kinds of AST nodes is it applicable to it becomes a bit more work with different arguments of a generic attribute.

> On Oct 20, 2020, at 1:12 PM, Gábor Horváth via cfe-dev <cfe-dev at lists.llvm.org> wrote:
> Sorry for the duplicate emails, I just realized that there is one more dimension of suppression mechanisms. There are separate tools such as CodeChecker that already support suppressing individual reports (either using comments or bug hashes). So adding an annotation could make it unclear who is responsible for the suppression and which feature should the user use.

I assume that the status quo is that there's no common standard for in-source suppression for bug-finding tools. That means that arguably we won't make it significantly worse by adding n+1.
Hypothetically if we design the annotation system well we might convince other projects to use it. And if I had to pick one tool to take into the account I would maybe think of GCC analyzer?

https://developers.redhat.com/blog/2020/03/26/static-analysis-in-gcc-10/

One thing to consider here is that if we want to have some cross-tool story we probably shouldn't use any vendor-specific prefix like clang::suppress or csa::suppress.

> On Oct 21, 2020, at 11:57 AM, Aaron Ballman via cfe-dev <cfe-dev at lists.llvm.org> wrote:
> 
> Thank you for exploring the options in this space -- I think having a
> uniform way for users to suppress diagnostics using attributes is a
> really interesting idea. I have personally never really liked using
> pragmas to do this work because that approach is usually quite verbose
> when applied to a single line of code.

IIUC we have use-cases for suppression of "diagnostics produced by Analyzer" only. While attribute-based suppression of cc1 diagnostics sounds interesting I'm worried that the necessary broad discussion would effectively halt this.
Would you mind if we keep the focus on Analyzer for now or do you believe we'd not converge to the right design unless we consider cc1 too?

Re: "stable identifiers". I feel that no matter what identifiers we decide to use we still might want some escape hatch in case we end up having to rename a "stable identifier". 
I am not aware of any solution for such situation with pragmas.
https://clang.llvm.org/docs/UsersManual.html#controlling-diagnostics-via-pragmas <https://clang.llvm.org/docs/UsersManual.html#controlling-diagnostics-via-pragmas>

But I am thinking that basically all we need is a notion of identifier alias - if we can have two names for an attribute we're fine.
Imagine we add a warning identifier "foo" to Analyzer in clang-12 and add an alias "bar" for it in clang-13. Projects using the "bar" identifier are fine and any project that is ok with minimal supported version being clang-13 can change the spelling in their sources to "bar".

Overall I suggest we:
- Adopt the suppress attribute Valeriy mentioned and extend it to declarations.
- Don't use prefix like clang:: or csa:: to make it an obvious interface for suppression to be used by other tools too.
- MVP can support just a single string identifier, later extend to comma-separated list of string identifiers.
- Use checker name as an identifier.


Cheers,

Jan

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20201022/0da4618d/attachment.html>