-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
[clang-tidy] Fix broken HeaderFilterRegex when read from config file #133582
Conversation
@llvm/pr-subscribers-clang-tidy Author: Carlos Galvez (carlosgalvezp) ChangesPR #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:
Fixes #118009, #121969, #133453 Full diff: https://github.com/llvm/llvm-project/pull/133582.diff 12 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
index 4c75b42270114..71e852545203e 100644
--- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -311,18 +311,7 @@ ClangTidyDiagnosticConsumer::ClangTidyDiagnosticConsumer(
: Context(Ctx), ExternalDiagEngine(ExternalDiagEngine),
RemoveIncompatibleErrors(RemoveIncompatibleErrors),
GetFixesFromNotes(GetFixesFromNotes),
- EnableNolintBlocks(EnableNolintBlocks) {
-
- if (Context.getOptions().HeaderFilterRegex &&
- !Context.getOptions().HeaderFilterRegex->empty())
- HeaderFilter =
- std::make_unique<llvm::Regex>(*Context.getOptions().HeaderFilterRegex);
-
- if (Context.getOptions().ExcludeHeaderFilterRegex &&
- !Context.getOptions().ExcludeHeaderFilterRegex->empty())
- ExcludeHeaderFilter = std::make_unique<llvm::Regex>(
- *Context.getOptions().ExcludeHeaderFilterRegex);
-}
+ EnableNolintBlocks(EnableNolintBlocks) {}
void ClangTidyDiagnosticConsumer::finalizeLastError() {
if (!Errors.empty()) {
@@ -571,17 +560,30 @@ void ClangTidyDiagnosticConsumer::checkFilters(SourceLocation Location,
}
StringRef FileName(File->getName());
- LastErrorRelatesToUserCode =
- LastErrorRelatesToUserCode || Sources.isInMainFile(Location) ||
- (HeaderFilter &&
- (HeaderFilter->match(FileName) &&
- !(ExcludeHeaderFilter && ExcludeHeaderFilter->match(FileName))));
+ LastErrorRelatesToUserCode = LastErrorRelatesToUserCode ||
+ Sources.isInMainFile(Location) ||
+ (getHeaderFilter()->match(FileName) &&
+ !getExcludeHeaderFilter()->match(FileName));
unsigned LineNumber = Sources.getExpansionLineNumber(Location);
LastErrorPassesLineFilter =
LastErrorPassesLineFilter || passesLineFilter(FileName, LineNumber);
}
+llvm::Regex *ClangTidyDiagnosticConsumer::getHeaderFilter() {
+ if (!HeaderFilter)
+ HeaderFilter =
+ std::make_unique<llvm::Regex>(*Context.getOptions().HeaderFilterRegex);
+ return HeaderFilter.get();
+}
+
+llvm::Regex *ClangTidyDiagnosticConsumer::getExcludeHeaderFilter() {
+ if (!ExcludeHeaderFilter)
+ ExcludeHeaderFilter = std::make_unique<llvm::Regex>(
+ *Context.getOptions().ExcludeHeaderFilterRegex);
+ return ExcludeHeaderFilter.get();
+}
+
void ClangTidyDiagnosticConsumer::removeIncompatibleErrors() {
// Each error is modelled as the set of intervals in which it applies
// replacements. To detect overlapping replacements, we use a sweep line
diff --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
index ff42f96a0477b..d6cf6a2b2731e 100644
--- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
+++ b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
@@ -302,6 +302,10 @@ class ClangTidyDiagnosticConsumer : public DiagnosticConsumer {
/// context.
llvm::Regex *getHeaderFilter();
+ /// Returns the \c ExcludeHeaderFilter constructed for the options set in the
+ /// context.
+ llvm::Regex *getExcludeHeaderFilter();
+
/// Updates \c LastErrorRelatesToUserCode and LastErrorPassesLineFilter
/// according to the diagnostic \p Location.
void checkFilters(SourceLocation Location, const SourceManager &Sources);
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 6cb8d572d3a78..6c1f05009df98 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -94,6 +94,9 @@ Improvements to clang-tidy
- Improved :program:`clang-tidy-diff.py` script. Add the `-warnings-as-errors`
argument to treat warnings as errors.
+- Fixed bug in :program:`clang-tidy` by which `HeaderFilterRegex` did not take
+ effect when passed via the `.clang-tidy` file.
+
New checks
^^^^^^^^^^
diff --git a/clang-tools-extra/test/clang-tidy/infrastructure/header-filter-from-config/inheritance/.clang-tidy b/clang-tools-extra/test/clang-tidy/infrastructure/header-filter-from-config/inheritance/.clang-tidy
new file mode 100644
index 0000000000000..f4210353f94de
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/infrastructure/header-filter-from-config/inheritance/.clang-tidy
@@ -0,0 +1 @@
+HeaderFilterRegex: '.*'
diff --git a/clang-tools-extra/test/clang-tidy/infrastructure/header-filter-from-config/inheritance/foo.cpp b/clang-tools-extra/test/clang-tidy/infrastructure/header-filter-from-config/inheritance/foo.cpp
new file mode 100644
index 0000000000000..79622418d6dff
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/infrastructure/header-filter-from-config/inheritance/foo.cpp
@@ -0,0 +1,3 @@
+// RUN: clang-tidy -checks=-*,google-explicit-constructor %s -- 2>&1 | FileCheck %s
+#include "foo.h"
+// CHECK: foo.h:1:12: warning: single-argument constructors must be marked explicit
diff --git a/clang-tools-extra/test/clang-tidy/infrastructure/header-filter-from-config/inheritance/foo.h b/clang-tools-extra/test/clang-tidy/infrastructure/header-filter-from-config/inheritance/foo.h
new file mode 100644
index 0000000000000..f61d4c2923b50
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/infrastructure/header-filter-from-config/inheritance/foo.h
@@ -0,0 +1 @@
+struct X { X(int); };
diff --git a/clang-tools-extra/test/clang-tidy/infrastructure/header-filter-from-config/inheritance/subfolder/.clang-tidy b/clang-tools-extra/test/clang-tidy/infrastructure/header-filter-from-config/inheritance/subfolder/.clang-tidy
new file mode 100644
index 0000000000000..96706c1428047
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/infrastructure/header-filter-from-config/inheritance/subfolder/.clang-tidy
@@ -0,0 +1,2 @@
+InheritParentConfig: true
+HeaderFilterRegex: 'subfolder/.*'
diff --git a/clang-tools-extra/test/clang-tidy/infrastructure/header-filter-from-config/inheritance/subfolder/bar.cpp b/clang-tools-extra/test/clang-tidy/infrastructure/header-filter-from-config/inheritance/subfolder/bar.cpp
new file mode 100644
index 0000000000000..7576b1af74bfa
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/infrastructure/header-filter-from-config/inheritance/subfolder/bar.cpp
@@ -0,0 +1,6 @@
+// RUN: clang-tidy -checks=-*,google-explicit-constructor %s -- -I $(dirname %S) 2>&1 | FileCheck %s
+#include "foo.h"
+// CHECK-NOT: foo.h:1:12: warning: single-argument constructors must be marked explicit
+
+#include "bar.h"
+// CHECK: bar.h:1:13: warning: single-argument constructors must be marked explicit
diff --git a/clang-tools-extra/test/clang-tidy/infrastructure/header-filter-from-config/inheritance/subfolder/bar.h b/clang-tools-extra/test/clang-tidy/infrastructure/header-filter-from-config/inheritance/subfolder/bar.h
new file mode 100644
index 0000000000000..ee12d00d334dd
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/infrastructure/header-filter-from-config/inheritance/subfolder/bar.h
@@ -0,0 +1 @@
+struct XX { XX(int); };
diff --git a/clang-tools-extra/test/clang-tidy/infrastructure/header-filter-from-config/simple/.clang-tidy b/clang-tools-extra/test/clang-tidy/infrastructure/header-filter-from-config/simple/.clang-tidy
new file mode 100644
index 0000000000000..f4210353f94de
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/infrastructure/header-filter-from-config/simple/.clang-tidy
@@ -0,0 +1 @@
+HeaderFilterRegex: '.*'
diff --git a/clang-tools-extra/test/clang-tidy/infrastructure/header-filter-from-config/simple/foo.cpp b/clang-tools-extra/test/clang-tidy/infrastructure/header-filter-from-config/simple/foo.cpp
new file mode 100644
index 0000000000000..79622418d6dff
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/infrastructure/header-filter-from-config/simple/foo.cpp
@@ -0,0 +1,3 @@
+// RUN: clang-tidy -checks=-*,google-explicit-constructor %s -- 2>&1 | FileCheck %s
+#include "foo.h"
+// CHECK: foo.h:1:12: warning: single-argument constructors must be marked explicit
diff --git a/clang-tools-extra/test/clang-tidy/infrastructure/header-filter-from-config/simple/foo.h b/clang-tools-extra/test/clang-tidy/infrastructure/header-filter-from-config/simple/foo.h
new file mode 100644
index 0000000000000..f61d4c2923b50
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/infrastructure/header-filter-from-config/simple/foo.h
@@ -0,0 +1 @@
+struct X { X(int); };
|
@llvm/pr-subscribers-clang-tools-extra Author: Carlos Galvez (carlosgalvezp) ChangesPR #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:
Fixes #118009, #121969, #133453 Full diff: https://github.com/llvm/llvm-project/pull/133582.diff 12 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
index 4c75b42270114..71e852545203e 100644
--- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -311,18 +311,7 @@ ClangTidyDiagnosticConsumer::ClangTidyDiagnosticConsumer(
: Context(Ctx), ExternalDiagEngine(ExternalDiagEngine),
RemoveIncompatibleErrors(RemoveIncompatibleErrors),
GetFixesFromNotes(GetFixesFromNotes),
- EnableNolintBlocks(EnableNolintBlocks) {
-
- if (Context.getOptions().HeaderFilterRegex &&
- !Context.getOptions().HeaderFilterRegex->empty())
- HeaderFilter =
- std::make_unique<llvm::Regex>(*Context.getOptions().HeaderFilterRegex);
-
- if (Context.getOptions().ExcludeHeaderFilterRegex &&
- !Context.getOptions().ExcludeHeaderFilterRegex->empty())
- ExcludeHeaderFilter = std::make_unique<llvm::Regex>(
- *Context.getOptions().ExcludeHeaderFilterRegex);
-}
+ EnableNolintBlocks(EnableNolintBlocks) {}
void ClangTidyDiagnosticConsumer::finalizeLastError() {
if (!Errors.empty()) {
@@ -571,17 +560,30 @@ void ClangTidyDiagnosticConsumer::checkFilters(SourceLocation Location,
}
StringRef FileName(File->getName());
- LastErrorRelatesToUserCode =
- LastErrorRelatesToUserCode || Sources.isInMainFile(Location) ||
- (HeaderFilter &&
- (HeaderFilter->match(FileName) &&
- !(ExcludeHeaderFilter && ExcludeHeaderFilter->match(FileName))));
+ LastErrorRelatesToUserCode = LastErrorRelatesToUserCode ||
+ Sources.isInMainFile(Location) ||
+ (getHeaderFilter()->match(FileName) &&
+ !getExcludeHeaderFilter()->match(FileName));
unsigned LineNumber = Sources.getExpansionLineNumber(Location);
LastErrorPassesLineFilter =
LastErrorPassesLineFilter || passesLineFilter(FileName, LineNumber);
}
+llvm::Regex *ClangTidyDiagnosticConsumer::getHeaderFilter() {
+ if (!HeaderFilter)
+ HeaderFilter =
+ std::make_unique<llvm::Regex>(*Context.getOptions().HeaderFilterRegex);
+ return HeaderFilter.get();
+}
+
+llvm::Regex *ClangTidyDiagnosticConsumer::getExcludeHeaderFilter() {
+ if (!ExcludeHeaderFilter)
+ ExcludeHeaderFilter = std::make_unique<llvm::Regex>(
+ *Context.getOptions().ExcludeHeaderFilterRegex);
+ return ExcludeHeaderFilter.get();
+}
+
void ClangTidyDiagnosticConsumer::removeIncompatibleErrors() {
// Each error is modelled as the set of intervals in which it applies
// replacements. To detect overlapping replacements, we use a sweep line
diff --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
index ff42f96a0477b..d6cf6a2b2731e 100644
--- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
+++ b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
@@ -302,6 +302,10 @@ class ClangTidyDiagnosticConsumer : public DiagnosticConsumer {
/// context.
llvm::Regex *getHeaderFilter();
+ /// Returns the \c ExcludeHeaderFilter constructed for the options set in the
+ /// context.
+ llvm::Regex *getExcludeHeaderFilter();
+
/// Updates \c LastErrorRelatesToUserCode and LastErrorPassesLineFilter
/// according to the diagnostic \p Location.
void checkFilters(SourceLocation Location, const SourceManager &Sources);
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 6cb8d572d3a78..6c1f05009df98 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -94,6 +94,9 @@ Improvements to clang-tidy
- Improved :program:`clang-tidy-diff.py` script. Add the `-warnings-as-errors`
argument to treat warnings as errors.
+- Fixed bug in :program:`clang-tidy` by which `HeaderFilterRegex` did not take
+ effect when passed via the `.clang-tidy` file.
+
New checks
^^^^^^^^^^
diff --git a/clang-tools-extra/test/clang-tidy/infrastructure/header-filter-from-config/inheritance/.clang-tidy b/clang-tools-extra/test/clang-tidy/infrastructure/header-filter-from-config/inheritance/.clang-tidy
new file mode 100644
index 0000000000000..f4210353f94de
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/infrastructure/header-filter-from-config/inheritance/.clang-tidy
@@ -0,0 +1 @@
+HeaderFilterRegex: '.*'
diff --git a/clang-tools-extra/test/clang-tidy/infrastructure/header-filter-from-config/inheritance/foo.cpp b/clang-tools-extra/test/clang-tidy/infrastructure/header-filter-from-config/inheritance/foo.cpp
new file mode 100644
index 0000000000000..79622418d6dff
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/infrastructure/header-filter-from-config/inheritance/foo.cpp
@@ -0,0 +1,3 @@
+// RUN: clang-tidy -checks=-*,google-explicit-constructor %s -- 2>&1 | FileCheck %s
+#include "foo.h"
+// CHECK: foo.h:1:12: warning: single-argument constructors must be marked explicit
diff --git a/clang-tools-extra/test/clang-tidy/infrastructure/header-filter-from-config/inheritance/foo.h b/clang-tools-extra/test/clang-tidy/infrastructure/header-filter-from-config/inheritance/foo.h
new file mode 100644
index 0000000000000..f61d4c2923b50
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/infrastructure/header-filter-from-config/inheritance/foo.h
@@ -0,0 +1 @@
+struct X { X(int); };
diff --git a/clang-tools-extra/test/clang-tidy/infrastructure/header-filter-from-config/inheritance/subfolder/.clang-tidy b/clang-tools-extra/test/clang-tidy/infrastructure/header-filter-from-config/inheritance/subfolder/.clang-tidy
new file mode 100644
index 0000000000000..96706c1428047
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/infrastructure/header-filter-from-config/inheritance/subfolder/.clang-tidy
@@ -0,0 +1,2 @@
+InheritParentConfig: true
+HeaderFilterRegex: 'subfolder/.*'
diff --git a/clang-tools-extra/test/clang-tidy/infrastructure/header-filter-from-config/inheritance/subfolder/bar.cpp b/clang-tools-extra/test/clang-tidy/infrastructure/header-filter-from-config/inheritance/subfolder/bar.cpp
new file mode 100644
index 0000000000000..7576b1af74bfa
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/infrastructure/header-filter-from-config/inheritance/subfolder/bar.cpp
@@ -0,0 +1,6 @@
+// RUN: clang-tidy -checks=-*,google-explicit-constructor %s -- -I $(dirname %S) 2>&1 | FileCheck %s
+#include "foo.h"
+// CHECK-NOT: foo.h:1:12: warning: single-argument constructors must be marked explicit
+
+#include "bar.h"
+// CHECK: bar.h:1:13: warning: single-argument constructors must be marked explicit
diff --git a/clang-tools-extra/test/clang-tidy/infrastructure/header-filter-from-config/inheritance/subfolder/bar.h b/clang-tools-extra/test/clang-tidy/infrastructure/header-filter-from-config/inheritance/subfolder/bar.h
new file mode 100644
index 0000000000000..ee12d00d334dd
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/infrastructure/header-filter-from-config/inheritance/subfolder/bar.h
@@ -0,0 +1 @@
+struct XX { XX(int); };
diff --git a/clang-tools-extra/test/clang-tidy/infrastructure/header-filter-from-config/simple/.clang-tidy b/clang-tools-extra/test/clang-tidy/infrastructure/header-filter-from-config/simple/.clang-tidy
new file mode 100644
index 0000000000000..f4210353f94de
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/infrastructure/header-filter-from-config/simple/.clang-tidy
@@ -0,0 +1 @@
+HeaderFilterRegex: '.*'
diff --git a/clang-tools-extra/test/clang-tidy/infrastructure/header-filter-from-config/simple/foo.cpp b/clang-tools-extra/test/clang-tidy/infrastructure/header-filter-from-config/simple/foo.cpp
new file mode 100644
index 0000000000000..79622418d6dff
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/infrastructure/header-filter-from-config/simple/foo.cpp
@@ -0,0 +1,3 @@
+// RUN: clang-tidy -checks=-*,google-explicit-constructor %s -- 2>&1 | FileCheck %s
+#include "foo.h"
+// CHECK: foo.h:1:12: warning: single-argument constructors must be marked explicit
diff --git a/clang-tools-extra/test/clang-tidy/infrastructure/header-filter-from-config/simple/foo.h b/clang-tools-extra/test/clang-tidy/infrastructure/header-filter-from-config/simple/foo.h
new file mode 100644
index 0000000000000..f61d4c2923b50
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/infrastructure/header-filter-from-config/simple/foo.h
@@ -0,0 +1 @@
+struct X { X(int); };
|
2ecbd54
to
99abf89
Compare
Tested by applying this PR on top of 3e742b5. Works great AFAICT! # curl -sL 'https://github.com/llvm/llvm-project/pull/133582.diff' | git apply |
P.S. Just a heads up that They need to be split to 3 |
7c94ad3
to
c2b1748
Compare
Was there anything else I should fix @HerrCai0907 ? It would be good to get this in in soon and cherrypick to branch 20. |
Otherwise maybe @PiotrZSL can you review, I believe you reviewed the original PR? |
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
c2b1748
to
12b63eb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
@tstellar Any chance we can cherry-pick this to the 20.x branch? |
/cherry-pick 6333fa5 |
/pull-request #134215 |
@@ -0,0 +1 @@ | |||
HeaderFilterRegex: '.*' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It appears this use case was broken by this patch, @HerrCai0907 .
There may be more things to fix.
PR #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 #118009
Fixes #121969
Fixes #133453