RFC: Pumping Clang-Tidy warnings through the Static Analyzer's BugReporter.

Clang Frontend
1

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. The opposite is already true because Clang-Tidy links to the Static Analyzer. However, existing Static Analyzer users (eg., users that rely on Scan-Build for their build system interception or use the Static Analyzer through Xcode) cannot easily take advantage of Clang-Tidy. Also, as far as i understand, Ericsson CodeChecker currently performs ad-hoc integration of Clang-Tidy by parsing text output, which most likely works just fine, but definitely contributes to anxiety. On the other hand, the BugReporter facility of the Static Analyzer has a variety of output modes, including fancy html reports and machine-readable plist output, so if we pump Clang-Tidy reports through the same BugReporter it'll open up some new possibilities.

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

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

It is going to be a bit annoying that DiagnosticBuilder treats warnings and their respective notes as completely unrelated diagnostics, while for the purposes of the BugReporter they need to be on the same BugReport object. I should be able to take care of this on the BugReporter side as long as clang-tidy checkers promise to always emit notes immediately after their respective warnings (which seems to be the case, but i didn't look at *all* the checks).

Finally, one feature that BugReporter was lacking until recently was the fix-it hint support. However, this is mostly out of the way since https://reviews.llvm.org/D65182.

Does any of that sound reasonable? Should i proceed with this?

2

Hi Artem,

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 :slight_smile: 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

3

I completely agree with what Dmitri has written. A couple of additional comments from me below.

Hi Artem,

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.

There are some distinct features (like FixIt hints or path-sensitive diagnostics) and use cases (e.g. the automated refactorings), that can be better or worse supported by different integrations and workflows. It should still be possible to take the best of two worlds and unify the user experience.

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.

This would be good for a few more reasons:

  1. Clear separation of functionality: clang’s main purpose is to compile code rather than perform static analysis. Clang and static analyzer need partially overlapping, but different sets of options.
  2. Clang binary size growth is a concern for maintainers of toolchains. Static analyzer (especially with AST matchers-based checkers) is contributing a lot to that growth. Building clang without CLANG_ENABLE_STATIC_ANALYZER somewhat solves this part of the problem, but then you need another clang binary (possibly in a separate package) to support static analyzer - I believe, that’s clearly suboptimal. Thus, having a completely different binary for the static analyzer (or teach clang-tidy do everything static analyzer needs to do - including all output formats and options) would be a much better solution.

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

No concerns here, as long as we ensure that BugReporter has support for FixItHints and other stuff DiagnosticBuilder provides.

4

Hi Artem,

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

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

5

>
> Hi Artem,
>
> >
> > 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

6

>
> >
> > Hi Artem,
> >
> > >
> > > 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

7

>
> >
> > >
> > > Hi Artem,
> > >
> > > >
> > > > 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.

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

Dmitri

8

>
> >
> > >
> > > >
> > > > Hi Artem,
> > > >
> > > > >
> > > > > 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.

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

9

>
> Hi Artem,
>
> >
> > 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!

Same here, there are other tools around that might benefit from that,
eg. clazy, and any other custom tools.

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

Same comment again, would be nice if the command line, and
configuration file handling could be shared and consistent b/w similar
tooling (clazy, custom, ...)

10

Does it makes sense to keep support for compiler wrapper?
The compilation database is a much better approach.

Chris

11

It depends whom we are optimizing for.

I think that generally, small projects will prefer direct compiler
integration (`clang -c -tidy -analyze`) or the compiler wrapper
approach.

Bigger projects with more complex build processes, a CI, pre-commit
testing etc. will probably prefer an approach based on the compilation
database.

If we want to cover everyone, I guess we have to maintain both approaches.

Dmitri

12

>
> >
> > >
> > > >
> > > > >
> > > > > >
> > > > > > Hi Artem,
> > > > > >
> > > > > > >
> > > > > > > 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.
> >
> > > > 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.
>
> Does it makes sense to keep support for compiler wrapper?
> The compilation database is a much better approach.

It depends whom we are optimizing for.

I think that generally, small projects will prefer direct compiler
integration (`clang -c -tidy -analyze`) or the compiler wrapper
approach.

Bigger projects with more complex build processes, a CI, pre-commit
testing etc. will probably prefer an approach based on the compilation
database.

If we want to cover everyone, I guess we have to maintain both approaches.

I think you may be correct.

~Aaron

13

Speaking as an outsider in the peanut gallery who does very little static analysis, this is very appealing to me. It would save binary size in clang and make building and testing clang faster, if the developer isn’t working in the static analyzer space.

14

Hi Artem,

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.

15

Generally we already have a cmake flag for this, -DCLANG_ENABLE_STATIC_ANALYZER=OFF (defaults to ON). This flag is definitely a must-have for people who try to make their clang binary as tiny as possible. But i understand that this flag is not exceptionally discoverable and a lot of people probably suffer unnecessarily long compile times simply because they didn’t discover it.

That said, i suspect that unless you work exclusively on CodeGen, you probably do not want to turn off the Static Analyzer, because the Static Analyzer has some interesting tests that you can accidentally break when you work on anything else (Preprocessor, Lexer, AST, Sema…). In particular, Richard Smith regularly touches our code :slight_smile:

16

Ugh, i just realized this.

Clang-Tidy doesn't really need BugReporter; all it needs is PathDiagnosticConsumer. (the word "Path" is rudimentary)

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.

Change of plans, i guess.

17

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! :slight_smile:

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! :slight_smile:

18

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. Sure, i still plan to land it.

19

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! :slight_smile:

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?

There are a few CFG-based checks. Some of them would benefit from a way to represent execution path in the same diagnostic. The way it can currently be done is using diagnostic notes (CSA diagnostics are represented the same way, when converted from PathDiagnostic in clang-tidy (the code is around clang-tidy/ClangTidy.cpp:70).

20

I like this approach better, however, I also think that
PathDiagnostic's API is a lot more complex than the current `diag()
<<` in ClangTidy. For example, it requires the caller to specify the
checker name. It also requires to specify a ton of locations and
decls, whereas we can probably infer most of the locations and decls
from just one location.

Again, something I would really like to see written down (and us agree
upon) is the data model for a diagnostic. When we know what data we
need to store, we can figure out how to store it, and a convenient API
to populate it.

Dmitri