[clang-tidy] Non-private member variables in classes (MISRA, CppCoreGuidelines, HICPP)

Summary:
Finds classes that not only contain the data (non-static member variables),
but also have logic (non-static member functions), and diagnoses all member
variables that have any other scope other than `private`. They should be
made `private`, and manipulated exclusively via the member functions.

Optionally, classes with all member variables being `public` could be
ignored, and optionally all `public` member variables could be ignored.

Options
-------

* IgnoreClassesWithAllMemberVariablesBeingPublic

  Allows to completely ignore classes if **all** the member variables in that
  class have `public` visibility.

* IgnorePublicMemberVariables

  Allows to ignore (not diagnose) **all** the member variables with `public`
  visibility scope.

References:
* MISRA 11-0-1 Member data in non-POD class types shall be private.
* https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#c2-use-class-if-the-class-has-an-invariant-use-struct-if-the-data-members-can-vary-independently
* https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rc-private
* https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rh-protected

Reviewers: JonasToth, aaron.ballman, alexfh, hokein, xazax.hun

Reviewed By: aaron.ballman

Subscribers: Eugene.Zelenko, zinovy.nis, cfe-commits, rnkovacs, nemanjai, mgorny, xazax.hun, kbarton

Tags: #clang-tools-extra

Differential Revision: https://reviews.llvm.org/D52771

llvm-svn: 344757

4 files changed, 143 insertions, 0 deletions

diff --git a/clang-tools-extra/clang-tidy/misc/CMakeLists.txt b/clang-tools-extra/clang-tidy/misc/CMakeLists.txt
index 08d5ceb9b3e..23dcfd6b26d 100644
--- a/clang-tools-extra/clang-tidy/misc/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/misc/CMakeLists.txt
@@ -6,6 +6,7 @@ add_clang_library(clangTidyMiscModule
   MisplacedConstCheck.cpp
   NewDeleteOverloadsCheck.cpp
   NonCopyableObjects.cpp
+  NonPrivateMemberVariablesInClassesCheck.cpp
   RedundantExpressionCheck.cpp
   StaticAssertCheck.cpp
   ThrowByValueCatchByReferenceCheck.cpp
diff --git a/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp b/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
index ebf38941dd4..dd9061a46aa 100644
--- a/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
@@ -14,6 +14,7 @@
 #include "MisplacedConstCheck.h"
 #include "NewDeleteOverloadsCheck.h"
 #include "NonCopyableObjects.h"
+#include "NonPrivateMemberVariablesInClassesCheck.h"
 #include "RedundantExpressionCheck.h"
 #include "StaticAssertCheck.h"
 #include "ThrowByValueCatchByReferenceCheck.h"
@@ -37,6 +38,8 @@ public:
         "misc-new-delete-overloads");
     CheckFactories.registerCheck<NonCopyableObjectsCheck>(
         "misc-non-copyable-objects");
+    CheckFactories.registerCheck<NonPrivateMemberVariablesInClassesCheck>(
+        "misc-non-private-member-variables-in-classes");
     CheckFactories.registerCheck<RedundantExpressionCheck>(
         "misc-redundant-expression");
     CheckFactories.registerCheck<StaticAssertCheck>("misc-static-assert");
diff --git a/clang-tools-extra/clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.cpp b/clang-tools-extra/clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.cpp
new file mode 100644
index 00000000000..273fe7bf410
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.cpp
@@ -0,0 +1,93 @@
+//===--- NonPrivateMemberVariablesInClassesCheck.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 "NonPrivateMemberVariablesInClassesCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+namespace {
+
+AST_MATCHER(CXXRecordDecl, hasMethods) {
+  return std::distance(Node.method_begin(), Node.method_end()) != 0;
+}
+
+AST_MATCHER(CXXRecordDecl, hasNonStaticMethod) {
+  return hasMethod(unless(isStaticStorageClass()))
+      .matches(Node, Finder, Builder);
+}
+
+AST_MATCHER(CXXRecordDecl, hasNonPublicMemberVariable) {
+  return cxxRecordDecl(has(fieldDecl(unless(isPublic()))))
+      .matches(Node, Finder, Builder);
+}
+
+AST_POLYMORPHIC_MATCHER_P(boolean, AST_POLYMORPHIC_SUPPORTED_TYPES(Stmt, Decl),
+                          bool, Boolean) {
+  return Boolean;
+}
+
+} // namespace
+
+NonPrivateMemberVariablesInClassesCheck::
+    NonPrivateMemberVariablesInClassesCheck(StringRef Name,
+                                            ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      IgnoreClassesWithAllMemberVariablesBeingPublic(
+          Options.get("IgnoreClassesWithAllMemberVariablesBeingPublic", false)),
+      IgnorePublicMemberVariables(
+          Options.get("IgnorePublicMemberVariables", false)) {}
+
+void NonPrivateMemberVariablesInClassesCheck::registerMatchers(
+    MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus)
+    return;
+
+  // We can ignore structs/classes with all member variables being public.
+  auto ShouldIgnoreRecord =
+      allOf(boolean(IgnoreClassesWithAllMemberVariablesBeingPublic),
+            unless(hasNonPublicMemberVariable()));
+
+  // We only want the records that not only contain the mutable data (non-static
+  // member variables), but also have some logic (non-static member functions).
+  // We may optionally ignore records where all the member variables are public.
+  auto RecordIsInteresting =
+      allOf(anyOf(isStruct(), isClass()), hasMethods(), hasNonStaticMethod(),
+            unless(ShouldIgnoreRecord));
+
+  // There are three visibility types: public, protected, private.
+  // If we are ok with public fields, then we only want to complain about
+  // protected fields, else we want to complain about all non-private fields.
+  // We can ignore public member variables in structs/classes, in unions.
+  auto InterestingField = fieldDecl(
+      IgnorePublicMemberVariables ? isProtected() : unless(isPrivate()));
+
+  Finder->addMatcher(cxxRecordDecl(RecordIsInteresting,
+                                   forEach(InterestingField.bind("field")))
+                         .bind("record"),
+                     this);
+}
+
+void NonPrivateMemberVariablesInClassesCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  const auto *Field = Result.Nodes.getNodeAs<FieldDecl>("field");
+  assert(Field && "We should have the field we are going to complain about");
+
+  diag(Field->getLocation(), "member variable %0 has %1 visibility")
+      << Field << Field->getAccess();
+}
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang
diff --git a/clang-tools-extra/clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.h b/clang-tools-extra/clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.h
new file mode 100644
index 00000000000..c39e356c96d
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.h
@@ -0,0 +1,46 @@
+//===--- NonPrivateMemberVariablesInClassesCheck.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_NONPRIVATEMEMBERVARIABLESINCLASSESCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_NONPRIVATEMEMBERVARIABLESINCLASSESCHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+/// This checker finds classes that not only contain the data
+/// (non-static member variables), but also have logic (non-static member
+/// functions), and diagnoses all member variables that have any other scope
+/// other than `private`. They should be made `private`, and manipulated
+/// exclusively via the member functions.
+///
+/// Optionally, classes with all member variables being `public` could be
+/// ignored and optionally all `public` member variables could be ignored.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/misc-non-private-member-variables-in-classes.html
+class NonPrivateMemberVariablesInClassesCheck : public ClangTidyCheck {
+public:
+  NonPrivateMemberVariablesInClassesCheck(StringRef Name,
+                                          ClangTidyContext *Context);
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  const bool IgnoreClassesWithAllMemberVariablesBeingPublic;
+  const bool IgnorePublicMemberVariables;
+};
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_NONPRIVATEMEMBERVARIABLESINCLASSESCHECK_H