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

[Artem Dergachev via cfe-dev]

On 9/3/19 5:31 PM, Kristóf Umann wrote:
> On Wed, 4 Sep 2019 at 01:17, Artem Dergachev <noqnoqneo at gmail.com 
> <mailto: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.

For must-problems i suspect that the really good way to present them to 
the user would be to present all reports simultaneously (eg., as part of 
a single HTML report file). For example: "this variable is never changed 
and may be declared as const (1) because this if-branch in which it's 
assigned is in fact dead (2) because its condition always evaluates to 
true (3)". These may be three bug reports emitted by three different 
CFG-based checkers, however this whole logical chain of "(1) because (2) 
because (3)" may be impossible to comprehend unless we show all three 
reports together. That could be fun to write a BugReporter for.

>     Change of plans, i guess.
>
>
> In any case, your changes to BugReport gave much needed clarity to the 
> code! :)

Sure, i still plan to land it.

>     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 <mailto: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/20190903/dce5863a/attachment.html>