[clang-tools-extra] [clang-tidy] Create bugprone-public-enable-shared-from-this check (PR #102299)

via cfe-commits cfe-commits at lists.llvm.org
Wed Aug 7 03:21:46 PDT 2024


https://github.com/MichelleCDjunaidi created https://github.com/llvm/llvm-project/pull/102299

This checks that classes/structs inheriting from ``std::enable_shared_from_this`` derive it with the ``public`` access specifier, so it prevents crashes due to ``std::make_shared`` and ``shared_from_this()`` getting called on a non-public class/struct. 

>From 75306bd83eb43d0606630f9f059fc04ad1b20b06 Mon Sep 17 00:00:00 2001
From: Michelle C Djunaidi <michellechrisalyn at gmail.com>
Date: Wed, 7 Aug 2024 13:10:02 +0800
Subject: [PATCH 1/2] [clang-tidy] Add bugprone-public-enable-shared-from-this
 check

This check identifies classes deriving from std::enable_shared_from_this that does not inherit with the public keyword,
which may cause problems when std::make_shared is called for that class.
---
 .../bugprone/BugproneTidyModule.cpp           |  3 +
 .../clang-tidy/bugprone/CMakeLists.txt        |  1 +
 .../PublicEnableSharedFromThisCheck.cpp       | 45 +++++++++++++++
 .../PublicEnableSharedFromThisCheck.h         | 33 +++++++++++
 clang-tools-extra/docs/ReleaseNotes.rst       |  5 ++
 .../bugprone/public-enable-shared-from-this   |  6 ++
 .../docs/clang-tidy/checks/list.rst           |  1 +
 .../public-enable-shared-from-this.cpp        | 56 +++++++++++++++++++
 8 files changed, 150 insertions(+)
 create mode 100644 clang-tools-extra/clang-tidy/bugprone/PublicEnableSharedFromThisCheck.cpp
 create mode 100644 clang-tools-extra/clang-tidy/bugprone/PublicEnableSharedFromThisCheck.h
 create mode 100644 clang-tools-extra/docs/clang-tidy/checks/bugprone/public-enable-shared-from-this
 create mode 100644 clang-tools-extra/test/clang-tidy/checkers/bugprone/public-enable-shared-from-this.cpp

diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
index 689eb92a3d8d17..f12f0cd1c47da3 100644
--- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
@@ -53,6 +53,7 @@
 #include "ParentVirtualCallCheck.h"
 #include "PointerArithmeticOnPolymorphicObjectCheck.h"
 #include "PosixReturnCheck.h"
+#include "PublicEnableSharedFromThisCheck.h"
 #include "RedundantBranchConditionCheck.h"
 #include "ReservedIdentifierCheck.h"
 #include "ReturnConstRefFromParameterCheck.h"
@@ -139,6 +140,8 @@ class BugproneModule : public ClangTidyModule {
         "bugprone-inaccurate-erase");
     CheckFactories.registerCheck<IncorrectEnableIfCheck>(
         "bugprone-incorrect-enable-if");
+    CheckFactories.registerCheck<PublicEnableSharedFromThisCheck>(
+        "bugprone-public-enable-shared-from-this");
     CheckFactories.registerCheck<ReturnConstRefFromParameterCheck>(
         "bugprone-return-const-ref-from-parameter");
     CheckFactories.registerCheck<SwitchMissingDefaultCaseCheck>(
diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
index cb0d8ae98bac58..c9bea094241edc 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
   ImplicitWideningOfMultiplicationResultCheck.cpp
   InaccurateEraseCheck.cpp
   IncorrectEnableIfCheck.cpp
+  PublicEnableSharedFromThisCheck.cpp
   ReturnConstRefFromParameterCheck.cpp
   SuspiciousStringviewDataUsageCheck.cpp
   SwitchMissingDefaultCaseCheck.cpp
diff --git a/clang-tools-extra/clang-tidy/bugprone/PublicEnableSharedFromThisCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/PublicEnableSharedFromThisCheck.cpp
new file mode 100644
index 00000000000000..dab3e0ac596fe7
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/bugprone/PublicEnableSharedFromThisCheck.cpp
@@ -0,0 +1,45 @@
+//===--- PublicEnableSharedFromThisCheck.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 "PublicEnableSharedFromThisCheck.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::bugprone {
+
+  void PublicEnableSharedFromThisCheck::registerMatchers(MatchFinder *match_finder) {
+      match_finder->addMatcher(
+              cxxRecordDecl(
+                  hasAnyBase(
+                      cxxBaseSpecifier(unless(isPublic()), 
+                      hasType(
+                          cxxRecordDecl(
+                              hasName("::std::enable_shared_from_this"))))
+                      )
+                  )
+              .bind("not-public-enable-shared"), this);
+  }
+
+  void PublicEnableSharedFromThisCheck::check(const MatchFinder::MatchResult &result) {
+      const auto *EnableSharedClassDecl =
+          result.Nodes.getNodeAs<CXXRecordDecl>("not-public-enable-shared");
+
+      for (const auto &Base : EnableSharedClassDecl->bases()) {
+          const auto *BaseType = Base.getType()->getAsCXXRecordDecl();
+          if (BaseType && BaseType->getQualifiedNameAsString() == "std::enable_shared_from_this") {
+              SourceLocation InsertLoc = Base.getBeginLoc();
+              FixItHint Hint = FixItHint::CreateInsertion(InsertLoc, "public ");
+              diag(EnableSharedClassDecl->getLocation(), "class %0 is not public even though it's derived from std::enable_shared_from_this", DiagnosticIDs::Warning)
+              << EnableSharedClassDecl->getNameAsString()
+              << Hint;
+              break;
+          }
+      }
+  }
+} // namespace clang::tidy::bugprone
diff --git a/clang-tools-extra/clang-tidy/bugprone/PublicEnableSharedFromThisCheck.h b/clang-tools-extra/clang-tidy/bugprone/PublicEnableSharedFromThisCheck.h
new file mode 100644
index 00000000000000..8a4013c3a46b7f
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/bugprone/PublicEnableSharedFromThisCheck.h
@@ -0,0 +1,33 @@
+//===--- PublicEnableSharedFromThisCheck.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_PUBLICENABLESHAREDFROMTHISCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_PUBLICENABLESHAREDFROMTHISCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang::tidy::bugprone {
+
+/// Checks if a class deriving from enable_shared_from_this has public access specifiers, because otherwise the program will crash when shared_from_this is called.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/public-enable-shared-from-this.html
+class PublicEnableSharedFromThisCheck : public ClangTidyCheck {
+public:
+  PublicEnableSharedFromThisCheck(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_PUBLICENABLESHAREDFROMTHISCHECK_H
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 642ad39cc0c1c5..67f6b1275a1adf 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -98,6 +98,11 @@ Improvements to clang-tidy
 New checks
 ^^^^^^^^^^
 
+- New :doc:`bugprone-public-enable-shared-from-this
+  <clang-tidy/checks/bugprone/public-enable-shared-from-this>` check.
+
+  Checks if a class deriving from enable_shared_from_this has public access specifiers, because otherwise the program will crash when shared_from_this is called.
+
 New check aliases
 ^^^^^^^^^^^^^^^^^
 
diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/public-enable-shared-from-this b/clang-tools-extra/docs/clang-tidy/checks/bugprone/public-enable-shared-from-this
new file mode 100644
index 00000000000000..d3f500eaa51460
--- /dev/null
+++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/public-enable-shared-from-this
@@ -0,0 +1,6 @@
+.. title:: clang-tidy - bugprone-public-enable-shared-from-this
+
+bugprone-public-enable-shared-from-this
+=======================================
+
+FIXME: Describe what patterns does the check detect and why. Give examples.
diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst
index a931ebf025a10e..2b918de89f7c19 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -120,6 +120,7 @@ Clang-Tidy Checks
    :doc:`bugprone-parent-virtual-call <bugprone/parent-virtual-call>`, "Yes"
    :doc:`bugprone-pointer-arithmetic-on-polymorphic-object <bugprone/pointer-arithmetic-on-polymorphic-object>`,
    :doc:`bugprone-posix-return <bugprone/posix-return>`, "Yes"
+   :doc:`bugprone-public-enable-shared-from-this <bugprone/public-enable-shared-from-this>`, "Yes"
    :doc:`bugprone-redundant-branch-condition <bugprone/redundant-branch-condition>`, "Yes"
    :doc:`bugprone-reserved-identifier <bugprone/reserved-identifier>`, "Yes"
    :doc:`bugprone-return-const-ref-from-parameter <bugprone/return-const-ref-from-parameter>`,
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/public-enable-shared-from-this.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/public-enable-shared-from-this.cpp
new file mode 100644
index 00000000000000..a3e6d0ee6ca952
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/public-enable-shared-from-this.cpp
@@ -0,0 +1,56 @@
+// RUN: %check_clang_tidy %s bugprone-public-enable-shared-from-this %t -- -- -I %S/Inputs/
+#include <memory>
+
+class BadExample : std::enable_shared_from_this<BadExample> {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: class BadExample is not public even though it's derived from std::enable_shared_from_this [bugprone-public-enable-shared-from-this]
+// CHECK-FIXES: :[[@LINE-2]]:19 public 
+        public:
+        BadExample* foo() { return shared_from_this().get(); }
+        void bar() { return; }
+};
+
+void using_not_public() {
+        auto bad_example = std::make_shared<BadExample>();
+        auto* b_ex = bad_example->foo();
+        b_ex->bar();
+}
+
+class GoodExample : public std::enable_shared_from_this<GoodExample> {
+        public:
+        GoodExample* foo() { return shared_from_this().get(); }
+        void bar() { return; }
+};
+
+void using_public() {
+        auto good_example = std::make_shared<GoodExample>();
+        auto* g_ex = good_example->foo();
+        g_ex->bar();
+}
+
+struct BaseClass {
+
+    void print() {
+        (void) State;
+        (void) Requester;
+    }
+    bool State;
+    int Requester;
+};
+
+class InheritPrivateBaseClass : BaseClass {
+    public:
+        void additionalFunction() {
+            (void) ID;
+        }
+    private: 
+        int ID;
+};
+
+class InheritPublicBaseClass : public BaseClass {
+    public:
+        void additionalFunction() {
+            (void) ID;
+        }
+    private: 
+        int ID;
+};
\ No newline at end of file

>From 4461045503c3efc2170da750911c20d83afdb0f1 Mon Sep 17 00:00:00 2001
From: Michelle C Djunaidi <michellechrisalyn at gmail.com>
Date: Wed, 7 Aug 2024 17:58:42 +0800
Subject: [PATCH 2/2] Fix test to be compatible with llvm-lit, add docs

---
 .../bugprone/public-enable-shared-from-this   |  6 --
 .../public-enable-shared-from-this.rst        | 26 ++++++
 .../public-enable-shared-from-this.cpp        | 93 ++++++++-----------
 3 files changed, 67 insertions(+), 58 deletions(-)
 delete mode 100644 clang-tools-extra/docs/clang-tidy/checks/bugprone/public-enable-shared-from-this
 create mode 100644 clang-tools-extra/docs/clang-tidy/checks/bugprone/public-enable-shared-from-this.rst

diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/public-enable-shared-from-this b/clang-tools-extra/docs/clang-tidy/checks/bugprone/public-enable-shared-from-this
deleted file mode 100644
index d3f500eaa51460..00000000000000
--- a/clang-tools-extra/docs/clang-tidy/checks/bugprone/public-enable-shared-from-this
+++ /dev/null
@@ -1,6 +0,0 @@
-.. title:: clang-tidy - bugprone-public-enable-shared-from-this
-
-bugprone-public-enable-shared-from-this
-=======================================
-
-FIXME: Describe what patterns does the check detect and why. Give examples.
diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/public-enable-shared-from-this.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/public-enable-shared-from-this.rst
new file mode 100644
index 00000000000000..616b2b52271e3f
--- /dev/null
+++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/public-enable-shared-from-this.rst
@@ -0,0 +1,26 @@
+.. title:: clang-tidy - bugprone-public-enable-shared-from-this
+
+bugprone-public-enable-shared-from-this
+=======================================
+
+Checks that classes/structs inheriting from ``std::enable_shared_from_this`` derive it with the ``public`` access specifier. If not, then issue a FixItHint that can be applied.
+
+Consider the following code:
+.. code-block:: c++
+        #include <memory>
+
+        class BadExample : std::enable_shared_from_this<BadExample> {
+        // warning: class BadExample is not public even though it's derived from std::enable_shared_from_this [bugprone-public-enable-shared-from-this]
+        // will insert the public keyword if -fix is applied 
+                public:
+                BadExample* foo() { return shared_from_this().get(); }
+                void bar() { return; }
+        };
+
+        void using_not_public() {
+                auto bad_example = std::make_shared<BadExample>();
+                auto* b_ex = bad_example->foo();
+                b_ex->bar();
+        }
+
+When ``using_not_public()`` is called, this code will crash.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/public-enable-shared-from-this.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/public-enable-shared-from-this.cpp
index a3e6d0ee6ca952..b0954898ddb783 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/public-enable-shared-from-this.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/public-enable-shared-from-this.cpp
@@ -1,56 +1,45 @@
-// RUN: %check_clang_tidy %s bugprone-public-enable-shared-from-this %t -- -- -I %S/Inputs/
-#include <memory>
+// RUN: %check_clang_tidy %s bugprone-public-enable-shared-from-this %t -- --
 
-class BadExample : std::enable_shared_from_this<BadExample> {
-// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: class BadExample is not public even though it's derived from std::enable_shared_from_this [bugprone-public-enable-shared-from-this]
-// CHECK-FIXES: :[[@LINE-2]]:19 public 
-        public:
-        BadExample* foo() { return shared_from_this().get(); }
-        void bar() { return; }
-};
+namespace std {
 
-void using_not_public() {
-        auto bad_example = std::make_shared<BadExample>();
-        auto* b_ex = bad_example->foo();
-        b_ex->bar();
-}
+    template <typename T> class enable_shared_from_this {};
 
-class GoodExample : public std::enable_shared_from_this<GoodExample> {
-        public:
-        GoodExample* foo() { return shared_from_this().get(); }
-        void bar() { return; }
-};
-
-void using_public() {
-        auto good_example = std::make_shared<GoodExample>();
-        auto* g_ex = good_example->foo();
-        g_ex->bar();
-}
-
-struct BaseClass {
-
-    void print() {
-        (void) State;
-        (void) Requester;
-    }
-    bool State;
-    int Requester;
-};
-
-class InheritPrivateBaseClass : BaseClass {
-    public:
-        void additionalFunction() {
-            (void) ID;
-        }
-    private: 
-        int ID;
-};
-
-class InheritPublicBaseClass : public BaseClass {
-    public:
-        void additionalFunction() {
-            (void) ID;
+    class BadExample : enable_shared_from_this<BadExample> {};
+    // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: class BadExample is not public even though it's derived from std::enable_shared_from_this [bugprone-public-enable-shared-from-this]
+    // CHECK-FIXES: public enable_shared_from_this<BadExample>
+
+    class Bad2Example : std::enable_shared_from_this<Bad2Example> {};
+    // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: class Bad2Example is not public even though it's derived from std::enable_shared_from_this [bugprone-public-enable-shared-from-this]
+    // CHECK-FIXES: public std::enable_shared_from_this<Bad2Example>
+
+    class GoodExample : public enable_shared_from_this<GoodExample> {
+    };
+
+    struct BaseClass {
+
+        void print() {
+            (void) State;
+            (void) Requester;
         }
-    private: 
-        int ID;
-};
\ No newline at end of file
+        bool State;
+        int Requester;
+    };
+
+    class InheritPrivateBaseClass : BaseClass {
+        public:
+            void additionalFunction() {
+                (void) ID;
+            }
+        private: 
+            int ID;
+    };
+
+    class InheritPublicBaseClass : public BaseClass {
+        public:
+            void additionalFunction() {
+                (void) ID;
+            }
+        private: 
+            int ID;
+    };
+}
\ No newline at end of file



More information about the cfe-commits mailing list