[clang-tidy] Adding Fuchsia checker for virtual inheritance

[juliehockett]

Summary

Adds a check to the Fuchsia module to warn if classes are defined or created with virtual inheritance.

See https://fuchsia.googlesource.com/zircon/+/master/docs/cxx.md for reference.

[Eugene.Zelenko]

test/clang-tidy/fuchsia-virtual-inheritance.cpp

43

void is redundant in C++. See modernize-redundant-void-arg.

[aaron.ballman]

clang-tidy/fuchsia/FuchsiaTidyModule.cpp

28–29

Formatting looks off here.

clang-tidy/fuchsia/VirtualInheritanceCheck.cpp

22

Elide spurious parens.

45

s/which/that

docs/clang-tidy/checks/fuchsia-virtual-inheritance.rst

13

Missing a semicolon on the class declaration.

[aaron.ballman]

test/clang-tidy/fuchsia-virtual-inheritance.cpp

31

I don't think that this should be diagnosed -- it will be confusing to the user to see this claimed as virtual inheritance when there's no virtual inheritance at this level.

35–37

I'm also not certain this should be diagnosed either. It's technically correct because it's calling the base class constructors here, but at the same time, it seems very low-value and likely to cause the user to do something really bad, like silence the warning by not calling the base class constructors.

[juliehockett]

Updated matcher to only match direct virtual base classes.

[juliehockett]

test/clang-tidy/fuchsia-virtual-inheritance.cpp

35–37

I see what you mean, but where then would you draw the line between warning and not? We could warn for construction everywhere except in initialization lists, but that seems like it might open the door to trivially get around the check in ways that should be disallowed.

[aaron.ballman]

test/clang-tidy/fuchsia-virtual-inheritance.cpp

35–37

Would it be sufficient to only flag at the point of inheritance, or do you have existing header files with class declarations using virtual inheritance and are worried about consumers inheriting from those?

e.g., flag declarations but not construction.

[juliehockett]

Updating checker to only warn on declarations.

[juliehockett]

test/clang-tidy/fuchsia-virtual-inheritance.cpp

35–37

Makes sense -- we think the declaration is the important one to limit here. The check for constructed virtual objects can be moved to a different checker if it becomes necessary.

[aaron.ballman]

Aside from some documentation nits, LGTM!

docs/clang-tidy/checks/fuchsia-virtual-inheritance.rst

7

Comment is out of date.

15

Comment out of date

clang-tidy/fuchsia/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyFuchsiaModule
   DefaultArgumentsCheck.cpp
   FuchsiaTidyModule.cpp
+  VirtualInheritanceCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyUtils
   )

clang-tidy/fuchsia/FuchsiaTidyModule.cpp

 //===--- FuchsiaTidyModule.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 "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "DefaultArgumentsCheck.h"
+#include "VirtualInheritanceCheck.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace fuchsia {
 
 /// This module is for Fuchsia-specific checks.
 class FuchsiaModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<DefaultArgumentsCheck>(
         "fuchsia-default-arguments");
+    CheckFactories.registerCheck<VirtualInheritanceCheck>(
+        "fuchsia-virtual-inheritance");
   }
 };
 // Register the FuchsiaTidyModule using this statically initialized variable.
 static ClangTidyModuleRegistry::Add<FuchsiaModule>
     X("fuchsia-module", "Adds Fuchsia platform checks.");
 } // namespace fuchsia
 
 // This anchor is used to force the linker to link in the generated object file
 // and thus register the FuchsiaModule.
 volatile int FuchsiaModuleAnchorSource = 0;
 
 } // namespace tidy
 } // namespace clang

clang-tidy/fuchsia/VirtualInheritanceCheck.h

+//===--- VirtualInheritanceCheck.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_FUCHSIA_VIRTUAL_INHERITANCE_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_FUCHSIA_VIRTUAL_INHERITANCE_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace fuchsia {
+
+/// Defining classes with virtual inheritance is disallowed.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/fuchsia-virtual-inheritance.html
+class VirtualInheritanceCheck : public ClangTidyCheck {
+ public:
+  VirtualInheritanceCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+}  // namespace fuchsia
+}  // namespace tidy
+}  // namespace clang
+
+#endif  // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_FUCHSIA_VIRTUAL_INHERITANCE_H

clang-tidy/fuchsia/VirtualInheritanceCheck.cpp

+//===--- VirtualInheritanceCheck.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 "VirtualInheritanceCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace fuchsia {
+
+AST_MATCHER(CXXRecordDecl, hasDirectVirtualBaseClass) {
+  if (!Node.hasDefinition()) return false;
+  if (!Node.getNumVBases()) return false;
+  for (const CXXBaseSpecifier &Base : Node.bases())
+    if (Base.isVirtual()) return true;
+  return false;
+}
+
+void VirtualInheritanceCheck::registerMatchers(MatchFinder *Finder) {
+  // Defining classes using direct virtual inheritance is disallowed.
+  Finder->addMatcher(cxxRecordDecl(hasDirectVirtualBaseClass()).bind("decl"),
+                     this);
+  // Calling constructors of classes using virtual inheritance is disallowed.
+  // To clarify, this matcher finds, in order:
+  //  1) Calls to Constructors,
+  //  2) The declarations of those constructors,
+  //  3) The classes those constructors belong to,
+  //  4) And matches the classes which have virtual base classes.
+  Finder->addMatcher(
+      cxxConstructExpr(hasDeclaration(cxxConstructorDecl(ofClass(
+                           cxxRecordDecl(hasDirectVirtualBaseClass())))))
+          .bind("stmt"),
+      this);
+}
+
+void VirtualInheritanceCheck::check(const MatchFinder::MatchResult &Result) {
+  if (const auto *D = Result.Nodes.getNodeAs<CXXRecordDecl>("decl"))
+    diag(D->getLocStart(), "direct virtual inheritance is disallowed");
+  else if (const auto *S = Result.Nodes.getNodeAs<CXXConstructExpr>("stmt"))
+    diag(S->getLocStart(),
+         "constructing a class that directly inherits a virtual base class is "
+         "disallowed");
+}
+
+}  // namespace fuchsia
+}  // namespace tidy
+}  // namespace clang

docs/ReleaseNotes.rst

- New `cppcoreguidelines-owning-memory <http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-owning-memory.html>`_ check
 
   This check implements the type-based semantic of ``gsl::owner<T*>``, but without
   flow analysis.
 
 - New `fuchsia-default-arguments
   <http://clang.llvm.org/extra/clang-tidy/checks/fuchsia-default-arguments.html>`_ check
 
   Warns if a function or method is declared or called with default arguments.
   
+- New `fuchsia-virtual-inheritance
+  <http://clang.llvm.org/extra/clang-tidy/checks/fuchsia-virtual-inheritance.html>`_ check
+
+  Warns if classes are defined or created with virtual inheritance.
+    
 - New `google-objc-avoid-throwing-exception
   <http://clang.llvm.org/extra/clang-tidy/checks/google-objc-avoid-throwing-exception.html>`_ check
 
   Finds uses of throwing exceptions usages in Objective-C files.
 
 - New `google-objc-global-variable-declaration
   <http://clang.llvm.org/extra/clang-tidy/checks/google-global-variable-declaration.html>`_ check
 

docs/clang-tidy/checks/fuchsia-virtual-inheritance.rst

+.. title:: clang-tidy - fuchsia-virtual-inheritance
+
+fuchsia-virtual-inheritance
+===========================
+
+Warns if classes are defined or created with virtual inheritance.
+
+For example, classes should not be defined with virtual inheritance:
+
+.. code-block:: c++
+
+  class B : public virtual A {};   // warning
+
+Classes with virtual inheritance should not be created:
+
+.. code-block:: c++
+
+  B *b = new B();   // warning
+
+See the features disallowed in Fuchsia at https://fuchsia.googlesource.com/zircon/+/master/docs/cxx.md

docs/clang-tidy/checks/list.rst

.. toctree::
    cppcoreguidelines-pro-type-member-init
    cppcoreguidelines-pro-type-reinterpret-cast
    cppcoreguidelines-pro-type-static-cast-downcast
    cppcoreguidelines-pro-type-union-access
    cppcoreguidelines-pro-type-vararg
    cppcoreguidelines-slicing
    cppcoreguidelines-special-member-functions
    fuchsia-default-arguments
+   fuchsia-virtual-inheritance
    google-build-explicit-make-pair
    google-build-namespaces
    google-build-using-namespace
    google-default-arguments
    google-explicit-constructor
    google-global-names-in-headers
    google-objc-avoid-throwing-exception
    google-objc-global-variable-declaration

test/clang-tidy/fuchsia-virtual-inheritance.cpp

+// RUN: %check_clang_tidy %s fuchsia-virtual-inheritance %t
+
+class A {
+public:
+  A(int value) : val(value) {}
+
+  int do_A() { return val; }
+
+private:
+  int val;
+};
+
+class B : public virtual A {
+  // CHECK-MESSAGES: [[@LINE-1]]:1: warning: direct virtual inheritance is disallowed [fuchsia-virtual-inheritance]
+  // CHECK-NEXT: class B : public virtual A {
+public:
+  B() : A(0) {}
+  int do_B() { return 1 + do_A(); }
+};
+
+class C : public virtual A {
+  // CHECK-MESSAGES: [[@LINE-1]]:1: warning: direct virtual inheritance is disallowed [fuchsia-virtual-inheritance]
+  // CHECK-NEXT: class C : public virtual A {
+public:
+  C() : A(0) {}
+  int do_C() { return 2 + do_A(); }
+};
+
+class D : public B, public C {
+public:
+  D(int value) : A(value), B(), C() {}
+  // CHECK-MESSAGES: [[@LINE-1]]:28: warning: constructing a class that directly inherits a virtual base class is disallowed [fuchsia-virtual-inheritance]
+  // CHECK-NEXT:  D(int value) : A(value), B(), C() {}
+  // CHECK-MESSAGES: [[@LINE-3]]:33: warning: constructing a class that directly inherits a virtual base class is disallowed [fuchsia-virtual-inheritance]
+  // CHECK-NEXT:  D(int value) : A(value), B(), C() {}
+
+  int do_D() { return do_A() + do_B() + do_C(); }
+};
+
+int main() {
+  A *a = new A(0);
+  B *b = new B();
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: constructing a class that directly inherits a virtual base class is disallowed [fuchsia-virtual-inheritance]
+  // CHECK-NEXT:  B *b = new B();
+  C *c = new C();
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: constructing a class that directly inherits a virtual base class is disallowed [fuchsia-virtual-inheritance]
+  // CHECK-NEXT:  C *c = new C();
+  D *d = new D(0);
+  return 0;
+}