[clang-tidy] Add a check for undelegated copy of base classes

Finds copy constructors where the constructor don't call
the copy constructor of the base class.

```
class X : public Copyable {
    X(const X &other) {} // Copyable(other) is missing
};
```

Differential Revision: https://reviews.llvm.org/D33722

llvm-svn: 318522

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

@@ -10,6 +10,7 @@
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
+#include "CopyConstructorInitCheck.h"
 #include "IntegerDivisionCheck.h"
 #include "SuspiciousMemsetUsageCheck.h"
 #include "UndefinedMemoryManipulationCheck.h"
@@ -21,6 +22,8 @@ namespace bugprone {
 class BugproneModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+    CheckFactories.registerCheck<CopyConstructorInitCheck>(
+        "misc-copy-constructor-init");
     CheckFactories.registerCheck<IntegerDivisionCheck>(
         "bugprone-integer-division");
     CheckFactories.registerCheck<SuspiciousMemsetUsageCheck>(

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

@@ -2,6 +2,7 @@ set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyBugproneModule
   BugproneTidyModule.cpp
+  CopyConstructorInitCheck.cpp
   IntegerDivisionCheck.cpp
   SuspiciousMemsetUsageCheck.cpp
   UndefinedMemoryManipulationCheck.cpp

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

@@ -0,0 +1,121 @@
+//===--- CopyConstructorInitCheck.cpp - clang-tidy-------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "CopyConstructorInitCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+void CopyConstructorInitCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus)
+    return;
+
+  // In the future this might be extended to move constructors?
+  Finder->addMatcher(
+      cxxConstructorDecl(
+          isCopyConstructor(),
+          hasAnyConstructorInitializer(cxxCtorInitializer(
+              isBaseInitializer(),
+              withInitializer(cxxConstructExpr(hasDeclaration(
+                  cxxConstructorDecl(isDefaultConstructor())))))),
+          unless(isInstantiated()))
+          .bind("ctor"),
+      this);
+}
+
+void CopyConstructorInitCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *Ctor = Result.Nodes.getNodeAs<CXXConstructorDecl>("ctor");
+  std::string ParamName = Ctor->getParamDecl(0)->getNameAsString();
+
+  // We want only one warning (and FixIt) for each ctor.
+  std::string FixItInitList;
+  bool HasRelevantBaseInit = false;
+  bool ShouldNotDoFixit = false;
+  bool HasWrittenInitializer = false;
+  SmallVector<FixItHint, 2> SafeFixIts;
+  for (const auto *Init : Ctor->inits()) {
+    bool CtorInitIsWritten = Init->isWritten();
+    HasWrittenInitializer = HasWrittenInitializer || CtorInitIsWritten;
+    if (!Init->isBaseInitializer())
+      continue;
+    const Type *BaseType = Init->getBaseClass();
+    // Do not do fixits if there is a type alias involved or one of the bases
+    // are explicitly initialized. In the latter case we not do fixits to avoid
+    // -Wreorder warnings.
+    if (const auto *TempSpecTy = dyn_cast<TemplateSpecializationType>(BaseType))
+      ShouldNotDoFixit = ShouldNotDoFixit || TempSpecTy->isTypeAlias();
+    ShouldNotDoFixit = ShouldNotDoFixit || isa<TypedefType>(BaseType);
+    ShouldNotDoFixit = ShouldNotDoFixit || CtorInitIsWritten;
+    const CXXRecordDecl *BaseClass =
+        BaseType->getAsCXXRecordDecl()->getDefinition();
+    if (BaseClass->field_empty() &&
+        BaseClass->forallBases(
+            [](const CXXRecordDecl *Class) { return Class->field_empty(); }))
+      continue;
+    bool NonCopyableBase = false;
+    for (const auto *Ctor : BaseClass->ctors()) {
+      if (Ctor->isCopyConstructor() &&
+          (Ctor->getAccess() == AS_private || Ctor->isDeleted())) {
+        NonCopyableBase = true;
+        break;
+      }
+    }
+    if (NonCopyableBase)
+      continue;
+    const auto *CExpr = dyn_cast<CXXConstructExpr>(Init->getInit());
+    if (!CExpr || !CExpr->getConstructor()->isDefaultConstructor())
+      continue;
+    HasRelevantBaseInit = true;
+    if (CtorInitIsWritten) {
+      if (!ParamName.empty())
+        SafeFixIts.push_back(
+            FixItHint::CreateInsertion(CExpr->getLocEnd(), ParamName));
+    } else {
+      if (Init->getSourceLocation().isMacroID() ||
+          Ctor->getLocation().isMacroID() || ShouldNotDoFixit)
+        break;
+      FixItInitList += BaseClass->getNameAsString();
+      FixItInitList += "(" + ParamName + "), ";
+    }
+  }
+  if (!HasRelevantBaseInit)
+    return;
+
+  auto Diag = diag(Ctor->getLocation(),
+                   "calling a base constructor other than the copy constructor")
+              << SafeFixIts;
+
+  if (FixItInitList.empty() || ParamName.empty() || ShouldNotDoFixit)
+    return;
+
+  std::string FixItMsg{FixItInitList.substr(0, FixItInitList.size() - 2)};
+  SourceLocation FixItLoc;
+  // There is no initialization list in this constructor.
+  if (!HasWrittenInitializer) {
+    FixItLoc = Ctor->getBody()->getLocStart();
+    FixItMsg = " : " + FixItMsg;
+  } else {
+    // We apply the missing ctors at the beginning of the initialization list.
+    FixItLoc = (*Ctor->init_begin())->getSourceLocation();
+    FixItMsg += ',';
+  }
+  FixItMsg += ' ';
+
+  Diag << FixItHint::CreateInsertion(FixItLoc, FixItMsg);
+} // namespace misc
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang

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

@@ -0,0 +1,36 @@
+//===--- CopyConstructorInitCheck.h - clang-tidy--------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_COPY_CONSTRUCTOR_INIT_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_COPY_CONSTRUCTOR_INIT_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+/// Finds copy constructors where the ctor don't call the copy constructor of
+/// the base class.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/misc-copy-constructor-init.html
+class CopyConstructorInitCheck : public ClangTidyCheck {
+public:
+  CopyConstructorInitCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_COPY_CONSTRUCTOR_INIT_H

clang-tools-extra/docs/ReleaseNotes.rst

@@ -136,6 +136,11 @@ Improvements to clang-tidy
   Checks if the required file flag ``MFD_CLOEXEC`` is present in the argument
   of ``memfd_create()``.
 
+- New `bugprone-copy-constructor-init
+  <http://clang.llvm.org/extra/clang-tidy/checks/bugprone-copy-constructor-init.html>`_ check
+
+  Finds copy constructors which don't call the copy constructor of the base class.
+
 - New `bugprone-integer-division
   <http://clang.llvm.org/extra/clang-tidy/checks/bugprone-integer-division.html>`_ check
 

clang-tools-extra/docs/clang-tidy/checks/bugprone-copy-constructor-init.rst

@@ -0,0 +1,29 @@
+.. title:: clang-tidy - bugprone-copy-constructor-init
+
+bugprone-copy-constructor-init
+==============================
+
+Finds copy constructors where the constructor doesn't call
+the copy constructor of the base class.
+
+.. code-block:: c++
+
+    class Copyable {
+    public:
+      Copyable() = default;
+      Copyable(const Copyable &) = default;
+    };
+    class X2 : public Copyable {
+      X2(const X2 &other) {} // Copyable(other) is missing
+    };
+
+Also finds copy constructors where the constructor of
+the base class don't have parameter.
+
+.. code-block:: c++
+
+    class X4 : public Copyable {
+      X4(const X4 &other) : Copyable() {} // other is missing
+    };
+
+The check also suggests a fix-its in some cases.

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

@@ -17,6 +17,7 @@ Clang-Tidy Checks
    android-cloexec-open
    android-cloexec-socket
    boost-use-to-string
+   bugprone-copy-constructor-init
    bugprone-integer-division
    bugprone-suspicious-memset-usage
    bugprone-undefined-memory-manipulation