[clang-tidy] Added check 'bugprone-function-visibility-change' #140086

balazske · 2025-05-15T16:01:26Z

No description provided.

llvmbot · 2025-05-15T16:01:57Z

@llvm/pr-subscribers-clang-tidy

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

Author: Balázs Kéri (balazske)

Changes

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 --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>{};
+}
+
+}

EugeneZelenko · 2025-05-16T14:20:11Z

clang-tools-extra/docs/ReleaseNotes.rst

+- New :doc:`bugprone-function-visibility-change
+  <clang-tidy/checks/bugprone/function-visibility-change>` check.
+
+  Check function visibility changes in subclasses.


Suggested change

Check function visibility changes in subclasses.

Checks function visibility changes in subclasses.

EugeneZelenko · 2025-05-16T14:20:26Z

clang-tools-extra/docs/clang-tidy/checks/bugprone/function-visibility-change.rst

+bugprone-function-visibility-change
+===================================
+
+Check changes in visibility of C++ member functions in subclasses. The check


Suggested change

Check changes in visibility of C++ member functions in subclasses. The check

Checks changes in visibility of C++ member functions in subclasses. The check

And avoid "The check", documentation is in scope of check.

EugeneZelenko · 2025-05-16T14:21:16Z

clang-tools-extra/docs/clang-tidy/checks/bugprone/function-visibility-change.rst

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


Excessive trailing newlines.

NagyDonat

LGTM if you implement the changes suggested by EugeneZelenko.

PiotrZSL

Idea is ok.

Missing some tests (scenarios not covered).
Maybe some options to deal with big number of false-positives would be needed.
Maybe some option to allow changing visibility to less visible would be needed.

PiotrZSL · 2025-05-17T12:19:36Z

clang-tools-extra/clang-tidy/bugprone/FunctionVisibilityChangeCheck.cpp

+    if (InheritanceWithStrictVisibility) {
+      diag(MatchedFunction->getLocation(),
+           "visibility of function %0 is changed from %1 (through %1 "
+           "inheritance of class %2) to %3")


consider adding "to %3 in class %4"

Will this not make the message too long? (The message is already located in the class %4.)

PiotrZSL · 2025-05-17T12:20:33Z

clang-tools-extra/clang-tidy/bugprone/FunctionVisibilityChangeCheck.cpp

+    }
+  }
+
+  if (ActualAccess != OverriddenAccess) {


Consider allowing changing access from wider to narrower, maybe some config option for that.

Option is added now to set the type of change.

PiotrZSL · 2025-05-17T12:20:56Z

clang-tools-extra/clang-tidy/bugprone/FunctionVisibilityChangeCheck.cpp

+    return;
+  const CXXBaseSpecifier *InheritanceWithStrictVisibility = nullptr;
+  for (const CXXBasePath &Path : Paths) {
+    for (auto Elem : Path) {


Suggested change

for (auto Elem : Path) {

for (const auto& Elem : Elem : Path) {

We should favor explicit type over auto

PiotrZSL · 2025-05-17T12:22:16Z

clang-tools-extra/clang-tidy/bugprone/FunctionVisibilityChangeCheck.cpp

+
+void FunctionVisibilityChangeCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      cxxMethodDecl(


consider excluding system code and add "isVirtual()"

fixed (inheritance from system code in non-system code is detected now)

PiotrZSL · 2025-05-17T12:23:33Z

clang-tools-extra/clang-tidy/bugprone/FunctionVisibilityChangeCheck.cpp

+          << MatchedFunction << OverriddenAccess
+          << InheritanceWithStrictVisibility->getType() << ActualAccess;
+      diag(InheritanceWithStrictVisibility->getBeginLoc(),
+           "this inheritance would make %0 %1", DiagnosticIDs::Note)


maybe better "%0 is inherited as %1 here".
I do not like that "this" at the begining

PiotrZSL · 2025-05-17T12:27:24Z

clang-tools-extra/docs/clang-tidy/checks/bugprone/function-visibility-change.rst

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


Source code of the check does not verify against operators, and destructors.
Probably tests are missing.

PiotrZSL · 2025-05-17T12:29:14Z

clang-tools-extra/docs/clang-tidy/checks/bugprone/function-visibility-change.rst

+                  // 'f_pub' would have private access but is forced to be
+                  // public
+  };
+


what about when user changes visibility of the function with "using declaration" ?

PiotrZSL · 2025-05-17T12:30:12Z

clang-tools-extra/docs/clang-tidy/checks/bugprone/function-visibility-change.rst

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


Proxy functions are out of date, in current days "using declaration" can be used for that.

PiotrZSL · 2025-05-17T12:30:29Z

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"


check does not provide fixes, remove "Yes"

PiotrZSL · 2025-05-17T12:30:59Z

clang-tools-extra/test/clang-tidy/checkers/bugprone/function-visibility-change.cpp

+C<int> fC() {
+  return C<int>{};
+}
+


missing tests with: destructor, operators, using declaration

vbvictor

I think we should make change the name of the check to include "method" or "member-function" words in it. It will make clear that we check for methods visibility and not some visibility attributes on functions.
Possible names:
bugprone-member-function-visibility-change
bugprone-virtual-member-function-visibility-change
bugprone-visibility-change-to-virtual-member-function (imho the best)

vbvictor · 2025-05-17T14:44:33Z

clang-tools-extra/clang-tidy/bugprone/FunctionVisibilityChangeCheck.cpp

+  if (!MatchedFunction->isCanonicalDecl())
+    return;


nit: Consider moving this lines before const auto *ParentClass declaration as not to declare unused variables

EugeneZelenko · 2025-05-19T16:17:40Z

clang-tools-extra/docs/clang-tidy/checks/bugprone/function-visibility-change.rst

+and declared as `public` in a subclass. The detected change is the modification
+of visibility resulting from keywords `public`, `protected`, `private` at
+overridden virtual functions. Use of the `using` keyword is not considered by


Suggested change

and declared as `public` in a subclass. The detected change is the modification

of visibility resulting from keywords `public`, `protected`, `private` at

overridden virtual functions. Use of the `using` keyword is not considered by

and declared as ``public`` in a subclass. The detected change is the modification

of visibility resulting from keywords ``public``, ``protected``, ``private`` at

overridden virtual functions. Use of the ``using`` keyword is not considered by

EugeneZelenko · 2025-05-19T16:18:25Z

clang-tools-extra/test/clang-tidy/checkers/bugprone/function-visibility-change.cpp

+  // CHECK-MESSAGES: :[[@LINE-13]]:11: note: function declared here
+};
+
+}


Please add newline.

balazske · 2025-05-19T16:22:00Z

The current code is not finished. I want to check for false positives and probably add options to disable check at destructors and operators.

balazske · 2025-05-21T11:00:17Z

I have renamed the check, if OK the class name will be changed too (in a new commit).

EugeneZelenko · 2025-05-21T14:32:14Z

clang-tools-extra/docs/clang-tidy/checks/bugprone/visibility-change-to-virtual-function.rst

+  member functions) too. This includes other special member functions (like
+  conversions) too. This option is probably useful only in rare cases because
+  operators and conversions are not often virtual functions.
+  Default value is false.


Suggested change

Default value is false.

Default value is `false`.

[clang-tidy] Added check 'bugprone-function-visibility-change'

65d44a4

llvmbot added clang-tools-extra clang-tidy labels May 15, 2025

EugeneZelenko reviewed May 16, 2025

View reviewed changes

EugeneZelenko requested review from PiotrZSL, carlosgalvezp and HerrCai0907 May 16, 2025 14:22

balazske requested a review from NagyDonat May 16, 2025 16:00

NagyDonat approved these changes May 17, 2025

View reviewed changes

PiotrZSL requested changes May 17, 2025

View reviewed changes

vbvictor reviewed May 17, 2025

View reviewed changes

code and doc fixes, added option

f19ab2e

EugeneZelenko reviewed May 19, 2025

View reviewed changes

improvements, added options, check rename

8f1b20e

EugeneZelenko reviewed May 21, 2025

View reviewed changes

	Check function visibility changes in subclasses.
	Checks function visibility changes in subclasses.

	Check changes in visibility of C++ member functions in subclasses. The check
	Checks changes in visibility of C++ member functions in subclasses. The check

	for (auto Elem : Path) {
	for (const auto& Elem : Elem : Path) {

[clang-tidy] Added check 'bugprone-function-visibility-change' #140086

Are you sure you want to change the base?

[clang-tidy] Added check 'bugprone-function-visibility-change' #140086

Uh oh!

Conversation

balazske commented May 15, 2025

Uh oh!

llvmbot commented May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NagyDonat left a comment

Choose a reason for hiding this comment

Uh oh!

PiotrZSL left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vbvictor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

balazske commented May 19, 2025

Uh oh!

balazske commented May 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

llvmbot commented May 15, 2025 •

edited

Loading