Skip to content

[clang-tidy][use-internal-linkage]fix false positives for ExportDecl #117901

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

Merged

Conversation

HerrCai0907
Copy link
Contributor

@HerrCai0907 HerrCai0907 commented Nov 27, 2024

Fixed: #97190

@llvmbot llvmbot added clang Clang issues not falling into any other category clang-tools-extra clang-tidy labels Nov 27, 2024
@llvmbot
Copy link
Member

llvmbot commented Nov 27, 2024

@llvm/pr-subscribers-clang-tidy

Author: Congcong Cai (HerrCai0907)

Changes
  • [ast-matcher] add exportDecl matcher
  • [clang-tidy][use-internal-linkage]fix false positives for ExportDecl

Full diff: https://github.com/llvm/llvm-project/pull/117901.diff

7 Files Affected:

  • (modified) clang-tools-extra/clang-tidy/misc/UseInternalLinkageCheck.cpp (+1-1)
  • (modified) clang-tools-extra/docs/ReleaseNotes.rst (+2-1)
  • (modified) clang-tools-extra/docs/clang-tidy/checks/misc/use-internal-linkage.rst (+5)
  • (added) clang-tools-extra/test/clang-tidy/checkers/misc/use-internal-linkage-module.cpp (+20)
  • (modified) clang/docs/ReleaseNotes.rst (+2)
  • (modified) clang/include/clang/ASTMatchers/ASTMatchers.h (+11)
  • (modified) clang/lib/ASTMatchers/ASTMatchersInternal.cpp (+1)
diff --git a/clang-tools-extra/clang-tidy/misc/UseInternalLinkageCheck.cpp b/clang-tools-extra/clang-tidy/misc/UseInternalLinkageCheck.cpp
index 71eb2d94cd4f26..0656238a0e6767 100644
--- a/clang-tools-extra/clang-tidy/misc/UseInternalLinkageCheck.cpp
+++ b/clang-tools-extra/clang-tidy/misc/UseInternalLinkageCheck.cpp
@@ -101,7 +101,7 @@ void UseInternalLinkageCheck::registerMatchers(MatchFinder *Finder) {
                 // 3. template
                 isExplicitTemplateSpecialization(),
                 // 4. friend
-                hasAncestor(friendDecl()))));
+                hasAncestor(decl(anyOf(friendDecl(), exportDecl()))))));
   Finder->addMatcher(
       functionDecl(Common, hasBody(), unless(cxxMethodDecl()), unless(isMain()))
           .bind("fn"),
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index f050391110385e..42b1e4e9fd1fe4 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -237,7 +237,8 @@ Changes in existing checks
 - Improved :doc:`misc-use-internal-linkage
   <clang-tidy/checks/misc/use-internal-linkage>` check to insert ``static``
   keyword before type qualifiers such as ``const`` and ``volatile`` and fix
-  false positives for function declaration without body.
+  false positives for function declaration without body and fix false positives
+  for C++20 export declarations.
 
 - Improved :doc:`modernize-avoid-c-arrays
   <clang-tidy/checks/modernize/avoid-c-arrays>` check to suggest using 
diff --git a/clang-tools-extra/docs/clang-tidy/checks/misc/use-internal-linkage.rst b/clang-tools-extra/docs/clang-tidy/checks/misc/use-internal-linkage.rst
index b8bbcc62706101..508b0cac09a912 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/misc/use-internal-linkage.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/misc/use-internal-linkage.rst
@@ -29,6 +29,11 @@ Example:
   void fn3(); // without function body in all declaration, maybe external linkage
   void fn3();
 
+  // export declarations
+  export void fn4() {}
+  export namespace t { void fn5() {} }
+  export int v2;
+
 Options
 -------
 
diff --git a/clang-tools-extra/test/clang-tidy/checkers/misc/use-internal-linkage-module.cpp b/clang-tools-extra/test/clang-tidy/checkers/misc/use-internal-linkage-module.cpp
new file mode 100644
index 00000000000000..9b0d8cde429b6b
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/misc/use-internal-linkage-module.cpp
@@ -0,0 +1,20 @@
+// RUN: %check_clang_tidy -std=c++20 %s misc-use-internal-linkage %t -- -- -I%S/Inputs/use-internal-linkage
+
+module;
+
+export module test;
+
+export void single_export_fn() {}
+export int single_export_var;
+
+export {
+  void group_export_fn1() {}
+  void group_export_fn2() {}
+  int group_export_var1;
+  int group_export_var2;
+}
+
+export namespace aa {
+void namespace_export_fn() {}
+int namespace_export_var;
+} // namespace aa
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 356b091c13af4f..4dfbd03d7756f2 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -926,6 +926,8 @@ AST Matchers
 - Ensure ``hasName`` matches template specializations across inline namespaces,
   making `matchesNodeFullSlow` and `matchesNodeFullFast` consistent.
 
+- Add ``exportDecl`` matches export declaration.
+
 clang-format
 ------------
 
diff --git a/clang/include/clang/ASTMatchers/ASTMatchers.h b/clang/include/clang/ASTMatchers/ASTMatchers.h
index 4bcaa953a61af2..efad600a3c58cd 100644
--- a/clang/include/clang/ASTMatchers/ASTMatchers.h
+++ b/clang/include/clang/ASTMatchers/ASTMatchers.h
@@ -1236,6 +1236,17 @@ AST_MATCHER_P(TemplateArgument, equalsIntegralValue,
 extern const internal::VariadicDynCastAllOfMatcher<Stmt,
        ObjCAutoreleasePoolStmt> autoreleasePoolStmt;
 
+/// Matches any export declaration.
+///
+/// Example matches following declarations.
+/// \code
+///   export void foo();
+///   export { void foo(); }
+///   export namespace { void foo(); }
+///   export int v;
+/// \endcode
+extern const internal::VariadicDynCastAllOfMatcher<Decl, ExportDecl> exportDecl;
+
 /// Matches any value declaration.
 ///
 /// Example matches A, B, C and F
diff --git a/clang/lib/ASTMatchers/ASTMatchersInternal.cpp b/clang/lib/ASTMatchers/ASTMatchersInternal.cpp
index 46dd44e6f2b24f..cdbdb65195409f 100644
--- a/clang/lib/ASTMatchers/ASTMatchersInternal.cpp
+++ b/clang/lib/ASTMatchers/ASTMatchersInternal.cpp
@@ -799,6 +799,7 @@ const internal::VariadicDynCastAllOfMatcher<TypeLoc, ElaboratedTypeLoc>
 
 const internal::VariadicDynCastAllOfMatcher<Stmt, UnaryExprOrTypeTraitExpr>
     unaryExprOrTypeTraitExpr;
+const internal::VariadicDynCastAllOfMatcher<Decl, ExportDecl> exportDecl;
 const internal::VariadicDynCastAllOfMatcher<Decl, ValueDecl> valueDecl;
 const internal::VariadicDynCastAllOfMatcher<Decl, CXXConstructorDecl>
     cxxConstructorDecl;

@llvmbot
Copy link
Member

llvmbot commented Nov 27, 2024

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

Author: Congcong Cai (HerrCai0907)

Changes
  • [ast-matcher] add exportDecl matcher
  • [clang-tidy][use-internal-linkage]fix false positives for ExportDecl

Full diff: https://github.com/llvm/llvm-project/pull/117901.diff

7 Files Affected:

  • (modified) clang-tools-extra/clang-tidy/misc/UseInternalLinkageCheck.cpp (+1-1)
  • (modified) clang-tools-extra/docs/ReleaseNotes.rst (+2-1)
  • (modified) clang-tools-extra/docs/clang-tidy/checks/misc/use-internal-linkage.rst (+5)
  • (added) clang-tools-extra/test/clang-tidy/checkers/misc/use-internal-linkage-module.cpp (+20)
  • (modified) clang/docs/ReleaseNotes.rst (+2)
  • (modified) clang/include/clang/ASTMatchers/ASTMatchers.h (+11)
  • (modified) clang/lib/ASTMatchers/ASTMatchersInternal.cpp (+1)
diff --git a/clang-tools-extra/clang-tidy/misc/UseInternalLinkageCheck.cpp b/clang-tools-extra/clang-tidy/misc/UseInternalLinkageCheck.cpp
index 71eb2d94cd4f26..0656238a0e6767 100644
--- a/clang-tools-extra/clang-tidy/misc/UseInternalLinkageCheck.cpp
+++ b/clang-tools-extra/clang-tidy/misc/UseInternalLinkageCheck.cpp
@@ -101,7 +101,7 @@ void UseInternalLinkageCheck::registerMatchers(MatchFinder *Finder) {
                 // 3. template
                 isExplicitTemplateSpecialization(),
                 // 4. friend
-                hasAncestor(friendDecl()))));
+                hasAncestor(decl(anyOf(friendDecl(), exportDecl()))))));
   Finder->addMatcher(
       functionDecl(Common, hasBody(), unless(cxxMethodDecl()), unless(isMain()))
           .bind("fn"),
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index f050391110385e..42b1e4e9fd1fe4 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -237,7 +237,8 @@ Changes in existing checks
 - Improved :doc:`misc-use-internal-linkage
   <clang-tidy/checks/misc/use-internal-linkage>` check to insert ``static``
   keyword before type qualifiers such as ``const`` and ``volatile`` and fix
-  false positives for function declaration without body.
+  false positives for function declaration without body and fix false positives
+  for C++20 export declarations.
 
 - Improved :doc:`modernize-avoid-c-arrays
   <clang-tidy/checks/modernize/avoid-c-arrays>` check to suggest using 
diff --git a/clang-tools-extra/docs/clang-tidy/checks/misc/use-internal-linkage.rst b/clang-tools-extra/docs/clang-tidy/checks/misc/use-internal-linkage.rst
index b8bbcc62706101..508b0cac09a912 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/misc/use-internal-linkage.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/misc/use-internal-linkage.rst
@@ -29,6 +29,11 @@ Example:
   void fn3(); // without function body in all declaration, maybe external linkage
   void fn3();
 
+  // export declarations
+  export void fn4() {}
+  export namespace t { void fn5() {} }
+  export int v2;
+
 Options
 -------
 
diff --git a/clang-tools-extra/test/clang-tidy/checkers/misc/use-internal-linkage-module.cpp b/clang-tools-extra/test/clang-tidy/checkers/misc/use-internal-linkage-module.cpp
new file mode 100644
index 00000000000000..9b0d8cde429b6b
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/misc/use-internal-linkage-module.cpp
@@ -0,0 +1,20 @@
+// RUN: %check_clang_tidy -std=c++20 %s misc-use-internal-linkage %t -- -- -I%S/Inputs/use-internal-linkage
+
+module;
+
+export module test;
+
+export void single_export_fn() {}
+export int single_export_var;
+
+export {
+  void group_export_fn1() {}
+  void group_export_fn2() {}
+  int group_export_var1;
+  int group_export_var2;
+}
+
+export namespace aa {
+void namespace_export_fn() {}
+int namespace_export_var;
+} // namespace aa
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 356b091c13af4f..4dfbd03d7756f2 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -926,6 +926,8 @@ AST Matchers
 - Ensure ``hasName`` matches template specializations across inline namespaces,
   making `matchesNodeFullSlow` and `matchesNodeFullFast` consistent.
 
+- Add ``exportDecl`` matches export declaration.
+
 clang-format
 ------------
 
diff --git a/clang/include/clang/ASTMatchers/ASTMatchers.h b/clang/include/clang/ASTMatchers/ASTMatchers.h
index 4bcaa953a61af2..efad600a3c58cd 100644
--- a/clang/include/clang/ASTMatchers/ASTMatchers.h
+++ b/clang/include/clang/ASTMatchers/ASTMatchers.h
@@ -1236,6 +1236,17 @@ AST_MATCHER_P(TemplateArgument, equalsIntegralValue,
 extern const internal::VariadicDynCastAllOfMatcher<Stmt,
        ObjCAutoreleasePoolStmt> autoreleasePoolStmt;
 
+/// Matches any export declaration.
+///
+/// Example matches following declarations.
+/// \code
+///   export void foo();
+///   export { void foo(); }
+///   export namespace { void foo(); }
+///   export int v;
+/// \endcode
+extern const internal::VariadicDynCastAllOfMatcher<Decl, ExportDecl> exportDecl;
+
 /// Matches any value declaration.
 ///
 /// Example matches A, B, C and F
diff --git a/clang/lib/ASTMatchers/ASTMatchersInternal.cpp b/clang/lib/ASTMatchers/ASTMatchersInternal.cpp
index 46dd44e6f2b24f..cdbdb65195409f 100644
--- a/clang/lib/ASTMatchers/ASTMatchersInternal.cpp
+++ b/clang/lib/ASTMatchers/ASTMatchersInternal.cpp
@@ -799,6 +799,7 @@ const internal::VariadicDynCastAllOfMatcher<TypeLoc, ElaboratedTypeLoc>
 
 const internal::VariadicDynCastAllOfMatcher<Stmt, UnaryExprOrTypeTraitExpr>
     unaryExprOrTypeTraitExpr;
+const internal::VariadicDynCastAllOfMatcher<Decl, ExportDecl> exportDecl;
 const internal::VariadicDynCastAllOfMatcher<Decl, ValueDecl> valueDecl;
 const internal::VariadicDynCastAllOfMatcher<Decl, CXXConstructorDecl>
     cxxConstructorDecl;

@llvmbot
Copy link
Member

llvmbot commented Nov 27, 2024

@llvm/pr-subscribers-clang

Author: Congcong Cai (HerrCai0907)

Changes
  • [ast-matcher] add exportDecl matcher
  • [clang-tidy][use-internal-linkage]fix false positives for ExportDecl

Full diff: https://github.com/llvm/llvm-project/pull/117901.diff

7 Files Affected:

  • (modified) clang-tools-extra/clang-tidy/misc/UseInternalLinkageCheck.cpp (+1-1)
  • (modified) clang-tools-extra/docs/ReleaseNotes.rst (+2-1)
  • (modified) clang-tools-extra/docs/clang-tidy/checks/misc/use-internal-linkage.rst (+5)
  • (added) clang-tools-extra/test/clang-tidy/checkers/misc/use-internal-linkage-module.cpp (+20)
  • (modified) clang/docs/ReleaseNotes.rst (+2)
  • (modified) clang/include/clang/ASTMatchers/ASTMatchers.h (+11)
  • (modified) clang/lib/ASTMatchers/ASTMatchersInternal.cpp (+1)
diff --git a/clang-tools-extra/clang-tidy/misc/UseInternalLinkageCheck.cpp b/clang-tools-extra/clang-tidy/misc/UseInternalLinkageCheck.cpp
index 71eb2d94cd4f26..0656238a0e6767 100644
--- a/clang-tools-extra/clang-tidy/misc/UseInternalLinkageCheck.cpp
+++ b/clang-tools-extra/clang-tidy/misc/UseInternalLinkageCheck.cpp
@@ -101,7 +101,7 @@ void UseInternalLinkageCheck::registerMatchers(MatchFinder *Finder) {
                 // 3. template
                 isExplicitTemplateSpecialization(),
                 // 4. friend
-                hasAncestor(friendDecl()))));
+                hasAncestor(decl(anyOf(friendDecl(), exportDecl()))))));
   Finder->addMatcher(
       functionDecl(Common, hasBody(), unless(cxxMethodDecl()), unless(isMain()))
           .bind("fn"),
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index f050391110385e..42b1e4e9fd1fe4 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -237,7 +237,8 @@ Changes in existing checks
 - Improved :doc:`misc-use-internal-linkage
   <clang-tidy/checks/misc/use-internal-linkage>` check to insert ``static``
   keyword before type qualifiers such as ``const`` and ``volatile`` and fix
-  false positives for function declaration without body.
+  false positives for function declaration without body and fix false positives
+  for C++20 export declarations.
 
 - Improved :doc:`modernize-avoid-c-arrays
   <clang-tidy/checks/modernize/avoid-c-arrays>` check to suggest using 
diff --git a/clang-tools-extra/docs/clang-tidy/checks/misc/use-internal-linkage.rst b/clang-tools-extra/docs/clang-tidy/checks/misc/use-internal-linkage.rst
index b8bbcc62706101..508b0cac09a912 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/misc/use-internal-linkage.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/misc/use-internal-linkage.rst
@@ -29,6 +29,11 @@ Example:
   void fn3(); // without function body in all declaration, maybe external linkage
   void fn3();
 
+  // export declarations
+  export void fn4() {}
+  export namespace t { void fn5() {} }
+  export int v2;
+
 Options
 -------
 
diff --git a/clang-tools-extra/test/clang-tidy/checkers/misc/use-internal-linkage-module.cpp b/clang-tools-extra/test/clang-tidy/checkers/misc/use-internal-linkage-module.cpp
new file mode 100644
index 00000000000000..9b0d8cde429b6b
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/misc/use-internal-linkage-module.cpp
@@ -0,0 +1,20 @@
+// RUN: %check_clang_tidy -std=c++20 %s misc-use-internal-linkage %t -- -- -I%S/Inputs/use-internal-linkage
+
+module;
+
+export module test;
+
+export void single_export_fn() {}
+export int single_export_var;
+
+export {
+  void group_export_fn1() {}
+  void group_export_fn2() {}
+  int group_export_var1;
+  int group_export_var2;
+}
+
+export namespace aa {
+void namespace_export_fn() {}
+int namespace_export_var;
+} // namespace aa
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 356b091c13af4f..4dfbd03d7756f2 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -926,6 +926,8 @@ AST Matchers
 - Ensure ``hasName`` matches template specializations across inline namespaces,
   making `matchesNodeFullSlow` and `matchesNodeFullFast` consistent.
 
+- Add ``exportDecl`` matches export declaration.
+
 clang-format
 ------------
 
diff --git a/clang/include/clang/ASTMatchers/ASTMatchers.h b/clang/include/clang/ASTMatchers/ASTMatchers.h
index 4bcaa953a61af2..efad600a3c58cd 100644
--- a/clang/include/clang/ASTMatchers/ASTMatchers.h
+++ b/clang/include/clang/ASTMatchers/ASTMatchers.h
@@ -1236,6 +1236,17 @@ AST_MATCHER_P(TemplateArgument, equalsIntegralValue,
 extern const internal::VariadicDynCastAllOfMatcher<Stmt,
        ObjCAutoreleasePoolStmt> autoreleasePoolStmt;
 
+/// Matches any export declaration.
+///
+/// Example matches following declarations.
+/// \code
+///   export void foo();
+///   export { void foo(); }
+///   export namespace { void foo(); }
+///   export int v;
+/// \endcode
+extern const internal::VariadicDynCastAllOfMatcher<Decl, ExportDecl> exportDecl;
+
 /// Matches any value declaration.
 ///
 /// Example matches A, B, C and F
diff --git a/clang/lib/ASTMatchers/ASTMatchersInternal.cpp b/clang/lib/ASTMatchers/ASTMatchersInternal.cpp
index 46dd44e6f2b24f..cdbdb65195409f 100644
--- a/clang/lib/ASTMatchers/ASTMatchersInternal.cpp
+++ b/clang/lib/ASTMatchers/ASTMatchersInternal.cpp
@@ -799,6 +799,7 @@ const internal::VariadicDynCastAllOfMatcher<TypeLoc, ElaboratedTypeLoc>
 
 const internal::VariadicDynCastAllOfMatcher<Stmt, UnaryExprOrTypeTraitExpr>
     unaryExprOrTypeTraitExpr;
+const internal::VariadicDynCastAllOfMatcher<Decl, ExportDecl> exportDecl;
 const internal::VariadicDynCastAllOfMatcher<Decl, ValueDecl> valueDecl;
 const internal::VariadicDynCastAllOfMatcher<Decl, CXXConstructorDecl>
     cxxConstructorDecl;

@HerrCai0907 HerrCai0907 changed the title users/ccc/fix/module use internal linkage [clang-tidy][use-internal-linkage]fix false positives for ExportDecl Nov 27, 2024
Copy link
Member

@PiotrZSL PiotrZSL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Documentation & Test for matcher is missing.
Except that everything looks ok.

@HerrCai0907 HerrCai0907 requested a review from PiotrZSL November 27, 2024 23:15
Copy link
Contributor

@5chmidti 5chmidti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -926,6 +926,8 @@ AST Matchers
- Ensure ``hasName`` matches template specializations across inline namespaces,
making `matchesNodeFullSlow` and `matchesNodeFullFast` consistent.

- Add ``exportDecl`` matches export declaration.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a exportDecl matcher to match export declarations.

@HerrCai0907
Copy link
Contributor Author

Please add the matcher to https://github.com/llvm/llvm-project/blob/main/clang/lib/ASTMatchers/Dynamic/Registry.cpp as well (clang-query).

i will do it separately to avoid this pr become bigger.

@5chmidti
Copy link
Contributor

Please add the matcher to https://github.com/llvm/llvm-project/blob/main/clang/lib/ASTMatchers/Dynamic/Registry.cpp as well (clang-query).

i will do it separately to avoid this pr become bigger.

I think you only have to add it into that file for it to work, and IMO a single PR for this change would be better. But as long as it gets added, I'm fine with it

@HerrCai0907 HerrCai0907 merged commit 010317e into llvm:main Dec 2, 2024
9 checks passed
@HerrCai0907 HerrCai0907 deleted the users/ccc/fix/module-use-internal-linkage branch December 2, 2024 05:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang Clang issues not falling into any other category clang-tidy clang-tools-extra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

misc-use-internal-linkage warns on exported functions
4 participants