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

[Artem Dergachev via cfe-dev]

On 8/14/19 5:43 AM, Aaron Ballman wrote:
> On Wed, Aug 14, 2019 at 8:38 AM Dmitri Gribenko <gribozavr at gmail.com> wrote:
>> On Wed, Aug 14, 2019 at 2:35 PM Aaron Ballman <aaron at aaronballman.com> wrote:
>>> 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.
>> Agreed, users who *can* integrate by analyzing during the build,
>> should be able to do so.
> Awesome; I also agree that there may be users who cannot go with this
> approach and we need to support them as well.

This is much more about delivery than it is about integration.

When it comes to integration, "clang file.cpp --analyze $FLAGS" isn't 
anyhow easier to integrate than "clang-tidy file.cpp -- $FLAGS". It 
needs to be a separate invocation anyway, because Clang cannot do normal 
compilation when it's in its "--analyze" mode. So "clang --analyze" is 
basically indistinguishable from a separate tool in this aspect.

But when it comes to delivering the tool to the users, there are a lot 
more people who already have Clang delivered to them than people who 
already have Clang-Tidy delivered to them. Not needing to ship 
additional packages to our users but instead having the tool always 
available at their fingertips is a nice advantage that we currently enjoy.


>>>> 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.
>> The build system won't proceed with the build if .o files and code
>> generation binaries are not produced.
> I think that depends on the build system and the project, but you're
> right that some build systems won't handle this well.
>
> ~Aaron
>
>> Dmitri
>>
>>> ~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>*/
>>
>>
>> --
>> 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>*/