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

[Kristóf Umann via cfe-dev]

On Wed, 4 Sep 2019 at 01:17, Artem Dergachev <noqnoqneo at gmail.com> wrote:

> Ugh, i just realized this.
>
> Clang-Tidy doesn't really need BugReporter; all it needs is
> PathDiagnosticConsumer. (the word "Path" is rudimentary)
>

Maybe its time to rename it! :)


> It should be perfectly comfortable to build PathDiagnostics by hand - it
> maps nicely to how you already build your warnings and notes. There's no
> need to force you to do the job of packing your notes into an auxiliary
> BugReport object and then use BugReporter to unwrap it back into a
> PathDiagnostic. It makes a huge lot of sense for path-sensitive reports
> to de-duplicate the work of turning a report into a PathDiagnostic, but
> it doesn't really mean much for path-insensitive reports.


A bit of an unrelated question, does clang-tidy house CFG-based checks, or
only AST based ones? For our upcoming reaching definitions analysis, we may
even want a CFGBugReport class.


> Change of plans, i guess.


In any case, your changes to BugReport gave much needed clarity to the
code! :)


> On 8/14/19 12:23 AM, Dmitri Gribenko 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.
> >
> >> 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.
> >
> > 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.
> >
> > Dmitri
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20190904/e3c7cb3f/attachment.html>