[clang-tidy] [Modules] Skip checking decls in clang-tidy #145630
Conversation
ChuanqiXu9
added
clang-tidy
clang:modules
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-tools-extra @llvm/pr-subscribers-clang-modules Author: Chuanqi Xu (ChuanqiXu9) ChangesClose #145628 Note that I am not sure if this proper. On the one hand, the fix lives in ASTMachers instead of clang-tidy. On the other hand, I feel this may be a more general fix. Full diff: https://github.com/llvm/llvm-project/pull/145630.diff 5 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/ClangTidy.cpp b/clang-tools-extra/clang-tidy/ClangTidy.cpp
index f4ab93b51f4a7..68192f7ad6240 100644
--- a/clang-tools-extra/clang-tidy/ClangTidy.cpp
+++ b/clang-tools-extra/clang-tidy/ClangTidy.cpp
@@ -417,6 +417,9 @@ ClangTidyASTConsumerFactory::createASTConsumer(
ast_matchers::MatchFinder::MatchFinderOptions FinderOptions;
+ // We should always skip the declarations in modules.
+ FinderOptions.SkipDeclsInModules = true;
+
std::unique_ptr<ClangTidyProfiling> Profiling;
if (Context.getEnableProfiling()) {
Profiling =
diff --git a/clang-tools-extra/test/clang-tidy/checkers/cxx20-modules.cppm b/clang-tools-extra/test/clang-tidy/checkers/cxx20-modules.cppm
new file mode 100644
index 0000000000000..b7e39e2295a1f
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/cxx20-modules.cppm
@@ -0,0 +1,29 @@
+// RUN: rm -fr %t
+// RUN: mkdir %t
+// RUN: split-file %s %t
+// RUN: mkdir %t/tmp
+//
+// RUN: %check_clang_tidy -std=c++20 -check-suffix=DEFAULT %t/a.cpp \
+// RUN: cppcoreguidelines-narrowing-conversions %t/a.cpp -- \
+// RUN: -config='{}'
+
+// RUN: %clang -std=c++20 -x c++-module %t/a.cpp --precompile -o %t/a.pcm
+
+// RUN: %check_clang_tidy -std=c++20 -check-suffix=DEFAULT %t/use.cpp \
+// RUN: cppcoreguidelines-narrowing-conversions %t/a.cpp -- \
+// RUN: -config='{}' -- -fmodule-file=a=%t/a.pcm
+
+//--- a.cpp
+export module a;
+export void most_narrowing_is_not_ok() {
+ int i;
+ long long ui;
+ i = ui;
+ // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:7: warning: narrowing conversion from 'long long' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+}
+
+//--- use.cpp
+import a;
+void use() {
+ most_narrowing_is_not_ok();
+}
diff --git a/clang-tools-extra/test/lit.cfg.py b/clang-tools-extra/test/lit.cfg.py
index 9f64fd3d2ffa2..73882851345bf 100644
--- a/clang-tools-extra/test/lit.cfg.py
+++ b/clang-tools-extra/test/lit.cfg.py
@@ -19,6 +19,7 @@
config.suffixes = [
".c",
".cpp",
+ ".cppm",
".hpp",
".m",
".mm",
diff --git a/clang/include/clang/ASTMatchers/ASTMatchFinder.h b/clang/include/clang/ASTMatchers/ASTMatchFinder.h
index 73cbcf1f25025..69d569a7b09cc 100644
--- a/clang/include/clang/ASTMatchers/ASTMatchFinder.h
+++ b/clang/include/clang/ASTMatchers/ASTMatchFinder.h
@@ -139,6 +139,11 @@ class MatchFinder {
///
/// It prints a report after match.
std::optional<Profiling> CheckProfiling;
+
+ bool SkipDeclsInModules = false;
+
+ MatchFinderOptions()
+ : CheckProfiling(std::nullopt), SkipDeclsInModules(false) {}
};
MatchFinder(MatchFinderOptions Options = MatchFinderOptions());
diff --git a/clang/lib/ASTMatchers/ASTMatchFinder.cpp b/clang/lib/ASTMatchers/ASTMatchFinder.cpp
index 6d0ba0b7907a1..224bc261fa9bd 100644
--- a/clang/lib/ASTMatchers/ASTMatchFinder.cpp
+++ b/clang/lib/ASTMatchers/ASTMatchFinder.cpp
@@ -20,6 +20,7 @@
#include "clang/AST/ASTContext.h"
#include "clang/AST/DeclCXX.h"
#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/Basic/Module.h"
#include "llvm/ADT/DenseMap.h"
#include "llvm/ADT/SmallPtrSet.h"
#include "llvm/ADT/StringMap.h"
@@ -1469,6 +1470,12 @@ bool MatchASTVisitor::TraverseDecl(Decl *DeclNode) {
return true;
}
+ if (Options.SkipDeclsInModules && DeclNode->isFromASTFile()) {
+ auto *M = DeclNode->getOwningModule();
+ if (M && (M->isInterfaceOrPartition() || M->isGlobalModule()))
+ return true;
+ }
+
bool ScopedTraversal =
TraversingASTNodeNotSpelledInSource || DeclNode->isImplicit();
bool ScopedChildren = TraversingASTChildrenNotSpelledInSource;
|
|
If modules are considered as system headers in clang-tidy, there was work in #128150 to reduce scope of traversal to match only in user code (I suppose it would affect modules too). But that PR had to be reverted due to appeared issues with downstream users. Hopefully it will reland in the future. @carlosgalvezp may shine some light on this matter. Treating modules separately doesn't seem right to me. |
I think modules are not system headers. For the example in the issue, the module interface is not system nor headers. It is another TU but we just can get AST from it. I think it may be a valid expectation to not check the same TU again and again for different consumers.
For "stmts" and other "nodes", my expectation is to exclude them as well. I just want to avoid duplications. |
|
I think this patch can be orthogonal to the system headers one, the original one from @njames93 contained similar logic I believe (but I chose to not include it for simplicity). It's probably good to apply this change now before modules become mainstream and we have debt to deal with, like it happened with my patch :) I agree however that there may be other things than Decls to take care of, but I don't have a complete picture here. |
|
/cc @alejandro-alvarez-sonarsource This might be interesting for you. |
steakhal
left a comment
steakhal
left a comment
There was a problem hiding this comment.
I also believe that the nodes from modules should not be traversed by default, but there should be an option to allow traversing those - just like it's implemented.
| if (Options.SkipDeclsInModules && DeclNode->isFromASTFile()) { | ||
| auto *M = DeclNode->getOwningModule(); | ||
| if (M && (M->isInterfaceOrPartition() || M->isGlobalModule())) | ||
| return true; | ||
| } | ||
|
|
There was a problem hiding this comment.
I'm not familiar with modules at all, I just want to make sure we check this the right way as there are about 8 member functions matching the is*Module() pattern.
There was a problem hiding this comment.
Yes, I think this is fine. isFromASTFile means this is deserialized. (M->isInterfaceOrPartition() || M->isGlobalModule()) means it is from named modules. There are other kinds of modules but they are header modules, which have headers semantics. While the C++20 named modules is another TU, not a header. So I think it is proper here.
There was a problem hiding this comment.
I picked a new API isInAnotherModuleUnit, this looks much more descriptive.
This commit is a re-do of e4a8969, which got reverted, with the same goal: dramatically speed-up clang-tidy by avoiding doing work in system headers (which is wasteful as warnings are later discarded). This proposal was already discussed here with favorable feedback: llvm#132725 The novelty of this patch is: - It's less aggressive: it does not fiddle with AST traversal. This solves the issue with the previous patch, which impacted the ability to inspect parents of a given node. - Instead, what we optimize for is exitting early in each Traverse* function of MatchASTVisitor if the node is in a system header, thus avoiding calling the match() function with its corresponding callback (when there is a match). - It does not cause any failing tests. - It does not move MatchFinderOptions outside - instead we add a user-defined default constructor which solves the same problem. - It introduces a function "shouldSkipNode" which can be extended for adding more conditions. For example there's a PR open about skipping modules in clang-tidy where this could come handy: llvm#145630 As a benchmark, I ran clang-tidy with all checks activated, on a single .cpp file which #includes all the standard C++ headers, then measure the time as well as found warnings. On trunk: Suppressed 213314 warnings (213314 in non-user code). real 0m14.311s user 0m14.126s sys 0m0.185s With this patch: Suppressed 149399 warnings (149399 in non-user code). real 0m3.583s user 0m3.454s sys 0m0.128s With the original patch that got reverted: Suppressed 8050 warnings (8050 in non-user code). Runtime: around 1 second. A lot of warnings remain and the runtime is sligthly higher, but we still got a dramatic reduction with no change in functionality. Further investigation has shown that all of the remaining warnings are due to PPCallbacks - implementing a similar system-header exclusion mechanism there can lead to almost no warnings left in system headers. This does not bring the runtime down as much, though. However this may not be as straightforward or wanted, it may even need to be done on a per-check basis (there's about 10 checks or so that would need to explicitly ignore system headers). I will leave that for another patch, it's low priority and does not improve the runtime much (it just prints better statistics). Fixes llvm#52959
This commit is a re-do of e4a8969, which got reverted, with the same goal: dramatically speed-up clang-tidy by avoiding doing work in system headers (which is wasteful as warnings are later discarded). This proposal was already discussed here with favorable feedback: #132725 The novelty of this patch is: - It's less aggressive: it does not fiddle with AST traversal. This solves the issue with the previous patch, which impacted the ability to inspect parents of a given node. - Instead, what we optimize for is exitting early in each `Traverse*` function of `MatchASTVisitor` if the node is in a system header, thus avoiding calling the `match()` function with its corresponding callback (when there is a match). - It does not cause any failing tests. - It does not move `MatchFinderOptions` - instead we add a user-defined default constructor which solves the same problem. - It introduces a function `shouldSkipNode` which can be extended for adding more conditions. For example there's a PR open about skipping modules in clang-tidy where this could come handy: #145630 As a benchmark, I ran clang-tidy with all checks activated, on a single .cpp file which #includes all the standard C++ headers, then measure the time as well as found warnings. On trunk: ``` Suppressed 75413 warnings (75413 in non-user code). real 0m12.418s user 0m12.270s sys 0m0.129s ``` With this patch: ``` Suppressed 11448 warnings (11448 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. real 0m1.666s user 0m1.538s sys 0m0.129s ``` With the original patch that got reverted: ``` Suppressed 11428 warnings (11428 in non-user code). real 0m1.193s user 0m1.096s sys 0m0.096s ``` We therefore get a dramatic reduction in number of warnings and runtime, with no change in functionality. The remaining warnings are due to `PPCallbacks` - implementing a similar system-header exclusion mechanism there can lead to almost no warnings left in system headers. This does not bring the runtime down as much, though, so it's probably not worth the effort. Fixes #52959 Co-authored-by: Carlos Gálvez <[email protected]>
This commit is a re-do of e4a8969, which got reverted, with the same goal: dramatically speed-up clang-tidy by avoiding doing work in system headers (which is wasteful as warnings are later discarded). This proposal was already discussed here with favorable feedback: llvm/llvm-project#132725 The novelty of this patch is: - It's less aggressive: it does not fiddle with AST traversal. This solves the issue with the previous patch, which impacted the ability to inspect parents of a given node. - Instead, what we optimize for is exitting early in each `Traverse*` function of `MatchASTVisitor` if the node is in a system header, thus avoiding calling the `match()` function with its corresponding callback (when there is a match). - It does not cause any failing tests. - It does not move `MatchFinderOptions` - instead we add a user-defined default constructor which solves the same problem. - It introduces a function `shouldSkipNode` which can be extended for adding more conditions. For example there's a PR open about skipping modules in clang-tidy where this could come handy: llvm/llvm-project#145630 As a benchmark, I ran clang-tidy with all checks activated, on a single .cpp file which #includes all the standard C++ headers, then measure the time as well as found warnings. On trunk: ``` Suppressed 75413 warnings (75413 in non-user code). real 0m12.418s user 0m12.270s sys 0m0.129s ``` With this patch: ``` Suppressed 11448 warnings (11448 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. real 0m1.666s user 0m1.538s sys 0m0.129s ``` With the original patch that got reverted: ``` Suppressed 11428 warnings (11428 in non-user code). real 0m1.193s user 0m1.096s sys 0m0.096s ``` We therefore get a dramatic reduction in number of warnings and runtime, with no change in functionality. The remaining warnings are due to `PPCallbacks` - implementing a similar system-header exclusion mechanism there can lead to almost no warnings left in system headers. This does not bring the runtime down as much, though, so it's probably not worth the effort. Fixes #52959 Co-authored-by: Carlos Gálvez <[email protected]>
5 participants
Close #145628
Note that I am not sure if this is the proper fix. On the one hand, the fix lives in ASTMachers instead of clang-tidy. On the other hand, I feel this may be a more general fix.
And if you don't feel good about this and you have a clear mindset how to fix this, feel free to take it.