[clang-tidy] [Modules] Skip checking decls in clang-tidy

[ChuanqiXu9]

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.

[llvmbot]

@llvm/pr-subscribers-clang-tools-extra
@llvm/pr-subscribers-clang
@llvm/pr-subscribers-clang-tidy

@llvm/pr-subscribers-clang-modules

Author: Chuanqi Xu (ChuanqiXu9)

Changes

Close #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:

• (modified) clang-tools-extra/clang-tidy/ClangTidy.cpp (+3)
• (added) clang-tools-extra/test/clang-tidy/checkers/cxx20-modules.cppm (+29)
• (modified) clang-tools-extra/test/lit.cfg.py (+1)
• (modified) clang/include/clang/ASTMatchers/ASTMatchFinder.h (+5)
• (modified) clang/lib/ASTMatchers/ASTMatchFinder.cpp (+7)

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;

[vbvictor]

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.
Also, If we exclude only decls what happens with stmts and other nodes, are they still getting matched and filtered? What happens if a check looks for a decl inside system header and later compare it to decl inside user-code?

[ChuanqiXu9]

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.

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.

Treating modules separately doesn't seem right to me. Also, If we exclude only decls what happens with stmts and other nodes, are they still getting matched and filtered? What happens if a check looks for a decl inside system header and later compare it to decl inside user-code?

For "stmts" and other "nodes", my expectation is to exclude them as well. I just want to avoid duplications.