[clang-tools-extra] [clang-tidy] Unsafe CRTP check (PR #82403)

[via cfe-commits]

================
@@ -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() + ';' +
----------------
isuckatcs wrote:

Well, before C++11 befriending typenames is not allowed at all. See [cppreference](https://en.cppreference.com/w/cpp/language/friend).

`friend class T` has a different meaning, like `class T` is a friend and not the template parameter. See [godbolt](https://godbolt.org/z/4sq3KeEG7).

Since the problem this check aims to address cannot be addressed before C++11, we should disable the check before that version of the standard.

By the way the compilers don't seem to care, except for MSVC, which doesn't allow you to go back in standard. See [godbolt](https://godbolt.org/z/a8M3a4v9a).

https://github.com/llvm/llvm-project/pull/82403