diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt b/clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt index 65c40ed58619..77f064633279 100644 --- a/clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt +++ b/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 diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp index 97c352ba9521..608e2ee14a42 100644 --- a/clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp +++ b/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( "cppcoreguidelines-pro-type-vararg"); + CheckFactories.registerCheck( + "cppcoreguidelines-slicing"); CheckFactories.registerCheck( "cppcoreguidelines-c-copy-assignment-signature"); } diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/SlicingCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/SlicingCheck.cpp new file mode 100644 index 000000000000..2a6cf098e697 --- /dev/null +++ b/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(Method) || isa(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()) { + if (const auto *BaseRecord = cast_or_null( + BaseRecordType->getDecl()->getDefinition())) + DiagnoseSlicedOverriddenMethods(Call, *BaseRecord, BaseDecl); + } + } +} + +void SlicingCheck::check(const MatchFinder::MatchResult &Result) { + const auto *BaseDecl = Result.Nodes.getNodeAs("BaseDecl"); + const auto *DerivedDecl = + Result.Nodes.getNodeAs("DerivedDecl"); + const auto *Call = Result.Nodes.getNodeAs("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(StateSize.getQuantity()); + } +} + +} // namespace cppcoreguidelines +} // namespace tidy +} // namespace clang diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/SlicingCheck.h b/clang-tools-extra/clang-tidy/cppcoreguidelines/SlicingCheck.h new file mode 100644 index 000000000000..9ee91bcd9a53 --- /dev/null +++ b/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 diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index e70bad54acda..e2e9bdbb1b71 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -59,7 +59,10 @@ The improvements are... Improvements to clang-tidy -------------------------- -The improvements are... +- New `cppcoreguidelines-slicing + `_ check + + Flags slicing of member variables or vtable. Improvements to include-fixer ----------------------------- diff --git a/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-slicing.rst b/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-slicing.rst new file mode 100644 index 000000000000..0b1bb6beed4f --- /dev/null +++ b/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 diff --git a/clang-tools-extra/test/clang-tidy/cppcoreguidelines-slicing.cpp b/clang-tools-extra/test/clang-tidy/cppcoreguidelines-slicing.cpp new file mode 100644 index 000000000000..ca2291c85c78 --- /dev/null +++ b/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' +}