Index: clang-tidy/bugprone/BugproneTidyModule.cpp =================================================================== --- clang-tidy/bugprone/BugproneTidyModule.cpp +++ clang-tidy/bugprone/BugproneTidyModule.cpp @@ -28,6 +28,7 @@ #include "MisplacedWideningCastCheck.h" #include "MoveForwardingReferenceCheck.h" #include "MultipleStatementMacroCheck.h" +#include "ParentVirtualCallCheck.h" #include "SizeofContainerCheck.h" #include "SizeofExpressionCheck.h" #include "StringConstructorCheck.h" @@ -90,6 +91,8 @@ "bugprone-move-forwarding-reference"); CheckFactories.registerCheck( "bugprone-multiple-statement-macro"); + CheckFactories.registerCheck( + "bugprone-parent-virtual-call"); CheckFactories.registerCheck( "bugprone-sizeof-container"); CheckFactories.registerCheck( Index: clang-tidy/bugprone/CMakeLists.txt =================================================================== --- clang-tidy/bugprone/CMakeLists.txt +++ clang-tidy/bugprone/CMakeLists.txt @@ -20,6 +20,7 @@ MisplacedWideningCastCheck.cpp MoveForwardingReferenceCheck.cpp MultipleStatementMacroCheck.cpp + ParentVirtualCallCheck.cpp SizeofContainerCheck.cpp SizeofExpressionCheck.cpp StringConstructorCheck.cpp Index: clang-tidy/bugprone/ParentVirtualCallCheck.h =================================================================== --- clang-tidy/bugprone/ParentVirtualCallCheck.h +++ 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 =================================================================== --- clang-tidy/bugprone/ParentVirtualCallCheck.cpp +++ clang-tidy/bugprone/ParentVirtualCallCheck.cpp @@ -0,0 +1,154 @@ +//===--- 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 "clang/Tooling/FixIt.h" +#include "llvm/ADT/STLExtras.h" +#include "llvm/ADT/SmallVector.h" +#include + +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, + const CXXMethodDecl &MemberDecl) { + BasesVector Result; + for (const auto &Base : ThisClass.bases()) { + const auto *BaseDecl = Base.getType()->getAsCXXRecordDecl(); + const CXXMethodDecl *ActualMemberDecl = + MemberDecl.getCorrespondingMethodInClass(BaseDecl); + if (!ActualMemberDecl) + continue; + // TypePtr is the nearest base class to ThisClass between ThisClass and + // GrandParent, where MemberDecl is overridden. TypePtr is the class the + // check proposes to fix to. + const Type *TypePtr = + ActualMemberDecl->getThisType(ActualMemberDecl->getASTContext()) + .getTypePtr(); + const CXXRecordDecl *RecordDeclType = TypePtr->getPointeeCXXRecordDecl(); + assert(RecordDeclType && "TypePtr is not a pointer to CXXRecordDecl!"); + if (RecordDeclType->getCanonicalDecl()->isDerivedFrom(&GrandParent)) + Result.emplace_back(RecordDeclType); + } + + 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(); +} + +// Returns E as written in the source code. Used to handle 'using' and +// 'typedef'ed names of grand-parent classes. +static std::string getExprAsString(const clang::Expr &E, + clang::ASTContext &AC) { + std::string Text = tooling::fixit::getText(E, AC).str(); + Text.erase(llvm::remove_if(Text, std::isspace), Text.end()); + return Text; +} + +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); + + const auto *Member = Result.Nodes.getNodeAs("member"); + assert(Member); + + if (!Member->getQualifier()) + return; + + const auto *MemberDecl = cast(Member->getMemberDecl()); + + const auto *ThisTypePtr = Result.Nodes.getNodeAs("thisType"); + assert(ThisTypePtr); + + const auto *ThisType = ThisTypePtr->getPointeeCXXRecordDecl(); + assert(ThisType); + + const auto *CastToTypePtr = Result.Nodes.getNodeAs("castToType"); + assert(CastToTypePtr); + + const auto *CastToType = CastToTypePtr->getAsCXXRecordDecl(); + assert(CastToType); + + if (isParentOf(*CastToType, *ThisType)) + return; + + const BasesVector Parents = + getParentsByGrandParent(*CastToType, *ThisType, *MemberDecl); + + if (Parents.empty()) + return; + + std::string ParentsStr; + ParentsStr.reserve(30 * Parents.size()); + for (const CXXRecordDecl *Parent : Parents) { + if (!ParentsStr.empty()) + ParentsStr.append(" or "); + ParentsStr.append("'").append(getNameAsString(Parent)).append("'"); + } + + assert(Member->getQualifierLoc().getSourceRange().getBegin().isValid()); + auto Diag = diag(Member->getQualifierLoc().getSourceRange().getBegin(), + "qualified name '%0' refers to a member overridden " + "in subclass%1; did you mean %2?") + << getExprAsString(*Member, *Result.Context) + << (Parents.size() > 1 ? "es" : "") << 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 @@ -64,6 +64,12 @@ - New module ``zircon`` for checks related to Fuchsia's Zircon kernel. +- New :doc:`bugprone-parent-virtual-call + ` check + + Detects and fixes calls to grand-...parent virtual methods instead of calls + to overridden parent's virtual methods. + - New :doc:`bugprone-throw-keyword-missing ` check Index: docs/clang-tidy/checks/bugprone-parent-virtual-call.rst =================================================================== --- docs/clang-tidy/checks/bugprone-parent-virtual-call.rst +++ docs/clang-tidy/checks/bugprone-parent-virtual-call.rst @@ -0,0 +1,23 @@ +.. title:: clang-tidy - bugprone-parent-virtual-call + +bugprone-parent-virtual-call +============================ + +Detects and fixes calls to grand-...parent virtual methods instead of calls +to overridden 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: qualified name A::foo refers to a member overridden in subclass; 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 @@ -36,6 +36,7 @@ bugprone-misplaced-widening-cast bugprone-move-forwarding-reference bugprone-multiple-statement-macro + bugprone-parent-virtual-call bugprone-sizeof-container bugprone-sizeof-expression bugprone-string-constructor Index: test/clang-tidy/bugprone-parent-virtual-call.cpp =================================================================== --- test/clang-tidy/bugprone-parent-virtual-call.cpp +++ test/clang-tidy/bugprone-parent-virtual-call.cpp @@ -0,0 +1,179 @@ +// 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 direct 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 name 'A::virt_1' refers to a member overridden in subclass; 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 name 'A::virt_1' {{.*}}; 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(); } +}; + +// Check aliased type names +using A1 = A; +typedef A A2; +#define A3 A + +class C2 : public B { +public: + int virt_1() override { return A1::virt_1() + A2::virt_1() + A3::virt_1(); } + // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: qualified name 'A1::virt_1' {{.*}}; did you mean 'B'? {{.*}} + // CHECK-MESSAGES: :[[@LINE-2]]:49: warning: qualified name 'A2::virt_1' {{.*}}; did you mean 'B'? {{.*}} + // CHECK-MESSAGES: :[[@LINE-3]]:64: warning: qualified name 'A3::virt_1' {{.*}}; did you mean 'B'? {{.*}} + // CHECK-FIXES: int virt_1() override { return B::virt_1() + B::virt_1() + B::virt_1(); } +}; + +// 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 name 'A::virt_1' {{.*}}; did you mean 'C'? {{.*}} + // CHECK-MESSAGES: :[[@LINE-2]]:48: warning: qualified name 'B::virt_1' {{.*}}; 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: qualified name 'A::virt_1' {{.*}}; did you mean 'C'? {{.*}} + // CHECK-MESSAGES: :[[@LINE-2]]:48: warning: qualified name 'B::virt_1' {{.*}}; did you mean 'C'? {{.*}} + // CHECK-FIXES: int virt_2() override { return C::virt_1() + C::virt_1() + D::virt_1(); } +}; + +// Test classes in 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 + +namespace N1 { +class A { +public: + A() = default; + virtual int virt_1() { return foo() + 1; } + virtual int virt_2() { return foo() + 2; } +}; +} // namespace N1 + +namespace N2 { +class BN : public N1::A { +public: + int virt_1() override { return A::virt_1() + 3; } + int virt_2() override { return A::virt_2() + 4; } +}; +} // namespace N2 + +class CN : public BN { +public: + int virt_1() override { return A::virt_1() + BN::virt_1(); } + // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: qualified name 'A::virt_1' {{.*}}; did you mean '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: qualified name 'A::virt_1' {{.*}}; did you mean 'BN'? {{.*}} + // CHECK-FIXES: int virt_2() override { return BN::virt_1() + BN::virt_1(); } +}; + +class CNN : public N2::BN { +public: + int virt_1() override { return N1::A::virt_1() + N2::BN::virt_1(); } + // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: qualified name 'N1::A::virt_1' {{.*}}; did you mean 'N2::BN'? {{.*}} + // CHECK-FIXES: int virt_1() override { return N2::BN::virt_1() + N2::BN::virt_1(); } + int virt_2() override { return N1::A::virt_1() + N2::BN::virt_1(); } + // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: qualified name 'N1::A::virt_1' {{.*}}; did you mean 'N2::BN'? {{.*}} + // CHECK-FIXES: int virt_2() override { return N2::BN::virt_1() + N2::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; + int virt_1() override { return AA::virt_1() + 3; } +}; + +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 name 'AA::virt_1' refers to a member overridden in subclasses; did you mean 'BB_1' or 'BB_2'? {{.*}} + // No fix available due to multiple choice of parent class. +}; + +// Test that virtual method is not diagnosed as not overridden in parent. +class BI : public A { +public: + BI() = default; +}; + +class CI : BI { + int virt_1() override { return A::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 name 'A::virt_1' {{.*}}; did you mean 'BF'? {{.*}} +}; + +// 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 name 'A::virt_1' {{.*}}; did you mean 'BF'? {{.*}} +}; + +// Just to instantiate DF. +int bar() { return (new DF())->virt_1(); }