[cfe-dev] RFC: Pumping Clang-Tidy warnings through the Static Analyzer's BugReporter.

Wed Aug 14 03:49:36 PDT 2019

I completely agree with what Dmitri has written. A couple of additional
comments from me below.

On Wed, Aug 14, 2019 at 9:24 AM Dmitri Gribenko <gribozavr at gmail.com> wrote:

> Hi Artem,
>
> On Wed, Aug 14, 2019 at 5:00 AM Artem Dergachev <noqnoqneo at gmail.com>
> wrote:
> >
> > As i've been vaguely hinting on EuroLLVM, i plan to invest some time in
> > decreasing the gap between Clang-Tidy users and Static Analyzer users
> > and make sure it's always possible for our users to take the best of
> > both worlds. In particular, i'd like to make Clang-Tidy easily
> > integratable into all UIs that already integrate the Static Analyzer.
>
> I like the idea of integrating the two tools more closely. From the
> user's point of view, I think, ClangTidy and Clang Static Analyzer are
> doing more or less the same thing, but somehow have completely
> different workflows and integrations.

There are some distinct features (like FixIt hints or path-sensitive
diagnostics) and use cases (e.g. the automated refactorings), that can be
better or worse supported by different integrations and workflows. It
should still be possible to take the best of two worlds and unify the user
experience.

> > Ideally i'd love to do the same to Clang-Tidy that Clang-Tidy does to
> > us: make the Static Analyzer load it as a library and expose Clang-Tidy
> > checks as its own, maybe in a separate package. This is harder to do
> > though, because Clang-Tidy lives in a separate repo and also it's a hard
> > sell to build it into the Clang binary. I'm open to suggestions here: we
> > can keep such integration under an off-by-default CMake flag (which
> > requires enabling compilation of clang-tools-extra) or we may even use
> > it as a run-time plugin (i mean, clang plugins are super wonky, but this
> > time it's actually fairly easy to avoid version conflicts, as they all
> > get compiled from the same sources simultaneously).
>
> I would like to suggest something different: move Clang Static
> Analyzer to clang-tools-extra. Build it either as a separate binary or
> compile it into the clang-tidy binary. Then let `clang -analyze`
> delegate to that binary for backwards compatibility.
>

This would be good for a few more reasons:
1. Clear separation of functionality: clang's main purpose is to compile
code rather than perform static analysis. Clang and static analyzer need
partially overlapping, but different sets of options.
2. Clang binary size growth is a concern for maintainers of toolchains.
Static analyzer (especially with AST matchers-based checkers) is
contributing a lot to that growth. Building clang
without CLANG_ENABLE_STATIC_ANALYZER somewhat solves this part of the
problem, but then you need another clang binary (possibly in a separate
package) to support static analyzer - I believe, that's clearly suboptimal.
Thus, having a completely different binary for the static analyzer (or
teach clang-tidy do everything static analyzer needs to do - including all
output formats and options) would be a much better solution.

> Clang Static Analyzer is being developed in the clang repository for
> historical reasons -- we didn't have clang-tools-extra at the time the
> project was started. The "non-core" nature of Clang Static Analyzer is
> also validated by the fact that it is an optional part of Clang -- we
> have a CMake flag `CLANG_ENABLE_STATIC_ANALYZER`.
>
> Long-term, my preference would be to have all static analysis tooling
> (Clang Static Analyzer, ClangTidy, any prospective dataflow-based
> analysis) to have a consistent UI -- including invocation, integration
> with build systems, configuration strategy etc. While the
> implementation of these tools is very different, I think the audience
> is largely the same.

> > But regardless of how far do i end up going with such integration, first
> > thing first: i'll anyway need to teach Clang-Tidy how to route its
> > diagnostics through our diagnostic engine. This is also the only thing
> > that's absolutely necessary; the rest can always be hacked up on the UI
> > side.
> >
> > It's a great question why didn't we extend Clang's DiagnosticEngine to
> > begin with, but instead made our own facility. I don't know the answer
> > to that; this happened way before i even learned what an AST is :) We
> > generally needed a much more sophisticated diagnostic engine because our
> > reports are much more complicated. While we could try to unify these
> > diagnostic engines completely, i don't think it's worth it.
> >
> > So i think it's more feasible to make Clang-Tidy's diag() method return
> > a proxy object that mimics the DiagnosticBuilder interface but may also
> > pump the diagnostics to an instance of the Static Analyzer's
> > BugReporter. I hope this would avoid API breakages for clang-tidy checks.
>
> I'm very concerned about introducing lookalike APIs that don't behave
> exactly the same way as original ones. In LLVM, we are generally not
> concerned with backwards compatibility for C++ APIs. Instead, we
> should be looking to keep our code maintainable in the long run.
>
> With that in mind, my suggestion is to refactor ClangTidy to use
> BugReporter instead of DiagnosticBuilder.

No concerns here, as long as we ensure that BugReporter has support for
FixItHints and other stuff DiagnosticBuilder provides.

> Dmitri
>
> --
> main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if
> (j){printf("%d\n",i);}}} /*Dmitri Gribenko <gribozavr at gmail.com>*/
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20190814/3e078e76/attachment.html>