[clang-tidy] Flag Classes Inheriting From Structs

[DennisMelamed]

Summary

Added a check which flags cases of a class inheriting from a struct, which can cause unintended scope issues since struct members are public by default.

Patch by Dennis Melamed

[lebedev.ri]

Please upload patches with full context.
Please do follow coding style, clang-format your patches.

Would it make sense to not catch-all all the class : struct, but consider the explicitly specified inheritance visibility?

clang-tidy/misc/ClassInheritFromStructCheck.cpp

21

Please linewrap to 80 chars.

test/clang-tidy/misc-class-inherit-from-struct.cpp

13

Missing cases:

• struct inheriting from struct
• Different inheritance visibility:
  • You only check the default visibility
  • What if class explicitly-publicly inherits from struct?
  • What if class explicitly-privately inherits from struct?
  • What if class explicitly-protectedly inherits from struct?
  • Same for struct inheriting from struct?

[aaron.ballman]

This strikes me as likely being a very chatty diagnostic. For instance, it will trigger on harmless code that uses empty base class optimization (in addition to other concerns pointed out by @lebedev.ri). It seems like the real concern here is unintentional information disclosure, but I don't see a good heuristic for determining that something is "unintentional" or not. I'd be curious to know how frequently this triggers on some large C++ code bases.

docs/clang-tidy/checks/misc-class-inherit-from-struct.rst

6–10

I don't agree with the predicate here -- structs aren't just used for C compatibility (look at <type_traits> as an example). They're also useful when all members need to be public, which is exactly the situation you claim may be unintentional.

[Eugene.Zelenko]

docs/ReleaseNotes.rst

109

Please use alphabetical order for new checks list.

docs/clang-tidy/checks/misc-class-inherit-from-struct.rst

6

Please make first statement same as in Release Notes.

15

I think warning is more correct term then error for Clang-tidy diagnistics.

[lebedev.ri]

test/clang-tidy/misc-class-inherit-from-struct.cpp

13

Also, i have only thought about it, there are no tests about what happens if you (say, publicly) inherit from struct, that does not have anything public.

clang-tidy/misc/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyMiscModule
+  ClassInheritFromStructCheck.cpp
   DefinitionsInHeadersCheck.cpp
   MiscTidyModule.cpp
   MisplacedConstCheck.cpp

clang-tidy/misc/ClassInheritFromStructCheck.h

+//===--- ClassInheritFromStructCheck.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_MISC_CLASSINHERITFROMSTRUCTCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_CLASSINHERITFROMSTRUCTCHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+/// Matches non-implicit classes which inherit from structs
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/misc-class-inherit-from-struct.html
+class ClassInheritFromStructCheck : public ClangTidyCheck {
+public:
+  ClassInheritFromStructCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_CLASSINHERITFROMSTRUCTCHECK_H

clang-tidy/misc/ClassInheritFromStructCheck.cpp

+//===--- ClassInheritFromStructCheck.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 "ClassInheritFromStructCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+void ClassInheritFromStructCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(cxxRecordDecl(isClass(),unless(isImplicit()),isDerivedFrom(cxxRecordDecl(isStruct()))).bind("class"), this);
+}
+
+void ClassInheritFromStructCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *MatchedDecl = Result.Nodes.getNodeAs<CXXRecordDecl>("class");
+  diag(MatchedDecl->getLocation(), "classes should not inherit from structs");
+}
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang

clang-tidy/misc/MiscTidyModule.cpp

 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
+#include "ClassInheritFromStructCheck.h"
 #include "DefinitionsInHeadersCheck.h"
 #include "MisplacedConstCheck.h"
 #include "NewDeleteOverloadsCheck.h"

 class MiscModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+    CheckFactories.registerCheck<ClassInheritFromStructCheck>(
+        "misc-class-inherit-from-struct");
     CheckFactories.registerCheck<DefinitionsInHeadersCheck>(
         "misc-definitions-in-headers");
     CheckFactories.registerCheck<MisplacedConstCheck>("misc-misplaced-const");

docs/ReleaseNotes.rst

   Detects usage of magic numbers, numbers that are used as literals instead of
   introduced via constants or symbols.
 
+- New :doc:`misc-class-inherit-from-struct
+  <clang-tidy/checks/misc-class-inherit-from-struct>` check.
+
+  Detects instances of classes inheriting from structs as that might lead to
+  unintended member access issues.
+
+
 Improvements to include-fixer
 -----------------------------
 

docs/clang-tidy/checks/list.rst

    llvm-include-order
    llvm-namespace-comment
    llvm-twine-local
+   misc-class-inherit-from-struct
    misc-definitions-in-headers
    misc-misplaced-const
    misc-new-delete-overloads

docs/clang-tidy/checks/misc-class-inherit-from-struct.rst

+.. title:: clang-tidy - misc-class-inherit-from-struct
+
+misc-class-inherit-from-struct
+==============================
+
+Finds instances of classes inheriting from structs. Structs are meant for 
+storing data and are often used to maintain C compatibility. Having a class
+inherit from them can lead to confusion with what type of object is being 
+dealt with. Additionally, the default public nature of struct members can 
+lead to unintentional exposure of members.
+
+.. code-block:: c++
+  
+  struct A {};
+  class B: public A {}; //throws an error, members of A might be
+                        //unintentionally exposed
+  class C {};
+  class D: public C {}; //fine

test/clang-tidy/misc-class-inherit-from-struct.cpp

+// RUN: %check_clang_tidy %s misc-class-inherit-from-struct %t
+
+struct A
+{
+    int a;
+};
+
+class B:A
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: classes should not inherit from structs [misc-class-inherit-from-struct]
+{
+    int b;
+};
+
+class C
+{
+    int c;
+};
+
+class D:C
+{
+    int d;
+};