[clang-tidy] Adding Fuchsia checker for multiple inheritance

[juliehockett]

Summary

Adds a check to the Fuchsia module to warn when a class inheritsfrom multiple classes that are not pure virtual.

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

[Eugene.Zelenko]

clang-tidy/fuchsia/MultipleInheritanceCheck.cpp

22

Please reduce indentation level.

59

const auto?

79

Could be (const?) auto *, since you spell type in cast. Same in other places.

docs/ReleaseNotes.rst

59–60

Please move this in alphabetical order after renamed checks.

60

Please move it to previous fuchsia check in alphabetical order.

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

9

Declaring -> declaring

[Eugene.Zelenko]

clang-tidy/fuchsia/MultipleInheritanceCheck.cpp

77

Could be const auto *.

99

Could be const auto *.

105

Could be const auto *.

107

Could be const auto *.

[juliehockett]

Fixing typo

[alexfh]

clang-tidy/fuchsia/MultipleInheritanceCheck.cpp

23

return Node.hasDefinition();

33

What's the reason to qualify InterfaceMap and other members of this class?

clang-tidy/fuchsia/MultipleInheritanceCheck.h

36–39

Do all these methods need to be public?

45

What's the reason to use a pointer and dynamically allocate the map? Just make it a value and clear it instead of deleting.

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

47

This is not about the check, rather about the underlying style guide. The document linked here doesn't explain why certain features are disallowed. I'd suggest putting some effort in expanding the document to include reasoning for each rule (e.g. see https://google.github.io/styleguide/cppguide.html#Multiple_Inheritance for a related rule in the Google C++ style guide).

[aaron.ballman]

clang-tidy/fuchsia/MultipleInheritanceCheck.cpp

22

This is another example of an AST matcher that should simply be made public in ASTMatchers.h (as a separate commit). Feel free to add me as a reviewer to that patch (it should be pretty trivial).

30–31

Formatting is off here -- be sure to run the patch through clang-format.

41

Rather than passing a pointer, why not pass a reference?

50

We don't put names into our TODO or FIXME comments.

55

Can elide braces, here and elsewhere.

59

Should be const auto *; also method does not meet our naming convention rules. Actually, the whole thing can be replaced by return llvm::none_of(Node->methods(), [](const CXXMethodDecl *M) { return M->isUserProvided() && !M->isPure(); });

103

Might as well make this unsigned rather than int.

[juliehockett]

Moved AST matcher to ASTMatchers.h (see D40611), addressing comments.

[Eugene.Zelenko]

Please rebase from trunk. I just enforced some order in chaos of Release Notes :-)

[juliehockett]

Rebasing for updated Release Notes -- so much nicer :)

[juliehockett]

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

47

Good point -- we're looking into updating it. Thanks!

[rsmith]

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

47

Hmm, the document linked here explicitly says that multiple inheritance of non-interface classes is allowed:

• Allowed
  • ...
  • Multiple implementation inheritance
    • But be judicious. This is used widely for e.g. intrusive container mixins.

That seems to directly contradict the warning produced by this check, inheriting mulitple classes which aren't pure virtual is disallowed. Should it just say "... is discouraged"?

[juliehockett]

Updated warning wording to more accurately reflect guidelines

[alexfh]

clang-tidy/fuchsia/MultipleInheritanceCheck.cpp

35–36

One lookup is enough here. Use StringMap::find() instead of count + lookup.

[aaron.ballman]

clang-tidy/fuchsia/MultipleInheritanceCheck.cpp

36

Don't use auto as the type is not spelled out explicitly in the initialization.

61

What about virtual bases (Node->vbases())? This would also be worth some test cases.

78

It might be nice to not bother matching class definitions that have no bases or virtual bases rather than matching every class definition. However, this could be done in a follow-up patch (it likely requires adding an AST matcher).

86

And virtual bases?

89

It might make sense to add a degenerate case here to ensure a base class without a definition doesn't cause a null pointer dereference. e.g.,

struct B;

struct D : B {}; // compile error, B is not a complete type

I'm not certain whether Clang's AST will contain the base or not.

clang-tidy/fuchsia/MultipleInheritanceCheck.h

35–37

I believe all these methods can be marked const.

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

81

Please add a test case consisting of only data members, e.g.,

struct B1 {
  int x;
};

struct B2 {
  int x;
};

struct D : B1, B2 {};

[Eugene.Zelenko]

clang-tidy/fuchsia/MultipleInheritanceCheck.cpp

36

But such cases are included in modernize-use-auto.

[aaron.ballman]

clang-tidy/fuchsia/MultipleInheritanceCheck.cpp

36

Hmm, I suppose you could maybe make a case that StringMap<bool>::iterator is "complex enough" and that the returned type is sufficiently clear to warrant using auto...

[juliehockett]

Updating tests

clang-tidy/fuchsia/MultipleInheritanceCheck.cpp

61

Added test cases for virtual, but aren't virtual bases also included in bases()?

78

Good point -- will do.

clang-tidy/fuchsia/MultipleInheritanceCheck.h

35–37

getInterfaceStatus and isInterface can't be -- they update the map. The other ones yes though!

[aaron.ballman]

clang-tidy/fuchsia/MultipleInheritanceCheck.cpp

61

No, they are separate in CXXRecordDecl.

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

49

The virtual base test cases I was thinking of were:

struct Base { virtual void foo() = 0; };
struct V1 : virtual Base {};
struct V2 : virtual Base {};
struct D : V1, V2 {}; // Should be fine
---
struct Base { virtual void foo(); };
struct V1 : virtual Base {};
struct V2 : virtual Base {};
struct D : V1, V2 {}; // Should be fine (there's only one concrete base)?
---
struct Base {};
struct V1 : virtual Base { virtual void f(); }
struct V2 : virtual Base { virtual void g(); }
struct D : V1, V2 {}; // Not okay
---
struct Base {};
struct V1 : virtual Base { virtual void f() = 0; }
struct V2 : virtual Base { virtual void g() = 0; }
struct D : V1, V2 {}; // Okay
---
struct Base { virtual void f(); };
struct V1 : virtual Base { virtual void f(); }
struct V2 : virtual base { virtual void g() = 0; }
struct D : V1, V2 {}; // Should be okay (V1::f() overrides Base::f() which is only inherited once)?

[juliehockett]

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

49

Ah okay I see what you mean. We're actually following the Google style guide (see here), so virtual inheritance is disallowed. There's another check that covers these cases (see D40813).

[aaron.ballman]

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

49

What about users who disable that check but leave this one enabled? I think the crux of the rule is that multiple inheritance of interface features is bad, and so I think there's a sensible answer here for virtual bases as well.

[juliehockett]

1. Updating check and tests to address virtual inheritance
2. Rebasing from trunk

[rsmith]

clang-tidy/fuchsia/MultipleInheritanceCheck.cpp

61

That's not quite right. bases() contains all direct bases, regardless of whether or not they're virtual. vbases() contains all virtual bases, regardless of whether or not they're direct.

[aaron.ballman]

clang-tidy/fuchsia/MultipleInheritanceCheck.cpp

61

Ah, I must have mis-remembered these APIs. Thanks, @rsmith and sorry for the noise @juliehockett!

[juliehockett]

Rebasing from trunk

[aaron.ballman]

LGTM aside from a few small nits.

clang-tidy/fuchsia/MultipleInheritanceCheck.cpp

86

No need to register the matchers for languages other than C++.

114–115

s/which/that

clang-tidy/fuchsia/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyFuchsiaModule
   DefaultArgumentsCheck.cpp
   FuchsiaTidyModule.cpp
+  MultipleInheritanceCheck.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 "MultipleInheritanceCheck.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<MultipleInheritanceCheck>(
+        "fuchsia-multiple-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/MultipleInheritanceCheck.h

+//===--- MultipleInheritanceCheck.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_MULTIPLE_INHERITANCE_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_FUCHSIA_MULTIPLE_INHERITANCE_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace fuchsia {
+
+/// Mulitple inheritance is disallowed.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/fuchsia-multiple-inheritance.html
+class MultipleInheritanceCheck : public ClangTidyCheck {
+public:
+  MultipleInheritanceCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+  void onEndOfTranslationUnit() override { InterfaceMap.clear(); }
+
+private:
+  void addNodeToInterfaceMap(const CXXRecordDecl *Node, bool isInterface);
+  bool getInterfaceStatus(const CXXRecordDecl *Node, bool &isInterface);
+  bool isCurrentClassInterface(const CXXRecordDecl *Node);
+  bool isInterface(const CXXRecordDecl *Node);
+
+  // Contains the identity of each named CXXRecord as an interface.  This is
+  // used to memoize lookup speeds and improve performance from O(N^2) to O(N),
+  // where N is the number of classes.
+  llvm::StringMap<bool> InterfaceMap;
+};
+
+} // namespace fuchsia
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_FUCHSIA_MULTIPLE_INHERITANCE_H

clang-tidy/fuchsia/MultipleInheritanceCheck.cpp

+//===--- MultipleInheritanceCheck.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 "MultipleInheritanceCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang;
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace fuchsia {
+
+// Adds a node (by name) to the interface map, if it was not present in the map
+// previously.
+void MultipleInheritanceCheck::addNodeToInterfaceMap(const CXXRecordDecl *Node,
+                                                     bool isInterface) {
+  StringRef Name = Node->getIdentifier()->getName();
+  InterfaceMap.insert(std::make_pair(Name, isInterface));
+}
+
+// Returns "true" if the boolean "isInterface" has been set to the
+// interface status of the current Node. Return "false" if the
+// interface status for the current node is not yet known.
+bool MultipleInheritanceCheck::getInterfaceStatus(const CXXRecordDecl *Node,
+                                                  bool &isInterface) {
+  StringRef Name = Node->getIdentifier()->getName();
+  if (InterfaceMap.count(Name)) {
+    isInterface = InterfaceMap.lookup(Name);
+    return true;
+  }
+  return false;
+}
+
+bool MultipleInheritanceCheck::isCurrentClassInterface(
+    const CXXRecordDecl *Node) {
+  // Interfaces should have no fields.
+  if (!Node->field_empty()) return false;
+
+  // Interfaces should have exclusively pure methods.
+  return llvm::none_of(Node->methods(), [](const CXXMethodDecl *M) {
+    return M->isUserProvided() && !M->isPure();
+  });
+}
+
+bool MultipleInheritanceCheck::isInterface(const CXXRecordDecl *Node) {
+  // Short circuit the lookup if we have analyzed this record before.
+  bool PreviousIsInterfaceResult;
+  if (getInterfaceStatus(Node, PreviousIsInterfaceResult)) 
+    return PreviousIsInterfaceResult;
+
+  // To be an interface, all base classes must be interfaces as well.
+  for (const auto &I : Node->bases()) {
+    const auto *Ty = I.getType()->getAs<RecordType>();
+    assert(Ty && "RecordType of base class is unknown");
+    const auto *Base = cast<CXXRecordDecl>(Ty->getDecl()->getDefinition());
+    if (!isInterface(Base)) {
+      addNodeToInterfaceMap(Node, false);
+      return false;
+    }
+  }
+
+  bool CurrentClassIsInterface = isCurrentClassInterface(Node);
+  addNodeToInterfaceMap(Node, CurrentClassIsInterface);
+  return CurrentClassIsInterface;
+}
+
+void MultipleInheritanceCheck::registerMatchers(MatchFinder *Finder) {
+  // Match declarations which have definitions.
+  Finder->addMatcher(cxxRecordDecl(hasDefinition()).bind("decl"), this);
+}
+
+void MultipleInheritanceCheck::check(const MatchFinder::MatchResult &Result) {
+  if (const auto *D = Result.Nodes.getNodeAs<CXXRecordDecl>("decl")) {
+    // Check against map to see if if the class inherits from multiple
+    // concrete classes
+    unsigned NumConcrete = 0;
+    for (const auto &I : D->bases()) {
+      const auto *Ty = I.getType()->getAs<RecordType>();
+      assert(Ty && "RecordType of base class is unknown");
+      const auto *Base = cast<CXXRecordDecl>(Ty->getDecl()->getDefinition());
+      if (!isInterface(Base)) NumConcrete++;
+    }
+
+    if (NumConcrete > 1) {
+      diag(D->getLocStart(),
+           "inheriting mulitple classes which aren't "
+           "pure virtual is discouraged");
+    }
+  }
+}
+
+}  // namespace fuchsia
+}  // namespace tidy
+}  // namespace clang

docs/ReleaseNotes.rst

 
 Improvements to clang-rename
 ----------------------------
 
 The improvements are...
 
 Improvements to clang-tidy
 --------------------------
 
 - New module `fuchsia` for Fuchsia style checks.
 
 - New module `objc` for Objective-C style checks.
 
 - New `android-cloexec-accept
   <http://clang.llvm.org/extra/clang-tidy/checks/android-cloexec-accept.html>`_ check
 
   Detects usage of ``accept()``.
 
- 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-multiple-inheritance
+  <http://clang.llvm.org/extra/clang-tidy/checks/fuchsia-multiple-inheritance.html>`_ check
+
+  Warns if a class inherits from multiple classes that are not pure virtual.
+
 - New `google-objc-avoid-throwing-exception
   <http://clang.llvm.org/extra/clang-tidy/checks/google-objc-avoid-throwing-exception.html>`_ check
 
   Add new check to detect throwing exceptions in Objective-C code, which should be avoided.
 
 - 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-multiple-inheritance.rst

+.. title:: clang-tidy - fuchsia-multiple-inheritance
+
+fuchsia-multiple-inheritance
+============================
+
+Warns if a class inherits from multiple classes that are not pure virtual.
+
+For example, declaring a class that inherits from multiple concrete classes is
+disallowed:
+
+.. code-block:: c++
+
+  class Base_A {
+  public:
+    virtual int foo() { return 0; }
+  };
+
+  class Base_B {
+  public:
+    virtual int bar() { return 0; }
+  };
+
+  // Warning
+  class Bad_Child1 : public Base_A, Base_B {};
+
+A class that inherits from a pure virtual is allowed:
+
+.. code-block:: c++
+
+  class Interface_A {
+  public:
+    virtual int foo() = 0;
+  };
+
+  class Interface_B {
+  public:
+    virtual int bar() = 0;
+  };
+
+  // No warning
+  class Good_Child1 : public Interface_A, Interface_B {
+    virtual int foo() override { return 0; }
+    virtual int bar() override { return 0; }
+  };
+
+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-multiple-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-multiple-inheritance.cpp

+// RUN: %check_clang_tidy %s fuchsia-multiple-inheritance %t
+
+class Base_A {
+public:
+  virtual int foo() { return 0; }
+};
+
+class Base_B {
+public:
+  virtual int bar() { return 0; }
+};
+
+class Base_A_child : public Base_A {
+public:
+  virtual int baz() { return 0; }
+};
+
+class Interface_A {
+public:
+  virtual int foo() = 0;
+};
+
+class Interface_B {
+public:
+  virtual int bar() = 0;
+};
+
+class Interface_C {
+public:
+  virtual int blat() = 0;
+};
+
+class Interface_A_with_member {
+public:
+  virtual int foo() = 0;
+  int val = 0;
+};
+
+class Interface_with_A_Parent : public Base_A {
+public:
+  virtual int baz() = 0;
+};
+
+// Inherits from multiple concrete classes.
+// CHECK-MESSAGES: [[@LINE+2]]:1: warning: inheriting mulitple classes which aren't pure virtual is discouraged [fuchsia-multiple-inheritance]
+// CHECK-NEXT: class Bad_Child1 : public Base_A, Base_B {};
+class Bad_Child1 : public Base_A, Base_B {};
+
+// CHECK-MESSAGES: [[@LINE+1]]:1: warning: inheriting mulitple classes which aren't pure virtual is discouraged [fuchsia-multiple-inheritance]
+class Bad_Child2 : public Base_A, Interface_A_with_member {
+  virtual int foo() override { return 0; }
+};
+
+// CHECK-MESSAGES: [[@LINE+2]]:1: warning: inheriting mulitple classes which aren't pure virtual is discouraged [fuchsia-multiple-inheritance]
+// CHECK-NEXT: class Bad_Child3 : public Interface_with_A_Parent, Base_B {
+class Bad_Child3 : public Interface_with_A_Parent, Base_B {
+  virtual int baz() override { return 0; }
+};
+
+// Easy cases of single inheritance
+class Simple_Child1 : public Base_A {};
+class Simple_Child2 : public Interface_A {
+  virtual int foo() override { return 0; }
+};
+
+// Valid uses of multiple inheritance
+class Good_Child1 : public Interface_A, Interface_B {
+  virtual int foo() override { return 0; }
+  virtual int bar() override { return 0; }
+};
+
+class Good_Child2 : public Base_A, Interface_B {
+  virtual int bar() override { return 0; }
+};
+
+class Good_Child3 : public Base_A_child, Interface_C, Interface_B {
+  virtual int bar() override { return 0; }
+  virtual int blat() override { return 0; }
+};
+
+int main(void) {
+  Bad_Child1 a;
+  Bad_Child2 b;
+  Bad_Child3 c;
+  Simple_Child1 d;
+  Simple_Child2 e;
+  Good_Child1 f;
+  Good_Child2 g;
+  Good_Child3 h;
+  return 0;
+}