-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[clang-tidy] Add misc-shadowed-namespace-function check #168406
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
base: main
Are you sure you want to change the base?
Changes from all commits
b7d665b
e9de03d
199ecec
8018941
320f6c7
5994fc0
4112759
89029b7
34dc72a
a249084
b1ffd92
1589ed5
3b36a43
1f7748d
d6bdb39
51eac24
532b761
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,140 @@ | ||
| //===----------------------------------------------------------------------===// | ||
| // | ||
| // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. | ||
| // See https://llvm.org/LICENSE.txt for license information. | ||
| // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception | ||
| // | ||
| //===----------------------------------------------------------------------===// | ||
|
|
||
| #include "ShadowedNamespaceFunctionCheck.h" | ||
| #include "../utils/FixItHintUtils.h" | ||
| #include "clang/AST/ASTContext.h" | ||
| #include "clang/AST/Decl.h" | ||
| #include "clang/AST/DeclCXX.h" | ||
| #include "clang/ASTMatchers/ASTMatchers.h" | ||
| #include "llvm/ADT/STLExtras.h" | ||
|
|
||
| using namespace clang; | ||
| using namespace clang::ast_matchers; | ||
| using namespace clang::tidy; | ||
|
|
||
| namespace clang::tidy::misc { | ||
|
|
||
| static bool hasSameParameters(const FunctionDecl *Func1, | ||
| const FunctionDecl *Func2) { | ||
| if (Func1->param_size() != Func2->param_size()) | ||
| return false; | ||
|
|
||
| return llvm::all_of_zip( | ||
| Func1->parameters(), Func2->parameters(), | ||
| [](const ParmVarDecl *Param1, const ParmVarDecl *Param2) { | ||
| return Param1->getType().getCanonicalType() == | ||
| Param2->getType().getCanonicalType(); | ||
| }); | ||
| } | ||
|
|
||
| void ShadowedNamespaceFunctionCheck::registerMatchers(MatchFinder *Finder) { | ||
| // Simple matcher for all function definitions | ||
| Finder->addMatcher(functionDecl(isDefinition()).bind("func"), this); | ||
| } | ||
|
|
||
| void ShadowedNamespaceFunctionCheck::check( | ||
| const MatchFinder::MatchResult &Result) { | ||
| const auto *Func = Result.Nodes.getNodeAs<FunctionDecl>("func"); | ||
|
|
||
| if (!Func || !Result.SourceManager) | ||
| return; | ||
|
Comment on lines
+45
to
+46
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just assert, we expect the matcher to work. |
||
|
|
||
| // Skip if not in global namespace | ||
| const DeclContext *DC = Func->getDeclContext(); | ||
| if (!DC->isTranslationUnit()) | ||
| return; | ||
|
Comment on lines
+48
to
+51
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should be checked in matchers, |
||
|
|
||
| // Skip templates, static functions, main, etc. | ||
| if (Func->isTemplated() || Func->isStatic() || Func->isMain() || | ||
| Func->isImplicit() || Func->isVariadic()) | ||
| return; | ||
|
Comment on lines
+54
to
+56
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should check it in matchers |
||
|
|
||
| const std::string FuncName = Func->getNameAsString(); | ||
| if (FuncName.empty()) | ||
| return; | ||
|
|
||
| const ASTContext *Context = Result.Context; | ||
|
|
||
| // Look for functions with the same name in namespaces | ||
| const FunctionDecl *ShadowedFunc = nullptr; | ||
| const NamespaceDecl *ShadowedNamespace = nullptr; | ||
|
|
||
| // Traverse all declarations in the translation unit | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto comments |
||
| for (const auto *Decl : Context->getTranslationUnitDecl()->decls()) { | ||
| if (const auto *NS = dyn_cast<NamespaceDecl>(Decl)) { | ||
| std::tie(ShadowedFunc, ShadowedNamespace) = | ||
| findShadowedInNamespace(NS, Func, FuncName); | ||
| if (ShadowedFunc) | ||
| break; | ||
| } | ||
| } | ||
|
Comment on lines
+68
to
+76
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you please check with Traversing all decls on function seems like a very heavy operation. |
||
|
|
||
| if (!ShadowedFunc || !ShadowedNamespace) | ||
| return; | ||
|
|
||
| if (ShadowedFunc->getDefinition()) | ||
| return; | ||
|
|
||
| // Generate warning message | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto comments |
||
| const std::string NamespaceName = | ||
| ShadowedNamespace->getQualifiedNameAsString(); | ||
| auto Diag = diag(Func->getLocation(), "free function %0 shadows '%1::%2'") | ||
| << Func->getDeclName() << NamespaceName | ||
| << ShadowedFunc->getDeclName().getAsString(); | ||
|
|
||
| // Generate fixit hint to add namespace qualification | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto comments |
||
| const SourceLocation NameLoc = Func->getLocation(); | ||
| if (NameLoc.isValid() && !Func->getPreviousDecl()) { | ||
| const std::string Fix = NamespaceName + "::"; | ||
| Diag << FixItHint::CreateInsertion(NameLoc, Fix); | ||
| } | ||
|
|
||
| // Note: Also show where the shadowed function is declared | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto comments |
||
| diag(ShadowedFunc->getLocation(), "function %0 declared here", | ||
| DiagnosticIDs::Note) | ||
| << ShadowedFunc->getDeclName(); | ||
| } | ||
|
|
||
| std::pair<const FunctionDecl *, const NamespaceDecl *> | ||
| ShadowedNamespaceFunctionCheck::findShadowedInNamespace( | ||
| const NamespaceDecl *NS, const FunctionDecl *GlobalFunc, | ||
| const std::string &GlobalFuncName) { | ||
|
Comment on lines
+104
to
+107
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does it need to be a class member and not a regular static function? |
||
|
|
||
| // Skip anonymous namespaces | ||
| if (NS->isAnonymousNamespace()) | ||
| return {nullptr, nullptr}; | ||
|
|
||
| for (const auto *Decl : NS->decls()) { | ||
| // Check nested namespaces | ||
| if (const auto *NestedNS = dyn_cast<NamespaceDecl>(Decl)) { | ||
| auto [ShadowedFunc, ShadowedNamespace] = | ||
| findShadowedInNamespace(NestedNS, GlobalFunc, GlobalFuncName); | ||
| if (ShadowedFunc) | ||
| return {ShadowedFunc, ShadowedNamespace}; | ||
| } | ||
|
|
||
| // Check functions | ||
| if (const auto *Func = dyn_cast<FunctionDecl>(Decl)) { | ||
| // Skip if it's the same function, templates, or definitions | ||
| if (Func == GlobalFunc || Func->isTemplated() || | ||
| Func->isThisDeclarationADefinition()) | ||
| continue; | ||
|
|
||
| if (Func->getNameAsString() == GlobalFuncName && !Func->isVariadic() && | ||
| hasSameParameters(Func, GlobalFunc) && | ||
| Func->getReturnType().getCanonicalType() == | ||
| GlobalFunc->getReturnType().getCanonicalType()) { | ||
| return {Func, NS}; | ||
| } | ||
| } | ||
| } | ||
| return {nullptr, nullptr}; | ||
| } | ||
|
|
||
| } // namespace clang::tidy::misc | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| //===----------------------------------------------------------------------===// | ||
| // | ||
| // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. | ||
| // See https://llvm.org/LICENSE.txt for license information. | ||
| // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception | ||
| // | ||
| //===----------------------------------------------------------------------===// | ||
|
|
||
| #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_SHADOWEDNAMESPACEFUNCTIONCHECK_H | ||
| #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_SHADOWEDNAMESPACEFUNCTIONCHECK_H | ||
|
|
||
| #include "../ClangTidyCheck.h" | ||
| #include <tuple> | ||
|
|
||
| namespace clang::tidy::misc { | ||
|
|
||
| /// Detects free functions in global namespace that shadow functions from other | ||
| /// namespaces. | ||
| /// | ||
| /// For the user-facing documentation see: | ||
| /// https://clang.llvm.org/extra/clang-tidy/checks/misc/shadowed-namespace-function.html | ||
| class ShadowedNamespaceFunctionCheck : public ClangTidyCheck { | ||
| public: | ||
| ShadowedNamespaceFunctionCheck(StringRef Name, ClangTidyContext *Context) | ||
| : ClangTidyCheck(Name, Context) {} | ||
| void registerMatchers(ast_matchers::MatchFinder *Finder) override; | ||
| void check(const ast_matchers::MatchFinder::MatchResult &Result) override; | ||
| bool isLanguageVersionSupported(const LangOptions &LangOpts) const override { | ||
| return LangOpts.CPlusPlus; | ||
| } | ||
|
|
||
| private: | ||
| std::pair<const FunctionDecl *, const NamespaceDecl *> | ||
| findShadowedInNamespace(const NamespaceDecl *NS, | ||
| const FunctionDecl *GlobalFunc, | ||
| const std::string &GlobalFuncName); | ||
| }; | ||
|
|
||
| } // namespace clang::tidy::misc | ||
|
|
||
| #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_SHADOWEDNAMESPACEFUNCTIONCHECK_H |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -226,6 +226,11 @@ New checks | |
| Finds virtual function overrides with different visibility than the function | ||
| in the base class. | ||
|
|
||
| - New :doc:`misc-shadowed-namespace-function | ||
| <clang-tidy/checks/misc/shadowed-namespace-function>` check. | ||
|
|
||
| Detects free functions in global namespace that shadow functions from other namespaces. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 80 char limit |
||
|
|
||
| - New :doc:`readability-redundant-parentheses | ||
| <clang-tidy/checks/readability/redundant-parentheses>` check. | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,63 @@ | ||||||||||||||
| .. title:: clang-tidy - misc-shadowed-namespace-function | ||||||||||||||
|
|
||||||||||||||
| misc-shadowed-namespace-function | ||||||||||||||
| ================================ | ||||||||||||||
|
|
||||||||||||||
| Detects free functions in the global namespace that shadow functions declared | ||||||||||||||
| in other namespaces. This check helps prevent accidental shadowing of namespace | ||||||||||||||
|
Comment on lines
+6
to
+7
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||
| functions, which can lead to confusion about which function is being called and | ||||||||||||||
| potential linking errors. | ||||||||||||||
|
|
||||||||||||||
| Examples | ||||||||||||||
| -------- | ||||||||||||||
|
|
||||||||||||||
| .. code-block:: c++ | ||||||||||||||
|
|
||||||||||||||
| namespace utils { | ||||||||||||||
| void process(); | ||||||||||||||
| void calculate(); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| // Warning: free function shadows utils::process | ||||||||||||||
| void process() {} | ||||||||||||||
|
|
||||||||||||||
| // No warning - static function | ||||||||||||||
| static void calculate() {} | ||||||||||||||
|
|
||||||||||||||
| The check will suggest adding the appropriate namespace qualification: | ||||||||||||||
|
|
||||||||||||||
| .. code-block:: diff | ||||||||||||||
| - void process() {} | ||||||||||||||
| + void utils::process() {} | ||||||||||||||
| The check will not warn about: | ||||||||||||||
|
|
||||||||||||||
| - Static functions or member functions; | ||||||||||||||
| - Functions in anonymous namespaces; | ||||||||||||||
| - The ``main`` function. | ||||||||||||||
|
|
||||||||||||||
| Limitations | ||||||||||||||
| ----------- | ||||||||||||||
|
|
||||||||||||||
| - Does not warn about friend functions: | ||||||||||||||
|
|
||||||||||||||
| .. code-block:: c++ | ||||||||||||||
|
|
||||||||||||||
| namespace llvm::gsym { | ||||||||||||||
| struct MergedFunctionsInfo { | ||||||||||||||
| friend bool operator==(const MergedFunctionsInfo &LHS, | ||||||||||||||
| const MergedFunctionsInfo &RHS); | ||||||||||||||
| }; | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| using namespace llvm::gsym; | ||||||||||||||
|
|
||||||||||||||
| bool operator==(const MergedFunctionsInfo &LHS, // no warning in this version | ||||||||||||||
| const MergedFunctionsInfo &RHS) { | ||||||||||||||
| return LHS.MergedFunctions == RHS.MergedFunctions; | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| - Does not warn about template functions | ||||||||||||||
| - Does not warn about variadic functions | ||||||||||||||
|
Comment on lines
+61
to
+62
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing punctuation at the end? |
||||||||||||||
|
|
||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Excessive newline. |
||||||||||||||
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.
Remove unrelated (obvious) comments