Index: clang-tidy/bugprone/BugproneTidyModule.cpp =================================================================== --- clang-tidy/bugprone/BugproneTidyModule.cpp +++ clang-tidy/bugprone/BugproneTidyModule.cpp @@ -27,6 +27,7 @@ #include "MisplacedWideningCastCheck.h" #include "MoveForwardingReferenceCheck.h" #include "MultipleStatementMacroCheck.h" +#include "ParentVirtualCallCheck.h" #include "StringConstructorCheck.h" #include "StringIntegerAssignmentCheck.h" #include "StringLiteralWithEmbeddedNulCheck.h" @@ -83,6 +84,8 @@ "bugprone-move-forwarding-reference"); CheckFactories.registerCheck( "bugprone-multiple-statement-macro"); + CheckFactories.registerCheck( + "bugprone-parent-virtual-call"); CheckFactories.registerCheck( "bugprone-string-constructor"); CheckFactories.registerCheck( Index: clang-tidy/bugprone/CMakeLists.txt =================================================================== --- clang-tidy/bugprone/CMakeLists.txt +++ clang-tidy/bugprone/CMakeLists.txt @@ -19,6 +19,7 @@ MisplacedWideningCastCheck.cpp MoveForwardingReferenceCheck.cpp MultipleStatementMacroCheck.cpp + ParentVirtualCallCheck.cpp StringConstructorCheck.cpp StringIntegerAssignmentCheck.cpp StringLiteralWithEmbeddedNulCheck.cpp Index: clang-tidy/bugprone/ParentVirtualCallCheck.h =================================================================== --- /dev/null +++ clang-tidy/bugprone/ParentVirtualCallCheck.h @@ -0,0 +1,35 @@ +//===--- ParentVirtualCallCheck.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_PARENTVIRTUALCALLCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_PARENTVIRTUALCALLCHECK_H + +#include "../ClangTidy.h" + +namespace clang { +namespace tidy { +namespace bugprone { + +/// Finds calls to grand..-parent virtual methods instead of parent's. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone-parent-virtual-call.html +class ParentVirtualCallCheck : public ClangTidyCheck { +public: + ParentVirtualCallCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; +}; + +} // namespace bugprone +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_PARENTVIRTUALCALLCHECK_H Index: clang-tidy/bugprone/ParentVirtualCallCheck.cpp =================================================================== --- /dev/null +++ clang-tidy/bugprone/ParentVirtualCallCheck.cpp @@ -0,0 +1,126 @@ +//===--- ParentVirtualCallCheck.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 "ParentVirtualCallCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "llvm/ADT/STLExtras.h" +#include "llvm/ADT/SmallVector.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace bugprone { + +using BasesVector = llvm::SmallVector; + +static bool IsParentOf(const CXXRecordDecl &Parent, + const CXXRecordDecl &ThisClass) { + if (Parent.getCanonicalDecl() == ThisClass.getCanonicalDecl()) + return true; + const auto ClassIter = llvm::find_if(ThisClass.bases(), [=](auto &Base) { + auto *BaseDecl = Base.getType()->getAsCXXRecordDecl(); + assert(BaseDecl); + return Parent.getCanonicalDecl() == BaseDecl->getCanonicalDecl(); + }); + return ClassIter != ThisClass.bases_end(); +} + +static BasesVector GetParentsByGrandParent(const CXXRecordDecl &GrandParent, + const CXXRecordDecl &ThisClass) { + BasesVector result; + for (auto &Base : ThisClass.bases()) { + auto *BaseDecl = Base.getType()->getAsCXXRecordDecl(); + if (BaseDecl->getCanonicalDecl()->isDerivedFrom(&GrandParent)) + result.emplace_back(BaseDecl->getCanonicalDecl()); + } + return result; +} + +static std::string getNameAsString(const NamedDecl *Decl) { + std::string QualName; + llvm::raw_string_ostream OS(QualName); + PrintingPolicy PP(Decl->getASTContext().getPrintingPolicy()); + PP.SuppressUnwrittenScope = true; + Decl->printQualifiedName(OS, PP); + return OS.str(); +} + +void ParentVirtualCallCheck::registerMatchers(MatchFinder *Finder) { + Finder->addMatcher( + cxxMemberCallExpr( + callee(memberExpr(hasDescendant(implicitCastExpr( + hasImplicitDestinationType(pointsTo( + type(anything()).bind("castToType"))), + hasSourceExpression(cxxThisExpr(hasType( + type(anything()).bind("thisType"))))))) + .bind("member")), + callee(cxxMethodDecl(isVirtual()))) + .bind("call"), + this); +} + +void ParentVirtualCallCheck::check(const MatchFinder::MatchResult &Result) { + const auto *MatchedDecl = Result.Nodes.getNodeAs("call"); + assert(MatchedDecl); + + auto *Member = Result.Nodes.getNodeAs("member"); + assert(Member); + + auto *ThisTypePtr = Result.Nodes.getNodeAs("thisType"); + assert(ThisTypePtr); + + auto *ThisType = ThisTypePtr->getPointeeCXXRecordDecl(); + assert(ThisType); + + auto *CastToTypePtr = Result.Nodes.getNodeAs("castToType"); + clang::CXXRecordDecl *CastToType = nullptr; + + if (CastToTypePtr) + CastToType = CastToTypePtr->getAsCXXRecordDecl(); + else if (auto *TemplateSpecializationTypePtr = + Result.Nodes.getNodeAs("castToType")) + CastToType = TemplateSpecializationTypePtr->getAsCXXRecordDecl(); + assert(CastToType); + + if (IsParentOf(*CastToType, *ThisType)) + return; + + const BasesVector Parents = GetParentsByGrandParent(*CastToType, *ThisType); + assert(!Parents.empty()); + + auto ParentIter = Parents.begin(); + std::string ParentsStr = "'" + getNameAsString(*ParentIter) + "'"; + for (++ParentIter; ParentIter != Parents.end(); ++ParentIter) + ParentsStr += " or '" + getNameAsString(*ParentIter) + "'"; + + if (!Member->getQualifier()) + return; + + std::string CalleeName = + getNameAsString(MatchedDecl->getCalleeDecl()->getAsFunction()); + assert(Member->getQualifierLoc().getSourceRange().getBegin().isValid()); + auto Diag = diag(Member->getQualifierLoc().getSourceRange().getBegin(), + "qualified function name %0 refers to a function not from a " + "direct base class; did you mean %1?") + << CalleeName << ParentsStr; + + // Propose a fix if there's only one parent class... + if (Parents.size() == 1 && + // ...unless parent class is templated + !isa(Parents.front())) + Diag << FixItHint::CreateReplacement( + Member->getQualifierLoc().getSourceRange(), + getNameAsString(Parents.front()) + "::"); +} + +} // namespace bugprone +} // namespace tidy +} // namespace clang Index: docs/ReleaseNotes.rst =================================================================== --- docs/ReleaseNotes.rst +++ docs/ReleaseNotes.rst @@ -70,6 +70,12 @@ with looping constructs. Every backward jump is rejected. Forward jumps are only allowed in nested loops. +- New `bugprone-parent-virtual-call + `_ check + + Detects and fixes calls to grand-...parent virtual methods instead of calls + to parent's virtual methods. + - New `fuchsia-multiple-inheritance `_ check Index: docs/clang-tidy/checks/bugprone-parent-virtual-call.rst =================================================================== --- /dev/null +++ docs/clang-tidy/checks/bugprone-parent-virtual-call.rst @@ -0,0 +1,30 @@ +.. title:: clang-tidy - bugprone-parent-virtual-call + +bugprone-parent-virtual-call +============================ + +Detects and fixes calls to grand-...parent virtual methods instead of calls +to parent's virtual methods. + +.. code-block:: c++ + + class A { + ... + int virtual foo() {...} + ... + } + + class B: public A { + ... + int foo() override {...} + ... + } + + class C: public B { + ... + int foo() override {... A::foo()...} + / ^^^^^^^^ warning: 'A::foo' is a grand-parent's method, + not parent's. Did you mean 'B'? + [bugprone-parent-virtual-call] + ... + } Index: docs/clang-tidy/checks/list.rst =================================================================== --- docs/clang-tidy/checks/list.rst +++ docs/clang-tidy/checks/list.rst @@ -34,6 +34,7 @@ bugprone-misplaced-widening-cast bugprone-move-forwarding-reference bugprone-multiple-statement-macro + bugprone-parent-virtual-call bugprone-string-constructor bugprone-string-integer-assignment bugprone-string-literal-with-embedded-nul Index: test/clang-tidy/bugprone-parent-virtual-call.cpp =================================================================== --- /dev/null +++ test/clang-tidy/bugprone-parent-virtual-call.cpp @@ -0,0 +1,139 @@ +// RUN: %check_clang_tidy %s bugprone-parent-virtual-call %t + +extern int foo(); + +class A { +public: + A() = default; + virtual ~A() = default; + + virtual int virt_1() { return foo() + 1; } + virtual int virt_2() { return foo() + 2; } + + int non_virt() { return foo() + 3; } + static int stat() { return foo() + 4; } +}; + +class B : public A { +public: + B() = default; + + // Nothing to fix: calls to parent. + int virt_1() override { return A::virt_1() + 3; } + int virt_2() override { return A::virt_2() + 4; } +}; + +class C : public B { +public: + int virt_1() override { return A::virt_1() + B::virt_1(); } + // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: qualified function name A::virt_1 refers to a function not from a direct base class; did you mean 'B'? [bugprone-parent-virtual-call] + // CHECK-FIXES: int virt_1() override { return B::virt_1() + B::virt_1(); } + int virt_2() override { return A::virt_1() + B::virt_1(); } + // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: qualified function name A::virt_1 refers to a function not from a direct base class; did you mean 'B'? [bugprone-parent-virtual-call] + // CHECK-FIXES: int virt_2() override { return B::virt_1() + B::virt_1(); } + + // Test that non-virtual and static methods are not affected by this cherker. + int method_c() { return A::stat() + A::non_virt(); } +}; + +// Test that the check affects grand-grand..-parent calls too. +class D : public C { +public: + int virt_1() override { return A::virt_1() + B::virt_1() + D::virt_1(); } + // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: qualified function name A::virt_1 refers to a function not from a direct base class; did you mean 'C'? [bugprone-parent-virtual-call] + // CHECK-MESSAGES: :[[@LINE-2]]:48: warning: qualified function name B::virt_1 refers to a function not from a direct base class; did you mean 'C'? [bugprone-parent-virtual-call] + // CHECK-FIXES: int virt_1() override { return C::virt_1() + C::virt_1() + D::virt_1(); } + int virt_2() override { return A::virt_1() + B::virt_1() + D::virt_1(); } + // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: qualified function name A::virt_1 refers to a function not from a direct base class; did you mean 'C'? [bugprone-parent-virtual-call] + // CHECK-MESSAGES: :[[@LINE-2]]:48: warning: qualified function name B::virt_1 refers to a function not from a direct base class; did you mean 'C'? [bugprone-parent-virtual-call] + // CHECK-FIXES: int virt_2() override { return C::virt_1() + C::virt_1() + D::virt_1(); } +}; + +// Test classes in anonymous namespaces. +namespace { +class BN : public A { +public: + int virt_1() override { return A::virt_1() + 3; } + int virt_2() override { return A::virt_2() + 4; } +}; +} // namespace N + +class CN : public BN { +public: + int virt_1() override { return A::virt_1() + BN::virt_1(); } + // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: qualified function name A::virt_1 refers to a function not from a direct base class; did you mean 'BN'? [bugprone-parent-virtual-call] + // CHECK-FIXES: int virt_1() override { return BN::virt_1() + BN::virt_1(); } + int virt_2() override { return A::virt_1() + BN::virt_1(); } + // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: qualified function name A::virt_1 refers to a function not from a direct base class; did you mean 'BN'? [bugprone-parent-virtual-call] + // CHECK-FIXES: int virt_2() override { return BN::virt_1() + BN::virt_1(); } +}; + +// Test multiple inheritance fixes +class AA { +public: + AA() = default; + virtual ~AA() = default; + + virtual int virt_1() { return foo() + 1; } + virtual int virt_2() { return foo() + 2; } + + int non_virt() { return foo() + 3; } + static int stat() { return foo() + 4; } +}; + +class BB_1 : virtual public AA { +public: + BB_1() = default; + + // Nothing to fix: calls to parent. + int virt_1() override { return AA::virt_1() + 3; } + int virt_2() override { return AA::virt_2() + 4; } +}; + +class BB_2 : virtual public AA { +public: + BB_2() = default; +}; + +class CC : public BB_1, public BB_2 { +public: + int virt_1() override { return AA::virt_1() + 3; } + // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: qualified function name AA::virt_1 refers to a function not from a direct base class; did you mean 'BB_1' or 'BB_2'? [bugprone-parent-virtual-call] + // No fix available due to multiple choice of parent class. +}; + +// Test virtual method is diagnosted although not overridden in parent. +class BI : public A { +public: + BI() = default; +}; + +class CI : BI { + int virt_1() override { return A::virt_1(); } + // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: qualified function name A::virt_1 refers to a function not from a direct base class; did you mean 'BI'? [bugprone-parent-virtual-call] + // CHECK-FIXES: int virt_1() override { return BI::virt_1(); } +}; + +// Test templated classes. +template class BF : public A { +public: + int virt_1() override { return A::virt_1() + 3; } +}; + +// Test templated parent class. +class CF : public BF { +public: + int virt_1() override { return A::virt_1(); } + // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: qualified function name A::virt_1 refers to a function not from a direct base class; did you mean 'BF'? [bugprone-parent-virtual-call] +}; + +// Test both templated class and its parent class. +template class DF : public BF { +public: + DF() = default; + int virt_1() override { return A::virt_1(); } + // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: qualified function name A::virt_1 refers to a function not from a direct base class; did you mean 'BF'? [bugprone-parent-virtual-call] +}; + +// Just to instantiate DF. +int bar() { return (new DF())->virt_1(); }