[clang-tidy] Non-private member variables in classes (MISRA, CppCoreG…

…uidelines, 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

clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp

@@ -10,6 +10,7 @@
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
+#include "../misc/NonPrivateMemberVariablesInClassesCheck.h"
 #include "../misc/UnconventionalAssignOperatorCheck.h"
 #include "../readability/MagicNumbersCheck.h"
 #include "AvoidGotoCheck.h"
@@ -47,6 +48,8 @@ class CppCoreGuidelinesModule : public ClangTidyModule {
     CheckFactories.registerCheck<NarrowingConversionsCheck>(
         "cppcoreguidelines-narrowing-conversions");
     CheckFactories.registerCheck<NoMallocCheck>("cppcoreguidelines-no-malloc");
+    CheckFactories.registerCheck<misc::NonPrivateMemberVariablesInClassesCheck>(
+        "cppcoreguidelines-non-private-member-variables-in-classes");
     CheckFactories.registerCheck<OwningMemoryCheck>(
         "cppcoreguidelines-owning-memory");
     CheckFactories.registerCheck<ProBoundsArrayToPointerDecayCheck>(
@@ -75,6 +78,16 @@ class CppCoreGuidelinesModule : public ClangTidyModule {
     CheckFactories.registerCheck<misc::UnconventionalAssignOperatorCheck>(
         "cppcoreguidelines-c-copy-assignment-signature");
   }
+
+  ClangTidyOptions getModuleOptions() override {
+    ClangTidyOptions Options;
+    ClangTidyOptions::OptionMap &Opts = Options.CheckOptions;
+
+    Opts["cppcoreguidelines-non-private-member-variables-in-classes."
+         "IgnoreClassesWithAllMemberVariablesBeingPublic"] = "1";
+
+    return Options;
+  }
 };
 
 // Register the LLVMTidyModule using this statically initialized variable.

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

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 @@ class MiscModule : public ClangTidyModule {
         "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");

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

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

clang-tools-extra/docs/ReleaseNotes.rst

@@ -103,6 +103,13 @@ Improvements to clang-tidy
   Flags uses of ``absl::StrCat()`` to append to a ``std::string``. Suggests
   ``absl::StrAppend()`` should be used instead.
 
+- New :doc:`misc-non-private-member-variables-in-classes
+  <clang-tidy/checks/misc-non-private-member-variables-in-classes>` check.
+
+  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``.
+
 - New :doc:`modernize-concat-nested-namespaces
   <clang-tidy/checks/modernize-concat-nested-namespaces>` check.
 
@@ -134,6 +141,12 @@ Improvements to clang-tidy
   <clang-tidy/checks/readability-uppercase-literal-suffix>`
   added.
 
+- New alias :doc:`cppcoreguidelines-non-private-member-variables-in-classes
+  <clang-tidy/checks/cppcoreguidelines-non-private-member-variables-in-classes>`
+  to :doc:`misc-non-private-member-variables-in-classes
+  <clang-tidy/checks/misc-non-private-member-variables-in-classes>`
+  added.
+
 - New alias :doc:`hicpp-uppercase-literal-suffix
   <clang-tidy/checks/hicpp-uppercase-literal-suffix>` to
   :doc:`readability-uppercase-literal-suffix

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-non-private-member-variables-in-classes.rst

@@ -0,0 +1,11 @@
+.. title:: clang-tidy - cppcoreguidelines-non-private-member-variables-in-classes
+.. meta::
+   :http-equiv=refresh: 5;URL=misc-non-private-member-variables-in-classes.html
+
+cppcoreguidelines-non-private-member-variables-in-classes
+=========================================================
+
+The cppcoreguidelines-non-private-member-variables-in-classes check is an alias,
+please see
+`misc-non-private-member-variables-in-classes <misc-non-private-member-variables-in-classes.html>`_
+for more information.

clang-tools-extra/docs/clang-tidy/checks/list.rst

@@ -91,6 +91,7 @@ Clang-Tidy Checks
    cppcoreguidelines-interfaces-global-init
    cppcoreguidelines-narrowing-conversions
    cppcoreguidelines-no-malloc
+   cppcoreguidelines-non-private-member-variables-in-classes (redirects to misc-non-private-member-variables-in-classes) <cppcoreguidelines-non-private-member-variables-in-classes>
    cppcoreguidelines-owning-memory
    cppcoreguidelines-pro-bounds-array-to-pointer-decay
    cppcoreguidelines-pro-bounds-constant-array-index
@@ -165,6 +166,7 @@ Clang-Tidy Checks
    misc-misplaced-const
    misc-new-delete-overloads
    misc-non-copyable-objects
+   misc-non-private-member-variables-in-classes
    misc-redundant-expression
    misc-static-assert
    misc-throw-by-value-catch-by-reference

clang-tools-extra/docs/clang-tidy/checks/misc-non-private-member-variables-in-classes.rst

@@ -0,0 +1,26 @@
+.. title:: clang-tidy - misc-non-private-member-variables-in-classes
+
+misc-non-private-member-variables-in-classes
+============================================
+
+`cppcoreguidelines-non-private-member-variables-in-classes` redirects here
+as an alias for this check.
+
+Finds classes that contain non-static data members in addition to non-static
+member functions and diagnose all data members declared with a non-``public``
+access specifier. The data members should be declared as ``private`` and
+accessed through member functions instead of exposed to derived classes or
+class consumers.
+
+Options
+-------
+
+.. option:: IgnoreClassesWithAllMemberVariablesBeingPublic
+
+   Allows to completely ignore classes if **all** the member variables in that
+   class a declared with a ``public`` access specifier.
+
+.. option:: IgnorePublicMemberVariables
+
+   Allows to ignore (not diagnose) **all** the member variables declared with
+   a ``public`` access specifier.