[clang-tidy] Create bugprone-incorrect-enable-shared-from-this check (#…

…102299)

This checks that classes/structs inheriting from
``std::enable_shared_from_this`` does so with public inheritance, so it
prevents crashes due to ``std::make_shared`` and ``shared_from_this()``
getting called when the internal weak pointer was not initialized (e.g.
due to private inheritance).

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

@@ -33,6 +33,7 @@
 #include "InaccurateEraseCheck.h"
 #include "IncDecInConditionsCheck.h"
 #include "IncorrectEnableIfCheck.h"
+#include "IncorrectEnableSharedFromThisCheck.h"
 #include "IncorrectRoundingsCheck.h"
 #include "InfiniteLoopCheck.h"
 #include "IntegerDivisionCheck.h"
@@ -144,6 +145,8 @@ class BugproneModule : public ClangTidyModule {
         "bugprone-inaccurate-erase");
     CheckFactories.registerCheck<IncorrectEnableIfCheck>(
         "bugprone-incorrect-enable-if");
+    CheckFactories.registerCheck<IncorrectEnableSharedFromThisCheck>(
+        "bugprone-incorrect-enable-shared-from-this");
     CheckFactories.registerCheck<ReturnConstRefFromParameterCheck>(
         "bugprone-return-const-ref-from-parameter");
     CheckFactories.registerCheck<SwitchMissingDefaultCaseCheck>(

clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt

@@ -27,6 +27,11 @@ add_clang_library(clangTidyBugproneModule STATIC
   ForwardingReferenceOverloadCheck.cpp
   ImplicitWideningOfMultiplicationResultCheck.cpp
   InaccurateEraseCheck.cpp
+  IncorrectEnableIfCheck.cpp
+  IncorrectEnableSharedFromThisCheck.cpp
+  ReturnConstRefFromParameterCheck.cpp
+  SuspiciousStringviewDataUsageCheck.cpp
+  SwitchMissingDefaultCaseCheck.cpp
   IncDecInConditionsCheck.cpp
   IncorrectEnableIfCheck.cpp
   IncorrectRoundingsCheck.cpp

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

@@ -0,0 +1,65 @@
+//===--- IncorrectEnableSharedFromThisCheck.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 "IncorrectEnableSharedFromThisCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::bugprone {
+
+void IncorrectEnableSharedFromThisCheck::registerMatchers(MatchFinder *Finder) {
+  const auto EnableSharedFromThis =
+      cxxRecordDecl(hasName("enable_shared_from_this"), isInStdNamespace());
+  const auto QType = hasCanonicalType(hasDeclaration(
+      cxxRecordDecl(
+          anyOf(EnableSharedFromThis.bind("enable_rec"),
+                cxxRecordDecl(hasAnyBase(cxxBaseSpecifier(
+                    isPublic(), hasType(hasCanonicalType(
+                                    hasDeclaration(EnableSharedFromThis))))))))
+          .bind("base_rec")));
+  Finder->addMatcher(
+      cxxRecordDecl(
+          unless(isExpansionInSystemHeader()),
+          hasDirectBase(cxxBaseSpecifier(unless(isPublic()), hasType(QType))
+                            .bind("base")))
+          .bind("derived"),
+      this);
+}
+
+void IncorrectEnableSharedFromThisCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  const auto *BaseSpec = Result.Nodes.getNodeAs<CXXBaseSpecifier>("base");
+  const auto *Base = Result.Nodes.getNodeAs<CXXRecordDecl>("base_rec");
+  const auto *Derived = Result.Nodes.getNodeAs<CXXRecordDecl>("derived");
+  const bool IsEnableSharedFromThisDirectBase =
+      Result.Nodes.getNodeAs<CXXRecordDecl>("enable_rec") == Base;
+  const bool HasWrittenAccessSpecifier =
+      BaseSpec->getAccessSpecifierAsWritten() != AS_none;
+  const auto ReplacementRange = CharSourceRange(
+      SourceRange(BaseSpec->getBeginLoc()), HasWrittenAccessSpecifier);
+  const llvm::StringRef Replacement =
+      HasWrittenAccessSpecifier ? "public" : "public ";
+  const FixItHint Hint =
+      IsEnableSharedFromThisDirectBase
+          ? FixItHint::CreateReplacement(ReplacementRange, Replacement)
+          : FixItHint();
+  diag(Derived->getLocation(),
+       "%2 is not publicly inheriting from "
+       "%select{%1 which inherits from |}0'std::enable_shared_"
+       "from_this', "
+       "which will cause unintended behaviour "
+       "when using 'shared_from_this'; make the inheritance "
+       "public",
+       DiagnosticIDs::Warning)
+      << IsEnableSharedFromThisDirectBase << Base << Derived << Hint;
+}
+
+} // namespace clang::tidy::bugprone

clang-tools-extra/clang-tidy/bugprone/IncorrectEnableSharedFromThisCheck.h

@@ -0,0 +1,35 @@
+//===--- IncorrectEnableSharedFromThisCheck.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_INCORRECTENABLESHAREDFROMTHISCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_INCORRECTENABLESHAREDFROMTHISCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang::tidy::bugprone {
+
+/// Detect classes or structs that do not publicly inherit from
+/// ``std::enable_shared_from_this``, because unintended behavior will
+/// otherwise occur when calling ``shared_from_this``.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/incorrect-enable-shared-from-this.html
+class IncorrectEnableSharedFromThisCheck : public ClangTidyCheck {
+public:
+  IncorrectEnableSharedFromThisCheck(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.CPlusPlus11;
+  }
+};
+
+} // namespace clang::tidy::bugprone
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_INCORRECTENABLESHAREDFROMTHISCHECK_H

clang-tools-extra/docs/ReleaseNotes.rst

@@ -145,6 +145,13 @@ New checks
   Warns about code that tries to cast between pointers by means of
   ``std::bit_cast`` or ``memcpy``.
 
+- New :doc:`bugprone-incorrect-enable-shared-from-this
+  <clang-tidy/checks/bugprone/incorrect-enable-shared-from-this>` check.
+
+  Detect classes or structs that do not publicly inherit from 
+  ``std::enable_shared_from_this``, because unintended behavior will 
+  otherwise occur when calling ``shared_from_this``.
+
 - New :doc:`bugprone-nondeterministic-pointer-iteration-order
   <clang-tidy/checks/bugprone/nondeterministic-pointer-iteration-order>`
   check.

clang-tools-extra/docs/clang-tidy/checks/bugprone/incorrect-enable-shared-from-this.rst

@@ -0,0 +1,34 @@
+.. title:: clang-tidy - bugprone-incorrect-enable-shared-from-this
+
+bugprone-incorrect-enable-shared-from-this
+==========================================
+
+Detect classes or structs that do not publicly inherit from 
+``std::enable_shared_from_this``, because unintended behavior will 
+otherwise occur when calling ``shared_from_this``.
+
+Consider the following code:
+
+.. code-block:: c++
+
+    #include <memory>
+
+    // private inheritance
+    class BadExample : std::enable_shared_from_this<BadExample> {
+
+    // ``shared_from_this``` unintended behaviour
+    // `libstdc++` implementation returns uninitialized ``weak_ptr``
+        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();
+    }
+
+Using `libstdc++` implementation, ``shared_from_this`` will throw 
+``std::bad_weak_ptr``. When ``using_not_public()`` is called, this code will 
+crash without exception handling.

clang-tools-extra/docs/clang-tidy/checks/list.rst

@@ -101,6 +101,7 @@ Clang-Tidy Checks
    :doc:`bugprone-inaccurate-erase <bugprone/inaccurate-erase>`, "Yes"
    :doc:`bugprone-inc-dec-in-conditions <bugprone/inc-dec-in-conditions>`,
    :doc:`bugprone-incorrect-enable-if <bugprone/incorrect-enable-if>`, "Yes"
+   :doc:`bugprone-incorrect-enable-shared-from-this <bugprone/incorrect-enable-shared-from-this>`, "Yes"
    :doc:`bugprone-incorrect-roundings <bugprone/incorrect-roundings>`,
    :doc:`bugprone-infinite-loop <bugprone/infinite-loop>`,
    :doc:`bugprone-integer-division <bugprone/integer-division>`,