[clang-tidy] Add calling virtual functions in constructors/destructors check.

[Eugene.Zelenko]

This check is duplicate of clang-analyzer-alpha.cplusplus.VirtualCall.

From my point of view, Clang-tidy is better place, since such calls doesn't depend of run-time paths.

I think will be good idea to try to establish better functionality separation between Clang/Clang-tidy/Clang Static Analyzer. Current situation looks like different teams try to take everything coming to them without considering big picture. This my impression based on including padding check in Clang Static Analyzer.

I may be wrong, but Clang seems even better place for such warnings.

[hokein]

In D16008#322811, @Eugene.Zelenko wrote:

This check is duplicate of clang-analyzer-alpha.cplusplus.VirtualCall.

Oops... Didn't notice there is an implementation already.

From my point of view, Clang-tidy is better place, since such calls doesn't depend of run-time paths.

I think will be good idea to try to establish better functionality separation between Clang/Clang-tidy/Clang Static Analyzer. Current situation looks like different teams try to take everything coming to them without considering big picture. This my impression based on including padding check in Clang Static Analyzer.

I may be wrong, but Clang seems even better place for such warnings.

However clang-tidy has a ' clang-analyzer-*' check option to run the clang static analyzer check. I'm not sure whether it's worthwhile to move VirtualCall check to the module in clang-tidy.

[Eugene.Zelenko]

See D14779 for discussion about Clang-tidy vs Static Analyzer.

[lebedev.ri]

Any further thoughts here?
I was slightly bitten by this recently, and i though that it already existed as a clang-tidy check (:

[alexfh]

If the CSA checker is still in alpha, I'd proceed with this check instead of investing time in polishing the CSA implementation.

[alexfh]

Aaron, WDYT?

[aaron.ballman]

In D16008#1035683, @alexfh wrote:

If the CSA checker is still in alpha, I'd proceed with this check instead of investing time in polishing the CSA implementation.

Do you know why the CSA checker is still in alpha? As best I can tell, that check and this one are attempting to do the same work. I'm kind of worried about having competing functionalities because users have to guess as to which one is the "correct" one to use.

[malcolm.parsons]

In D16008#1035789, @aaron.ballman wrote:

Do you know why the CSA checker is still in alpha?

It isn't - https://reviews.llvm.org/D26768 moved it to optin.

I don't know why https://reviews.llvm.org/D34275 didn't turn it on by default.

[aaron.ballman]

In D16008#1035948, @malcolm.parsons wrote:

In D16008#1035789, @aaron.ballman wrote:

Do you know why the CSA checker is still in alpha?

It isn't - https://reviews.llvm.org/D26768 moved it to optin.

I don't know why https://reviews.llvm.org/D34275 didn't turn it on by default.

Then I'm not certain the utility of this check given that you can call the CSA check from clang-tidy directly; however, I do like the idea of exposing the CSA check under the name cert-oop50-cpp.

clang-tidy/cert/CERTTidyModule.cpp

 #include "../misc/NewDeleteOverloadsCheck.h"
 #include "../misc/NonCopyableObjects.h"
 #include "../misc/StaticAssertCheck.h"
 #include "../misc/ThrowByValueCatchByReferenceCheck.h"
 #include "SetLongJmpCheck.h"
 #include "StaticObjectExceptionCheck.h"
 #include "ThrownExceptionTypeCheck.h"
 #include "VariadicFunctionDefCheck.h"
+#include "VirtualFunctionsInConstructorsOrDestructorsCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace cert {
 
 class CERTModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     // C++ checkers
     // DCL
     CheckFactories.registerCheck<VariadicFunctionDefCheck>(
         "cert-dcl50-cpp");
     CheckFactories.registerCheck<misc::NewDeleteOverloadsCheck>(
         "cert-dcl54-cpp");
     CheckFactories.registerCheck<google::build::UnnamedNamespaceInHeaderCheck>(
         "cert-dcl59-cpp");
     // OOP
     CheckFactories.registerCheck<MoveConstructorInitCheck>(
         "cert-oop11-cpp");
+    CheckFactories.registerCheck<
+        VirtualFunctionsInConstructorsOrDestructorsCheck>("cert-oop50-cpp");
     // ERR
     CheckFactories.registerCheck<SetLongJmpCheck>(
         "cert-err52-cpp");
     CheckFactories.registerCheck<StaticObjectExceptionCheck>(
         "cert-err58-cpp");
     CheckFactories.registerCheck<ThrownExceptionTypeCheck>(
         "cert-err60-cpp");
     CheckFactories.registerCheck<ThrowByValueCatchByReferenceCheck>(

clang-tidy/cert/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyCERTModule
   CERTTidyModule.cpp
   SetLongJmpCheck.cpp
   StaticObjectExceptionCheck.cpp
   ThrownExceptionTypeCheck.cpp
   VariadicFunctionDefCheck.cpp
+  VirtualFunctionsInConstructorsOrDestructorsCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyGoogleModule
   clangTidyMiscModule
   clangTidyUtils
   )

clang-tidy/cert/VirtualFunctionsInConstructorsOrDestructorsCheck.h

+//===--- VirtualFunctionsInConstructorsOrDestructorsCheck.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_CERT_VIRTUAL_FUNCTIONS_IN_CONSTRUCTORS_OR_DESTRUCTORS_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CERT_VIRTUAL_FUNCTIONS_IN_CONSTRUCTORS_OR_DESTRUCTORS_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace cert {
+
+/// Checks whether there are virtual functions called in constructors or
+/// destructors.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/cert-oop50-cpp.html
+class VirtualFunctionsInConstructorsOrDestructorsCheck : public ClangTidyCheck {
+public:
+  VirtualFunctionsInConstructorsOrDestructorsCheck(
+      StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace cert
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CERT_VIRTUAL_FUNCTIONS_IN_CONSTRUCTORS_OR_DESTRUCTORS_H

clang-tidy/cert/VirtualFunctionsInConstructorsOrDestructorsCheck.cpp

+//===--- VirtualFunctionsInConstructorsOrDestructorsCheck.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 "VirtualFunctionsInConstructorsOrDestructorsCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace cert {
+
+namespace {
+
+AST_MATCHER(CXXMemberCallExpr, isThisMemberCallExpr) {
+  const MemberExpr *ME = dyn_cast<MemberExpr>(Node.getCallee());
+  // CXXMemberCallExpr
+  // `-MemberExpr
+  //   `-CXXThisExpr
+  if (isa<CXXThisExpr>(ME->getBase()))
+    return true;
+
+  // CXXMemberCallExpr
+  // `-MemberExpr
+  //   `-ImplicitCastExpr
+  //     `-CXXThisExpr
+  if (const auto* ICE = dyn_cast<ImplicitCastExpr>(ME->getBase()))
+    return isa<CXXThisExpr>(ICE->getSubExpr());
+  return false;
+}
+
+} // namespace
+
+void VirtualFunctionsInConstructorsOrDestructorsCheck::registerMatchers(
+    MatchFinder *Finder) {
+  Finder->addMatcher(
+      cxxMemberCallExpr(
+          isThisMemberCallExpr(),
+          hasAncestor(
+              decl(anyOf(cxxConstructorDecl(), cxxDestructorDecl()))))
+      .bind("this-member-call"), this);
+}
+
+void VirtualFunctionsInConstructorsOrDestructorsCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  const CXXMemberCallExpr *MC = Result.Nodes.getNodeAs<CXXMemberCallExpr>(
+      "this-member-call");
+  const CXXMethodDecl *MD = MC->getMethodDecl();
+  if (MD && MD->isVirtual()) {
+    // OOP50-CPP-EX1: It is permissible to call the virtual function with an
+    // explicitly qualified ID like A::f();
+    const MemberExpr *ME = dyn_cast<MemberExpr>(MC->getCallee());
+    if (ME->getQualifier())
+      return;
+
+    // OOP50-CPP-EX2: It is permissible to call a virtual function that has the
+    // final virt-specifier from a constructor or destructor. Similarly, it is
+    // also permissible to call a virtual function from a constructor or
+    // destructor of a class that has the final class-virt-specifier.
+    if (MD->hasAttr<FinalAttr>() ||
+        MC->getRecordDecl()->hasAttr<FinalAttr>())
+      return;
+
+    diag(MC->getLocStart(),
+         "Calling virtual function '%0' in a constructor or destructor "
+         "is prohibited")
+        << MD->getNameInfo().getName().getAsString();
+  }
+}
+
+} // namespace cert
+} // namespace tidy
+} // namespace clang

docs/clang-tidy/checks/cert-oop50-cpp.rst

+.. title:: clang-tidy - cert-oop50-cpp
+
+cert-oop50-cpp
+==============
+
+This check flags all virtual functions called in constructors or destructors.
+
+This check corresponds to the CERT C++ Coding Standard rule
+`OOP50-CPP. Do not invoke virtual functions from constructors or destructors
+<https://www.securecoding.cert.org/confluence/display/cplusplus/OOP50-CPP.+Do+not+invoke+virtual+functions+from+constructors+or+destructors>`_.

docs/clang-tidy/checks/list.rst

 .. title:: clang-tidy - Clang-Tidy Checks
 
 Clang-Tidy Checks
 =========================
 
 .. toctree::   
    cert-dcl50-cpp
    cert-err52-cpp
    cert-err58-cpp
    cert-err60-cpp
+   cert-oop50-cpp
    cppcoreguidelines-pro-bounds-array-to-pointer-decay
    cppcoreguidelines-pro-bounds-constant-array-index
    cppcoreguidelines-pro-bounds-pointer-arithmetic
    cppcoreguidelines-pro-type-const-cast
    cppcoreguidelines-pro-type-cstyle-cast
    cppcoreguidelines-pro-type-reinterpret-cast
    cppcoreguidelines-pro-type-static-cast-downcast
    cppcoreguidelines-pro-type-union-access

test/clang-tidy/cert-virtual-functions-in-constructors-or-destructors.cpp

+// RUN: %check_clang_tidy %s cert-oop50-cpp %t
+
+class A {
+public:
+  virtual void f() {}
+};
+
+class B {
+public:
+  B() {
+    f();
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: Calling virtual function 'f' in a constructor or destructor is prohibited [cert-oop50-cpp]
+    this->f();
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: Calling virtual function 'f' in a constructor or destructor is prohibited
+    g();
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: Calling virtual function 'g' in a constructor or destructor is prohibited
+    this->g();
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: Calling virtual function 'g' in a constructor or destructor is prohibited
+    B::f();
+    B::g();
+    e();
+    a_->f();
+  }
+  B(const B& b) {
+    b.f();
+    f();
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: Calling virtual function 'f' in a constructor or destructor is prohibited
+    e();
+  }
+  virtual ~B() {
+    f();
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: Calling virtual function 'f' in a constructor or destructor is prohibited
+    e();
+  }
+
+  void e() {}
+  virtual void f() const {};
+  virtual void g() {}
+
+  A* a_;
+};
+
+class D : public B {
+public:
+  D() {
+    f();
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: Calling virtual function 'f' in a constructor or destructor is prohibited
+    B::f();
+    g();
+  }
+  D(const D& d) {
+    f();
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: Calling virtual function 'f' in a constructor or destructor is prohibited
+    d.f();
+  }
+  ~D() {
+    f();
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: Calling virtual function 'f' in a constructor or destructor is prohibited
+  }
+  void f() const override {}
+  void g() final override {}
+};
+
+class E final : public B {
+public:
+  E() {
+    f();
+  }
+  void f() const override { }
+};