Skip to content

[clang-tidy] HeaderFilterRegex in .clang-tidy is not applied consistently and depends on working directory #118009

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
PWVEC opened this issue Nov 28, 2024 · 11 comments · Fixed by #133582
Assignees

Comments

@PWVEC
Copy link

PWVEC commented Nov 28, 2024

I've encountered a strange behavior regarding "HeaderFilterRegex" (other options might be affected as well...).
I've tested this with clang-tidy version 19.1.2.
With older versions (e.g. clang-tidy 18.1.2) HeaderFilterRegex works as expected, so this seems to be a new issue.

Given the following setup in a project:

.
└── src
    ├── .clang-tidy
    ├── external.h
    ├── internal.h
    └── main.cpp

With the following file contents:

$ cat src/.clang-tidy
Checks: "-*,modernize-use-trailing-return-type"
HeaderFilterRegex: '.*internal.*'

$ cat src/internal.h
int GetSomeValue();

$ cat src/external.h
int GetSomeLibValue();

$ cat src/main.cpp
#include "internal.h"
#include "external.h"

int GetSomeValue() { return 42; }

int main() {
  return GetSomeValue() + GetSomeLibValue();
}

The expectation is that when clang-tidy is run on "main.cpp", it will report the diagnostics for "internal.h" but not for "external.h".
But this does not work consistently. It seems relevant in which workding directory clang-tidy is invoked.

$ clang-tidy src/main.cpp 2>/dev/null
PATH/TO/PROJ/src/main.cpp:4:5: warning: use a trailing return type for this function [modernize-use-trailing-return-type]
    4 | int GetSomeValue() { return 42; }
      | ~~~ ^
      | auto               -> int
PATH/TO/PROJ/src/main.cpp:6:5: warning: use a trailing return type for this function [modernize-use-trailing-return-type]
    6 | int main() {
      | ~~~ ^
      | auto       -> int

No diagnostic is applied, i.e. the "HeaderFilterRegex" is essentially set to ''.
If I run clang-tidy in "src", then this works as expected (internal.h is reported, external.h is not):

cd src/ && clang-tidy main.cpp 2>/dev/null
PATH/TO/PROJ/src/internal.h:1:5: warning: use a trailing return type for this function [modernize-use-trailing-return-type]
    1 | int GetSomeValue();
      | ~~~ ^
      | auto               -> int
PATH/TO/PROJ/src/main.cpp:4:5: warning: use a trailing return type for this function [modernize-use-trailing-return-type]
    4 | int GetSomeValue() { return 42; }
      | ~~~ ^
      | auto               -> int
PATH/TO/PROJ/src/main.cpp:6:5: warning: use a trailing return type for this function [modernize-use-trailing-return-type]
    6 | int main() {
      | ~~~ ^
      | auto       -> int

Notably, if I dump the configuration with "--dump-config" the resulting configuration is identical in both cases and it clearly sets "HeaderFilterRegex" to ".*internal.*".

@PWVEC PWVEC changed the title [clang-tidy] InheritParentConfig does not seem to work consistently with HeaderFilterRegex [clang-tidy] HeaderFilterRegex in .clang-tidy does not seem to work consistently and depends on working directory Nov 28, 2024
@PWVEC PWVEC changed the title [clang-tidy] HeaderFilterRegex in .clang-tidy does not seem to work consistently and depends on working directory [clang-tidy] HeaderFilterRegex in .clang-tidy is not applied consistently and depends on working directory Nov 28, 2024
@Deewiant
Copy link

As pointed out in #121969, this seems to have been introduced with #91400.

From what I can tell, the problem might be due to how the initialization of HeaderFilter was moved to the constructor of ClangTidyDiagnosticConsumer. Based on cursory inspection:

Previously, HeaderFilter was initialized in the call chain from ClangTidyDiagnosticConsumer::HandleDiagnostic, so definitely after the context had seen all the options.

Again, this is based on a quick look, and I'm not very familiar with the codebase so I don't know if this analysis holds water at all. (For example: is getOptionsForFile actually even related to the .clang-tidy file? It sounds like it to me, but I haven't dived that deep.) Hopefully this is at least a helpful hint and not complete misdirection.

@drobilla
Copy link

I'm also having this problem. It seems that ExcludeHeaderFilterRegex is broken in the same way (so you can't work around the issue by inverting the pattern in the configuration file).

@Wheest
Copy link
Contributor

Wheest commented Feb 20, 2025

I might be facing this issue also. I've got a reproducible repo Wheest/clang-tidy-issue, where we don't want to run clang-tidy on any files under external.

And yet with my config., I still get

./external/extern_proj/funky.hpp:3:15: error: static assertion failed due to requirement '201703L == 202302L': Expected to be compiled with C++23 [clang-diagnostic-error]
    3 | static_assert(__cplusplus == 202302L, "Expected to be compiled with C++23");
      |               ^
note: expanded from macro '__cplusplus'
Found compiler error(s).

Version:

clang-tidy --version
Ubuntu LLVM version 18.1.3
Optimized build.

@jmcclellan-figma
Copy link

jmcclellan-figma commented Mar 21, 2025

@Wheest static_asserts are compilation errors will always be shown regardless of HeaderFilterRegex.

Here's a minimal repro for this issue:

q/x.cpp

#include "x.h"

q/.clang-tidy

HeaderFilterRegex: '.*'

q/x.h

#warning "Test"

run: clang-tidy q/x.cpp

EXPECTED: warning is shown

ACTUAL: warning is suppressed because HeaderFilterRegex is being ignored

Note that before #91400, or if #91400 is reverted, we get the expected behavior.

This caused us to accidentally stop linting all header files after upgrading clang-tidy and seems like a rather serious issue!

@carlosgalvezp
Copy link
Contributor

carlosgalvezp commented Mar 28, 2025

Strange, we are running latest trunk and do not observe this issue...

Edit: that's probably because we have the .clang-tidy file at the root instead of a subfolder.

@tearfur
Copy link

tearfur commented Mar 28, 2025

@carlosgalvezp I do have a reproducible example on 316bb89 at #133453 (comment).

@carlosgalvezp
Copy link
Contributor

Yes, I can repro on our side as well.

@carlosgalvezp
Copy link
Contributor

Actually it doesn't even work having all files at the root. HeaderFilterRegex is completely ignored?!

@carlosgalvezp carlosgalvezp self-assigned this Mar 29, 2025
carlosgalvezp pushed a commit to carlosgalvezp/llvm-project that referenced this issue Mar 29, 2025
PR llvm#91400 broke the usage
of HeaderFilterRegex via config file, because it is now created at a
different point in the execution and leads to a different value.

The result of that is that using HeaderFilterRegex only in the config
file does NOT work, in other words clang-tidy stops triggering warnings
on header files, thereby losing a lot of coverage.

This patch reverts the logic so that the header filter is created
upon calling the getHeaderFilter() function.

Additionally, this patch adds 2 unit tests to prevent regressions in
the future:

- One of them, "simple", tests the most basic use case with a single
  top-level .clang-tidy file.

- The second one, "inheritance", demonstrates that the subfolder only
  gets warnings from headers within it, and not from parent headers.

Fixes llvm#118009, llvm#121969, llvm#133453
carlosgalvezp pushed a commit to carlosgalvezp/llvm-project that referenced this issue Mar 29, 2025
PR llvm#91400 broke the usage
of HeaderFilterRegex via config file, because it is now created at a
different point in the execution and leads to a different value.

The result of that is that using HeaderFilterRegex only in the config
file does NOT work, in other words clang-tidy stops triggering warnings
on header files, thereby losing a lot of coverage.

This patch reverts the logic so that the header filter is created
upon calling the getHeaderFilter() function.

Additionally, this patch adds 2 unit tests to prevent regressions in
the future:

- One of them, "simple", tests the most basic use case with a single
  top-level .clang-tidy file.

- The second one, "inheritance", demonstrates that the subfolder only
  gets warnings from headers within it, and not from parent headers.

Fixes llvm#118009, llvm#121969, llvm#133453
carlosgalvezp pushed a commit to carlosgalvezp/llvm-project that referenced this issue Mar 29, 2025
PR llvm#91400 broke the usage
of HeaderFilterRegex via config file, because it is now created at a
different point in the execution and leads to a different value.

The result of that is that using HeaderFilterRegex only in the config
file does NOT work, in other words clang-tidy stops triggering warnings
on header files, thereby losing a lot of coverage.

This patch reverts the logic so that the header filter is created
upon calling the getHeaderFilter() function.

Additionally, this patch adds 2 unit tests to prevent regressions in
the future:

- One of them, "simple", tests the most basic use case with a single
  top-level .clang-tidy file.

- The second one, "inheritance", demonstrates that the subfolder only
  gets warnings from headers within it, and not from parent headers.

Fixes llvm#118009, llvm#121969, llvm#133453
carlosgalvezp pushed a commit to carlosgalvezp/llvm-project that referenced this issue Mar 29, 2025
PR llvm#91400 broke the usage
of HeaderFilterRegex via config file, because it is now created at a
different point in the execution and leads to a different value.

The result of that is that using HeaderFilterRegex only in the config
file does NOT work, in other words clang-tidy stops triggering warnings
on header files, thereby losing a lot of coverage.

This patch reverts the logic so that the header filter is created
upon calling the getHeaderFilter() function.

Additionally, this patch adds 2 unit tests to prevent regressions in
the future:

- One of them, "simple", tests the most basic use case with a single
  top-level .clang-tidy file.

- The second one, "inheritance", demonstrates that the subfolder only
  gets warnings from headers within it, and not from parent headers.

Fixes llvm#118009, llvm#121969, llvm#133453
@carlosgalvezp
Copy link
Contributor

I believe the linked PR should fix all the related issues, but it'd be great if you guys can test it!

carlosgalvezp pushed a commit to carlosgalvezp/llvm-project that referenced this issue Mar 29, 2025
PR llvm#91400 broke the usage
of HeaderFilterRegex via config file, because it is now created at a
different point in the execution and leads to a different value.

The result of that is that using HeaderFilterRegex only in the config
file does NOT work, in other words clang-tidy stops triggering warnings
on header files, thereby losing a lot of coverage.

This patch reverts the logic so that the header filter is created
upon calling the getHeaderFilter() function.

Additionally, this patch adds 2 unit tests to prevent regressions in
the future:

- One of them, "simple", tests the most basic use case with a single
  top-level .clang-tidy file.

- The second one, "inheritance", demonstrates that the subfolder only
  gets warnings from headers within it, and not from parent headers.

Fixes llvm#118009, llvm#121969, llvm#133453
carlosgalvezp pushed a commit to carlosgalvezp/llvm-project that referenced this issue Mar 29, 2025
PR llvm#91400 broke the usage
of HeaderFilterRegex via config file, because it is now created at a
different point in the execution and leads to a different value.

The result of that is that using HeaderFilterRegex only in the config
file does NOT work, in other words clang-tidy stops triggering warnings
on header files, thereby losing a lot of coverage.

This patch reverts the logic so that the header filter is created
upon calling the getHeaderFilter() function.

Additionally, this patch adds 2 unit tests to prevent regressions in
the future:

- One of them, "simple", tests the most basic use case with a single
  top-level .clang-tidy file.

- The second one, "inheritance", demonstrates that the subfolder only
  gets warnings from headers within it, and not from parent headers.

Fixes llvm#118009, llvm#121969, llvm#133453
carlosgalvezp pushed a commit to carlosgalvezp/llvm-project that referenced this issue Mar 30, 2025
PR llvm#91400 broke the usage
of HeaderFilterRegex via config file, because it is now created at a
different point in the execution and leads to a different value.

The result of that is that using HeaderFilterRegex only in the config
file does NOT work, in other words clang-tidy stops triggering warnings
on header files, thereby losing a lot of coverage.

This patch reverts the logic so that the header filter is created
upon calling the getHeaderFilter() function.

Additionally, this patch adds 2 unit tests to prevent regressions in
the future:

- One of them, "simple", tests the most basic use case with a single
  top-level .clang-tidy file.

- The second one, "inheritance", demonstrates that the subfolder only
  gets warnings from headers within it, and not from parent headers.

Fixes llvm#118009, llvm#121969, llvm#133453
carlosgalvezp pushed a commit to carlosgalvezp/llvm-project that referenced this issue Mar 30, 2025
PR llvm#91400 broke the usage
of HeaderFilterRegex via config file, because it is now created at a
different point in the execution and leads to a different value.

The result of that is that using HeaderFilterRegex only in the config
file does NOT work, in other words clang-tidy stops triggering warnings
on header files, thereby losing a lot of coverage.

This patch reverts the logic so that the header filter is created
upon calling the getHeaderFilter() function.

Additionally, this patch adds 2 unit tests to prevent regressions in
the future:

- One of them, "simple", tests the most basic use case with a single
  top-level .clang-tidy file.

- The second one, "inheritance", demonstrates that the subfolder only
  gets warnings from headers within it, and not from parent headers.

Fixes llvm#118009, llvm#121969, llvm#133453
@jmcclellan-figma
Copy link

I believe the linked PR should fix all the related issues, but it'd be great if you guys can test it!

Pardon my ignorance but is there a way to get a pre-built clang-tidy from a PR? Otherwise I can build and test - reading the code it does look like it would solve it

@carlosgalvezp
Copy link
Contributor

Pardon my ignorance but is there a way to get a pre-built clang-tidy from a PR? Otherwise I can build and test - reading the code it does look like it would solve it

Unfortunately not, one needs to apply the diff and build from source.

@github-project-automation github-project-automation bot moved this to Needs Triage in LLVM Release Status Apr 2, 2025
carlosgalvezp pushed a commit to carlosgalvezp/llvm-project that referenced this issue Apr 3, 2025
PR llvm#91400 broke the usage
of HeaderFilterRegex via config file, because it is now created at a
different point in the execution and leads to a different value.

The result of that is that using HeaderFilterRegex only in the config
file does NOT work, in other words clang-tidy stops triggering warnings
on header files, thereby losing a lot of coverage.

This patch reverts the logic so that the header filter is created
upon calling the getHeaderFilter() function.

Additionally, this patch adds 2 unit tests to prevent regressions in
the future:

- One of them, "simple", tests the most basic use case with a single
  top-level .clang-tidy file.

- The second one, "inheritance", demonstrates that the subfolder only
  gets warnings from headers within it, and not from parent headers.

Fixes llvm#118009, llvm#121969, llvm#133453
@github-project-automation github-project-automation bot moved this from Needs Triage to Done in LLVM Release Status Apr 3, 2025
swift-ci pushed a commit to swiftlang/llvm-project that referenced this issue Apr 11, 2025
…lvm#133582)

PR llvm#91400 broke the usage of
HeaderFilterRegex via config file, because it is now created at a
different point in the execution and leads to a different value.

The result of that is that using HeaderFilterRegex only in the config
file does NOT work, in other words clang-tidy stops triggering warnings
on header files, thereby losing a lot of coverage.

This patch reverts the logic so that the header filter is created upon
calling the getHeaderFilter() function.

Additionally, this patch adds 2 unit tests to prevent regressions in the
future:

- One of them, "simple", tests the most basic use case with a single
top-level .clang-tidy file.

- The second one, "inheritance", demonstrates that the subfolder only
gets warnings from headers within it, and not from parent headers.

Fixes llvm#118009
Fixes llvm#121969
Fixes llvm#133453

Co-authored-by: Carlos Gálvez <[email protected]>
(cherry picked from commit 6333fa5)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging a pull request may close this issue.

7 participants