Open sourcing a batch of checkers

Hello everyone,

I am an intern at a company where we developed several checkers based on the clang infrastructure. We have about 30 checkers that are either C++ core language or STL related. We would like to contribute some of these checkers (together with the documentation and the test cases). The ones that are well tested and have good false positive rates.

Most of the checkers were originally developed find certain design rule violations. One example of a design rule at this company is: „The emptiness of a container should be checked using the empty method instead of the size method. It is not guaranteed that size is a constant-time function, and it is generally more efficient and also shows clearer intent to use empty. Furthermore some containers may implement the empty method but not implement the size method. Using empty whenever possible makes it easier to switch to another container in the future.” We realized that, maybe such checkers could be useful for the clang community as well.

Our tool right now is a clang plugin that runs custom frontend actions on the analyzed source code. The reason is that, some of the checkers are only consist of ASTMatchers, some of them using RecursiveASTVisitors, some of them are clang Static Analyzer checkers. We are wondering what would be the desired way to contribute those checkers back. Should we focus on migrating them to clang-tidy? Should we focus on transforming them to Static Analyzer checkers?

Thanks,

Gábor Horváth

+Manuel

clang-tidy seems like a natural place for things that find “design rule violations”.

– Sean Silva

This sounds very interesting. For a start it may be easier if some of the
source code and tests in question are available to take a look at. Is it on
github?

In general and not specific to your request for feedback - I'd say keep
patience in mind. The community tends to review code from "friends" at a
much higher priority. Don't be discouraged if you don't get an immediate
reply from one of the main contributors. Holidays, internal release
deadlines and many things could in theory contribute to delayed feedback or
eventual reviews. (OpenMP work is a good example of persistence)

Thanks and good luck

Yep, the general idea is:

  1. if it’s using ASTMatchers or PPCallbacks, make it a clang-tidy check
  2. if it’s using the CFG / being path sensitive, make it a Static Analyzer checker

Note that static analyzer checkers are integrated in clang-tidy automatically - usually style rule stuff is much easier expressed in clang-tidy though (and I don’t think static analyzer checks are integrated well with fix-its (but I’m not sure), and style violations usually have easy fix-its).

Yep, the general idea is:
1. if it's using ASTMatchers or PPCallbacks, make it a clang-tidy check
2. if it's using the CFG / being path sensitive, make it a Static Analyzer
checker

Note that static analyzer checkers are integrated in clang-tidy
automatically - usually style rule stuff is much easier expressed in
clang-tidy though (and I don't think static analyzer checks are integrated
well with fix-its (but I'm not sure), and style violations usually have
easy fix-its).

+1
If your checks don't yet suggest fixes, I'd urge you to implement fixes at
least for all checks, where it's straightforward. Style warnings are close
to useless without fix-its, because not many people like spending time to
fix manually style nits in their code.

If you have any questions re: porting the checks to clang-tidy, I'll be
happy to help.

-- Alex

Hi!

I started to investigate how Clang Tidy works, and I find it very promising. We are likely to convert our ASTMatcher/PPCallback/ASTVisitor checks to clang tidy checkers and submit patches. We will submit our Static Analyzer checkers as well. Not all of our checkers are style based, but we will add fix-its where appropriate when doing the conversions.

I have some questions about the Clang Tidy though:

  • Is it possible to report issues in plist output format? Right now we are consuming the plist output of the Static Analyzer. Once we converted our codebase to use Clang Tidy we would like to continue to parse the plist output. If it is not possible now, we also would like to develop and contribute the plist reporting to Clang Tidy (in case you find it a worthy addition).

  • As far as I can see, one can specify key value pairs as configuration options for checkers. Can these values passed to Static Analyzer checkers as well? If not, we also would like to add this capability and contribute it if you find it useful enough.

  • Is there any documentation related to the specific checkers? What we do right now: there is a markdown file next to each of our checker implementation files and we have a build option to generate a HTML documentation that contains the user documentation of each checker (when to use, guidelines to fix, code examples, limitations, rationales etc). I think it might be useful to have similar system in clang tidy. What do you think?

Thanks,

Gábor

Hi Gábor,

Hi!

I started to investigate how Clang Tidy works, and I find it very
promising. We are likely to convert our ASTMatcher/PPCallback/ASTVisitor
checks to clang tidy checkers and submit patches. We will submit our Static
Analyzer checkers as well. Not all of our checkers are style based, but we
will add fix-its where appropriate when doing the conversions.

I have some questions about the Clang Tidy though:
- Is it possible to report issues in plist output format? Right now we are
consuming the plist output of the Static Analyzer. Once we converted our
codebase to use Clang Tidy we would like to continue to parse the plist
output. If it is not possible now, we also would like to develop and
contribute the plist reporting to Clang Tidy (in case you find it a worthy
addition).

Clang-tidy checks use Clang's DiagnosticsEngine to report warnings which
limits the structure of information passed from checks to what
DiagnosticsEngine supports. This is also valid for the Static Analyzer:
clang-tidy gets its results as Clang diagnostics which probably leads to
some loss of information already. However, if you're fine with the
information that is already passed through clang-tidy (see the
ClangTidyError structure in ClangTidyDiagnosticConsumer.h), then adding
another output format should be easy.

- As far as I can see, one can specify key value pairs as configuration
options for checkers. Can these values passed to Static Analyzer checkers
as well? If not, we also would like to add this capability and contribute
it if you find it useful enough.

Currently configuration options are not passed to the Static Analyzer. If
it supports this kind of configuration, it should be easy to forward
options there.

- Is there any documentation related to the specific checkers?

Only in the class comments for each check.

What we do right now: there is a markdown file next to each of our checker
implementation files and we have a build option to generate a HTML
documentation that contains the user documentation of each checker (when to
use, guidelines to fix, code examples, limitations, rationales etc). I
think it might be useful to have similar system in clang tidy. What do you
think?

That would be awesome. I'm happy to review patches (but expect long review
times, as I'm on vacation until Jan 2).

Regards,
Alex

Hi Alex!

I have a few more questions.

I did not see any checkers in clang tidy that are not ASTMatcher or PPCallbacks based. We have a checker for local variables that could be declared in a more narrow scope. It was more natural to implement that checker using RecursiveASTVisitors. Should we add a matcher for example to match function decls and clas the visitor on each function body or should we add direct support for matchers that are consists of only Visitors but no matchers?

The report from the Static Analyzer are consumed by the AnalyzerDiagnosticConsumer which has all the information we needs. The DiagnosticBuilder however does not support a structured way to store the path sensitive information. To add plist output support we should be able to pass those information to DiagnosticBuilder and we would like to reuse as much code from the current PlistDiagnostics. Unfortunately the implementation of PlistDiagnostics is not part of the public API. Is it ok to make it public? What is the convention?

I think we will start to port the checkers to clang tidy and implement the plist reporting in january. Where do you expect the patches to be submitted? This mailing list, cfe-commits or the phabricator?

Thanks,
Gábor Horváth

Hi Alex!

I have a few more questions.

I did not see any checkers in clang tidy that are not ASTMatcher or PPCallbacks based. We have a checker for local variables that could be declared in a more narrow scope. It was more natural to implement that checker using RecursiveASTVisitors. Should we add a matcher for example to match function decls

There is already a matcher to match function decls?

and clas the visitor on each function body or should we add direct support for matchers that are consists of only Visitors but no matchers?

Generally, I’d be curious what exactly you’re trying to match that is more naturally expressed by writing down a RAV yourself.

The report from the Static Analyzer are consumed by the AnalyzerDiagnosticConsumer which has all the information we needs. The DiagnosticBuilder however does not support a structured way to store the path sensitive information.

To add plist output support we should be able to pass those information to DiagnosticBuilder and we would like to reuse as much code from the current PlistDiagnostics. Unfortunately the implementation of PlistDiagnostics is not part of the public API. Is it ok to make it public? What is the convention?

That is indeed an interesting question - I’d start a new top-level thread discussing only this part; I know that some people had interest in seeing plist based diagnostics available more generally, so I think there will be support, but the solution must be carefully designed.

I think we will start to port the checkers to clang tidy and implement the plist reporting in january. Where do you expect the patches to be submitted? This mailing list, cfe-commits or the phabricator?

cfe-commits is the mailing list to submit patches to, and phabricator is a way to submit patches to the mailing list :slight_smile:

Yep, the general idea is:

  1. if it’s using ASTMatchers or PPCallbacks, make it a clang-tidy check
  2. if it’s using the CFG / being path sensitive, make it a Static Analyzer checker

Note that static analyzer checkers are integrated in clang-tidy automatically - usually style rule stuff is much easier expressed in clang-tidy though (and I don’t think static analyzer checks are integrated well with fix-its (but I’m not sure), and style violations usually have easy fix-its).

+1

We do have several non-path sensitive checkers in the Static Analyzer (for example checkers based on RecursiveASTVisitors), so adding those to the static analyzer could also be an option.

One issue I can see here us that the clang-tidy output does not support reporting paths from the path sensitive checks. Adding plist output option to clang-tidy which supports paths reporting could be a useful enhancement.

(Please, CC me on all the patches to the Static Analyzer.)