[clang-tidy] CRTP Constructor Accessibility Check #82403
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
isuckatcs
requested review from
PiotrZSL,
njames93,
EugeneZelenko and
balazske
|
@llvm/pr-subscribers-clang-tidy Author: None (isuckatcs) ChangesFinds CRTP used in an error-prone way. If the constructor of a class intended to be used in a CRTP is public, then template <typename T> class CRTP {
public:
CRTP() = default;
};
class Good : CRTP<Good> {};
Good GoodInstance;
CRTP<int> BadInstance;If the constructor is protected, the possibility of an accidental instantiation template <typename T> class CRTP {
protected:
CRTP() = default;
};
class Good : CRTP<Good> {};
Good GoodInstance;
class Bad : CRTP<Good> {};
Bad BadInstance;To ensure that no accidental instantiation happens, the best practice is to make template <typename T> class CRTP {
CRTP() = default;
friend T;
};
class Good : CRTP<Good> {};
Good GoodInstance;
class Bad : CRTP<Good> {};
Bad CompileTimeError;
CRTP<int> AlsoCompileTimeError;Patch is 20.90 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/82403.diff 8 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
index 7a910037368c83..50b791ae10faf0 100644
--- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
@@ -82,6 +82,7 @@
#include "UnhandledExceptionAtNewCheck.h"
#include "UnhandledSelfAssignmentCheck.h"
#include "UniquePtrArrayMismatchCheck.h"
+#include "UnsafeCrtpCheck.h"
#include "UnsafeFunctionsCheck.h"
#include "UnusedRaiiCheck.h"
#include "UnusedReturnValueCheck.h"
@@ -233,6 +234,8 @@ class BugproneModule : public ClangTidyModule {
"bugprone-unhandled-exception-at-new");
CheckFactories.registerCheck<UniquePtrArrayMismatchCheck>(
"bugprone-unique-ptr-array-mismatch");
+ CheckFactories.registerCheck<UnsafeCrtpCheck>(
+ "bugprone-unsafe-crtp");
CheckFactories.registerCheck<UnsafeFunctionsCheck>(
"bugprone-unsafe-functions");
CheckFactories.registerCheck<UnusedRaiiCheck>("bugprone-unused-raii");
diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
index d443fd8d1452f1..572ff622b47473 100644
--- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
@@ -78,6 +78,7 @@ add_clang_library(clangTidyBugproneModule
UnhandledExceptionAtNewCheck.cpp
UnhandledSelfAssignmentCheck.cpp
UniquePtrArrayMismatchCheck.cpp
+ UnsafeCrtpCheck.cpp
UnsafeFunctionsCheck.cpp
UnusedRaiiCheck.cpp
UnusedReturnValueCheck.cpp
diff --git a/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
new file mode 100644
index 00000000000000..7bb50aaf950cbe
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
@@ -0,0 +1,167 @@
+//===--- UnsafeCrtpCheck.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 "UnsafeCrtpCheck.h"
+#include "../utils/LexerUtils.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::bugprone {
+
+namespace {
+// Finds a node if it's already a bound node.
+AST_MATCHER_P(CXXRecordDecl, isBoundNode, std::string, ID) {
+ return Builder->removeBindings(
+ [&](const ast_matchers::internal::BoundNodesMap &Nodes) {
+ const auto *BoundRecord = Nodes.getNodeAs<CXXRecordDecl>(ID);
+ return BoundRecord != &Node;
+ });
+}
+
+bool hasPrivateConstructor(const CXXRecordDecl *RD) {
+ for (auto &&Ctor : RD->ctors()) {
+ if (Ctor->getAccess() == AS_private)
+ return true;
+ }
+
+ return false;
+}
+
+bool isDerivedParameterBefriended(const CXXRecordDecl *CRTP,
+ const NamedDecl *Param) {
+ for (auto &&Friend : CRTP->friends()) {
+ const auto *TTPT =
+ dyn_cast<TemplateTypeParmType>(Friend->getFriendType()->getType());
+
+ if (TTPT && TTPT->getDecl() == Param)
+ return true;
+ }
+
+ return false;
+}
+
+bool isDerivedClassBefriended(const CXXRecordDecl *CRTP,
+ const CXXRecordDecl *Derived) {
+ for (auto &&Friend : CRTP->friends()) {
+ if (Friend->getFriendType()->getType()->getAsCXXRecordDecl() == Derived)
+ return true;
+ }
+
+ return false;
+}
+
+std::optional<const NamedDecl *>
+getDerivedParameter(const ClassTemplateSpecializationDecl *CRTP,
+ const CXXRecordDecl *Derived) {
+ size_t Idx = 0;
+ bool Found = false;
+ for (auto &&TemplateArg : CRTP->getTemplateArgs().asArray()) {
+ if (TemplateArg.getKind() == TemplateArgument::Type &&
+ TemplateArg.getAsType()->getAsCXXRecordDecl() == Derived) {
+ Found = true;
+ break;
+ }
+ ++Idx;
+ }
+
+ if (!Found)
+ return std::nullopt;
+
+ return CRTP->getSpecializedTemplate()->getTemplateParameters()->getParam(Idx);
+}
+
+std::vector<FixItHint> hintMakeCtorPrivate(const CXXConstructorDecl *Ctor,
+ const std::string &OriginalAccess,
+ const SourceManager &SM,
+ const LangOptions &LangOpts) {
+ std::vector<FixItHint> Hints;
+
+ Hints.emplace_back(FixItHint::CreateInsertion(
+ Ctor->getBeginLoc().getLocWithOffset(-1), "private:\n"));
+
+ SourceLocation CtorEndLoc =
+ Ctor->isExplicitlyDefaulted()
+ ? utils::lexer::findNextTerminator(Ctor->getEndLoc(), SM, LangOpts)
+ : Ctor->getEndLoc();
+ Hints.emplace_back(FixItHint::CreateInsertion(
+ CtorEndLoc.getLocWithOffset(1), '\n' + OriginalAccess + ':' + '\n'));
+
+ return Hints;
+}
+} // namespace
+
+void UnsafeCrtpCheck::registerMatchers(MatchFinder *Finder) {
+ Finder->addMatcher(
+ classTemplateSpecializationDecl(
+ decl().bind("crtp"),
+ hasAnyTemplateArgument(refersToType(recordType(hasDeclaration(
+ cxxRecordDecl(isDerivedFrom(cxxRecordDecl(isBoundNode("crtp"))))
+ .bind("derived")))))),
+ this);
+}
+
+void UnsafeCrtpCheck::check(const MatchFinder::MatchResult &Result) {
+ const auto *CRTPInstantiation =
+ Result.Nodes.getNodeAs<ClassTemplateSpecializationDecl>("crtp");
+ const auto *DerivedRecord = Result.Nodes.getNodeAs<CXXRecordDecl>("derived");
+ const CXXRecordDecl *CRTPDeclaration =
+ CRTPInstantiation->getSpecializedTemplate()->getTemplatedDecl();
+
+ if (!CRTPDeclaration->hasUserDeclaredConstructor()) {
+ bool IsStruct = CRTPDeclaration->isStruct();
+
+ diag(CRTPDeclaration->getLocation(),
+ "the implicit default constructor of the CRTP is publicly accessible")
+ << CRTPDeclaration
+ << FixItHint::CreateInsertion(
+ CRTPDeclaration->getBraceRange().getBegin().getLocWithOffset(1),
+ (IsStruct ? "\nprivate:\n" : "\n") +
+ CRTPDeclaration->getNameAsString() + "() = default;\n" +
+ (IsStruct ? "public:\n" : ""));
+ diag(CRTPDeclaration->getLocation(), "consider making it private",
+ DiagnosticIDs::Note);
+ }
+
+ const auto *DerivedTemplateParameter =
+ *getDerivedParameter(CRTPInstantiation, DerivedRecord);
+
+ if (hasPrivateConstructor(CRTPDeclaration) &&
+ !isDerivedParameterBefriended(CRTPDeclaration,
+ DerivedTemplateParameter) &&
+ !isDerivedClassBefriended(CRTPDeclaration, DerivedRecord)) {
+ diag(CRTPDeclaration->getLocation(),
+ "the CRTP cannot be constructed from the derived class")
+ << CRTPDeclaration
+ << FixItHint::CreateInsertion(
+ CRTPDeclaration->getBraceRange().getEnd(),
+ "friend " + DerivedTemplateParameter->getNameAsString() + ';' +
+ '\n');
+ diag(CRTPDeclaration->getLocation(),
+ "consider declaring the derived class as friend", DiagnosticIDs::Note);
+ }
+
+ for (auto &&Ctor : CRTPDeclaration->ctors()) {
+ if (Ctor->getAccess() == AS_private)
+ continue;
+
+ bool IsPublic = Ctor->getAccess() == AS_public;
+ std::string Access = IsPublic ? "public" : "protected";
+
+ diag(Ctor->getLocation(),
+ "%0 contructor allows the CRTP to be %select{inherited "
+ "from|constructed}1 as a regular template class")
+ << Access << IsPublic << Ctor
+ << hintMakeCtorPrivate(Ctor, Access, *Result.SourceManager,
+ getLangOpts());
+ diag(Ctor->getLocation(), "consider making it private",
+ DiagnosticIDs::Note);
+ }
+}
+
+} // namespace clang::tidy::bugprone
diff --git a/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.h b/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.h
new file mode 100644
index 00000000000000..ac1a8f09208f95
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.h
@@ -0,0 +1,30 @@
+//===--- UnsafeCrtpCheck.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_UNSAFECRTPCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_UNSAFECRTPCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang::tidy::bugprone {
+
+/// Finds CRTP used in an error-prone way.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/unsafe-crtp.html
+class UnsafeCrtpCheck : public ClangTidyCheck {
+public:
+ UnsafeCrtpCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context) {}
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace clang::tidy::bugprone
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_UNSAFECRTPCHECK_H
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index b5348384e965ab..1506e4631806a4 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -167,6 +167,11 @@ New checks
extracted from an optional-like type and then used to create a new instance
of the same optional-like type.
+- New :doc:`bugprone-unsafe-crtp
+ <clang-tidy/checks/bugprone/unsafe-crtp>` check.
+
+ Detects error-prone CRTP usage.
+
- New :doc:`cppcoreguidelines-no-suspend-with-lock
<clang-tidy/checks/cppcoreguidelines/no-suspend-with-lock>` check.
diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/unsafe-crtp.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/unsafe-crtp.rst
new file mode 100644
index 00000000000000..8e6955999512a6
--- /dev/null
+++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/unsafe-crtp.rst
@@ -0,0 +1,62 @@
+.. title:: clang-tidy - bugprone-unsafe-crtp
+
+bugprone-unsafe-crtp
+====================
+
+Finds CRTP used in an error-prone way.
+
+If the constructor of a class intended to be used in a CRTP is public, then
+it allows users to construct that class on its own.
+
+Example:
+
+.. code-block:: c++
+
+ template <typename T> class CRTP {
+ public:
+ CRTP() = default;
+ };
+
+ class Good : CRTP<Good> {};
+ Good GoodInstance;
+
+ CRTP<int> BadInstance;
+
+If the constructor is protected, the possibility of an accidental instantiation
+is prevented, however it can fade an error, when a different class is used as
+the template parameter instead of the derived one.
+
+Example:
+
+.. code-block:: c++
+
+ template <typename T> class CRTP {
+ protected:
+ CRTP() = default;
+ };
+
+ class Good : CRTP<Good> {};
+ Good GoodInstance;
+
+ class Bad : CRTP<Good> {};
+ Bad BadInstance;
+
+To ensure that no accidental instantiation happens, the best practice is to make
+the constructor private and declare the derived class as friend.
+
+Example:
+
+.. code-block:: c++
+
+ template <typename T> class CRTP {
+ CRTP() = default;
+ friend T;
+ };
+
+ class Good : CRTP<Good> {};
+ Good GoodInstance;
+
+ class Bad : CRTP<Good> {};
+ Bad CompileTimeError;
+
+ CRTP<int> AlsoCompileTimeError;
diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst
index 6f987ba1672e3f..9083b6d28f1400 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -148,6 +148,7 @@ Clang-Tidy Checks
:doc:`bugprone-unhandled-exception-at-new <bugprone/unhandled-exception-at-new>`,
:doc:`bugprone-unhandled-self-assignment <bugprone/unhandled-self-assignment>`,
:doc:`bugprone-unique-ptr-array-mismatch <bugprone/unique-ptr-array-mismatch>`, "Yes"
+ :doc:`bugprone-unsafe-crtp <bugprone/unsafe-crtp>`, "Yes"
:doc:`bugprone-unsafe-functions <bugprone/unsafe-functions>`,
:doc:`bugprone-unused-raii <bugprone/unused-raii>`, "Yes"
:doc:`bugprone-unused-return-value <bugprone/unused-return-value>`,
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-crtp.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-crtp.cpp
new file mode 100644
index 00000000000000..edcfd67677fd7b
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-crtp.cpp
@@ -0,0 +1,232 @@
+// RUN: %check_clang_tidy %s bugprone-unsafe-crtp %t
+
+namespace class_implicit_ctor {
+template <typename T>
+class CRTP {};
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: the implicit default constructor of the CRTP is publicly accessible [bugprone-unsafe-crtp]
+// CHECK-MESSAGES: :[[@LINE-2]]:7: note: consider making it private
+// CHECK-FIXES: CRTP() = default;
+
+class A : CRTP<A> {};
+} // namespace class_implicit_ctor
+
+namespace class_uncostructible {
+template <typename T>
+class CRTP {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: the CRTP cannot be constructed from the derived class [bugprone-unsafe-crtp]
+// CHECK-MESSAGES: :[[@LINE-2]]:7: note: consider declaring the derived class as friend
+// CHECK-FIXES: friend T;
+ CRTP() = default;
+};
+
+class A : CRTP<A> {};
+} // namespace class_uncostructible
+
+namespace class_public_default_ctor {
+template <typename T>
+class CRTP {
+public:
+ CRTP() = default;
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: public contructor allows the CRTP to be constructed as a regular template class [bugprone-unsafe-crtp]
+ // CHECK-MESSAGES: :[[@LINE-2]]:5: note: consider making it private
+ // CHECK-FIXES: private:{{[[:space:]]*}}CRTP() = default;{{[[:space:]]*}}public:
+};
+
+class A : CRTP<A> {};
+} // namespace class_public_default_ctor
+
+namespace class_public_user_provided_ctor {
+template <typename T>
+class CRTP {
+public:
+ CRTP(int) {}
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: public contructor allows the CRTP to be constructed as a regular template class [bugprone-unsafe-crtp]
+ // CHECK-MESSAGES: :[[@LINE-2]]:5: note: consider making it private
+ // CHECK-FIXES: private:{{[[:space:]]*}}CRTP(int) {}{{[[:space:]]*}}public:
+};
+
+class A : CRTP<A> {};
+} // namespace class_public_user_provided_ctor
+
+namespace class_public_multiple_user_provided_ctors {
+template <typename T>
+class CRTP {
+public:
+ CRTP(int) {}
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: public contructor allows the CRTP to be constructed as a regular template class [bugprone-unsafe-crtp]
+ // CHECK-MESSAGES: :[[@LINE-2]]:5: note: consider making it private
+ // CHECK-FIXES: private:{{[[:space:]]*}}CRTP(int) {}{{[[:space:]]*}}public:
+ CRTP(float) {}
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: public contructor allows the CRTP to be constructed as a regular template class [bugprone-unsafe-crtp]
+ // CHECK-MESSAGES: :[[@LINE-2]]:5: note: consider making it private
+ // CHECK-FIXES: private:{{[[:space:]]*}}CRTP(float) {}{{[[:space:]]*}}public:
+};
+
+class A : CRTP<A> {};
+} // namespace class_public_multiple_user_provided_ctors
+
+namespace class_protected_ctors {
+template <typename T>
+class CRTP {
+protected:
+ CRTP(int) {}
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: protected contructor allows the CRTP to be inherited from as a regular template class [bugprone-unsafe-crtp]
+ // CHECK-MESSAGES: :[[@LINE-2]]:5: note: consider making it private
+ // CHECK-FIXES: private:{{[[:space:]]*}}CRTP(int) {}{{[[:space:]]*}}protected:
+ CRTP() = default;
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: protected contructor allows the CRTP to be inherited from as a regular template class [bugprone-unsafe-crtp]
+ // CHECK-MESSAGES: :[[@LINE-2]]:5: note: consider making it private
+ // CHECK-FIXES: private:{{[[:space:]]*}}CRTP() = default;{{[[:space:]]*}}protected:
+ CRTP(float) {}
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: protected contructor allows the CRTP to be inherited from as a regular template class [bugprone-unsafe-crtp]
+ // CHECK-MESSAGES: :[[@LINE-2]]:5: note: consider making it private
+ // CHECK-FIXES: private:{{[[:space:]]*}}CRTP(float) {}{{[[:space:]]*}}protected:
+};
+
+class A : CRTP<A> {};
+} // namespace class_protected_ctors
+
+namespace struct_implicit_ctor {
+template <typename T>
+struct CRTP {};
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: the implicit default constructor of the CRTP is publicly accessible [bugprone-unsafe-crtp]
+// CHECK-MESSAGES: :[[@LINE-2]]:8: note: consider making it private
+// CHECK-FIXES: private:{{[[:space:]]*}}CRTP() = default;{{[[:space:]]*}}public:
+
+class A : CRTP<A> {};
+} // namespace struct_implicit_ctor
+
+namespace struct_default_ctor {
+template <typename T>
+struct CRTP {
+ CRTP() = default;
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: public contructor allows the CRTP to be constructed as a regular template class [bugprone-unsafe-crtp]
+ // CHECK-MESSAGES: :[[@LINE-2]]:5: note: consider making it private
+ // CHECK-FIXES: private:{{[[:space:]]*}}CRTP() = default;{{[[:space:]]*}}public:
+};
+
+class A : CRTP<A> {};
+} // namespace struct_default_ctor
+
+namespace same_class_multiple_crtps {
+template <typename T>
+struct CRTP {};
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: the implicit default constructor of the CRTP is publicly accessible [bugprone-unsafe-crtp]
+// CHECK-MESSAGES: :[[@LINE-2]]:8: note: consider making it private
+// CHECK-FIXES: private:{{[[:space:]]*}}CRTP() = default;{{[[:space:]]*}}public:
+
+template <typename T>
+struct CRTP2 {};
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: the implicit default constructor of the CRTP is publicly accessible [bugprone-unsafe-crtp]
+// CHECK-MESSAGES: :[[@LINE-2]]:8: note: consider making it private
+// CHECK-FIXES: private:{{[[:space:]]*}}CRTP2() = default;{{[[:space:]]*}}public:
+
+class A : CRTP<A>, CRTP2<A> {};
+} // namespace same_class_multiple_crtps
+
+namespace same_crtp_multiple_classes {
+template <typename T>
+class CRTP {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: the CRTP cannot be constructed from the derived class [bugprone-unsafe-crtp]
+// CHECK-MESSAGES: :[[@LINE-2]]:7: note: consider declaring the derived class as friend
+// CHECK-FIXES: friend T;
+ CRTP() = default;
+};
+
+class A : CRTP<A> {};
+class B : CRTP<B> {};
+} // namespace same_crtp_multiple_classes
+
+namespace crtp_template {
+template <typename T, typename U>
+class CRTP {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: the CRTP cannot be constructed from the derived class [bugprone-unsafe-crtp]
+// CHECK-MESSAGES: :[[@LINE-2]]:7: note: consider declaring the derived class as friend
+// CHECK-FIXES: friend U;
+ CRTP() = default;
+};
+
+class A : CRTP<int, A> {};
+} // namespace crtp_template
+
+namespace crtp_template2 {
+template <typename T, typename U>
+class CRTP {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: the CRTP cannot be constructed from the derived class [bugprone-unsafe-crtp]
+// CHECK-MESSAGES: :[[@LINE-2]]:7: note: consider declaring the derived class as friend
+// CHECK-FIXES: friend T;
+ CRTP() = default;
+};
+
+class A : CRTP<A, A> {};
+} // namespace crtp_template2
+
+namespace template_derived {
+template <typename T>
+class CRTP {};
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: the implicit default constructor of the CRTP is publicly accessible [bugprone-unsafe-crtp]
+// CHECK-MESSAGES: :[[@LINE-2]]:7: note: consider making it private
+// CHECK-FIXES: CRTP() = default;
+
+template<typename T>
+class A : CRTP<A<T>> {};
+
+// FIXME: Ideally the warning should be triggered without instantiation.
+void foo() {
+ A<int> A;
+ (void) A;
+}
+} // namespace template_derived
+
+namespace template_derived_explicit_specialization {
+template <typename T>
+class CRTP {};
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: the implicit default constructor of the CRTP is publicly accessible [bugprone-unsafe-crtp]
+// CHECK-MESSAGES: :[[@LINE-2]]:7: note: consider making it private
+// CHECK-FIXES: CRTP() = default;
+
+template<typename T>
+class A : CRTP<A<T>> {};
+
+te...
[truncated]
|
|
@llvm/pr-subscribers-clang-tools-extra Author: None (isuckatcs) ChangesFinds CRTP used in an error-prone way. If the constructor of a class intended to be used in a CRTP is public, then template <typename T> class CRTP {
public:
CRTP() = default;
};
class Good : CRTP<Good> {};
Good GoodInstance;
CRTP<int> BadInstance;If the constructor is protected, the possibility of an accidental instantiation template <typename T> class CRTP {
protected:
CRTP() = default;
};
class Good : CRTP<Good> {};
Good GoodInstance;
class Bad : CRTP<Good> {};
Bad BadInstance;To ensure that no accidental instantiation happens, the best practice is to make template <typename T> class CRTP {
CRTP() = default;
friend T;
};
class Good : CRTP<Good> {};
Good GoodInstance;
class Bad : CRTP<Good> {};
Bad CompileTimeError;
CRTP<int> AlsoCompileTimeError;Patch is 20.90 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/82403.diff 8 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
index 7a910037368c83..50b791ae10faf0 100644
--- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
@@ -82,6 +82,7 @@
#include "UnhandledExceptionAtNewCheck.h"
#include "UnhandledSelfAssignmentCheck.h"
#include "UniquePtrArrayMismatchCheck.h"
+#include "UnsafeCrtpCheck.h"
#include "UnsafeFunctionsCheck.h"
#include "UnusedRaiiCheck.h"
#include "UnusedReturnValueCheck.h"
@@ -233,6 +234,8 @@ class BugproneModule : public ClangTidyModule {
"bugprone-unhandled-exception-at-new");
CheckFactories.registerCheck<UniquePtrArrayMismatchCheck>(
"bugprone-unique-ptr-array-mismatch");
+ CheckFactories.registerCheck<UnsafeCrtpCheck>(
+ "bugprone-unsafe-crtp");
CheckFactories.registerCheck<UnsafeFunctionsCheck>(
"bugprone-unsafe-functions");
CheckFactories.registerCheck<UnusedRaiiCheck>("bugprone-unused-raii");
diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
index d443fd8d1452f1..572ff622b47473 100644
--- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
@@ -78,6 +78,7 @@ add_clang_library(clangTidyBugproneModule
UnhandledExceptionAtNewCheck.cpp
UnhandledSelfAssignmentCheck.cpp
UniquePtrArrayMismatchCheck.cpp
+ UnsafeCrtpCheck.cpp
UnsafeFunctionsCheck.cpp
UnusedRaiiCheck.cpp
UnusedReturnValueCheck.cpp
diff --git a/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
new file mode 100644
index 00000000000000..7bb50aaf950cbe
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
@@ -0,0 +1,167 @@
+//===--- UnsafeCrtpCheck.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 "UnsafeCrtpCheck.h"
+#include "../utils/LexerUtils.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::bugprone {
+
+namespace {
+// Finds a node if it's already a bound node.
+AST_MATCHER_P(CXXRecordDecl, isBoundNode, std::string, ID) {
+ return Builder->removeBindings(
+ [&](const ast_matchers::internal::BoundNodesMap &Nodes) {
+ const auto *BoundRecord = Nodes.getNodeAs<CXXRecordDecl>(ID);
+ return BoundRecord != &Node;
+ });
+}
+
+bool hasPrivateConstructor(const CXXRecordDecl *RD) {
+ for (auto &&Ctor : RD->ctors()) {
+ if (Ctor->getAccess() == AS_private)
+ return true;
+ }
+
+ return false;
+}
+
+bool isDerivedParameterBefriended(const CXXRecordDecl *CRTP,
+ const NamedDecl *Param) {
+ for (auto &&Friend : CRTP->friends()) {
+ const auto *TTPT =
+ dyn_cast<TemplateTypeParmType>(Friend->getFriendType()->getType());
+
+ if (TTPT && TTPT->getDecl() == Param)
+ return true;
+ }
+
+ return false;
+}
+
+bool isDerivedClassBefriended(const CXXRecordDecl *CRTP,
+ const CXXRecordDecl *Derived) {
+ for (auto &&Friend : CRTP->friends()) {
+ if (Friend->getFriendType()->getType()->getAsCXXRecordDecl() == Derived)
+ return true;
+ }
+
+ return false;
+}
+
+std::optional<const NamedDecl *>
+getDerivedParameter(const ClassTemplateSpecializationDecl *CRTP,
+ const CXXRecordDecl *Derived) {
+ size_t Idx = 0;
+ bool Found = false;
+ for (auto &&TemplateArg : CRTP->getTemplateArgs().asArray()) {
+ if (TemplateArg.getKind() == TemplateArgument::Type &&
+ TemplateArg.getAsType()->getAsCXXRecordDecl() == Derived) {
+ Found = true;
+ break;
+ }
+ ++Idx;
+ }
+
+ if (!Found)
+ return std::nullopt;
+
+ return CRTP->getSpecializedTemplate()->getTemplateParameters()->getParam(Idx);
+}
+
+std::vector<FixItHint> hintMakeCtorPrivate(const CXXConstructorDecl *Ctor,
+ const std::string &OriginalAccess,
+ const SourceManager &SM,
+ const LangOptions &LangOpts) {
+ std::vector<FixItHint> Hints;
+
+ Hints.emplace_back(FixItHint::CreateInsertion(
+ Ctor->getBeginLoc().getLocWithOffset(-1), "private:\n"));
+
+ SourceLocation CtorEndLoc =
+ Ctor->isExplicitlyDefaulted()
+ ? utils::lexer::findNextTerminator(Ctor->getEndLoc(), SM, LangOpts)
+ : Ctor->getEndLoc();
+ Hints.emplace_back(FixItHint::CreateInsertion(
+ CtorEndLoc.getLocWithOffset(1), '\n' + OriginalAccess + ':' + '\n'));
+
+ return Hints;
+}
+} // namespace
+
+void UnsafeCrtpCheck::registerMatchers(MatchFinder *Finder) {
+ Finder->addMatcher(
+ classTemplateSpecializationDecl(
+ decl().bind("crtp"),
+ hasAnyTemplateArgument(refersToType(recordType(hasDeclaration(
+ cxxRecordDecl(isDerivedFrom(cxxRecordDecl(isBoundNode("crtp"))))
+ .bind("derived")))))),
+ this);
+}
+
+void UnsafeCrtpCheck::check(const MatchFinder::MatchResult &Result) {
+ const auto *CRTPInstantiation =
+ Result.Nodes.getNodeAs<ClassTemplateSpecializationDecl>("crtp");
+ const auto *DerivedRecord = Result.Nodes.getNodeAs<CXXRecordDecl>("derived");
+ const CXXRecordDecl *CRTPDeclaration =
+ CRTPInstantiation->getSpecializedTemplate()->getTemplatedDecl();
+
+ if (!CRTPDeclaration->hasUserDeclaredConstructor()) {
+ bool IsStruct = CRTPDeclaration->isStruct();
+
+ diag(CRTPDeclaration->getLocation(),
+ "the implicit default constructor of the CRTP is publicly accessible")
+ << CRTPDeclaration
+ << FixItHint::CreateInsertion(
+ CRTPDeclaration->getBraceRange().getBegin().getLocWithOffset(1),
+ (IsStruct ? "\nprivate:\n" : "\n") +
+ CRTPDeclaration->getNameAsString() + "() = default;\n" +
+ (IsStruct ? "public:\n" : ""));
+ diag(CRTPDeclaration->getLocation(), "consider making it private",
+ DiagnosticIDs::Note);
+ }
+
+ const auto *DerivedTemplateParameter =
+ *getDerivedParameter(CRTPInstantiation, DerivedRecord);
+
+ if (hasPrivateConstructor(CRTPDeclaration) &&
+ !isDerivedParameterBefriended(CRTPDeclaration,
+ DerivedTemplateParameter) &&
+ !isDerivedClassBefriended(CRTPDeclaration, DerivedRecord)) {
+ diag(CRTPDeclaration->getLocation(),
+ "the CRTP cannot be constructed from the derived class")
+ << CRTPDeclaration
+ << FixItHint::CreateInsertion(
+ CRTPDeclaration->getBraceRange().getEnd(),
+ "friend " + DerivedTemplateParameter->getNameAsString() + ';' +
+ '\n');
+ diag(CRTPDeclaration->getLocation(),
+ "consider declaring the derived class as friend", DiagnosticIDs::Note);
+ }
+
+ for (auto &&Ctor : CRTPDeclaration->ctors()) {
+ if (Ctor->getAccess() == AS_private)
+ continue;
+
+ bool IsPublic = Ctor->getAccess() == AS_public;
+ std::string Access = IsPublic ? "public" : "protected";
+
+ diag(Ctor->getLocation(),
+ "%0 contructor allows the CRTP to be %select{inherited "
+ "from|constructed}1 as a regular template class")
+ << Access << IsPublic << Ctor
+ << hintMakeCtorPrivate(Ctor, Access, *Result.SourceManager,
+ getLangOpts());
+ diag(Ctor->getLocation(), "consider making it private",
+ DiagnosticIDs::Note);
+ }
+}
+
+} // namespace clang::tidy::bugprone
diff --git a/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.h b/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.h
new file mode 100644
index 00000000000000..ac1a8f09208f95
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.h
@@ -0,0 +1,30 @@
+//===--- UnsafeCrtpCheck.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_UNSAFECRTPCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_UNSAFECRTPCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang::tidy::bugprone {
+
+/// Finds CRTP used in an error-prone way.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/unsafe-crtp.html
+class UnsafeCrtpCheck : public ClangTidyCheck {
+public:
+ UnsafeCrtpCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context) {}
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace clang::tidy::bugprone
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_UNSAFECRTPCHECK_H
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index b5348384e965ab..1506e4631806a4 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -167,6 +167,11 @@ New checks
extracted from an optional-like type and then used to create a new instance
of the same optional-like type.
+- New :doc:`bugprone-unsafe-crtp
+ <clang-tidy/checks/bugprone/unsafe-crtp>` check.
+
+ Detects error-prone CRTP usage.
+
- New :doc:`cppcoreguidelines-no-suspend-with-lock
<clang-tidy/checks/cppcoreguidelines/no-suspend-with-lock>` check.
diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/unsafe-crtp.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/unsafe-crtp.rst
new file mode 100644
index 00000000000000..8e6955999512a6
--- /dev/null
+++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/unsafe-crtp.rst
@@ -0,0 +1,62 @@
+.. title:: clang-tidy - bugprone-unsafe-crtp
+
+bugprone-unsafe-crtp
+====================
+
+Finds CRTP used in an error-prone way.
+
+If the constructor of a class intended to be used in a CRTP is public, then
+it allows users to construct that class on its own.
+
+Example:
+
+.. code-block:: c++
+
+ template <typename T> class CRTP {
+ public:
+ CRTP() = default;
+ };
+
+ class Good : CRTP<Good> {};
+ Good GoodInstance;
+
+ CRTP<int> BadInstance;
+
+If the constructor is protected, the possibility of an accidental instantiation
+is prevented, however it can fade an error, when a different class is used as
+the template parameter instead of the derived one.
+
+Example:
+
+.. code-block:: c++
+
+ template <typename T> class CRTP {
+ protected:
+ CRTP() = default;
+ };
+
+ class Good : CRTP<Good> {};
+ Good GoodInstance;
+
+ class Bad : CRTP<Good> {};
+ Bad BadInstance;
+
+To ensure that no accidental instantiation happens, the best practice is to make
+the constructor private and declare the derived class as friend.
+
+Example:
+
+.. code-block:: c++
+
+ template <typename T> class CRTP {
+ CRTP() = default;
+ friend T;
+ };
+
+ class Good : CRTP<Good> {};
+ Good GoodInstance;
+
+ class Bad : CRTP<Good> {};
+ Bad CompileTimeError;
+
+ CRTP<int> AlsoCompileTimeError;
diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst
index 6f987ba1672e3f..9083b6d28f1400 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -148,6 +148,7 @@ Clang-Tidy Checks
:doc:`bugprone-unhandled-exception-at-new <bugprone/unhandled-exception-at-new>`,
:doc:`bugprone-unhandled-self-assignment <bugprone/unhandled-self-assignment>`,
:doc:`bugprone-unique-ptr-array-mismatch <bugprone/unique-ptr-array-mismatch>`, "Yes"
+ :doc:`bugprone-unsafe-crtp <bugprone/unsafe-crtp>`, "Yes"
:doc:`bugprone-unsafe-functions <bugprone/unsafe-functions>`,
:doc:`bugprone-unused-raii <bugprone/unused-raii>`, "Yes"
:doc:`bugprone-unused-return-value <bugprone/unused-return-value>`,
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-crtp.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-crtp.cpp
new file mode 100644
index 00000000000000..edcfd67677fd7b
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-crtp.cpp
@@ -0,0 +1,232 @@
+// RUN: %check_clang_tidy %s bugprone-unsafe-crtp %t
+
+namespace class_implicit_ctor {
+template <typename T>
+class CRTP {};
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: the implicit default constructor of the CRTP is publicly accessible [bugprone-unsafe-crtp]
+// CHECK-MESSAGES: :[[@LINE-2]]:7: note: consider making it private
+// CHECK-FIXES: CRTP() = default;
+
+class A : CRTP<A> {};
+} // namespace class_implicit_ctor
+
+namespace class_uncostructible {
+template <typename T>
+class CRTP {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: the CRTP cannot be constructed from the derived class [bugprone-unsafe-crtp]
+// CHECK-MESSAGES: :[[@LINE-2]]:7: note: consider declaring the derived class as friend
+// CHECK-FIXES: friend T;
+ CRTP() = default;
+};
+
+class A : CRTP<A> {};
+} // namespace class_uncostructible
+
+namespace class_public_default_ctor {
+template <typename T>
+class CRTP {
+public:
+ CRTP() = default;
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: public contructor allows the CRTP to be constructed as a regular template class [bugprone-unsafe-crtp]
+ // CHECK-MESSAGES: :[[@LINE-2]]:5: note: consider making it private
+ // CHECK-FIXES: private:{{[[:space:]]*}}CRTP() = default;{{[[:space:]]*}}public:
+};
+
+class A : CRTP<A> {};
+} // namespace class_public_default_ctor
+
+namespace class_public_user_provided_ctor {
+template <typename T>
+class CRTP {
+public:
+ CRTP(int) {}
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: public contructor allows the CRTP to be constructed as a regular template class [bugprone-unsafe-crtp]
+ // CHECK-MESSAGES: :[[@LINE-2]]:5: note: consider making it private
+ // CHECK-FIXES: private:{{[[:space:]]*}}CRTP(int) {}{{[[:space:]]*}}public:
+};
+
+class A : CRTP<A> {};
+} // namespace class_public_user_provided_ctor
+
+namespace class_public_multiple_user_provided_ctors {
+template <typename T>
+class CRTP {
+public:
+ CRTP(int) {}
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: public contructor allows the CRTP to be constructed as a regular template class [bugprone-unsafe-crtp]
+ // CHECK-MESSAGES: :[[@LINE-2]]:5: note: consider making it private
+ // CHECK-FIXES: private:{{[[:space:]]*}}CRTP(int) {}{{[[:space:]]*}}public:
+ CRTP(float) {}
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: public contructor allows the CRTP to be constructed as a regular template class [bugprone-unsafe-crtp]
+ // CHECK-MESSAGES: :[[@LINE-2]]:5: note: consider making it private
+ // CHECK-FIXES: private:{{[[:space:]]*}}CRTP(float) {}{{[[:space:]]*}}public:
+};
+
+class A : CRTP<A> {};
+} // namespace class_public_multiple_user_provided_ctors
+
+namespace class_protected_ctors {
+template <typename T>
+class CRTP {
+protected:
+ CRTP(int) {}
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: protected contructor allows the CRTP to be inherited from as a regular template class [bugprone-unsafe-crtp]
+ // CHECK-MESSAGES: :[[@LINE-2]]:5: note: consider making it private
+ // CHECK-FIXES: private:{{[[:space:]]*}}CRTP(int) {}{{[[:space:]]*}}protected:
+ CRTP() = default;
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: protected contructor allows the CRTP to be inherited from as a regular template class [bugprone-unsafe-crtp]
+ // CHECK-MESSAGES: :[[@LINE-2]]:5: note: consider making it private
+ // CHECK-FIXES: private:{{[[:space:]]*}}CRTP() = default;{{[[:space:]]*}}protected:
+ CRTP(float) {}
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: protected contructor allows the CRTP to be inherited from as a regular template class [bugprone-unsafe-crtp]
+ // CHECK-MESSAGES: :[[@LINE-2]]:5: note: consider making it private
+ // CHECK-FIXES: private:{{[[:space:]]*}}CRTP(float) {}{{[[:space:]]*}}protected:
+};
+
+class A : CRTP<A> {};
+} // namespace class_protected_ctors
+
+namespace struct_implicit_ctor {
+template <typename T>
+struct CRTP {};
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: the implicit default constructor of the CRTP is publicly accessible [bugprone-unsafe-crtp]
+// CHECK-MESSAGES: :[[@LINE-2]]:8: note: consider making it private
+// CHECK-FIXES: private:{{[[:space:]]*}}CRTP() = default;{{[[:space:]]*}}public:
+
+class A : CRTP<A> {};
+} // namespace struct_implicit_ctor
+
+namespace struct_default_ctor {
+template <typename T>
+struct CRTP {
+ CRTP() = default;
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: public contructor allows the CRTP to be constructed as a regular template class [bugprone-unsafe-crtp]
+ // CHECK-MESSAGES: :[[@LINE-2]]:5: note: consider making it private
+ // CHECK-FIXES: private:{{[[:space:]]*}}CRTP() = default;{{[[:space:]]*}}public:
+};
+
+class A : CRTP<A> {};
+} // namespace struct_default_ctor
+
+namespace same_class_multiple_crtps {
+template <typename T>
+struct CRTP {};
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: the implicit default constructor of the CRTP is publicly accessible [bugprone-unsafe-crtp]
+// CHECK-MESSAGES: :[[@LINE-2]]:8: note: consider making it private
+// CHECK-FIXES: private:{{[[:space:]]*}}CRTP() = default;{{[[:space:]]*}}public:
+
+template <typename T>
+struct CRTP2 {};
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: the implicit default constructor of the CRTP is publicly accessible [bugprone-unsafe-crtp]
+// CHECK-MESSAGES: :[[@LINE-2]]:8: note: consider making it private
+// CHECK-FIXES: private:{{[[:space:]]*}}CRTP2() = default;{{[[:space:]]*}}public:
+
+class A : CRTP<A>, CRTP2<A> {};
+} // namespace same_class_multiple_crtps
+
+namespace same_crtp_multiple_classes {
+template <typename T>
+class CRTP {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: the CRTP cannot be constructed from the derived class [bugprone-unsafe-crtp]
+// CHECK-MESSAGES: :[[@LINE-2]]:7: note: consider declaring the derived class as friend
+// CHECK-FIXES: friend T;
+ CRTP() = default;
+};
+
+class A : CRTP<A> {};
+class B : CRTP<B> {};
+} // namespace same_crtp_multiple_classes
+
+namespace crtp_template {
+template <typename T, typename U>
+class CRTP {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: the CRTP cannot be constructed from the derived class [bugprone-unsafe-crtp]
+// CHECK-MESSAGES: :[[@LINE-2]]:7: note: consider declaring the derived class as friend
+// CHECK-FIXES: friend U;
+ CRTP() = default;
+};
+
+class A : CRTP<int, A> {};
+} // namespace crtp_template
+
+namespace crtp_template2 {
+template <typename T, typename U>
+class CRTP {
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: the CRTP cannot be constructed from the derived class [bugprone-unsafe-crtp]
+// CHECK-MESSAGES: :[[@LINE-2]]:7: note: consider declaring the derived class as friend
+// CHECK-FIXES: friend T;
+ CRTP() = default;
+};
+
+class A : CRTP<A, A> {};
+} // namespace crtp_template2
+
+namespace template_derived {
+template <typename T>
+class CRTP {};
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: the implicit default constructor of the CRTP is publicly accessible [bugprone-unsafe-crtp]
+// CHECK-MESSAGES: :[[@LINE-2]]:7: note: consider making it private
+// CHECK-FIXES: CRTP() = default;
+
+template<typename T>
+class A : CRTP<A<T>> {};
+
+// FIXME: Ideally the warning should be triggered without instantiation.
+void foo() {
+ A<int> A;
+ (void) A;
+}
+} // namespace template_derived
+
+namespace template_derived_explicit_specialization {
+template <typename T>
+class CRTP {};
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: the implicit default constructor of the CRTP is publicly accessible [bugprone-unsafe-crtp]
+// CHECK-MESSAGES: :[[@LINE-2]]:7: note: consider making it private
+// CHECK-FIXES: CRTP() = default;
+
+template<typename T>
+class A : CRTP<A<T>> {};
+
+te...
[truncated]
|
|
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
clang-tools-extra/docs/clang-tidy/checks/bugprone/unsafe-crtp.rst
Outdated
Show resolved
Hide resolved
PiotrZSL
requested changes
Feb 20, 2024
There was a problem hiding this comment.
I'm not fan of using "friends", currently they consider more a code-smell, and i'm not fan of exposing other private implementation to derived class, this could cause some issues, like give access to class that isn't a derived class (Bad example), even if object of such CRTP would never be created simply because static methods would be used.
I assume that this check base on this approach: https://www.fluentcpp.com/2017/05/12/curiously-recurring-template-pattern/
Consider changing check name to more detailed, updating diagnostic, fixing known issues, adding more tests and proper support for all C++ standards, and then well, it could be merged.
clang-tools-extra/docs/clang-tidy/checks/bugprone/unsafe-crtp.rst
Outdated
Show resolved
Hide resolved
clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-crtp.cpp
Outdated
Show resolved
Hide resolved
| class CRTP { | ||
| // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: the CRTP cannot be constructed from the derived class [bugprone-unsafe-crtp] | ||
| // CHECK-MESSAGES: :[[@LINE-2]]:7: note: consider declaring the derived class as friend | ||
| // CHECK-FIXES: friend T; |
There was a problem hiding this comment.
this isn't a bug, maybe someone want this class be to not constructable.
boost::noncopiable could be such example.
There was a problem hiding this comment.
If you want to use boost::noncopyable as a CRPT, then it is an interesting question.
The idea that the checker is built around is that we want to help the user ensuring, that their CRTP class won't be instantiated accidentally. This could be achieved by making the constructor accessible only to the derived class.
boost::noncopyable uses a protected constructor and makes the copy-constructor private, which if used as a CRTP would automatically trigger the other issue, when a protected constructor allows any template parameter to be passed. E.g.: class Derived : Noncopyable<Foo> {};
As a result I think a functionality such as boost::noncopyable is not suitable for a CRTP, but if the user insists on creating it // NOLINT(bugprone-unsafe-crtp) could be applied.
There was a problem hiding this comment.
boost::noncopyable may not be best example, but thing is that private constructor is not an issue, simply check shouldn't say, "hey you forget friend" because such class may have private constructor on purpose, for example to use an factory function or just to provide static methods.
There was a problem hiding this comment.
I don't see how a factory function in a CRTP context would make sense. If the only constructor is for a static factory function, which is unaccessible by the derived class, then the derived class cannot be constructed either, right? Also since the goal of the CRTP is to provide some functionality to it's template parameter, having a factory method would just result in instantiating it on it's own, which also doesn't make sense, right?
Otherwise since the goal is to prevent any kind of accidental instantiation, the check assumes that every other constructor is private, and then we need the friend, since the derived class has no way to instantiate the CRTP. If anything else other than the derived class and the CRTP itself has access to the constructors, then accidental instantiation can happen, which we want to prevent.
If the class only provides static methods, then why would any other class inherit from it, or use it as a CRTP?
There was a problem hiding this comment.
If the only constructor is for a static factory function, which is unaccessible by the derived class, then the derived class cannot be constructed either, right?
@isuckatcs I think you are right, at least I tried to give this some thought (actually by trying to construct a counter-example).
template <class T>
class Base
{
Base(int I) {}
protected:
static Base factory(int I) { return Base{I}; }
};
class Derived : public Base<Derived>
{
// Derived() : Base(0) {} // Does not work.
Derived() : Base(Base::factory(1)) {}
};
void test()
{
Derived D;
}<source>:18:13: error: calling a private constructor of class 'Derived'
18 | Derived D;
| ^
<source>:13:5: note: implicitly declared private here
13 | Derived() : Base(Base::factory(1)) {}
| ^
There was a problem hiding this comment.
@whisperity I think you meant struct Derived, and then your example proves me wrong.
The check should also warn about the accessibility of any function, which can construct the CRTP from anywhere other than the derived class and itself.
There was a problem hiding this comment.
Since checking if a public or protected function can construct the class would require path sensitive analysis I'd add this example as a limitation to the docs.
E.g.:
template <class T>
class CRTP
{
CRTP(int I) {}
public:
static void hardToDetect(CRTP* Escape, int X) {
if(X > 2) {
Escape = new CRTP{1};
}
}
};Another solution would be to walk the callgraph, but that would just pollute the user with non-meaningful warnings.
E.g.:
template <class T>
class CRTP
{
CRTP(int I) {}
public:
void hardToDetect(CRTP* Escape, int X) {
if(CRTP && X > 2) {
Escape = new CRTP{1};
}
}
void warning1() { hardToDetect(nullptr, 3); }
void warning2() { warning1(); }
void warning3() { warning2(); }
};In the above CRTP we could report a warning for every function, right?
| class Bad : CRTP<Good> {}; | ||
| Bad CompileTimeError; | ||
|
|
||
| CRTP<int> AlsoCompileTimeError; |
There was a problem hiding this comment.
Wold be nice if check had an option that would allow to select one of two:
- prefer protected constructor
- prefer private constructor + friend
There was a problem hiding this comment.
I think protected is still as bugprone as public is. It will fade an accidentaly wrong inheritance, e.g.: class Derived : CRTP<Foo> {};, I'm not sure if we want to suggest users a fix like this, because it still results in the problem, which we want to fix.
There was a problem hiding this comment.
The thing is that some projects may consider friend to invasive, or simply forbid using friends.
There was a problem hiding this comment.
Also, protected constructor might be needed in cases where the class we are currently checking a base class in a hierarchy (while also using a CRTP) and there is no reason to allow its direct instantiation by clients, because clients should instantiate the current class's deriveds to get to useful objects. However, those deriveds might not be using CRTP on their own level.
Contrived example:
template <class NodeFamily>
struct ASTNode {
protected:
// Users should not construct this base class directly.
ASTNode(...) { ... }
};
struct Stmt : ASTNode<Stmt> {
protected:
// Users should not construct this base class directly, either.
Stmt(...) : ASTNode(...) { ... }
};
struct IfStmt : Stmt {
// This should be constructible directly by clients.
IfStmt(...) : Stmt(...) { ... }
};There was a problem hiding this comment.
If the constructor is protected as in your example, the following will also compile, which is not desired.
struct Decl {};
struct Stmt : ASTNode<Decl> {
protected:
Stmt() : ASTNode() { }
};This is exactly the kind of usage, this check wants to catch.
Also note that with the suggested fix, your example still compiles.
template <class NodeFamily>
struct ASTNode {
private:
ASTNode() { }
friend NodeFamily;
};
struct Stmt : ASTNode<Stmt> {
protected:
Stmt() : ASTNode() { }
};
struct IfStmt : Stmt {
IfStmt() : Stmt() { }
};IfStmt calls the constructor of Stmt, and not the constructor of ASTNode<>. The latter constructor is called by Stmt, which has access to it either way. Derived classes supposed to invoke the constructor of the base class, not the the constructor of the base class of their base class.
For the record, AFAIK the CRTP is supposed to describe has a relationships and not is a relationships, so making ASTNode<> a CRTP might not be an ideal design choice.
clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-crtp.cpp
Outdated
Show resolved
Hide resolved
whisperity
reviewed
Feb 22, 2024
Comment on lines
27
to
28
| bool hasPrivateConstructor(const CXXRecordDecl *RD) { | ||
| for (auto &&Ctor : RD->ctors()) { |
There was a problem hiding this comment.
You take by pointer but do not check for nullness before dereferencing. If it is guaranteed and expected that a valid value will be here, consider using a reference instead.
There was a problem hiding this comment.
After taking a look at other similar helpers throughout the clang-tidy codebase (grep static .*\(.*\*.*), none of them actually check for nullptr, or assert the opposite. With llvm::any_of(), all of these are 1 liners, so I wouldn't pollute them with an additional check either.
Comment on lines
127
to
128
| diag(CRTPDeclaration->getLocation(), "consider making it private", | ||
| DiagnosticIDs::Note); |
There was a problem hiding this comment.
Why is this a separate note? And if this is a separate note, why is the action suggested by the note (making the entity private) handled by a fixit that's attached to the warning instead?
There was a problem hiding this comment.
See the answer in a previous discussion.
clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-crtp.cpp
Outdated
Show resolved
Hide resolved
| const CXXRecordDecl *CRTPDeclaration = | ||
| CRTPInstantiation->getSpecializedTemplate()->getTemplatedDecl(); | ||
|
|
||
| if (!CRTPDeclaration->hasUserDeclaredConstructor()) { |
There was a problem hiding this comment.
(Following from 💬...)
How possible it is that these cases appear in conjunction with one another? Based on style, it would look like the returns are missing from the end of the statements.
Perhaps you could create helper functions that create the diagnostics for each individual case with a telling name, and then the check() function can be simplified to if (condition) diagnoseCaseX(); if (condition2) diagnoseCaseY(); etc.
There was a problem hiding this comment.
Private constructors are always checked, but I think that logic is fine in the current function, in case of the implicit default ctor and friend declaration the extracted function would only contain 1 diag() call, so I don't see the value of those either.
clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-crtp.cpp
Outdated
Show resolved
Hide resolved
| class CRTP { | ||
| // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: the CRTP cannot be constructed from the derived class [bugprone-unsafe-crtp] | ||
| // CHECK-MESSAGES: :[[@LINE-2]]:7: note: consider declaring the derived class as friend | ||
| // CHECK-FIXES: friend T; |
There was a problem hiding this comment.
If the only constructor is for a static factory function, which is unaccessible by the derived class, then the derived class cannot be constructed either, right?
@isuckatcs I think you are right, at least I tried to give this some thought (actually by trying to construct a counter-example).
template <class T>
class Base
{
Base(int I) {}
protected:
static Base factory(int I) { return Base{I}; }
};
class Derived : public Base<Derived>
{
// Derived() : Base(0) {} // Does not work.
Derived() : Base(Base::factory(1)) {}
};
void test()
{
Derived D;
}<source>:18:13: error: calling a private constructor of class 'Derived'
18 | Derived D;
| ^
<source>:13:5: note: implicitly declared private here
13 | Derived() : Base(Base::factory(1)) {}
| ^
| namespace class_uncostructible { | ||
| template <typename T> | ||
| class CRTP { | ||
| // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: the CRTP cannot be constructed from the derived class [bugprone-unsafe-crtp] |
There was a problem hiding this comment.
(Note: These warnings are matched as regexes and substrings, so you can simplify the test file by not repeating the check's name on every warning line.)
There was a problem hiding this comment.
Some checker test files leave the name of the checker there, some don't, some mixes the two. I think it doesn't hurt to know that the message should be expected from this checker, though it might be obvious based on the name of the test.
PiotrZSL
reviewed
Feb 23, 2024
clang-tools-extra/clang-tidy/bugprone/CrtpConstructorAccessibilityCheck.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/bugprone/CrtpConstructorAccessibilityCheck.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/bugprone/CrtpConstructorAccessibilityCheck.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/bugprone/CrtpConstructorAccessibilityCheck.cpp
Show resolved
Hide resolved
clang-tools-extra/docs/clang-tidy/checks/bugprone/crtp-constructor-accessibility.rst
Outdated
Show resolved
Hide resolved
clang-tools-extra/docs/clang-tidy/checks/bugprone/crtp-constructor-accessibility.rst
Show resolved
Hide resolved
|
I ran the checker on LLVM, Clang and Clang-Tidy ( template <typename Derived, typename KeyType, typename UnversionedDataType>
class VersionedTableInfo { ... };template <typename ImplT, typename IteratorT, typename CollectionT>
class CalcLiveRangeUtilBase {
protected:
LiveRange *LR;
protected:
CalcLiveRangeUtilBase(LiveRange *LR) : LR(LR) {}
...
}
class CalcLiveRangeUtilVector;
using CalcLiveRangeUtilVectorBase =
CalcLiveRangeUtilBase<CalcLiveRangeUtilVector, LiveRange::iterator,
LiveRange::Segments>;
class CalcLiveRangeUtilVector : public CalcLiveRangeUtilVectorBase { ... }template <typename T>
class TestVisitor : public clang::RecursiveASTVisitor<T> { ... }
class ClassDeclXVisitor : public TestVisitor<ClassDeclXVisitor> { ... }template <class Derived> struct DiagTextVisitor { ... }template <typename Derived> class OpSplitter { ... }template <typename Derived>
class Class5Tmpl : private llvm::TrailingObjects<Derived, float, int> { ... }template <typename Derived>
struct SimpleTransformVisitor : public TypeVisitor<Derived, QualType> { ... }template <typename DERIVED>
class ClusterAnalysis { ... }template <typename DerivedCCG, typename FuncTy, typename CallTy>
class CallsiteContextGraph { ... }template <typename DerivedT, typename IRUnitT,
typename AnalysisManagerT = AnalysisManager<IRUnitT>,
typename... ExtraArgTs>
class MockAnalysisHandleBase { ... }template <typename DerivedT, typename IRUnitT,
typename AnalysisManagerT = AnalysisManager<IRUnitT>,
typename... ExtraArgTs>
class MockPassHandleBase { ... }template <class Derived> struct StructVisitor { ... }
template <class Derived, bool IsMove>
struct CopyStructVisitor : StructVisitor<Derived>,
CopiedTypeVisitor<Derived, IsMove> { ... }template <class Derived>
struct GenUnaryFuncName : StructVisitor<Derived>, GenFuncNameBase<Derived> { ... }struct GenBinaryFunc : CopyStructVisitor<Derived, IsMove>,
GenFuncBase<Derived> { ... }template <typename DerivedT, typename IRUnitT,
typename AnalysisManagerT = AnalysisManager<IRUnitT>,
typename... ExtraArgTs>
class MockAnalysisHandleBase { ... }template <typename DerivedT, typename IRUnitT,
typename AnalysisManagerT = AnalysisManager<IRUnitT>,
typename... ExtraArgTs>
class MockPassHandleBase { ... }
class MockPassHandleBase {
public:
class Pass : public PassInfoMixin<Pass> { ... }
...
}template <class Derived>
struct DestroyNRVOVariable : EHScopeStack::Cleanup { ... }
template <class Derived>
class ExprEvaluatorBase
: public ConstStmtVisitor<Derived, bool> { ... }template<class Derived>
class LValueExprEvaluatorBase
: public ExprEvaluatorBase<Derived> { ... }template <typename Derived, typename KeyType, typename UnversionedDataType>
class VersionedTableInfo { ... }template <typename Derived, typename UnversionedDataType>
class CommonTypeTableInfo
: public VersionedTableInfo<Derived, ContextTableKey, UnversionedDataType> { ... }template <typename Derived, typename UnversionedDataType>
class CommonTypeTableInfo
: public VersionedTableInfo<Derived, ContextTableKey, UnversionedDataType> { ... }/// A CRTP base class for emitting expressions of retainable object
/// pointer type in ARC.
template <typename Impl, typename Result> class ARCExprEmitter { ... }template<typename Derived, typename ResultList, typename Result,
typename Subobject>
class DefaultedComparisonVisitor { ... }/// CRTP base class for visiting operations performed by a special member
/// function (or inherited constructor).
template<typename Derived>
struct SpecialMemberVisitor { ... }template <typename Derived, typename KeyType, typename UnversionedDataType>
class VersionedTableInfo { ... }TLDR, |
isuckatcs
changed the title
|
ping @PiotrZSL @whisperity |
PiotrZSL
approved these changes
Mar 4, 2024
There was a problem hiding this comment.
Overall looks fine, at least fine to be merged.
I will try to take a deeper look into particular cases, but even if some changes could be needed, probably better would be to merge this, and push fixes in separate PR.
So lest merge this, and let other dev to test this check.
clang-tools-extra/clang-tidy/bugprone/CrtpConstructorAccessibilityCheck.cpp
Outdated
Show resolved
Hide resolved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Finds CRTP used in an error-prone way.
If the constructor of a class intended to be used in a CRTP is public, then
it allows users to construct that class on its own.
If the constructor is protected, the possibility of an accidental instantiation
is prevented, however it can fade an error, when a different class is used as
the template parameter instead of the derived one.
To ensure that no accidental instantiation happens, the best practice is to make
the constructor private and declare the derived class as friend.