llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
28.87k stars 11.93k forks source link

Feature Request: Option to not check included files in clang-tidy #52959

Open cinderblock opened 2 years ago

cinderblock commented 2 years ago

When using clang-tidy to lint files, it needs to #include the dependencies. More often than not, however, dependencies are much larger and out of control of the consumer that is running clang-tidy.

clang-tidy, by default, currently does the correct thing and does not report errors/warnings from the #included files. However, from watching it run, it's clearly still actually checking those files for errors (52k+ warnings suppressed!). This dramatically increases the time it takes for clang-tidy to run from nearly instant to ~3 seconds for a large header file like <napi.h> (and this is a rather fast desktop). This really gets in the way of tools that integrate linters into code editors as every change requires that 3 second delay.

I see options about filtering headers or even lines but, at best, it looks like I'd need to list out every single range of my included header files which is actually a rather large number of files which could also change outside of my control. I have tried a variety of these options and not noticed a significant change in behavior in ways I care about.

Is there a way to do what I'd like? Or is there some fundamental reason why this wouldn't work? Frankly, if feels like this should even be the default.

njames93 commented 2 years ago

clang-tidy needs to build the entire AST for a translation unit so it has to parse all the includes. However there is some work in a way to disable some checking in header files, but that is not quite ready.

For editor integrations, have you tried clangd. This builds a preamble saving the need to reparse all your header files and then when clang-tidy checks get ran on the AST, they ignore all Declarations that aren't in the main file.

cinderblock commented 2 years ago

clang-tidy needs to build the entire AST for a translation unit so it has to parse all the includes. However there is some work in a way to disable some checking in header files, but that is not quite ready.

Yes, it's clear that the preprocessor directives need to all be evaluated, which includes #includes. However clang can traverse all of those way faster than clang-tidy does, so it's not an issue of loading the files/parsing the preprocessor. It's clearly the 50k+ warnings that are "suppressed" that are causing the delays.

For editor integrations, have you tried clangd. This builds a preamble saving the need to reparse all your header files and then when clang-tidy checks get ran on the AST, they ignore all Declarations that aren't in the main file.

I have not. I'm actually building with gcc and just using clang-tidy (and clang-format) for "linting" my C/C++. Specifically, I'm using the notskm.clang-tidy VS Code extension. Looks like llvm-vs-code-extensions.vscode-clangd provides this integration and supports clang-tidy. I wonder if this will work for me as I'm using some slightly custom clang-tidy binary wrappers to handle automatically finding some includes.

njames93 commented 2 years ago

I wonder if this will work for me as I'm using some slightly custom clang-tidy binary wrappers to handle automatically finding some includes.

It should be configurable for your needs. You can set it up with additional include folders if there are any issues with them not being found.

fbridault commented 2 years ago

I was looking if this issue was reported and I am glad to find it here. Of course, I guess this is not trivial to implement.

I just made some tests and for a simple file c++ file, just include <memory>, takes 5 seconds to complete on my laptop, even with system headers excluded. If you add boost/thread/thread.hpp, this takes up to 15 seconds. A regular clang build of a file takes less than a second of course, so as said above, this seems not the preprocessing part but well the detection of warnings.

In the general case, I doubt most C++ developers would care anyway about reports of defects in the STL or any 3rd part library, so that's a big waste of time to spend time for that in my opinion. clang-tidy is so widely used today, that I believe that if you address this, you would reduce a significant part of the carbon footprint of humanity. :rofl:

Well that being said, that's always simpler to request something than to do it, so thank you for all your hard work. I just wanted to motivate the need a bit, if that was even necessary... If there is anything else I can do to help please let me know.

tonygould commented 1 year ago

Not parsing headers that aren't part of your project would be great. clang-tidy is a fantastic tool, but it is very problematic to run it in a script (e.g. on CI) and for it to take ~6x longer. I see there was an attempt to add an option to skip parsing header files, https://reviews.llvm.org/D98710, in 2021. Anyone know why that got abandoned -- looks like some non-trivial work went into it? And whether that work could reasonably be used as a basis for implementing the feature?

mikael-s-persson commented 10 months ago

Just adding a +1 to the need for this feature. Here is the output from a clang-tidy run I'm staring at right now (very typical output):

132165 warnings generated.
Suppressed 132439 warnings (132155 in non-user code, 10 due to line filter, 274 NOLINT).

It would nice for those 132155 checks to have been skipped entirely. On the bright side, it's nice that clang-tidy is at least honest about how much work it's doing for no reason. ;)

thomas-seiler-snkeos commented 8 months ago

It would also be highly apreciated by us if this could be fixed. It feels very inefficient to take so much time for "skipping" 99.99% of all checks because they are not in the current scope.

carlosgalvezp commented 2 weeks ago

I see that some checks use unless(isExpansionInSystemHeader(). Does this fix the problem? Or is there still significant runtime involved in traversing the full AST instead of a partial AST?

If this fixes the problem, can't we apply this to all/most existing checks?