<html><head><meta http-equiv="Content-Type" content="text/html; charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class=""><div class="">Hi all,</div><div class=""><br class=""></div><div class="">I'm sorry I'm a little late to the party!</div><div class=""><br class=""></div><div class="">I have couple thoughts first and a proposal at the end.</div><div class=""><br class=""><blockquote type="cite" class="">On Oct 20, 2020, at 1:06 PM, Gábor Horváth via cfe-dev <<a href="mailto:cfe-dev@lists.llvm.org" class="">cfe-dev@lists.llvm.org</a>> wrote:</blockquote></div><div class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""></div></div></blockquote><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="">I think an ideal suppress annotation would:</div><div class="">* be applicable to a wide range of scopes, not just function scope</div><div class="">* have clear documentation indicating this is the last resort and refer to some guidelines how to suppress false positives by improving the code<br class=""></div><div class="">* come with an easy mechanism to check whether the annotation makes any difference, so we can easily get rid of redundant ones</div><div class="">* 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?)</div></div></div></blockquote></div><div class=""><br class=""></div><div class=""><div class="">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?</div><div class="">(In case someone asks - I would strongly prefer to not introduce versioning of these attributes.)</div></div><div class=""><br class=""></div><div class=""><blockquote type="cite" class=""><div class="">On Oct 20, 2020, at 11:24 AM, Valeriy Savchenko via cfe-dev <<a href="mailto:cfe-dev@lists.llvm.org" class="">cfe-dev@lists.llvm.org</a>> wrote:</div><div class=""><div class="" style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;"><div class=""><div class="">However, the hardest choice here is the string argument for the attribute. Again, it would be perfect if we can avoid weird identifiers like “<a href="http://java.com/" class="">java.com</a>.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.</div><div class="">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”). </div><div class=""><br class=""></div><div class="">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!</div></div></div></div></blockquote><br class=""></div><div class="">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:</div><div class=""><br class=""></div><span class="">checker name:</span><div class=""><span class=""><span class="Apple-tab-span" style="white-space:pre"> </span>webkit.UncountedLambdaCapturesChecker<br class=""></span><span class="">error messages (depend on the context):<br class=""></span><span class=""><span class="Apple-tab-span" style="white-space:pre"> </span>warning: Captured raw-pointer 'ref_countable' to uncounted type is unsafe [webkit.UncountedLambdaCapturesChecker]<br class=""></span><span class=""><span class="Apple-tab-span" style="white-space:pre"> </span>warning: Implicitly captured raw-pointer 'ref_countable' to uncounted type is unsafe [webkit.UncountedLambdaCapturesChecker]<br class=""></span><span class="">bug category:</span></div><div class=""><span class=""><span class="Apple-tab-span" style="white-space:pre"> </span>WebKit coding guidelines<br class=""></span><span class="">bug type:</span></div><div class=""><span class=""><span class="Apple-tab-span" style="white-space:pre"> </span>Uncounted call argument for a raw pointer/reference parameter<br class=""></span><span class=""><br class="">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.</span></div><div class=""><span class="">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.<br class=""></span><span class=""><br class=""></span><div class=""><blockquote type="cite" class=""><div class="">On Oct 20, 2020, at 1:11 PM, Valeriy Savchenko via cfe-dev <<a href="mailto:cfe-dev@lists.llvm.org" class="">cfe-dev@lists.llvm.org</a>> wrote:</div><br class="Apple-interchange-newline"><div class="">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.</div></blockquote><br class=""></div><div class="">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.</div><div class="">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.</div><div class=""><br class=""></div><blockquote type="cite" class="">On Oct 20, 2020, at 1:12 PM, Gábor Horváth via cfe-dev <<a href="mailto:cfe-dev@lists.llvm.org" class="">cfe-dev@lists.llvm.org</a>> wrote:</blockquote><blockquote type="cite" class=""><div dir="ltr" class=""><div class="">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.</div></div></blockquote><div class=""><br class=""></div><div class="">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.</div><div class="">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?</div><div class=""><br class=""></div><div class=""><a href="https://developers.redhat.com/blog/2020/03/26/static-analysis-in-gcc-10/" class="">https://developers.redhat.com/blog/2020/03/26/static-analysis-in-gcc-10/</a></div><div class=""><br class=""></div><div class="">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.</div><div><br class=""><blockquote type="cite" class=""><div class="">On Oct 21, 2020, at 11:57 AM, Aaron Ballman via cfe-dev <<a href="mailto:cfe-dev@lists.llvm.org" class="">cfe-dev@lists.llvm.org</a>> wrote:</div><div class=""><div class=""><br class="">Thank you for exploring the options in this space -- I think having a<br class="">uniform way for users to suppress diagnostics using attributes is a<br class="">really interesting idea. I have personally never really liked using<br class="">pragmas to do this work because that approach is usually quite verbose<br class="">when applied to a single line of code.<br class=""></div></div></blockquote><div><br class=""></div>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.</div><div>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?</div><div><br class=""></div><div>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". </div><div>I am not aware of any solution for such situation with pragmas.</div><div><a href="https://clang.llvm.org/docs/UsersManual.html#controlling-diagnostics-via-pragmas" class="">https://clang.llvm.org/docs/UsersManual.html#controlling-diagnostics-via-pragmas</a></div><div><br class=""></div><div>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.</div><div>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".</div><div><br class=""></div><div>Overall I suggest we:</div><div>- Adopt the suppress attribute Valeriy mentioned and extend it to declarations.</div><div>- Don't use prefix like clang:: or csa:: to make it an obvious interface for suppression to be used by other tools too.</div><div>- MVP can support just a single string identifier, later extend to comma-separated list of string identifiers.</div><div>- Use checker name as an identifier.</div></div><div><br class=""></div><div><br class=""></div><div>Cheers,</div><div><br class=""></div><div>Jan</div><div><br class=""></div></body></html>