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

[Aaron Ballman via cfe-dev]

On Wed, Aug 14, 2019 at 8:30 AM Dmitri Gribenko <gribozavr at gmail.com> wrote:
>
> 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.

I agree, my point was that a stand-alone tool doesn't work for all
users either. For my needs, moving the static analyzer into clang-tidy
makes things harder, not easier, whereas moving clang-tidy into the
compiler proper makes things easier rather than harder.

> 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).

This is why analysis is controlled by a separate flag. I see no reason
why clang-tidy checks would not behave similarly.

> 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.

I don't see how that follows; -fsyntax-only exists to support that
sort of use case.

~Aaron

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