llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tidy @llvm/pr-subscribers-clang-tools-extra Author: Balázs Kéri (balazske) <details> <summary>Changes</summary> --- Full diff: https://github.com/llvm/llvm-project/pull/140086.diff 8 Files Affected: - (modified) clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp (+3) - (modified) clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt (+1) - (added) clang-tools-extra/clang-tidy/bugprone/FunctionVisibilityChangeCheck.cpp (+74) - (added) clang-tools-extra/clang-tidy/bugprone/FunctionVisibilityChangeCheck.h (+33) - (modified) clang-tools-extra/docs/ReleaseNotes.rst (+5) - (added) clang-tools-extra/docs/clang-tidy/checks/bugprone/function-visibility-change.rst (+43) - (modified) clang-tools-extra/docs/clang-tidy/checks/list.rst (+1) - (added) clang-tools-extra/test/clang-tidy/checkers/bugprone/function-visibility-change.cpp (+234) ``````````diff diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp index b780a85bdf3fe..7cf58c5218969 100644 --- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp @@ -30,6 +30,7 @@ #include "FoldInitTypeCheck.h" #include "ForwardDeclarationNamespaceCheck.h" #include "ForwardingReferenceOverloadCheck.h" +#include "FunctionVisibilityChangeCheck.h" #include "ImplicitWideningOfMultiplicationResultCheck.h" #include "InaccurateEraseCheck.h" #include "IncDecInConditionsCheck.h" @@ -143,6 +144,8 @@ class BugproneModule : public ClangTidyModule { "bugprone-forward-declaration-namespace"); CheckFactories.registerCheck<ForwardingReferenceOverloadCheck>( "bugprone-forwarding-reference-overload"); + CheckFactories.registerCheck<FunctionVisibilityChangeCheck>( + "bugprone-function-visibility-change"); CheckFactories.registerCheck<ImplicitWideningOfMultiplicationResultCheck>( "bugprone-implicit-widening-of-multiplication-result"); CheckFactories.registerCheck<InaccurateEraseCheck>( diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt index e310ea9c94543..b4f7ba76f4cee 100644 --- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt @@ -26,6 +26,7 @@ add_clang_library(clangTidyBugproneModule STATIC FoldInitTypeCheck.cpp ForwardDeclarationNamespaceCheck.cpp ForwardingReferenceOverloadCheck.cpp + FunctionVisibilityChangeCheck.cpp ImplicitWideningOfMultiplicationResultCheck.cpp InaccurateEraseCheck.cpp IncorrectEnableIfCheck.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/FunctionVisibilityChangeCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/FunctionVisibilityChangeCheck.cpp new file mode 100644 index 0000000000000..7ea4ed20705ed --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/FunctionVisibilityChangeCheck.cpp @@ -0,0 +1,74 @@ +//===--- FunctionVisibilityChangeCheck.cpp - clang-tidy -------------------===// +// +// 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 "FunctionVisibilityChangeCheck.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; + +namespace clang::tidy::bugprone { + +void FunctionVisibilityChangeCheck::registerMatchers(MatchFinder *Finder) { + Finder->addMatcher( + cxxMethodDecl( + ofClass(cxxRecordDecl().bind("class")), + forEachOverridden(cxxMethodDecl(ofClass(cxxRecordDecl().bind("base"))) + .bind("base_func"))) + .bind("func"), + this); +} + +void FunctionVisibilityChangeCheck::check( + const MatchFinder::MatchResult &Result) { + const auto *MatchedFunction = Result.Nodes.getNodeAs<FunctionDecl>("func"); + const auto *ParentClass = Result.Nodes.getNodeAs<CXXRecordDecl>("class"); + const auto *OverriddenFunction = + Result.Nodes.getNodeAs<FunctionDecl>("base_func"); + const auto *BaseClass = Result.Nodes.getNodeAs<CXXRecordDecl>("base"); + + if (!MatchedFunction->isCanonicalDecl()) + return; + + AccessSpecifier ActualAccess = MatchedFunction->getAccess(); + AccessSpecifier OverriddenAccess = OverriddenFunction->getAccess(); + + CXXBasePaths Paths; + if (!ParentClass->isDerivedFrom(BaseClass, Paths)) + return; + const CXXBaseSpecifier *InheritanceWithStrictVisibility = nullptr; + for (const CXXBasePath &Path : Paths) { + for (auto Elem : Path) { + if (Elem.Base->getAccessSpecifier() > OverriddenAccess) { + OverriddenAccess = Elem.Base->getAccessSpecifier(); + InheritanceWithStrictVisibility = Elem.Base; + } + } + } + + if (ActualAccess != OverriddenAccess) { + if (InheritanceWithStrictVisibility) { + diag(MatchedFunction->getLocation(), + "visibility of function %0 is changed from %1 (through %1 " + "inheritance of class %2) to %3") + << MatchedFunction << OverriddenAccess + << InheritanceWithStrictVisibility->getType() << ActualAccess; + diag(InheritanceWithStrictVisibility->getBeginLoc(), + "this inheritance would make %0 %1", DiagnosticIDs::Note) + << MatchedFunction << OverriddenAccess; + } else { + diag(MatchedFunction->getLocation(), + "visibility of function %0 is changed from %1 in class %2 to %3") + << MatchedFunction << OverriddenAccess << BaseClass << ActualAccess; + } + diag(OverriddenFunction->getLocation(), "function declared here as %0", + DiagnosticIDs::Note) + << OverriddenFunction->getAccess(); + } +} + +} // namespace clang::tidy::bugprone diff --git a/clang-tools-extra/clang-tidy/bugprone/FunctionVisibilityChangeCheck.h b/clang-tools-extra/clang-tidy/bugprone/FunctionVisibilityChangeCheck.h new file mode 100644 index 0000000000000..ec7152fb63acb --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/FunctionVisibilityChangeCheck.h @@ -0,0 +1,33 @@ +//===--- FunctionVisibilityChangeCheck.h - clang-tidy -----------*- C++ -*-===// +// +// 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_BUGPRONE_FUNCTIONVISIBILITYCHANGECHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_FUNCTIONVISIBILITYCHANGECHECK_H + +#include "../ClangTidyCheck.h" + +namespace clang::tidy::bugprone { + +/// Check function visibility changes in subclasses. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/function-visibility-change.html +class FunctionVisibilityChangeCheck : public ClangTidyCheck { +public: + FunctionVisibilityChangeCheck(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; + } +}; + +} // namespace clang::tidy::bugprone + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_FUNCTIONVISIBILITYCHANGECHECK_H diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 579fca54924d5..52f61a78fa3b7 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -124,6 +124,11 @@ New checks pointer and store it as class members without handle the copy and move constructors and the assignments. +- New :doc:`bugprone-function-visibility-change + <clang-tidy/checks/bugprone/function-visibility-change>` check. + + Check function visibility changes in subclasses. + - New :doc:`bugprone-unintended-char-ostream-output <clang-tidy/checks/bugprone/unintended-char-ostream-output>` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/function-visibility-change.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/function-visibility-change.rst new file mode 100644 index 0000000000000..3b7940e5e584e --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/function-visibility-change.rst @@ -0,0 +1,43 @@ +.. title:: clang-tidy - bugprone-function-visibility-change + +bugprone-function-visibility-change +=================================== + +Check changes in visibility of C++ member functions in subclasses. The check +detects if a virtual function is overridden with a different visibility than in +the base class declaration. Only normal functions are detected, no constructors, +operators, conversions or other special functions. + +.. code-block:: c++ + + class A { + public: + virtual void f_pub(); + private: + virtual void f_priv(); + }; + + class B: public A { + public: + void f_priv(); // warning: changed visibility from private to public + private: + void f_pub(); // warning: changed visibility from public to private + }; + + class C: private A { + // no warning: f_pub becomes private in this case but this is from the + // private inheritance + }; + + class D: private A { + public: + void f_pub(); // warning: changed visibility from private to public + // 'f_pub' would have private access but is forced to be + // public + }; + +The changed visibility can be an indicator of bad design or a result of +coding error or code changes. If it is intentional, it can be avoided by +adding an additional virtual function with the new access. + + diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index 18f1467285fab..59653a2059e64 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -98,6 +98,7 @@ Clang-Tidy Checks :doc:`bugprone-fold-init-type <bugprone/fold-init-type>`, :doc:`bugprone-forward-declaration-namespace <bugprone/forward-declaration-namespace>`, :doc:`bugprone-forwarding-reference-overload <bugprone/forwarding-reference-overload>`, + :doc:`bugprone-function-visibility-change <bugprone/function-visibility-change>`, "Yes" :doc:`bugprone-implicit-widening-of-multiplication-result <bugprone/implicit-widening-of-multiplication-result>`, "Yes" :doc:`bugprone-inaccurate-erase <bugprone/inaccurate-erase>`, "Yes" :doc:`bugprone-inc-dec-in-conditions <bugprone/inc-dec-in-conditions>`, diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/function-visibility-change.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/function-visibility-change.cpp new file mode 100644 index 0000000000000..eb4532374267b --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/function-visibility-change.cpp @@ -0,0 +1,234 @@ +// RUN: %check_clang_tidy %s bugprone-function-visibility-change %t + +class A { +public: + virtual void pub_foo1() {} + virtual void pub_foo2() {} + virtual void pub_foo3() {} +protected: + virtual void prot_foo1(); + virtual void prot_foo2(); + virtual void prot_foo3(); +private: + virtual void priv_foo1() {} + virtual void priv_foo2() {} + virtual void priv_foo3() {} +}; + +void A::prot_foo1() {} +void A::prot_foo2() {} +void A::prot_foo3() {} + +namespace test1 { + +class B: public A { +public: + void pub_foo1() override {} + void prot_foo1() override {} + // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: visibility of function 'prot_foo1' is changed from protected in class 'A' to public [bugprone-function-visibility-change] + // CHECK-MESSAGES: :9:16: note: function declared here as protected + void priv_foo1() override {} + // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: visibility of function 'priv_foo1' is changed from private in class 'A' to public [bugprone-function-visibility-change] + // CHECK-MESSAGES: :13:16: note: function declared here as private +protected: + void pub_foo2() override {} + // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: visibility of function 'pub_foo2' is changed from public in class 'A' to protected [bugprone-function-visibility-change] + // CHECK-MESSAGES: :6:16: note: function declared here as public + void prot_foo2() override {} + void priv_foo2() override {} + // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: visibility of function 'priv_foo2' is changed from private in class 'A' to protected [bugprone-function-visibility-change] + // CHECK-MESSAGES: :14:16: note: function declared here as private +private: + void pub_foo3() override {} + // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: visibility of function 'pub_foo3' is changed from public in class 'A' to private [bugprone-function-visibility-change] + // CHECK-MESSAGES: :7:16: note: function declared here as public + void prot_foo3() override {} + // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: visibility of function 'prot_foo3' is changed from protected in class 'A' to private [bugprone-function-visibility-change] + // CHECK-MESSAGES: :11:16: note: function declared here as protected + void priv_foo3() override {} +}; + +class C: public B { +public: + void pub_foo1() override; +protected: + void prot_foo1() override; + // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: visibility of function 'prot_foo1' is changed from public in class 'B' to protected [bugprone-function-visibility-change] + // CHECK-MESSAGES: :27:8: note: function declared here as public +private: + void priv_foo1() override; + // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: visibility of function 'priv_foo1' is changed from public in class 'B' to private [bugprone-function-visibility-change] + // CHECK-MESSAGES: :30:8: note: function declared here as public +}; + +void C::prot_foo1() {} +void C::priv_foo1() {} + +} + +namespace test2 { + +class B: public A { +public: + void pub_foo1() override; +protected: + void prot_foo1() override; +private: + void priv_foo1() override; +}; + +class C: public B { +public: + void pub_foo1() override; + void prot_foo1() override; + // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: visibility of function 'prot_foo1' is changed from protected in class 'B' to public + // CHECK-MESSAGES: :75:8: note: function declared here as protected + void priv_foo1() override; + // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: visibility of function 'priv_foo1' is changed from private in class 'B' to public + // CHECK-MESSAGES: :77:8: note: function declared here as private + + void pub_foo2() override; + void prot_foo2() override; + // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: visibility of function 'prot_foo2' is changed from protected in class 'A' to public + // CHECK-MESSAGES: :10:16: note: function declared here as protected + void priv_foo2() override; + // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: visibility of function 'priv_foo2' is changed from private in class 'A' to public + // CHECK-MESSAGES: :14:16: note: function declared here as private +}; + +} + +namespace test3 { + +class B: private A { +public: + void pub_foo1() override; + // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: visibility of function 'pub_foo1' is changed from private (through private inheritance of class 'A') to public + // CHECK-MESSAGES: :103:10: note: this inheritance would make 'pub_foo1' private + // CHECK-MESSAGES: :5:16: note: function declared here as public +protected: + void prot_foo1() override; + // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: visibility of function 'prot_foo1' is changed from private (through private inheritance of class 'A') to protected + // CHECK-MESSAGES: :103:10: note: this inheritance would make 'prot_foo1' private + // CHECK-MESSAGES: :9:16: note: function declared here as protected +private: + void priv_foo1() override; + +public: + void prot_foo2() override; + // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: visibility of function 'prot_foo2' is changed from private (through private inheritance of class 'A') to public + // CHECK-MESSAGES: :103:10: note: this inheritance would make 'prot_foo2' private + // CHECK-MESSAGES: :10:16: note: function declared here as protected + void priv_foo2() override; + // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: visibility of function 'priv_foo2' is changed from private in class 'A' to public + // CHECK-MESSAGES: :14:16: note: function declared here as private + +private: + void pub_foo3() override; + void prot_foo3() override; +}; + +class C: private A { +}; + +class D: public C { +public: + void pub_foo1() override; + // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: visibility of function 'pub_foo1' is changed from private (through private inheritance of class 'A') to public + // CHECK-MESSAGES: :131:10: note: this inheritance would make 'pub_foo1' private + // CHECK-MESSAGES: :5:16: note: function declared here as public +}; + + +} + +namespace test4 { + +struct Base1 { +public: + virtual void foo1(); +private: + virtual void foo2(); +}; + +struct Base2 { +public: + virtual void foo2(); +private: + virtual void foo1(); +}; + +struct A : public Base1, public Base2 { +protected: + void foo1() override; + // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: visibility of function 'foo1' is changed from private in class 'Base2' to protected + // CHECK-MESSAGES: :158:16: note: function declared here as private + // CHECK-MESSAGES: :[[@LINE-3]]:8: warning: visibility of function 'foo1' is changed from public in class 'Base1' to protected + // CHECK-MESSAGES: :149:16: note: function declared here as public +private: + void foo2() override; + // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: visibility of function 'foo2' is changed from public in class 'Base2' to private + // CHECK-MESSAGES: :156:16: note: function declared here as public +}; + +} + +namespace test5 { + +struct B1: virtual public A {}; +struct B2: virtual private A {}; +struct B: public B1, public B2 { +public: + void pub_foo1() override; + // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: visibility of function 'pub_foo1' is changed from private (through private inheritance of class 'A') to public + // CHECK-MESSAGES: :179:12: note: this inheritance would make 'pub_foo1' private + // CHECK-MESSAGES: :5:16: note: function declared here as public +}; + +} + +namespace test6 { +class A { +private: + A(int); +}; +class B: public A { +public: + using A::A; +}; +} + +namespace test7 { + +template <typename T> +class A { +protected: + virtual T foo(); +}; + +template <typename T> +class B: public A<T> { +private: + T foo() override; + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: visibility of function 'foo' is changed from protected in class 'A<int>' to private + // CHECK-MESSAGES: :206:13: note: function declared here as protected +}; + +template <typename T> +class C: private A<T> { +public: + T foo() override; + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: visibility of function 'foo' is changed from private (through private inheritance of class 'A<int>') to public + // CHECK-MESSAGES: :218:10: note: this inheritance would make 'foo' private + // CHECK-MESSAGES: :206:13: note: function declared here as protected +}; + +B<int> fB() { + return B<int>{}; +} + +C<int> fC() { + return C<int>{}; +} + +} `````````` </details> https://github.com/llvm/llvm-project/pull/140086 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
