[clang-tools-extra] [clang-tidy] Unsafe CRTP check (PR #82403)
via cfe-commits
cfe-commits at lists.llvm.org
Tue Feb 20 10:51:18 PST 2024
https://github.com/isuckatcs created https://github.com/llvm/llvm-project/pull/82403
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.
```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.
```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.
``` 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;
```
>From 042b27c1268cf01b37e102cd653b094c6c0f2d5d Mon Sep 17 00:00:00 2001
From: isuckatcs <65320245+isuckatcs at users.noreply.github.com>
Date: Fri, 16 Feb 2024 00:25:29 +0100
Subject: [PATCH 01/12] added new checker
---
.../bugprone/BugproneTidyModule.cpp | 3 +
.../clang-tidy/bugprone/CMakeLists.txt | 1 +
.../clang-tidy/bugprone/UnsafeCrtpCheck.cpp | 92 +++++++++++++++++++
.../clang-tidy/bugprone/UnsafeCrtpCheck.h | 30 ++++++
clang-tools-extra/docs/ReleaseNotes.rst | 5 +
.../checks/bugprone/unsafe-crtp.rst | 6 ++
.../docs/clang-tidy/checks/list.rst | 1 +
.../checkers/bugprone/unsafe-crtp.cpp | 14 +++
8 files changed, 152 insertions(+)
create mode 100644 clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
create mode 100644 clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.h
create mode 100644 clang-tools-extra/docs/clang-tidy/checks/bugprone/unsafe-crtp.rst
create mode 100644 clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-crtp.cpp
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..eb0f6187e58b2e
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
@@ -0,0 +1,92 @@
+//===--- 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 "clang/AST/ASTContext.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;
+ });
+}
+} // namespace
+
+void UnsafeCrtpCheck::registerMatchers(MatchFinder *Finder) {
+ Finder->addMatcher(
+ cxxRecordDecl(
+ decl().bind("record"),
+ hasAnyBase(hasType(
+ classTemplateSpecializationDecl(
+ hasAnyTemplateArgument(refersToType(recordType(
+ hasDeclaration(cxxRecordDecl(isBoundNode("record")))))))
+ .bind("crtp")))),
+ this);
+}
+
+void UnsafeCrtpCheck::check(const MatchFinder::MatchResult &Result) {
+ const auto *MatchedCRTP = Result.Nodes.getNodeAs<CXXRecordDecl>("crtp");
+
+ MatchedCRTP->dump();
+
+ for (auto &&ctor : MatchedCRTP->ctors()) {
+ if (ctor->getAccess() != AS_private) {
+ ctor->dump();
+ };
+ }
+
+ return;
+
+ // if (!MatchedDecl->hasDefinition())
+ // return;
+
+ // for (auto &&Base : MatchedDecl->bases()) {
+ // const auto *TemplateSpecDecl =
+ // llvm::dyn_cast_if_present<ClassTemplateSpecializationDecl>(
+ // Base.getType()->getAsTagDecl());
+
+ // if (!TemplateSpecDecl)
+ // continue;
+
+ // TemplateSpecDecl->dump();
+
+ // for (auto &&TemplateArg : TemplateSpecDecl->getTemplateArgs().asArray())
+ // {
+ // if (TemplateArg.getKind() != TemplateArgument::Type)
+ // continue;
+
+ // const auto *Record = TemplateArg.getAsType()->getAsCXXRecordDecl();
+
+ // if (Record && Record == MatchedDecl) {
+ // diag(Record->getLocation(), "CRTP found");
+
+ // diag(TemplateSpecDecl->getLocation(), "used as CRTP",
+ // DiagnosticIDs::Note);
+ // TemplateSpecDecl->dump();
+ // }
+ // }
+ // }
+
+ // if (!MatchedDecl->getIdentifier() ||
+ // MatchedDecl->getName().startswith("awesome_"))
+ // return;
+ // diag(MatchedDecl->getLocation(), "function %0 is insufficiently awesome")
+ // << MatchedDecl
+ // << FixItHint::CreateInsertion(MatchedDecl->getLocation(), "awesome_");
+ // diag(MatchedDecl->getLocation(), "insert 'awesome'", 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..7c41bbb0678521
--- /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 {
+
+/// FIXME: Write a short description.
+///
+/// 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..a96a18bfbacf96 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.
+
+ FIXME: add release notes.
+
- 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..3352af02a8744f
--- /dev/null
+++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/unsafe-crtp.rst
@@ -0,0 +1,6 @@
+.. title:: clang-tidy - bugprone-unsafe-crtp
+
+bugprone-unsafe-crtp
+====================
+
+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 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..90350ad7abc578
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-crtp.cpp
@@ -0,0 +1,14 @@
+// RUN: %check_clang_tidy %s bugprone-unsafe-crtp %t
+
+// FIXME: Add something that triggers the check here.
+void f();
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: function 'f' is insufficiently awesome [bugprone-unsafe-crtp]
+
+// FIXME: Verify the applied fix.
+// * Make the CHECK patterns specific enough and try to make verified lines
+// unique to avoid incorrect matches.
+// * Use {{}} for regular expressions.
+// CHECK-FIXES: {{^}}void awesome_f();{{$}}
+
+// FIXME: Add something that doesn't trigger the check here.
+void awesome_f2();
>From 6a674888c092a3949e23e701251f95d6756f8f0c Mon Sep 17 00:00:00 2001
From: isuckatcs <65320245+isuckatcs at users.noreply.github.com>
Date: Fri, 16 Feb 2024 01:18:56 +0100
Subject: [PATCH 02/12] use a different matcher
---
.../clang-tidy/bugprone/UnsafeCrtpCheck.cpp | 53 ++++---------------
1 file changed, 9 insertions(+), 44 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
index eb0f6187e58b2e..33ad37d7fbbfdb 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
@@ -26,15 +26,12 @@ AST_MATCHER_P(CXXRecordDecl, isBoundNode, std::string, ID) {
} // namespace
void UnsafeCrtpCheck::registerMatchers(MatchFinder *Finder) {
- Finder->addMatcher(
- cxxRecordDecl(
- decl().bind("record"),
- hasAnyBase(hasType(
- classTemplateSpecializationDecl(
- hasAnyTemplateArgument(refersToType(recordType(
- hasDeclaration(cxxRecordDecl(isBoundNode("record")))))))
- .bind("crtp")))),
- this);
+ Finder->addMatcher(classTemplateSpecializationDecl(
+ decl().bind("crtp"),
+ hasAnyTemplateArgument(refersToType(recordType(
+ hasDeclaration(cxxRecordDecl(isDerivedFrom(
+ cxxRecordDecl(isBoundNode("crtp"))))))))),
+ this);
}
void UnsafeCrtpCheck::check(const MatchFinder::MatchResult &Result) {
@@ -42,44 +39,12 @@ void UnsafeCrtpCheck::check(const MatchFinder::MatchResult &Result) {
MatchedCRTP->dump();
- for (auto &&ctor : MatchedCRTP->ctors()) {
- if (ctor->getAccess() != AS_private) {
- ctor->dump();
+ for (auto &&Ctor : MatchedCRTP->ctors()) {
+ if (Ctor->getAccess() != AS_private) {
+ Ctor->dump();
};
}
- return;
-
- // if (!MatchedDecl->hasDefinition())
- // return;
-
- // for (auto &&Base : MatchedDecl->bases()) {
- // const auto *TemplateSpecDecl =
- // llvm::dyn_cast_if_present<ClassTemplateSpecializationDecl>(
- // Base.getType()->getAsTagDecl());
-
- // if (!TemplateSpecDecl)
- // continue;
-
- // TemplateSpecDecl->dump();
-
- // for (auto &&TemplateArg : TemplateSpecDecl->getTemplateArgs().asArray())
- // {
- // if (TemplateArg.getKind() != TemplateArgument::Type)
- // continue;
-
- // const auto *Record = TemplateArg.getAsType()->getAsCXXRecordDecl();
-
- // if (Record && Record == MatchedDecl) {
- // diag(Record->getLocation(), "CRTP found");
-
- // diag(TemplateSpecDecl->getLocation(), "used as CRTP",
- // DiagnosticIDs::Note);
- // TemplateSpecDecl->dump();
- // }
- // }
- // }
-
// if (!MatchedDecl->getIdentifier() ||
// MatchedDecl->getName().startswith("awesome_"))
// return;
>From 13bd5decdc6a2e8953a2e833877415422147cfe8 Mon Sep 17 00:00:00 2001
From: isuckatcs <65320245+isuckatcs at users.noreply.github.com>
Date: Sat, 17 Feb 2024 17:22:59 +0100
Subject: [PATCH 03/12] implemented implicit constructor and friend declaration
logic
---
.../clang-tidy/bugprone/UnsafeCrtpCheck.cpp | 79 ++++++++++++++++---
1 file changed, 67 insertions(+), 12 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
index 33ad37d7fbbfdb..9063b385c0d1b4 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
@@ -9,6 +9,7 @@
#include "UnsafeCrtpCheck.h"
#include "clang/AST/ASTContext.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
using namespace clang::ast_matchers;
@@ -23,26 +24,80 @@ AST_MATCHER_P(CXXRecordDecl, isBoundNode, std::string, ID) {
return BoundRecord != &Node;
});
}
+
+bool hasPrivateConstructor(const CXXRecordDecl *RD) {
+ for (auto &&Ctor : RD->ctors()) {
+ if (Ctor->getAccess() == AS_private)
+ return true;
+ }
+
+ return false;
+}
+
+bool isDerivedBefriended(const CXXRecordDecl *CRTP,
+ const CXXRecordDecl *Derived) {
+ for (auto &&Friend : CRTP->friends()) {
+ if (Friend->getFriendType()->getType()->getAsCXXRecordDecl() == Derived)
+ return true;
+ }
+
+ return false;
+}
} // namespace
void UnsafeCrtpCheck::registerMatchers(MatchFinder *Finder) {
- Finder->addMatcher(classTemplateSpecializationDecl(
- decl().bind("crtp"),
- hasAnyTemplateArgument(refersToType(recordType(
- hasDeclaration(cxxRecordDecl(isDerivedFrom(
- cxxRecordDecl(isBoundNode("crtp"))))))))),
- this);
+ 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 *MatchedCRTP = Result.Nodes.getNodeAs<CXXRecordDecl>("crtp");
- MatchedCRTP->dump();
+ const auto *MatchedCRTP =
+ Result.Nodes.getNodeAs<ClassTemplateSpecializationDecl>("crtp");
+ const auto *MatchedDerived = Result.Nodes.getNodeAs<CXXRecordDecl>("derived");
+
+ if (!MatchedCRTP->hasUserDeclaredConstructor()) {
+ diag(MatchedCRTP->getLocation(),
+ "the implicit default constructor of the CRTP is publicly accessible")
+ << MatchedCRTP
+ << FixItHint::CreateInsertion(
+ MatchedCRTP->getBraceRange().getBegin().getLocWithOffset(1),
+ "private: " + MatchedCRTP->getNameAsString() + "() = default;");
+
+ diag(MatchedCRTP->getLocation(), "consider making it private",
+ DiagnosticIDs::Note);
+ }
+
+ // FIXME: Extract this.
+ size_t idx = 0;
+ for (auto &&TemplateArg : MatchedCRTP->getTemplateArgs().asArray()) {
+ if (TemplateArg.getKind() == TemplateArgument::Type &&
+ TemplateArg.getAsType()->getAsCXXRecordDecl() == MatchedDerived) {
+ break;
+ }
+ ++idx;
+ }
- for (auto &&Ctor : MatchedCRTP->ctors()) {
- if (Ctor->getAccess() != AS_private) {
- Ctor->dump();
- };
+ if (hasPrivateConstructor(MatchedCRTP) &&
+ !isDerivedBefriended(MatchedCRTP, MatchedDerived)) {
+ diag(MatchedCRTP->getLocation(),
+ "the CRTP cannot be constructed from the derived class")
+ << MatchedCRTP
+ << FixItHint::CreateInsertion(
+ MatchedCRTP->getBraceRange().getEnd().getLocWithOffset(-1),
+ "friend " +
+ MatchedCRTP->getSpecializedTemplate()
+ ->getTemplateParameters()
+ ->asArray()[idx]
+ ->getNameAsString() +
+ ';');
+ diag(MatchedCRTP->getLocation(),
+ "consider declaring the derived class as friend", DiagnosticIDs::Note);
}
// if (!MatchedDecl->getIdentifier() ||
>From e090d84c3f30e13693ec752af8b84150e5cfc588 Mon Sep 17 00:00:00 2001
From: isuckatcs <65320245+isuckatcs at users.noreply.github.com>
Date: Sun, 18 Feb 2024 02:18:18 +0100
Subject: [PATCH 04/12] fix friend detection
---
.../clang-tidy/bugprone/UnsafeCrtpCheck.cpp | 75 +++++++++++--------
1 file changed, 45 insertions(+), 30 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
index 9063b385c0d1b4..05d9218e9c3a71 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
@@ -34,15 +34,39 @@ bool hasPrivateConstructor(const CXXRecordDecl *RD) {
return false;
}
-bool isDerivedBefriended(const CXXRecordDecl *CRTP,
- const CXXRecordDecl *Derived) {
+bool isDerivedParameterBefriended(const CXXRecordDecl *CRTP,
+ const NamedDecl *Derived) {
for (auto &&Friend : CRTP->friends()) {
- if (Friend->getFriendType()->getType()->getAsCXXRecordDecl() == Derived)
+ const auto *TTPT =
+ dyn_cast<TemplateTypeParmType>(Friend->getFriendType()->getType());
+
+ if (TTPT && TTPT->getDecl() == 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);
+}
+
} // namespace
void UnsafeCrtpCheck::registerMatchers(MatchFinder *Finder) {
@@ -61,42 +85,33 @@ void UnsafeCrtpCheck::check(const MatchFinder::MatchResult &Result) {
Result.Nodes.getNodeAs<ClassTemplateSpecializationDecl>("crtp");
const auto *MatchedDerived = Result.Nodes.getNodeAs<CXXRecordDecl>("derived");
- if (!MatchedCRTP->hasUserDeclaredConstructor()) {
- diag(MatchedCRTP->getLocation(),
+ const auto *CRTPTemplate =
+ MatchedCRTP->getSpecializedTemplate()->getTemplatedDecl();
+
+ if (!CRTPTemplate->hasUserDeclaredConstructor()) {
+ diag(CRTPTemplate->getLocation(),
"the implicit default constructor of the CRTP is publicly accessible")
- << MatchedCRTP
+ << CRTPTemplate
<< FixItHint::CreateInsertion(
- MatchedCRTP->getBraceRange().getBegin().getLocWithOffset(1),
- "private: " + MatchedCRTP->getNameAsString() + "() = default;");
+ CRTPTemplate->getBraceRange().getBegin().getLocWithOffset(1),
+ "private: " + CRTPTemplate->getNameAsString() + "() = default;");
- diag(MatchedCRTP->getLocation(), "consider making it private",
+ diag(CRTPTemplate->getLocation(), "consider making it private",
DiagnosticIDs::Note);
}
- // FIXME: Extract this.
- size_t idx = 0;
- for (auto &&TemplateArg : MatchedCRTP->getTemplateArgs().asArray()) {
- if (TemplateArg.getKind() == TemplateArgument::Type &&
- TemplateArg.getAsType()->getAsCXXRecordDecl() == MatchedDerived) {
- break;
- }
- ++idx;
- }
+ const auto *DerivedTemplateParameter =
+ *getDerivedParameter(MatchedCRTP, MatchedDerived);
- if (hasPrivateConstructor(MatchedCRTP) &&
- !isDerivedBefriended(MatchedCRTP, MatchedDerived)) {
- diag(MatchedCRTP->getLocation(),
+ if (hasPrivateConstructor(CRTPTemplate) &&
+ !isDerivedParameterBefriended(CRTPTemplate, DerivedTemplateParameter)) {
+ diag(CRTPTemplate->getLocation(),
"the CRTP cannot be constructed from the derived class")
- << MatchedCRTP
+ << CRTPTemplate
<< FixItHint::CreateInsertion(
- MatchedCRTP->getBraceRange().getEnd().getLocWithOffset(-1),
- "friend " +
- MatchedCRTP->getSpecializedTemplate()
- ->getTemplateParameters()
- ->asArray()[idx]
- ->getNameAsString() +
- ';');
- diag(MatchedCRTP->getLocation(),
+ CRTPTemplate->getBraceRange().getEnd().getLocWithOffset(-1),
+ "friend " + DerivedTemplateParameter->getNameAsString() + ';');
+ diag(CRTPTemplate->getLocation(),
"consider declaring the derived class as friend", DiagnosticIDs::Note);
}
>From 32da30933a3ccc9f023fd6fdd6abca7358b79d89 Mon Sep 17 00:00:00 2001
From: isuckatcs <65320245+isuckatcs at users.noreply.github.com>
Date: Sun, 18 Feb 2024 02:28:22 +0100
Subject: [PATCH 05/12] cleanup
---
.../clang-tidy/bugprone/UnsafeCrtpCheck.cpp | 48 ++++++++-----------
1 file changed, 19 insertions(+), 29 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
index 05d9218e9c3a71..09d2db4d993984 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
@@ -7,9 +7,7 @@
//===----------------------------------------------------------------------===//
#include "UnsafeCrtpCheck.h"
-#include "clang/AST/ASTContext.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
-#include "clang/Lex/Lexer.h"
using namespace clang::ast_matchers;
@@ -80,48 +78,40 @@ void UnsafeCrtpCheck::registerMatchers(MatchFinder *Finder) {
}
void UnsafeCrtpCheck::check(const MatchFinder::MatchResult &Result) {
-
- const auto *MatchedCRTP =
+ const auto *CRTPInstantiation =
Result.Nodes.getNodeAs<ClassTemplateSpecializationDecl>("crtp");
- const auto *MatchedDerived = Result.Nodes.getNodeAs<CXXRecordDecl>("derived");
+ const auto *Derived = Result.Nodes.getNodeAs<CXXRecordDecl>("derived");
- const auto *CRTPTemplate =
- MatchedCRTP->getSpecializedTemplate()->getTemplatedDecl();
+ const CXXRecordDecl *CRTPDeclaration =
+ CRTPInstantiation->getSpecializedTemplate()->getTemplatedDecl();
- if (!CRTPTemplate->hasUserDeclaredConstructor()) {
- diag(CRTPTemplate->getLocation(),
+ if (!CRTPDeclaration->hasUserDeclaredConstructor()) {
+ diag(CRTPDeclaration->getLocation(),
"the implicit default constructor of the CRTP is publicly accessible")
- << CRTPTemplate
+ << CRTPDeclaration
<< FixItHint::CreateInsertion(
- CRTPTemplate->getBraceRange().getBegin().getLocWithOffset(1),
- "private: " + CRTPTemplate->getNameAsString() + "() = default;");
-
- diag(CRTPTemplate->getLocation(), "consider making it private",
+ CRTPDeclaration->getBraceRange().getBegin().getLocWithOffset(1),
+ "private: " + CRTPDeclaration->getNameAsString() +
+ "() = default;");
+ diag(CRTPDeclaration->getLocation(), "consider making it private",
DiagnosticIDs::Note);
}
const auto *DerivedTemplateParameter =
- *getDerivedParameter(MatchedCRTP, MatchedDerived);
+ *getDerivedParameter(CRTPInstantiation, Derived);
- if (hasPrivateConstructor(CRTPTemplate) &&
- !isDerivedParameterBefriended(CRTPTemplate, DerivedTemplateParameter)) {
- diag(CRTPTemplate->getLocation(),
+ if (hasPrivateConstructor(CRTPDeclaration) &&
+ !isDerivedParameterBefriended(CRTPDeclaration,
+ DerivedTemplateParameter)) {
+ diag(CRTPDeclaration->getLocation(),
"the CRTP cannot be constructed from the derived class")
- << CRTPTemplate
+ << CRTPDeclaration
<< FixItHint::CreateInsertion(
- CRTPTemplate->getBraceRange().getEnd().getLocWithOffset(-1),
+ CRTPDeclaration->getBraceRange().getEnd().getLocWithOffset(-1),
"friend " + DerivedTemplateParameter->getNameAsString() + ';');
- diag(CRTPTemplate->getLocation(),
+ diag(CRTPDeclaration->getLocation(),
"consider declaring the derived class as friend", DiagnosticIDs::Note);
}
-
- // if (!MatchedDecl->getIdentifier() ||
- // MatchedDecl->getName().startswith("awesome_"))
- // return;
- // diag(MatchedDecl->getLocation(), "function %0 is insufficiently awesome")
- // << MatchedDecl
- // << FixItHint::CreateInsertion(MatchedDecl->getLocation(), "awesome_");
- // diag(MatchedDecl->getLocation(), "insert 'awesome'", DiagnosticIDs::Note);
}
} // namespace clang::tidy::bugprone
>From 3ca0ce95a9e0d93ac4e3ba4733b20f6b128a35a7 Mon Sep 17 00:00:00 2001
From: isuckatcs <65320245+isuckatcs at users.noreply.github.com>
Date: Sun, 18 Feb 2024 04:17:25 +0100
Subject: [PATCH 06/12] implement ctor checks
---
.../clang-tidy/bugprone/UnsafeCrtpCheck.cpp | 36 +++++++++++++++++++
1 file changed, 36 insertions(+)
diff --git a/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
index 09d2db4d993984..85d8b8a8ab8cd8 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
@@ -7,6 +7,7 @@
//===----------------------------------------------------------------------===//
#include "UnsafeCrtpCheck.h"
+#include "../utils/LexerUtils.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
using namespace clang::ast_matchers;
@@ -65,6 +66,24 @@ getDerivedParameter(const ClassTemplateSpecializationDecl *CRTP,
return CRTP->getSpecializedTemplate()->getTemplateParameters()->getParam(Idx);
}
+std::vector<FixItHint> hintMakeCtorPrivate(const CXXConstructorDecl *Ctor,
+ std::string OriginalAccess,
+ const SourceManager &SM,
+ const LangOptions &LangOpts) {
+ std::vector<FixItHint> Hints;
+
+ Hints.emplace_back(FixItHint::CreateInsertion(
+ Ctor->getBeginLoc().getLocWithOffset(-1), "private:\n"));
+
+ Hints.emplace_back(FixItHint::CreateInsertion(
+ Ctor->isExplicitlyDefaulted()
+ ? utils::lexer::findNextTerminator(Ctor->getEndLoc(), SM, LangOpts)
+ .getLocWithOffset(1)
+ : Ctor->getEndLoc().getLocWithOffset(1),
+ '\n' + OriginalAccess + ':' + '\n'));
+
+ return Hints;
+}
} // namespace
void UnsafeCrtpCheck::registerMatchers(MatchFinder *Finder) {
@@ -112,6 +131,23 @@ void UnsafeCrtpCheck::check(const MatchFinder::MatchResult &Result) {
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
>From b188eda2600d0be2a450acb6abb27eb37264f203 Mon Sep 17 00:00:00 2001
From: isuckatcs <65320245+isuckatcs at users.noreply.github.com>
Date: Sun, 18 Feb 2024 04:23:18 +0100
Subject: [PATCH 07/12] fix struct default ctor generation
---
clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
index 85d8b8a8ab8cd8..9295ba0009ac24 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
@@ -111,7 +111,8 @@ void UnsafeCrtpCheck::check(const MatchFinder::MatchResult &Result) {
<< FixItHint::CreateInsertion(
CRTPDeclaration->getBraceRange().getBegin().getLocWithOffset(1),
"private: " + CRTPDeclaration->getNameAsString() +
- "() = default;");
+ "() = default;" +
+ (CRTPDeclaration->isStruct() ? "\npublic:\n" : ""));
diag(CRTPDeclaration->getLocation(), "consider making it private",
DiagnosticIDs::Note);
}
>From f35da02386597472528ad0614823af9f13971834 Mon Sep 17 00:00:00 2001
From: isuckatcs <65320245+isuckatcs at users.noreply.github.com>
Date: Sun, 18 Feb 2024 18:09:47 +0100
Subject: [PATCH 08/12] testing
---
.../clang-tidy/bugprone/UnsafeCrtpCheck.cpp | 11 +-
.../checkers/bugprone/unsafe-crtp.cpp | 172 ++++++++++++++++--
2 files changed, 166 insertions(+), 17 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
index 9295ba0009ac24..b97e0142420be4 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
@@ -110,9 +110,9 @@ void UnsafeCrtpCheck::check(const MatchFinder::MatchResult &Result) {
<< CRTPDeclaration
<< FixItHint::CreateInsertion(
CRTPDeclaration->getBraceRange().getBegin().getLocWithOffset(1),
- "private: " + CRTPDeclaration->getNameAsString() +
- "() = default;" +
- (CRTPDeclaration->isStruct() ? "\npublic:\n" : ""));
+ (CRTPDeclaration->isStruct() ? "\nprivate:\n" : "\n") +
+ CRTPDeclaration->getNameAsString() + "() = default;" +
+ (CRTPDeclaration->isStruct() ? "\npublic:\n" : "\n"));
diag(CRTPDeclaration->getLocation(), "consider making it private",
DiagnosticIDs::Note);
}
@@ -127,8 +127,9 @@ void UnsafeCrtpCheck::check(const MatchFinder::MatchResult &Result) {
"the CRTP cannot be constructed from the derived class")
<< CRTPDeclaration
<< FixItHint::CreateInsertion(
- CRTPDeclaration->getBraceRange().getEnd().getLocWithOffset(-1),
- "friend " + DerivedTemplateParameter->getNameAsString() + ';');
+ CRTPDeclaration->getBraceRange().getEnd(),
+ "friend " + DerivedTemplateParameter->getNameAsString() + ';' +
+ '\n');
diag(CRTPDeclaration->getLocation(),
"consider declaring the derived class as friend", DiagnosticIDs::Note);
}
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
index 90350ad7abc578..b8e38aa18e5e41 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-crtp.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-crtp.cpp
@@ -1,14 +1,162 @@
// RUN: %check_clang_tidy %s bugprone-unsafe-crtp %t
-// FIXME: Add something that triggers the check here.
-void f();
-// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: function 'f' is insufficiently awesome [bugprone-unsafe-crtp]
-
-// FIXME: Verify the applied fix.
-// * Make the CHECK patterns specific enough and try to make verified lines
-// unique to avoid incorrect matches.
-// * Use {{}} for regular expressions.
-// CHECK-FIXES: {{^}}void awesome_f();{{$}}
-
-// FIXME: Add something that doesn't trigger the check here.
-void awesome_f2();
+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
>From 4f4b728faa18b46166fe2e6c3cd44b31606e81c5 Mon Sep 17 00:00:00 2001
From: isuckatcs <65320245+isuckatcs at users.noreply.github.com>
Date: Tue, 20 Feb 2024 15:53:16 +0100
Subject: [PATCH 09/12] docs
---
.../clang-tidy/bugprone/UnsafeCrtpCheck.h | 2 +-
clang-tools-extra/docs/ReleaseNotes.rst | 2 +-
.../checks/bugprone/unsafe-crtp.rst | 58 ++++++++++++++++++-
3 files changed, 59 insertions(+), 3 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.h b/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.h
index 7c41bbb0678521..ac1a8f09208f95 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.h
+++ b/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.h
@@ -13,7 +13,7 @@
namespace clang::tidy::bugprone {
-/// FIXME: Write a short description.
+/// 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
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index a96a18bfbacf96..62914c5480a717 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -170,7 +170,7 @@ New checks
- New :doc:`bugprone-unsafe-crtp
<clang-tidy/checks/bugprone/unsafe-crtp>` check.
- FIXME: add release notes.
+ Detects error-prone CRTP.
- 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
index 3352af02a8744f..8e6955999512a6 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/bugprone/unsafe-crtp.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/unsafe-crtp.rst
@@ -3,4 +3,60 @@
bugprone-unsafe-crtp
====================
-FIXME: Describe what patterns does the check detect and why. Give examples.
+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;
>From 3e4ffa29ea30b035102b76cade2e383edd2c257d Mon Sep 17 00:00:00 2001
From: isuckatcs <65320245+isuckatcs at users.noreply.github.com>
Date: Tue, 20 Feb 2024 18:36:19 +0100
Subject: [PATCH 10/12] cleanup
---
.../clang-tidy/bugprone/UnsafeCrtpCheck.cpp | 19 ++++++++++---------
clang-tools-extra/docs/ReleaseNotes.rst | 2 +-
2 files changed, 11 insertions(+), 10 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
index b97e0142420be4..a1caf934ef2cd6 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
@@ -67,7 +67,7 @@ getDerivedParameter(const ClassTemplateSpecializationDecl *CRTP,
}
std::vector<FixItHint> hintMakeCtorPrivate(const CXXConstructorDecl *Ctor,
- std::string OriginalAccess,
+ const std::string &OriginalAccess,
const SourceManager &SM,
const LangOptions &LangOpts) {
std::vector<FixItHint> Hints;
@@ -75,12 +75,12 @@ std::vector<FixItHint> hintMakeCtorPrivate(const CXXConstructorDecl *Ctor,
Hints.emplace_back(FixItHint::CreateInsertion(
Ctor->getBeginLoc().getLocWithOffset(-1), "private:\n"));
- Hints.emplace_back(FixItHint::CreateInsertion(
+ SourceLocation CtorEndLoc =
Ctor->isExplicitlyDefaulted()
? utils::lexer::findNextTerminator(Ctor->getEndLoc(), SM, LangOpts)
- .getLocWithOffset(1)
- : Ctor->getEndLoc().getLocWithOffset(1),
- '\n' + OriginalAccess + ':' + '\n'));
+ : Ctor->getEndLoc();
+ Hints.emplace_back(FixItHint::CreateInsertion(
+ CtorEndLoc.getLocWithOffset(1), '\n' + OriginalAccess + ':' + '\n'));
return Hints;
}
@@ -100,19 +100,20 @@ void UnsafeCrtpCheck::check(const MatchFinder::MatchResult &Result) {
const auto *CRTPInstantiation =
Result.Nodes.getNodeAs<ClassTemplateSpecializationDecl>("crtp");
const auto *Derived = 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),
- (CRTPDeclaration->isStruct() ? "\nprivate:\n" : "\n") +
- CRTPDeclaration->getNameAsString() + "() = default;" +
- (CRTPDeclaration->isStruct() ? "\npublic:\n" : "\n"));
+ (IsStruct ? "\nprivate:\n" : "\n") +
+ CRTPDeclaration->getNameAsString() + "() = default;\n" +
+ (IsStruct ? "public:\n" : ""));
diag(CRTPDeclaration->getLocation(), "consider making it private",
DiagnosticIDs::Note);
}
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 62914c5480a717..1506e4631806a4 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -170,7 +170,7 @@ New checks
- New :doc:`bugprone-unsafe-crtp
<clang-tidy/checks/bugprone/unsafe-crtp>` check.
- Detects error-prone CRTP.
+ Detects error-prone CRTP usage.
- New :doc:`cppcoreguidelines-no-suspend-with-lock
<clang-tidy/checks/cppcoreguidelines/no-suspend-with-lock>` check.
>From 61e6cd5ee97950e2fabc8f394aa967a032f639ea Mon Sep 17 00:00:00 2001
From: isuckatcs <65320245+isuckatcs at users.noreply.github.com>
Date: Tue, 20 Feb 2024 18:50:41 +0100
Subject: [PATCH 11/12] more testing
---
.../checkers/bugprone/unsafe-crtp.cpp | 42 +++++++++++++++++++
1 file changed, 42 insertions(+)
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
index b8e38aa18e5e41..13bd19610ed942 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-crtp.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-crtp.cpp
@@ -160,3 +160,45 @@ class CRTP {
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>> {};
+
+template<>
+class A<int> : CRTP<A<int>> {};
+} // namespace template_derived_explicit_specialization
+
+namespace no_warning {
+template <typename T>
+class CRTP
+{
+ CRTP() = default;
+ friend T;
+};
+
+class A : CRTP<A> {};
+} // namespace no_warning
>From 732b59209f68ef98f414697ab5b62ce6c3b51c61 Mon Sep 17 00:00:00 2001
From: isuckatcs <65320245+isuckatcs at users.noreply.github.com>
Date: Tue, 20 Feb 2024 19:46:13 +0100
Subject: [PATCH 12/12] extend
---
.../clang-tidy/bugprone/UnsafeCrtpCheck.cpp | 21 ++++++++++----
.../checkers/bugprone/unsafe-crtp.cpp | 28 +++++++++++++++++++
2 files changed, 44 insertions(+), 5 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
index a1caf934ef2cd6..7bb50aaf950cbe 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/UnsafeCrtpCheck.cpp
@@ -34,12 +34,22 @@ bool hasPrivateConstructor(const CXXRecordDecl *RD) {
}
bool isDerivedParameterBefriended(const CXXRecordDecl *CRTP,
- const NamedDecl *Derived) {
+ const NamedDecl *Param) {
for (auto &&Friend : CRTP->friends()) {
const auto *TTPT =
dyn_cast<TemplateTypeParmType>(Friend->getFriendType()->getType());
- if (TTPT && TTPT->getDecl() == Derived)
+ 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;
}
@@ -99,7 +109,7 @@ void UnsafeCrtpCheck::registerMatchers(MatchFinder *Finder) {
void UnsafeCrtpCheck::check(const MatchFinder::MatchResult &Result) {
const auto *CRTPInstantiation =
Result.Nodes.getNodeAs<ClassTemplateSpecializationDecl>("crtp");
- const auto *Derived = Result.Nodes.getNodeAs<CXXRecordDecl>("derived");
+ const auto *DerivedRecord = Result.Nodes.getNodeAs<CXXRecordDecl>("derived");
const CXXRecordDecl *CRTPDeclaration =
CRTPInstantiation->getSpecializedTemplate()->getTemplatedDecl();
@@ -119,11 +129,12 @@ void UnsafeCrtpCheck::check(const MatchFinder::MatchResult &Result) {
}
const auto *DerivedTemplateParameter =
- *getDerivedParameter(CRTPInstantiation, Derived);
+ *getDerivedParameter(CRTPInstantiation, DerivedRecord);
if (hasPrivateConstructor(CRTPDeclaration) &&
!isDerivedParameterBefriended(CRTPDeclaration,
- DerivedTemplateParameter)) {
+ DerivedTemplateParameter) &&
+ !isDerivedClassBefriended(CRTPDeclaration, DerivedRecord)) {
diag(CRTPDeclaration->getLocation(),
"the CRTP cannot be constructed from the derived class")
<< CRTPDeclaration
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
index 13bd19610ed942..edcfd67677fd7b 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-crtp.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-crtp.cpp
@@ -192,6 +192,34 @@ template<>
class A<int> : CRTP<A<int>> {};
} // namespace template_derived_explicit_specialization
+namespace explicit_derived_friend {
+class A;
+
+template <typename T>
+class CRTP {
+ CRTP() = default;
+ friend A;
+};
+
+class A : CRTP<A> {};
+} // namespace explicit_derived_friend
+
+namespace explicit_derived_friend_multiple {
+class A;
+
+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;
+ friend A;
+};
+
+class A : CRTP<A> {};
+class B : CRTP<B> {};
+} // namespace explicit_derived_friend_multiple
+
namespace no_warning {
template <typename T>
class CRTP
More information about the cfe-commits
mailing list