[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.

[carlosgalvezp]

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.

[steakhal]

/cc @alejandro-alvarez-sonarsource This might be interesting for you.

[steakhal]

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.

[steakhal]

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.

[ChuanqiXu9]

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.

[ChuanqiXu9]

I picked a new API isInAnotherModuleUnit, this looks much more descriptive.

[vbvictor]

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.

Indeed, they are handled different.

Left a minor comment.