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

[Dmitri Gribenko via cfe-dev]

On Wed, Aug 14, 2019 at 2:16 PM Aaron Ballman <aaron at aaronballman.com> wrote:
>
> On Wed, Aug 14, 2019 at 3:24 AM Dmitri Gribenko via cfe-dev
> <cfe-dev at lists.llvm.org> 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.
>
> Strong +1 from me!
>
> > > 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.
>
> I am not keen on this approach. A stand-alone tool is harder for users
> to integrate into their workflows compared to the compiler itself,
> which is already (generally) a part of their existing workflow. It's
> far easier for a user to add a flag like -tidy to an existing clang
> execution than it is to insert clang-tidy into a complex build system.
> Personally, I would much rather see libClangTidy integrated into Clang
> and exposed via a driver flag, similar to how the static analyzer
> already works.

Integrating by adding a flag to the build is only one possibility, and
it does not work for all users. Not everyone is willing to pay the
analysis slowdown during compilation (otherwise we would just expose
static analyzer and clang-tidy as -W flags in the compiler as the only
integration option). Running a separate build just to analyze forces
the compiler to run code generation and produce object files and
binaries, which are not necessary for static analysis.

Dmitri

> > 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`.
>
> FWIW, I've never liked the fact that clang-tidy is a stand-alone tool
> and have always considered that a deficiency that has hindered
> adoption. For instance, running clang-tidy required separate effort to
> get it up on godbolt, whereas --analyze has worked there since the
> first time I tried it.
>
> > 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.
>
> I think this is a good goal.
>
> ~Aaron
>
> > > 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.
> >
> > 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>*/
> > _______________________________________________
> > cfe-dev mailing list
> > cfe-dev at lists.llvm.org
> > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev



-- 
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>*/