[clang-tools-extra] [clang-tidy] Added check 'bugprone-function-visibility-change' (PR #140086)
Balázs Kéri via cfe-commits
cfe-commits at lists.llvm.org
Thu May 15 09:01:34 PDT 2025
https://github.com/balazske created https://github.com/llvm/llvm-project/pull/140086
None
>From 65d44a4eb9621e49a96f1ac43e5a1bbd6691dc13 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?= <balazs.keri at ericsson.com>
Date: Thu, 15 May 2025 17:41:16 +0200
Subject: [PATCH] [clang-tidy] Added check
'bugprone-function-visibility-change'
---
.../bugprone/BugproneTidyModule.cpp | 3 +
.../clang-tidy/bugprone/CMakeLists.txt | 1 +
.../FunctionVisibilityChangeCheck.cpp | 74 ++++++
.../bugprone/FunctionVisibilityChangeCheck.h | 33 +++
clang-tools-extra/docs/ReleaseNotes.rst | 5 +
.../bugprone/function-visibility-change.rst | 43 ++++
.../docs/clang-tidy/checks/list.rst | 1 +
.../bugprone/function-visibility-change.cpp | 234 ++++++++++++++++++
8 files changed, 394 insertions(+)
create mode 100644 clang-tools-extra/clang-tidy/bugprone/FunctionVisibilityChangeCheck.cpp
create mode 100644 clang-tools-extra/clang-tidy/bugprone/FunctionVisibilityChangeCheck.h
create mode 100644 clang-tools-extra/docs/clang-tidy/checks/bugprone/function-visibility-change.rst
create mode 100644 clang-tools-extra/test/clang-tidy/checkers/bugprone/function-visibility-change.cpp
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>{};
+}
+
+}
More information about the cfe-commits
mailing list