Revert "Revert "[clang-tidy] new cppcoreguidelines-slicing""

Second try for r276408

llvm-svn: 276415

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

@@ -13,6 +13,7 @@ add_clang_library(clangTidyCppCoreGuidelinesModule
   ProTypeStaticCastDowncastCheck.cpp
   ProTypeUnionAccessCheck.cpp
   ProTypeVarargCheck.cpp
+  SlicingCheck.cpp
 
   LINK_LIBS
   clangAST

clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp

@@ -22,6 +22,7 @@
 #include "ProTypeStaticCastDowncastCheck.h"
 #include "ProTypeUnionAccessCheck.h"
 #include "ProTypeVarargCheck.h"
+#include "SlicingCheck.h"
 
 namespace clang {
 namespace tidy {
@@ -53,6 +54,8 @@ public:
         "cppcoreguidelines-pro-type-union-access");
     CheckFactories.registerCheck<ProTypeVarargCheck>(
         "cppcoreguidelines-pro-type-vararg");
+    CheckFactories.registerCheck<SlicingCheck>(
+        "cppcoreguidelines-slicing");
     CheckFactories.registerCheck<misc::UnconventionalAssignOperatorCheck>(
         "cppcoreguidelines-c-copy-assignment-signature");
   }

clang-tools-extra/clang-tidy/cppcoreguidelines/SlicingCheck.cpp

@@ -0,0 +1,135 @@
+//===--- SlicingCheck.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 "SlicingCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/RecordLayout.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace cppcoreguidelines {
+
+void SlicingCheck::registerMatchers(MatchFinder *Finder) {
+  // When we see:
+  //   class B : public A { ... };
+  //   A a;
+  //   B b;
+  //   a = b;
+  // The assignment is OK if:
+  //   - the assignment operator is defined as taking a B as second parameter,
+  //   or
+  //   - B does not define any additional members (either variables or
+  //   overrides) wrt A.
+  //
+  // The same holds for copy ctor calls. This also captures stuff like:
+  //   void f(A a);
+  //   f(b);
+
+  //  Helpers.
+  const auto OfBaseClass = ofClass(cxxRecordDecl().bind("BaseDecl"));
+  const auto IsDerivedFromBaseDecl =
+      cxxRecordDecl(isDerivedFrom(equalsBoundNode("BaseDecl")))
+          .bind("DerivedDecl");
+  const auto HasTypeDerivedFromBaseDecl =
+      anyOf(hasType(IsDerivedFromBaseDecl),
+            hasType(references(IsDerivedFromBaseDecl)));
+  const auto IsWithinDerivedCtor =
+      hasParent(cxxConstructorDecl(ofClass(equalsBoundNode("DerivedDecl"))));
+
+  // Assignement slicing: "a = b;" and "a = std::move(b);" variants.
+  const auto SlicesObjectInAssignment =
+      callExpr(callee(cxxMethodDecl(anyOf(isCopyAssignmentOperator(),
+                                          isMoveAssignmentOperator()),
+                                    OfBaseClass)),
+               hasArgument(1, HasTypeDerivedFromBaseDecl));
+
+  // Construction slicing: "A a{b};" and "f(b);" variants. Note that in case of
+  // slicing the letter will create a temporary and therefore call a ctor.
+  const auto SlicesObjectInCtor = cxxConstructExpr(
+      hasDeclaration(cxxConstructorDecl(
+          anyOf(isCopyConstructor(), isMoveConstructor()), OfBaseClass)),
+      hasArgument(0, HasTypeDerivedFromBaseDecl),
+      // We need to disable matching on the call to the base copy/move
+      // constructor in DerivedDecl's constructors.
+      unless(IsWithinDerivedCtor));
+
+  Finder->addMatcher(
+      expr(anyOf(SlicesObjectInAssignment, SlicesObjectInCtor)).bind("Call"),
+      this);
+}
+
+/// Warns on methods overridden in DerivedDecl with respect to BaseDecl.
+/// FIXME: this warns on all overrides outside of the sliced path in case of
+/// multiple inheritance.
+void SlicingCheck::DiagnoseSlicedOverriddenMethods(
+    const Expr &Call, const CXXRecordDecl &DerivedDecl,
+    const CXXRecordDecl &BaseDecl) {
+  if (DerivedDecl.getCanonicalDecl() == BaseDecl.getCanonicalDecl())
+    return;
+  for (const auto &Method : DerivedDecl.methods()) {
+    // Virtual destructors are OK. We're ignoring constructors since they are
+    // tagged as overrides.
+    if (isa<CXXConstructorDecl>(Method) || isa<CXXDestructorDecl>(Method))
+      continue;
+    if (Method->size_overridden_methods() > 0) {
+      diag(Call.getExprLoc(),
+           "slicing object from type %0 to %1 discards override %2")
+          << &DerivedDecl << &BaseDecl << Method;
+    }
+  }
+  // Recursively process bases.
+  for (const auto &Base : DerivedDecl.bases()) {
+    if (const auto *BaseRecordType = Base.getType()->getAs<RecordType>()) {
+      if (const auto *BaseRecord = cast_or_null<CXXRecordDecl>(
+              BaseRecordType->getDecl()->getDefinition()))
+        DiagnoseSlicedOverriddenMethods(Call, *BaseRecord, BaseDecl);
+    }
+  }
+}
+
+void SlicingCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *BaseDecl = Result.Nodes.getNodeAs<CXXRecordDecl>("BaseDecl");
+  const auto *DerivedDecl =
+      Result.Nodes.getNodeAs<CXXRecordDecl>("DerivedDecl");
+  const auto *Call = Result.Nodes.getNodeAs<Expr>("Call");
+  assert(BaseDecl != nullptr);
+  assert(DerivedDecl != nullptr);
+  assert(Call != nullptr);
+
+  // Warn when slicing the vtable.
+  // We're looking through all the methods in the derived class and see if they
+  // override some methods in the base class.
+  // It's not enough to just test whether the class is polymorphic because we
+  // would be fine slicing B to A if no method in B (or its bases) overrides
+  // anything in A:
+  //   class A { virtual void f(); };
+  //   class B : public A {};
+  // because in that case calling A::f is the same as calling B::f.
+  DiagnoseSlicedOverriddenMethods(*Call, *DerivedDecl, *BaseDecl);
+
+  // Warn when slicing member variables.
+  const auto &BaseLayout =
+      BaseDecl->getASTContext().getASTRecordLayout(BaseDecl);
+  const auto &DerivedLayout =
+      DerivedDecl->getASTContext().getASTRecordLayout(DerivedDecl);
+  const auto StateSize = DerivedLayout.getDataSize() - BaseLayout.getDataSize();
+  if (StateSize.isPositive()) {
+    diag(Call->getExprLoc(), "slicing object from type %0 to %1 discards "
+                             "%2*sizeof(char) bytes of state")
+        << DerivedDecl << BaseDecl << static_cast<int>(StateSize.getQuantity());
+  }
+}
+
+} // namespace cppcoreguidelines
+} // namespace tidy
+} // namespace clang

clang-tools-extra/clang-tidy/cppcoreguidelines/SlicingCheck.h

@@ -0,0 +1,45 @@
+//===--- SlicingCheck.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_CPPCOREGUIDELINES_SLICING_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_SLICING_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace cppcoreguidelines {
+
+/// Flags slicing (incomplete copying of an object's state) of member variables
+/// or vtable. See:
+///   - https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es63-dont-slice
+///     for the former, and
+///   - https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#c145-access-polymorphic-objects-through-pointers-and-references
+///     for the latter
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-slicing.html
+class SlicingCheck : public ClangTidyCheck {
+public:
+  SlicingCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  void DiagnoseSlicedOverriddenMethods(const Expr &call,
+                                       const CXXRecordDecl &DerivedDecl,
+                                       const CXXRecordDecl &BaseDecl);
+};
+
+} // namespace cppcoreguidelines
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_SLICING_H

clang-tools-extra/docs/ReleaseNotes.rst

@@ -59,7 +59,10 @@ The improvements are...
 Improvements to clang-tidy
 --------------------------
 
-The improvements are...
+- New `cppcoreguidelines-slicing
+  <http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-slicing.html>`_ check
+
+  Flags slicing of member variables or vtable.
 
 Improvements to include-fixer
 -----------------------------

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-slicing.rst

@@ -0,0 +1,23 @@
+.. title:: clang-tidy - cppcoreguidelines-slicing
+
+cppcoreguidelines-slicing
+=========================
+
+Flags slicing of member variables or vtable. Slicing happens when copying a
+derived object into a base object: the members of the derived object (both
+member variables and virtual member functions) will be discarded.
+This can be misleading especially for member function slicing, for example:
+
+.. code:: c++
+
+	struct B { int a; virtual int f(); };
+	struct D : B { int b; int f() override; };
+	void use(B b) {  // Missing reference, intended ?
+	  b.f();  // Calls B::f.
+	}
+	D d;
+	use(d);  // Slice.
+
+See the relevant CppCoreGuidelines sections for details:
+https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es63-dont-slice
+https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#c145-access-polymorphic-objects-through-pointers-and-references

clang-tools-extra/test/clang-tidy/cppcoreguidelines-slicing.cpp

@@ -0,0 +1,100 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-slicing %t
+
+class Base {
+  int i;
+  void f() {}
+  virtual void g() {}
+};
+
+class DerivedWithMemberVariables : public Base {
+  void f();
+  int j;
+};
+
+class TwiceDerivedWithNoMemberVariables : public DerivedWithMemberVariables {
+  void f();
+};
+
+class DerivedWithOverride : public Base {
+  void f();
+  void g() override {}
+};
+
+class TwiceDerivedWithNoOverride : public DerivedWithOverride {
+  void f();
+};
+
+void TakesBaseByValue(Base base);
+
+DerivedWithMemberVariables ReturnsDerived();
+
+void positivesWithMemberVariables() {
+  DerivedWithMemberVariables b;
+  Base a{b};
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: slicing object from type 'DerivedWithMemberVariables' to 'Base' discards {{[0-9]*}}*sizeof(char) bytes of state [cppcoreguidelines-slicing]
+  a = b;
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: slicing object from type 'DerivedWithMemberVariables' to 'Base' discards {{[0-9]*}}*sizeof(char) bytes of state
+  TakesBaseByValue(b);
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: slicing object from type 'DerivedWithMemberVariables' to 'Base' discards {{[0-9]*}}*sizeof(char) bytes of state
+
+  TwiceDerivedWithNoMemberVariables c;
+  a = c;
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: slicing object from type 'TwiceDerivedWithNoMemberVariables' to 'Base' discards {{[0-9]*}}*sizeof(char) bytes of state
+
+  a = ReturnsDerived();
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: slicing object from type 'DerivedWithMemberVariables' to 'Base' discards {{[0-9]*}}*sizeof(char) bytes of state
+}
+
+void positivesWithOverride() {
+  DerivedWithOverride b;
+  Base a{b};
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: slicing object from type 'DerivedWithOverride' to 'Base' discards override 'g'
+  a = b;
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: slicing object from type 'DerivedWithOverride' to 'Base' discards override 'g'
+  TakesBaseByValue(b);
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: slicing object from type 'DerivedWithOverride' to 'Base' discards override 'g'
+
+  TwiceDerivedWithNoOverride c;
+  a = c;
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: slicing object from type 'DerivedWithOverride' to 'Base' discards override 'g'
+}
+
+void TakesBaseByReference(Base &base);
+
+class DerivedThatAddsVirtualH : public Base {
+  virtual void h();
+};
+
+class DerivedThatOverridesH : public DerivedThatAddsVirtualH {
+  void h() override;
+};
+
+void negatives() {
+  // OK, simple copying from the same type.
+  Base a;
+  TakesBaseByValue(a);
+  DerivedWithMemberVariables b;
+  DerivedWithMemberVariables c{b};
+  b = c;
+
+  // OK, derived type does not have extra state.
+  TwiceDerivedWithNoMemberVariables d;
+  DerivedWithMemberVariables e{d};
+  e = d;
+
+  // OK, derived does not override any method.
+  TwiceDerivedWithNoOverride f;
+  DerivedWithOverride g{f};
+  g = f;
+
+  // OK, no copying.
+  TakesBaseByReference(d);
+  TakesBaseByReference(f);
+
+  // Derived type overrides methods, but these methods are not in the base type,
+  // so cannot be called accidentally. Right now this triggers, but we might
+  // want to allow it.
+  DerivedThatOverridesH h;
+  a = h;
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: slicing object from type 'DerivedThatOverridesH' to 'Base' discards override 'h'
+}