Skip to content

Commit 8e56fb8

Browse files
isuckatcsisuckatcs
authored
[clang-tidy] CRTP Constructor Accessibility Check (#82403)
Detects error-prone Curiously Recurring Template Pattern usage, when the CRTP can be constructed outside itself and the derived class.
1 parent 05390df commit 8e56fb8

File tree

8 files changed

+577
-0
lines changed

8 files changed

+577
-0
lines changed

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
#include "ChainedComparisonCheck.h"
2121
#include "ComparePointerToMemberVirtualFunctionCheck.h"
2222
#include "CopyConstructorInitCheck.h"
23+
#include "CrtpConstructorAccessibilityCheck.h"
2324
#include "DanglingHandleCheck.h"
2425
#include "DynamicStaticInitializersCheck.h"
2526
#include "EasilySwappableParametersCheck.h"
@@ -237,6 +238,8 @@ class BugproneModule : public ClangTidyModule {
237238
"bugprone-unhandled-exception-at-new");
238239
CheckFactories.registerCheck<UniquePtrArrayMismatchCheck>(
239240
"bugprone-unique-ptr-array-mismatch");
241+
CheckFactories.registerCheck<CrtpConstructorAccessibilityCheck>(
242+
"bugprone-crtp-constructor-accessibility");
240243
CheckFactories.registerCheck<UnsafeFunctionsCheck>(
241244
"bugprone-unsafe-functions");
242245
CheckFactories.registerCheck<UnusedLocalNonTrivialVariableCheck>(

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,7 @@ add_clang_library(clangTidyBugproneModule
7979
UnhandledExceptionAtNewCheck.cpp
8080
UnhandledSelfAssignmentCheck.cpp
8181
UniquePtrArrayMismatchCheck.cpp
82+
CrtpConstructorAccessibilityCheck.cpp
8283
UnsafeFunctionsCheck.cpp
8384
UnusedLocalNonTrivialVariableCheck.cpp
8485
UnusedRaiiCheck.cpp

‎clang-tools-extra/clang-tidy/bugprone/CrtpConstructorAccessibilityCheck.cpp

Lines changed: 181 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,181 @@
1+
//===--- CrtpConstructorAccessibilityCheck.cpp - clang-tidy ---------------===//
2+
//
3+
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
4+
// See https://llvm.org/LICENSE.txt for license information.
5+
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
6+
//
7+
//===----------------------------------------------------------------------===//
8+
9+
#include "CrtpConstructorAccessibilityCheck.h"
10+
#include "../utils/LexerUtils.h"
11+
#include "clang/ASTMatchers/ASTMatchFinder.h"
12+
13+
using namespace clang::ast_matchers;
14+
15+
namespace clang::tidy::bugprone {
16+
17+
static bool hasPrivateConstructor(const CXXRecordDecl *RD) {
18+
return llvm::any_of(RD->ctors(), [](const CXXConstructorDecl *Ctor) {
19+
return Ctor->getAccess() == AS_private;
20+
});
21+
}
22+
23+
static bool isDerivedParameterBefriended(const CXXRecordDecl *CRTP,
24+
const NamedDecl *Param) {
25+
return llvm::any_of(CRTP->friends(), [&](const FriendDecl *Friend) {
26+
const TypeSourceInfo *const FriendType = Friend->getFriendType();
27+
if (!FriendType) {
28+
return false;
29+
}
30+
31+
const auto *const TTPT =
32+
dyn_cast<TemplateTypeParmType>(FriendType->getType());
33+
34+
return TTPT && TTPT->getDecl() == Param;
35+
});
36+
}
37+
38+
static bool isDerivedClassBefriended(const CXXRecordDecl *CRTP,
39+
const CXXRecordDecl *Derived) {
40+
return llvm::any_of(CRTP->friends(), [&](const FriendDecl *Friend) {
41+
const TypeSourceInfo *const FriendType = Friend->getFriendType();
42+
if (!FriendType) {
43+
return false;
44+
}
45+
46+
return FriendType->getType()->getAsCXXRecordDecl() == Derived;
47+
});
48+
}
49+
50+
static const NamedDecl *
51+
getDerivedParameter(const ClassTemplateSpecializationDecl *CRTP,
52+
const CXXRecordDecl *Derived) {
53+
size_t Idx = 0;
54+
const bool AnyOf = llvm::any_of(
55+
CRTP->getTemplateArgs().asArray(), [&](const TemplateArgument &Arg) {
56+
++Idx;
57+
return Arg.getKind() == TemplateArgument::Type &&
58+
Arg.getAsType()->getAsCXXRecordDecl() == Derived;
59+
});
60+
61+
return AnyOf ? CRTP->getSpecializedTemplate()
62+
->getTemplateParameters()
63+
->getParam(Idx - 1)
64+
: nullptr;
65+
}
66+
67+
static std::vector<FixItHint>
68+
hintMakeCtorPrivate(const CXXConstructorDecl *Ctor,
69+
const std::string &OriginalAccess) {
70+
std::vector<FixItHint> Hints;
71+
72+
Hints.emplace_back(FixItHint::CreateInsertion(
73+
Ctor->getBeginLoc().getLocWithOffset(-1), "private:\n"));
74+
75+
const ASTContext &ASTCtx = Ctor->getASTContext();
76+
const SourceLocation CtorEndLoc =
77+
Ctor->isExplicitlyDefaulted()
78+
? utils::lexer::findNextTerminator(Ctor->getEndLoc(),
79+
ASTCtx.getSourceManager(),
80+
ASTCtx.getLangOpts())
81+
: Ctor->getEndLoc();
82+
Hints.emplace_back(FixItHint::CreateInsertion(
83+
CtorEndLoc.getLocWithOffset(1), '\n' + OriginalAccess + ':' + '\n'));
84+
85+
return Hints;
86+
}
87+
88+
void CrtpConstructorAccessibilityCheck::registerMatchers(MatchFinder *Finder) {
89+
Finder->addMatcher(
90+
classTemplateSpecializationDecl(
91+
decl().bind("crtp"),
92+
hasAnyTemplateArgument(refersToType(recordType(hasDeclaration(
93+
cxxRecordDecl(
94+
isDerivedFrom(cxxRecordDecl(equalsBoundNode("crtp"))))
95+
.bind("derived")))))),
96+
this);
97+
}
98+
99+
void CrtpConstructorAccessibilityCheck::check(
100+
const MatchFinder::MatchResult &Result) {
101+
const auto *CRTPInstantiation =
102+
Result.Nodes.getNodeAs<ClassTemplateSpecializationDecl>("crtp");
103+
const auto *DerivedRecord = Result.Nodes.getNodeAs<CXXRecordDecl>("derived");
104+
const CXXRecordDecl *CRTPDeclaration =
105+
CRTPInstantiation->getSpecializedTemplate()->getTemplatedDecl();
106+
107+
if (!CRTPDeclaration->hasDefinition()) {
108+
return;
109+
}
110+
111+
const auto *DerivedTemplateParameter =
112+
getDerivedParameter(CRTPInstantiation, DerivedRecord);
113+
114+
assert(DerivedTemplateParameter &&
115+
"No template parameter corresponds to the derived class of the CRTP.");
116+
117+
bool NeedsFriend = !isDerivedParameterBefriended(CRTPDeclaration,
118+
DerivedTemplateParameter) &&
119+
!isDerivedClassBefriended(CRTPDeclaration, DerivedRecord);
120+
121+
const FixItHint HintFriend = FixItHint::CreateInsertion(
122+
CRTPDeclaration->getBraceRange().getEnd(),
123+
"friend " + DerivedTemplateParameter->getNameAsString() + ';' + '\n');
124+
125+
if (hasPrivateConstructor(CRTPDeclaration) && NeedsFriend) {
126+
diag(CRTPDeclaration->getLocation(),
127+
"the CRTP cannot be constructed from the derived class; consider "
128+
"declaring the derived class as friend")
129+
<< HintFriend;
130+
}
131+
132+
auto WithFriendHintIfNeeded =
133+
[&](const DiagnosticBuilder &Diag,
134+
bool NeedsFriend) -> const DiagnosticBuilder & {
135+
if (NeedsFriend)
136+
Diag << HintFriend;
137+
138+
return Diag;
139+
};
140+
141+
if (!CRTPDeclaration->hasUserDeclaredConstructor()) {
142+
const bool IsStruct = CRTPDeclaration->isStruct();
143+
144+
WithFriendHintIfNeeded(
145+
diag(CRTPDeclaration->getLocation(),
146+
"the implicit default constructor of the CRTP is publicly "
147+
"accessible; consider making it private%select{| and declaring "
148+
"the derived class as friend}0")
149+
<< NeedsFriend
150+
<< FixItHint::CreateInsertion(
151+
CRTPDeclaration->getBraceRange().getBegin().getLocWithOffset(
152+
1),
153+
(IsStruct ? "\nprivate:\n" : "\n") +
154+
CRTPDeclaration->getNameAsString() + "() = default;\n" +
155+
(IsStruct ? "public:\n" : "")),
156+
NeedsFriend);
157+
}
158+
159+
for (auto &&Ctor : CRTPDeclaration->ctors()) {
160+
if (Ctor->getAccess() == AS_private)
161+
continue;
162+
163+
const bool IsPublic = Ctor->getAccess() == AS_public;
164+
const std::string Access = IsPublic ? "public" : "protected";
165+
166+
WithFriendHintIfNeeded(
167+
diag(Ctor->getLocation(),
168+
"%0 contructor allows the CRTP to be %select{inherited "
169+
"from|constructed}1 as a regular template class; consider making "
170+
"it private%select{| and declaring the derived class as friend}2")
171+
<< Access << IsPublic << NeedsFriend
172+
<< hintMakeCtorPrivate(Ctor, Access),
173+
NeedsFriend);
174+
}
175+
}
176+
177+
bool CrtpConstructorAccessibilityCheck::isLanguageVersionSupported(
178+
const LangOptions &LangOpts) const {
179+
return LangOpts.CPlusPlus11;
180+
}
181+
} // namespace clang::tidy::bugprone

‎clang-tools-extra/clang-tidy/bugprone/CrtpConstructorAccessibilityCheck.h

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
//===--- CrtpConstructorAccessibilityCheck.h - clang-tidy -------*- C++ -*-===//
2+
//
3+
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
4+
// See https://llvm.org/LICENSE.txt for license information.
5+
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
6+
//
7+
//===----------------------------------------------------------------------===//
8+
9+
#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_CRTPCONSTRUCTORACCESSIBILITYCHECK_H
10+
#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_CRTPCONSTRUCTORACCESSIBILITYCHECK_H
11+
12+
#include "../ClangTidyCheck.h"
13+
14+
namespace clang::tidy::bugprone {
15+
16+
/// Detects error-prone Curiously Recurring Template Pattern usage, when the
17+
/// CRTP can be constructed outside itself and the derived class.
18+
///
19+
/// For the user-facing documentation see:
20+
/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/crtp-constructor-accessibility.html
21+
class CrtpConstructorAccessibilityCheck : public ClangTidyCheck {
22+
public:
23+
CrtpConstructorAccessibilityCheck(StringRef Name, ClangTidyContext *Context)
24+
: ClangTidyCheck(Name, Context) {}
25+
void registerMatchers(ast_matchers::MatchFinder *Finder) override;
26+
void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
27+
bool isLanguageVersionSupported(const LangOptions &LangOpts) const override;
28+
};
29+
30+
} // namespace clang::tidy::bugprone
31+
32+
#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_CRTPCONSTRUCTORACCESSIBILITYCHECK_H

‎clang-tools-extra/docs/ReleaseNotes.rst

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,12 @@ Improvements to clang-tidy
104104
New checks
105105
^^^^^^^^^^
106106

107+
- New :doc:`bugprone-crtp-constructor-accessibility
108+
<clang-tidy/checks/bugprone/crtp-constructor-accessibility>` check.
109+
110+
Detects error-prone Curiously Recurring Template Pattern usage, when the CRTP
111+
can be constructed outside itself and the derived class.
112+
107113
- New :doc:`modernize-use-designated-initializers
108114
<clang-tidy/checks/modernize/use-designated-initializers>` check.
109115

‎clang-tools-extra/docs/clang-tidy/checks/bugprone/crtp-constructor-accessibility.rst

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,98 @@
1+
.. title:: clang-tidy - bugprone-crtp-constructor-accessibility
2+
3+
bugprone-crtp-constructor-accessibility
4+
=======================================
5+
6+
Detects error-prone Curiously Recurring Template Pattern usage, when the CRTP
7+
can be constructed outside itself and the derived class.
8+
9+
The CRTP is an idiom, in which a class derives from a template class, where
10+
itself is the template argument. It should be ensured that if a class is
11+
intended to be a base class in this idiom, it can only be instantiated if
12+
the derived class is it's template argument.
13+
14+
Example:
15+
16+
.. code-block:: c++
17+
18+
template <typename T> class CRTP {
19+
private:
20+
CRTP() = default;
21+
friend T;
22+
};
23+
24+
class Derived : CRTP<Derived> {};
25+
26+
Below can be seen some common mistakes that will allow the breaking of the
27+
idiom.
28+
29+
If the constructor of a class intended to be used in a CRTP is public, then
30+
it allows users to construct that class on its own.
31+
32+
Example:
33+
34+
.. code-block:: c++
35+
36+
template <typename T> class CRTP {
37+
public:
38+
CRTP() = default;
39+
};
40+
41+
class Good : CRTP<Good> {};
42+
Good GoodInstance;
43+
44+
CRTP<int> BadInstance;
45+
46+
If the constructor is protected, the possibility of an accidental instantiation
47+
is prevented, however it can fade an error, when a different class is used as
48+
the template parameter instead of the derived one.
49+
50+
Example:
51+
52+
.. code-block:: c++
53+
54+
template <typename T> class CRTP {
55+
protected:
56+
CRTP() = default;
57+
};
58+
59+
class Good : CRTP<Good> {};
60+
Good GoodInstance;
61+
62+
class Bad : CRTP<Good> {};
63+
Bad BadInstance;
64+
65+
To ensure that no accidental instantiation happens, the best practice is to
66+
make the constructor private and declare the derived class as friend. Note
67+
that as a tradeoff, this also gives the derived class access to every other
68+
private members of the CRTP.
69+
70+
Example:
71+
72+
.. code-block:: c++
73+
74+
template <typename T> class CRTP {
75+
CRTP() = default;
76+
friend T;
77+
};
78+
79+
class Good : CRTP<Good> {};
80+
Good GoodInstance;
81+
82+
class Bad : CRTP<Good> {};
83+
Bad CompileTimeError;
84+
85+
CRTP<int> AlsoCompileTimeError;
86+
87+
Limitations:
88+
89+
* The check is not supported below C++11
90+
91+
* The check does not handle when the derived class is passed as a variadic
92+
template argument
93+
94+
* Accessible functions that can construct the CRTP, like factory functions
95+
are not checked
96+
97+
The check also suggests a fix-its in some cases.
98+

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,7 @@ Clang-Tidy Checks
8686
:doc:`bugprone-chained-comparison <bugprone/chained-comparison>`,
8787
:doc:`bugprone-compare-pointer-to-member-virtual-function <bugprone/compare-pointer-to-member-virtual-function>`,
8888
:doc:`bugprone-copy-constructor-init <bugprone/copy-constructor-init>`, "Yes"
89+
:doc:`bugprone-crtp-constructor-accessibility <bugprone/crtp-constructor-accessibility>`, "Yes"
8990
:doc:`bugprone-dangling-handle <bugprone/dangling-handle>`,
9091
:doc:`bugprone-dynamic-static-initializers <bugprone/dynamic-static-initializers>`,
9192
:doc:`bugprone-easily-swappable-parameters <bugprone/easily-swappable-parameters>`,

0 commit comments

Comments
 (0)