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,150 @@ +//===--- 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 +#include + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace bugprone { + +static bool IsParentOf(const CXXRecordDecl *Parent, + const CXXRecordDecl *ThisClass) { + assert(Parent); + assert(ThisClass); + if (Parent->getCanonicalDecl() == ThisClass->getCanonicalDecl()) + return true; + const auto ClassIter = std::find_if( + ThisClass->bases_begin(), ThisClass->bases_end(), [=](auto &Base) { + assert(Base.getType()->getAsCXXRecordDecl()); + return Parent->getCanonicalDecl() == + Base.getType()->getAsCXXRecordDecl()->getCanonicalDecl(); + }); + return ClassIter != ThisClass->bases_end(); +} + +static bool IsRecursiveParentOf(const CXXRecordDecl *Parent, + const CXXRecordDecl *ThisClass) { + assert(Parent); + assert(ThisClass); + return ThisClass->isDerivedFrom(Parent); +} + +static std::list +GetParentsByGrandParent(const CXXRecordDecl *GrandParent, + const CXXRecordDecl *ThisClass) { + + assert(GrandParent != nullptr); + assert(ThisClass != nullptr); + std::list result; + for (auto &Base : ThisClass->bases()) { + if (IsRecursiveParentOf( + GrandParent, + Base.getType()->getAsCXXRecordDecl()->getCanonicalDecl())) + result.push_back( + Base.getType()->getAsCXXRecordDecl()->getCanonicalDecl()); + } + return result; +} + +// The same as NamedDecl::getQualifiedNameAsString, but with custom +// PrintingPolicy for anonymous namespaces. +static std::string GetNameAsString(const NamedDecl &Decl) { + PrintingPolicy PP(Decl.getASTContext().getPrintingPolicy()); + PP.SuppressUnwrittenScope = true; + std::string QualName; + llvm::raw_string_ostream OS(QualName); + 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 auto Parents = GetParentsByGrandParent(CastToType, ThisType); + assert(!Parents.empty()); + + auto ParentIter = Parents.begin(); + std::string ParentsStr = + "'" + (*ParentIter)->getQualifiedNameAsString() + "'"; + for (++ParentIter; ParentIter != Parents.end(); ++ParentIter) + ParentsStr += " or '" + (*ParentIter)->getQualifiedNameAsString() + "'"; + + if (!Member->getQualifier()) + return; + + std::string CalleeName; + { + llvm::raw_string_ostream OStream(CalleeName); + MatchedDecl->getCalleeDecl()->getAsFunction()->printQualifiedName(OStream); + } + + assert(Member->getQualifierLoc().getSourceRange().getBegin().isValid()); + auto Diag = + diag(Member->getQualifierLoc().getSourceRange().getBegin(), + "'%0' is a grand-parent's method, not parent's. Did you mean %1?") + << CalleeName << ParentsStr; + + if (Parents.size() == 1 && + Member->getQualifierLoc().getSourceRange().getBegin().isValid()) { + 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,129 @@ +// 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: 'A::virt_1' is a grand-parent's method, not parent's. Did you mean 'B'? + // 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: 'A::virt_1' is a grand-parent's method, not parent's. Did you mean 'B'? + // 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: 'A::virt_1' is a grand-parent's method, not parent's. Did you mean 'C'? + // CHECK-MESSAGES: :[[@LINE-2]]:48: warning: 'B::virt_1' is a grand-parent's method, not parent's. Did you mean 'C'? + // 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: 'A::virt_1' is a grand-parent's method, not parent's. Did you mean 'C'? + // CHECK-MESSAGES: :[[@LINE-2]]:48: warning: 'B::virt_1' is a grand-parent's method, not parent's. Did you mean 'C'? + // 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: 'A::virt_1' is a grand-parent's method, not parent's. Did you mean '(anonymous namespace)::BN'? + // 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: 'A::virt_1' is a grand-parent's method, not parent's. Did you mean '(anonymous namespace)::BN'? + // 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: 'AA::virt_1' is a grand-parent's method, not parent's. Did you mean 'BB_1' or 'BB_2'? [bugprone-parent-virtual-call] + // No fix available due to multiple choice of parent class. +}; + +// 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: 'A::virt_1' is a grand-parent's method, not parent's. Did you mean 'BF'? + // CHECK-FIXES: int virt_1() override { return BF::virt_1(); } +}; + +// 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: 'A::virt_1' is a grand-parent's method, not parent's. Did you mean 'BF'? + // CHECK-FIXES: int virt_1() override { return BF::virt_1(); } +}; + +// Just to instantiate DF. +int bar() { return (new DF())->virt_1(); }