[clang-tidy] Fix checks filter with warnings-as-errors

[kiwixz]

Summary

Since commit 5d12b13b0b26bc58b02ee23c369da8b83240cceb, warnings are internally classified as errors which skip the check filters (see ClangTidyDiagnosticConsumer.cpp:324).

One usecase is particularly affected: you cannot selectively disable clang-analyzer-core checks, they are force-enabled because required by others. So enabling warning as errors will show new (and unwanted) errors !

Good:

32693 warnings generated.
Suppressed 32693 warnings (32692 in non-user code, 1 with check filters).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.

Bad (-warnings-as-errors=*):

33057 warnings generated.
...
error: Called C++ object pointer is uninitialized [clang-analyzer-core.CallAndMessage,-warnings-as-errors]
...
Suppressed 33054 warnings (33054 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
1 warning treated as error

Related issue: https://github.com/llvm/llvm-project/issues/61520

[PiotrZSL]

Looks fine, and looks like it's working.

[carlosgalvezp]

I'm not sure I follow - if you enable warnings as error for all checks, then the expectation is that you indeed get an error if you violate one of the clang-analyzer checks. In what way is this not wanted?

If you could post a minimal self-contained example (e.g. on compiler explorer) that would be awesome!

[carlosgalvezp]

Also, regarding clang-analyzer-core checks, I have 2 tickets open:

https://github.com/llvm/llvm-project/issues/59588
https://github.com/llvm/llvm-project/issues/59589

[carlosgalvezp]

Reading through Github I found the associated ticket (it would be good if you could mention it in the commit message):
https://github.com/llvm/llvm-project/issues/61520

So if I understand correctly, what you are trying to do is continue to let the clang-analyzer-core checks run, and simply ignore the warnings coming from them when enabling warnings_as_errors. I'm not entirely convinced that's the best approach, it's hiding the real problem - that as a user one cannot disable clang-analyzer-core checks (which is being fixed in another patch by PiotrZSL). I also find strange to move error-handling logic away from the DiagnosticConsumer class out to the main ClangTidy class.

Would be good if @njames93 or @aaron.ballman could comment on this, given their experience.

[kiwixz]

Your understanding is correct, I'm currently just trying to hide them at the end (warnings-as-errors or not).
This is how it worked until 5d12b13b0b26bc58b02ee23c369da8b83240cceb (present in LLVM 16) which inadvertently bypass the filters with warnings-as-errors.

Note that I'm just trying to restore the previous behavior, while I'm sure PiotrZSL and others will make the whole thing more elegant for next releases.
My hope here is to fix LLVM 16 so we don't have errors that we explicitly disabled showing up, and only when enabling warnings-as-errors.

My patch does two things:

• deleting the two lines in ClangTidyDiagnosticConsumer.cpp which introduced the new and bad behavior
• adding two lines in ClangTidy.cpp to classify warnings as errors when appropriate in the YAML (this was the goal of the aforementioned commit)

[aaron.ballman]

In D146520#4221314, @carlosgalvezp wrote:

Reading through Github I found the associated ticket (it would be good if you could mention it in the commit message):
https://github.com/llvm/llvm-project/issues/61520

So if I understand correctly, what you are trying to do is continue to let the clang-analyzer-core checks run, and simply ignore the warnings coming from them when enabling warnings_as_errors. I'm not entirely convinced that's the best approach, it's hiding the real problem - that as a user one cannot disable clang-analyzer-core checks (which is being fixed in another patch by PiotrZSL). I also find strange to move error-handling logic away from the DiagnosticConsumer class out to the main ClangTidy class.

Would be good if @njames93 or @aaron.ballman could comment on this, given their experience.

Ideally, users should be able to disable the core checks from running at all (the reason users want to disable checks is both because of the diagnostics they produce and because of the cost of running the check itself). However, this is solving the issue accidentally introduced which is incremental forward progress.

I'd say this is tentatively okay, but definitely needs test coverage for the change. I'd hope that the fix to no longer run the core checks at all will obviate the need for this code, though, so it might be appropriate to add a FIXME comment about that.

[PiotrZSL]

In D146520#4224281, @aaron.ballman wrote:

Ideally, users should be able to disable the core checks from running at all (the reason users want to disable checks is both because of the diagnostics they produce and because of the cost of running the check itself).

I'm trying to enable this here: https://reviews.llvm.org/D146396, but problem is that I'm not sure what core checkers should be enabled by default. Simply Static Analysis config file does not show that.

[aaron.ballman]

In D146520#4224483, @PiotrZSL wrote:

In D146520#4224281, @aaron.ballman wrote:

Ideally, users should be able to disable the core checks from running at all (the reason users want to disable checks is both because of the diagnostics they produce and because of the cost of running the check itself).

I'm trying to enable this here: https://reviews.llvm.org/D146396, but problem is that I'm not sure what core checkers should be enabled by default. Simply Static Analysis config file does not show that.

CC @NoQ @xazax.hun for a question about what core checkers should be enabled by default in clang-tidy (and general awareness that this is about changes to how clang-tidy surfaces the static analyzer).

[tiagoma]

Applying this patch seems to fix issues with C++20 as well. See https://github.com/llvm/llvm-project/issues/61969
Can we get it in?

[carlosgalvezp]

Having another look and reading your comments I got a better understanding of the situation:

• YAML output did not have correct warnings as errors.
• Thus a fix was introduced in ClangTidyDiagnosticConsumer, however this was a too broad place to put the fix, breaking diagnostics output by command-line.
• Instead, the fix should have been put in a narrower scope, in the part of the code that deals with YAML output.
• This patch does exactly that.

So this patch is not really altering the original diagnostic behavior. In that sense I'm ok with it! I suppose it would have been clearer from a review perspective if first a revert was made and then re-landed with the correct code, but it's probably too much annoyance for such a small change.

[carlosgalvezp]

It would still be good to add some test that ensures this does not happen again in the future, though.

[kiwixz]

I'm not sure how to test this, is there any check dependency we can rely on with hardcoded tests ?

[kiwixz]

Ping.