[clang-tidy] new cppcoreguidelines-slicing

[courbet]

Summary

Flags slicing of member variables or vtable. See:

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

[Eugene.Zelenko]

Please mention this check in docs/ReleaseNotes.rst (in alphabetical order).

[courbet]

Update release notes.

[alexfh]

Thanks for the new awesome check!

Please run the check on LLVM and include your analysis of the results in the patch description. Another couple of comments below.

clang-tidy/cppcoreguidelines/SlicingCheck.cpp

94

Please use auto *

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

8

Links to the C++ Core Guidelines are good, but a few words of explanation and an example here wouldn't hurt either.

[hokein]

clang-tidy/cppcoreguidelines/SlicingCheck.cpp

49

Looks like you are missing some cases here, like assigning a Subclass object from a function call to a Baseclass object, passing a Subclass object to a function whose parameter is a BaseClass.

But I think it's fine to implement in a separate patch, but you'd better to add a TODO here.

SubClass f1();
BaseClass a = f1();

void f1(BaseClass a);
SubClass sc;
f1(sc);

85

There are two diagnostic messages in the code. For subclasses with override base class methods and extra member, this will print two messages, which is a bit of redundant.

95

The code can be simplified like:

if (const auto *BR = cast_or_null<CXXRecordDecl>(BaseRecordType->getDecl()->getDefinition())) {
  DiagnoseSlicedOverriddenMethods(Call, *BR, BaseDecl); 
}

113

s/method/methods

[courbet]

• Add a test case following comments.
• Add more comments in tests.
• Add examples in doc.
• Simplify code a bit.

[courbet]

Thanks for the comments.

clang-tidy/cppcoreguidelines/SlicingCheck.cpp

49

Actually these will still create a CXXConstructExpr in the AST, e.g for case (2):

CallExpr 0x3d6e560 </usr/local/google/home/courbet/test2.cc:10:3, col:6> 'void'
|-ImplicitCastExpr 0x3d6e548 <col:3> 'void (*)(class A)' <FunctionToPointerDecay>
| `-DeclRefExpr 0x3d6e4f8 <col:3> 'void (class A)' lvalue Function 0x3d66550 'g' 'void (class A)'
`-CXXConstructExpr 0x3d6e5c8 <col:5> 'class A' 'void (const class A &) throw()'
  `-ImplicitCastExpr 0x3d6e5b0 <col:5> 'const class A' lvalue <NoOp>
    `-ImplicitCastExpr 0x3d6e590 <col:5> 'class A' lvalue <DerivedToBase (A)>
      `-DeclRefExpr 0x3d6e4d0 <col:5> 'class B' lvalue Var 0x3d6e300 'b' 'class B'

I alreagy have a test to case (2) and I've added one for case (1).

85

I think we still want to point out to the user which overrides are going to be discarded:

class A { virtual void f(); virtual void g(); }
class B : public A { void f() override; void g() override; int a; }

The messages will be:
"slicing object from type 'B' to 'A' discards override 'f' "
"slicing object from type 'B' to 'A' discards override 'g' "
"slicing object from type 'B' to 'A' discards 4*sizeof(char) bytes of state"

[courbet]

I've ran this check on llvm. There are 0 instances of virtual function slicing (which is not surprising since these usually result in actual bugs) and 71 instances of member varaible slicing:

• 'FullSourceLoc' to 'SourceLocation': 57
• 'APSInt' to 'APInt': 5
• 'DeducedTemplateArgument' to 'TemplateArgument': 3
• 'SemaDiagnosticBuilder' to 'DiagnosticBuilder': 2
• 'RegHalf' to 'RegisterRef': 2
• 'PathDiagnosticRange' to 'SourceRange': 2

Most are harmless (but still true positives). The 'SemaDiagnosticBuilder' and 'APSInt' are actually interesting:

llvm/llvm/tools/clang/lib/Sema/SemaExprObjC.cpp:3555
DiagnosticBuilder DiagB = … should be: SemaDiagnosticBuilder DiagB = …
DiagB is then passed by reference to a function, so there really is no reason to slice it.

AST/ExprConstant.cpp:3150
explicit APSInt(APInt I, bool isUnsigned = true)
Here it’s easy to write: APSInt MyInt(MyOtherInt); and think it’s a copy, but it’s not (it just changed the signedness). APSInt MyInt(MyOtherInt.toAPInt()); would make it clear what’s happening.

[hokein]

clang-tidy/cppcoreguidelines/SlicingCheck.cpp

49

Oh, I see it now. Sorry for missing them and thanks for explanation. It'd be more clear to if you can update comments at SlicesObjectInCtor.

86

Maybe alexfh@ has idea about this. Output multiple warning messages in a same location is not the pattern of clang-tidy message. I think we should combine three message into exact one, something like "slicing object from type 'B' to 'A' discards override 'f', 'g', 4*sizeof(char) bytes of state".

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

25

There is an extra blank line at the bottom.

test/clang-tidy/cppcoreguidelines-slicing.cpp

96

s/tnow/now

[courbet]

Cosmetics.

[alexfh]

LG with a nit.

clang-tidy/cppcoreguidelines/SlicingCheck.cpp

87

The "slicing ... discards x bytes of state" message is not going to be repeated for the same location, so it's fine on its own.

Re: the "discards override" messages, they quite infrequent, IIUC, so the risk of spamming people with diagnostics is rather low. In case it's still problematic, we can change the repeated parts to notes.

92

Let's make the code more consistent:

if (const auto *BaseRecordType = Base.getType()->getAs<RecordType>()) {
  if (const auto *BaseRecord =
        cast_or_null<CXXRecordDecl>(BaseRecordType->getDecl()->getDefinition()))
      DiagnoseSlicedOverriddenMethods(Call, *BaseRecord, BaseDecl);
}

[courbet]

cosmetics

[courbet]

Thanks for the review.

clang-tidy/cppcoreguidelines/SlicingCheck.cpp

87

Thanks for the insight.

[courbet]

rebase

[Prazek]

clang-tidy/cppcoreguidelines/SlicingCheck.cpp

31

What is A and what is B? I guess you are missing very important fact, that A is a base class of B

clang-tidy/cppcoreguidelines/SlicingCheck.h

20

some short description what does this check do?

[courbet]

clang-tidy/cppcoreguidelines/SlicingCheck.h

20

There is already a more detialed explanation provided in extra/clang-tidy/checks/cppcoreguidelines-slicing.html, and I feel that the links explain it better than anything short we could put in the comments there. Or should I copy-paste what's in extra/clang-tidy/checks/cppcoreguidelines-slicing.html ?

[Prazek]

Is it in upstream yet?

clang-tidy/cppcoreguidelines/SlicingCheck.h

20

Well, it would be good to at least say what is slicing :)

[hokein]

Looks good now. Do you need I submit for you?

[courbet]

I can submit, thanks.

clang-tidy/cppcoreguidelines/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyCppCoreGuidelinesModule
   CppCoreGuidelinesTidyModule.cpp
   InterfacesGlobalInitCheck.cpp
   ProBoundsArrayToPointerDecayCheck.cpp
   ProBoundsConstantArrayIndexCheck.cpp
   ProBoundsPointerArithmeticCheck.cpp
   ProTypeConstCastCheck.cpp
   ProTypeCstyleCastCheck.cpp
   ProTypeMemberInitCheck.cpp
   ProTypeReinterpretCastCheck.cpp
   ProTypeStaticCastDowncastCheck.cpp
   ProTypeUnionAccessCheck.cpp
   ProTypeVarargCheck.cpp
+  SlicingCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyMiscModule
   clangTidyUtils
   clangTooling
   )

clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp

 #include "ProBoundsPointerArithmeticCheck.h"
 #include "ProTypeConstCastCheck.h"
 #include "ProTypeCstyleCastCheck.h"
 #include "ProTypeMemberInitCheck.h"
 #include "ProTypeReinterpretCastCheck.h"
 #include "ProTypeStaticCastDowncastCheck.h"
 #include "ProTypeUnionAccessCheck.h"
 #include "ProTypeVarargCheck.h"
+#include "SlicingCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace cppcoreguidelines {
 
 /// A module containing checks of the C++ Core Guidelines
 class CppCoreGuidelinesModule : public ClangTidyModule {
 public:
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<ProTypeReinterpretCastCheck>(
         "cppcoreguidelines-pro-type-reinterpret-cast");
     CheckFactories.registerCheck<ProTypeStaticCastDowncastCheck>(
         "cppcoreguidelines-pro-type-static-cast-downcast");
     CheckFactories.registerCheck<ProTypeUnionAccessCheck>(
         "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");
   }
 };
 
 // Register the LLVMTidyModule using this statically initialized variable.
 static ClangTidyModuleRegistry::Add<CppCoreGuidelinesModule>
     X("cppcoreguidelines-module", "Adds checks for the C++ Core Guidelines.");

clang-tidy/cppcoreguidelines/SlicingCheck.h

+//===--- 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-tidy/cppcoreguidelines/SlicingCheck.cpp

+//===--- 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

docs/ReleaseNotes.rst

 Improvements to clang-rename
 ----------------------------
 
 The improvements are...
 
 Improvements to clang-tidy
 --------------------------
 
-...
+- New `cppcoreguidelines-slicing
+  <http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-slicing.html>`_ check
+
+  Flags slicing of member variables or vtable.
 
 Clang-tidy changes from 3.9 to 4.0
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 ...
 
 Improvements to modularize
 --------------------------
 
 The improvements are...

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

+.. 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

test/clang-tidy/cppcoreguidelines-slicing.cpp

+// 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 4*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'
+}