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

[lebedev.ri]

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

[lebedev.ri]

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.
• http://www.codingstandard.com/rule/11-1-1-declare-all-data-members-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

[JonasToth]

clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.cpp

26 ↗

(On Diff #167886)

I think this and the next matcher can be a normal variable.

auto HasNonStaticMethod = hasMethod(unless(isStaticStorageClass()));

...
... allOf(anyOf(isStruct(), isClass()), HasMethods, HasNonStaticMethod)
...

55 ↗

(On Diff #167886)

Please make this variable uppercase, same for next matcher

76 ↗

(On Diff #167886)

Please add assertions that these pointers are not-null. Some random bug could match only one of these too and result in null-deref

clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.h

31 ↗

(On Diff #167886)

i believe usually the private state is written at the bottom of a class. I dont have a strong opinion, but i believe alex does prefer it.

docs/ReleaseNotes.rst

99 ↗

(On Diff #167886)

Please shorten the text in the release notes a bit. Usually one sentence should be enough, you can explain more in the docs

test/clang-tidy/misc-non-private-member-variables-in-classes.cpp

1 ↗

(On Diff #167886)

I would prefer multiple test-files instead of mixing them all together.

9 ↗

(On Diff #167886)

The test miss

• templated classes (should be diagnosed normally)
• inheritance (public, private, protected) (should not add a new warning in the subclass)
• and macros creating members. (dont know what to do there, but just warn?)

[lebedev.ri]

Address review notes - more tests.

clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.cpp

26 ↗

(On Diff #167886)

They could, but these have a very meaningful meaning, so i'd argue it would not be worse to keep them
out here refactored as proper matchers, in case someone else needs them elsewhere (so he could just
move them from here into more general place, as opposed to writing a duplicating matcher.)

test/clang-tidy/misc-non-private-member-variables-in-classes.cpp

1 ↗

(On Diff #167886)

Hmm, no. Then the tests will have to be duplicated in N files,
because i really do want to see what each of these 4 configurations do on the *same* input.

It only looks ugly because the script only supports -check-suffix=,
and not -check-suffixes=, which would significantly cut down on check-lines.

[aaron.ballman]

I can't help but notice how badly C.133 and C.9 interact with C.131 and I'm worried we will wind up with clang-tidy checks that leave the user in an impossible situation where they need to make data members private and provide trivial accessors for them. Do you have thoughts on how to avoid that? The C++ Core Guidelines seem silent on the matter -- this might be worth raising with the authors.

The HIC++ standard has an exception that requires the data type to be in an extern "C" context, but I don't see that reflected in the tests or code.

clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.cpp

22 ↗

(On Diff #168162)

Can drop the clang:: qualifiers -- you're in the namespace clang scope already.

56 ↗

(On Diff #168162)

Neither the C++ Core Guidelines nor HIC++ carve out an exception for union types. I think you should talk to the guideline authors to see what they think. Perhaps first try to diagnose unions the same as structs and classes and see how the results look to you -- if they look bad, you can incorporate those as examples when discussing with the authors.

82 ↗

(On Diff #168162)

Drop the single quotes above and just pass in Field and Record directly -- the diagnostic printer will do the right thing.

[JonasToth]

clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.cpp

26 ↗

(On Diff #167886)

Ok.

test/clang-tidy/misc-non-private-member-variables-in-classes.cpp

1 ↗

(On Diff #167886)

Ok, if you prefer this.

[JonasToth]

In D52771#1256432, @aaron.ballman wrote:

I can't help but notice how badly C.133 and C.9 interact with C.131 and I'm worried we will wind up with clang-tidy checks that leave the user in an impossible situation where they need to make data members private and provide trivial accessors for them. Do you have thoughts on how to avoid that? The C++ Core Guidelines seem silent on the matter -- this might be worth raising with the authors.

I have an idea to avoid trivial set/get methods: Flag public data if it is modified internally. This could serve as a heuristic for pre/postconditions on that variable.
In general one could resolve all data-dependencies inside the class and figure out if a private variable would need to change if a public variable got changed. But that is way more complex!

Still worth asking the authors.

[aaron.ballman]

In D52771#1256511, @JonasToth wrote:

In D52771#1256432, @aaron.ballman wrote:

I can't help but notice how badly C.133 and C.9 interact with C.131 and I'm worried we will wind up with clang-tidy checks that leave the user in an impossible situation where they need to make data members private and provide trivial accessors for them. Do you have thoughts on how to avoid that? The C++ Core Guidelines seem silent on the matter -- this might be worth raising with the authors.

I have an idea to avoid trivial set/get methods: Flag public data if it is modified internally. This could serve as a heuristic for pre/postconditions on that variable.
In general one could resolve all data-dependencies inside the class and figure out if a private variable would need to change if a public variable got changed. But that is way more complex!

Hmm, I don't know that it would help with code like this:

class Base {
  int f; // Suggested by C.133/C.9

protected:
//  int f; // Disallowed by C.133

  int getF() const { return f; } // Suggested by C.133, disallowed by C.131
  void setF(int NewF) { f = NewF; }  // Suggested by C.133, disallowed by C.131
};

Still worth asking the authors.

[JonasToth]

C.131 seems to imply a minimal amount of trivial getters/setters before
diagnosing.

I feel that the CPPCG just want to force the programmer to think twice
instead of forbidding it totally.
It might be worth to have a more chatty/specific check for the CPPCG and
a strict "Don't do it" for HICPP.

From my experience with asking the CPPCG authors it takes a while for a
response and that in the end is
not necessarily telling which way to go. Experience might differ
depending on topic though.

Hmm, I don't know that it would help with code like this:

class Base {
  int f; // Suggested by C.133/C.9

protected:
//  int f; // Disallowed by C.133

  int getF() const { return f; } // Suggested by C.133, disallowed by C.131
  void setF(int NewF) { f = NewF; }  // Suggested by C.133, disallowed by C.131
};

Still worth asking the authors.

[zinovy.nis]

test/clang-tidy/misc-non-private-member-variables-in-classes.cpp

1 ↗

(On Diff #167886)

Not an issue anymore. You can use -check-suffixes since now.

[lebedev.ri]

Let's try to wrap this up.

• Dropped HICPP alias. I/we really don't understand what those guidelines require.
• Don't touch unions. https://github.com/isocpp/CppCoreGuidelines/issues/1281
• Deduplicate check lines, yay.
• Some code cleanups.

FIXME: IgnoreClassesWithAllMemberVariablesBeingPublic needs to be somehow enabled for cppcoreguidelines check.
I don't know if it is possible, and how.

Anything i missed?

clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.cpp

56 ↗

(On Diff #168162)

As discussed, let's just drop HICPP alias,
and CPPCG only talks about struct/class, not union.
https://github.com/isocpp/CppCoreGuidelines/issues/1281

82 ↗

(On Diff #168162)

Nice, but that does not seem to dump getKindName().

test/clang-tidy/misc-non-private-member-variables-in-classes.cpp

1 ↗

(On Diff #167886)

Yay, look at this beauty :)

[lebedev.ri]

Dead code elimination.

[JonasToth]

test/clang-tidy/misc-non-private-member-variables-in-classes.cpp

1 ↗

(On Diff #167886)

;)

[Eugene.Zelenko]

docs/ReleaseNotes.rst

113 ↗

(On Diff #169405)

Please move new alias after new checks.

[lebedev.ri]

docs/ReleaseNotes.rst

113 ↗

(On Diff #169405)

You mean after all the new checks, at the very bottom?
Because right now it is right after the check itself.

[JonasToth]

docs/ReleaseNotes.rst

113 ↗

(On Diff #169405)

Yup, the aliases were mentioned last in the release notes (at least last release).

[lebedev.ri]

Sort release notes.

docs/ReleaseNotes.rst

113 ↗

(On Diff #169405)

Seems rather arbitrarily and undocumented but whatever.

[aaron.ballman]

In D52771#1263404, @lebedev.ri wrote:

FIXME: IgnoreClassesWithAllMemberVariablesBeingPublic needs to be somehow enabled for cppcoreguidelines check.
I don't know if it is possible, and how.

IIRC, the idea is to override getModuleOptions() and specify different default configuration options for the alias.

clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.cpp

82 ↗

(On Diff #168162)

Correct - it only works for automatically quoting NamedDecl objects

90 ↗

(On Diff #169555)

I cannot think of a situation in which the class name is actually useful information to present to the user, so I'd recommend dropping it (and the kind name as well). However, if you have a specific case in mind where the class name is useful for understanding how to fix the issue, I'd love to see it.

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

9 ↗

(On Diff #169555)

I'd reword this a bit to:

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.

14–15 ↗

(On Diff #169555)

I'd drop this paragraph entirely.

28 ↗

(On Diff #169555)

with public visibility scope -> declared with a public access specifier

[lebedev.ri]

• Address nits
• Proper default for CPPCG
• Don't show record name. I indeed don't have a strong case here, it seems to be more useful to dump that info, but maybe not? ¯\_(ツ)_/¯

[aaron.ballman]

LGTM aside from a minor nit.

clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp

84 ↗

(On Diff #170065)

Don't use auto here.

[lebedev.ri]

In D52771#1268367, @aaron.ballman wrote:

LGTM aside from a minor nit.

Thank you for the review!

[lebedev.ri]

Don't use auto in getModuleOptions().

[lebedev.ri]

Rebased to fix a merge conflict in release notes.

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

 //===--- CppCoreGuidelinesModule.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 "../misc/NonPrivateMemberVariablesInClassesCheck.h"
 #include "../misc/UnconventionalAssignOperatorCheck.h"
 #include "../readability/MagicNumbersCheck.h"
 #include "AvoidGotoCheck.h"
 #include "InterfacesGlobalInitCheck.h"
 #include "NarrowingConversionsCheck.h"
 #include "NoMallocCheck.h"
 #include "OwningMemoryCheck.h"
 #include "ProBoundsArrayToPointerDecayCheck.h"
CheckFactories.registerCheck<AvoidGotoCheck>(
         "cppcoreguidelines-avoid-goto");
     CheckFactories.registerCheck<readability::MagicNumbersCheck>(
         "cppcoreguidelines-avoid-magic-numbers");
     CheckFactories.registerCheck<InterfacesGlobalInitCheck>(
         "cppcoreguidelines-interfaces-global-init");
     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>(
         "cppcoreguidelines-pro-bounds-array-to-pointer-decay");
     CheckFactories.registerCheck<ProBoundsConstantArrayIndexCheck>(
         "cppcoreguidelines-pro-bounds-constant-array-index");
     CheckFactories.registerCheck<ProBoundsPointerArithmeticCheck>(
         "cppcoreguidelines-pro-bounds-pointer-arithmetic");
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<ProTypeVarargCheck>(
         "cppcoreguidelines-pro-type-vararg");
     CheckFactories.registerCheck<SpecialMemberFunctionsCheck>(
         "cppcoreguidelines-special-member-functions");
     CheckFactories.registerCheck<SlicingCheck>("cppcoreguidelines-slicing");
     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.
 static ClangTidyModuleRegistry::Add<CppCoreGuidelinesModule>
     X("cppcoreguidelines-module", "Adds checks for the C++ Core Guidelines.");
 
 } // namespace cppcoreguidelines
 
 // This anchor is used to force the linker to link in the generated object file
 // and thus register the CppCoreGuidelinesModule.
 volatile int CppCoreGuidelinesModuleAnchorSource = 0;
 
 } // namespace tidy
 } // namespace clang

clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyMiscModule
   DefinitionsInHeadersCheck.cpp
   MiscTidyModule.cpp
   MisplacedConstCheck.cpp
   NewDeleteOverloadsCheck.cpp
   NonCopyableObjects.cpp
+  NonPrivateMemberVariablesInClassesCheck.cpp
   RedundantExpressionCheck.cpp
   StaticAssertCheck.cpp
   ThrowByValueCatchByReferenceCheck.cpp
   UnconventionalAssignOperatorCheck.cpp
   UniqueptrResetReleaseCheck.cpp
   UnusedAliasDeclsCheck.cpp
   UnusedParametersCheck.cpp
   UnusedUsingDeclsCheck.cpp

clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp

 //===--- MiscTidyModule.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 "DefinitionsInHeadersCheck.h"
 #include "MisplacedConstCheck.h"
 #include "NewDeleteOverloadsCheck.h"
 #include "NonCopyableObjects.h"
+#include "NonPrivateMemberVariablesInClassesCheck.h"
 #include "RedundantExpressionCheck.h"
 #include "StaticAssertCheck.h"
 #include "ThrowByValueCatchByReferenceCheck.h"
 #include "UnconventionalAssignOperatorCheck.h"
 #include "UniqueptrResetReleaseCheck.h"
 #include "UnusedAliasDeclsCheck.h"
 #include "UnusedParametersCheck.h"
 #include "UnusedUsingDeclsCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace misc {
 
 class MiscModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<DefinitionsInHeadersCheck>(
         "misc-definitions-in-headers");
     CheckFactories.registerCheck<MisplacedConstCheck>("misc-misplaced-const");
     CheckFactories.registerCheck<NewDeleteOverloadsCheck>(
         "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");
     CheckFactories.registerCheck<ThrowByValueCatchByReferenceCheck>(
         "misc-throw-by-value-catch-by-reference");
     CheckFactories.registerCheck<UnconventionalAssignOperatorCheck>(
         "misc-unconventional-assign-operator");
     CheckFactories.registerCheck<UniqueptrResetReleaseCheck>(

clang-tools-extra/trunk/clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.h

+//===--- 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/trunk/clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.cpp

+//===--- 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/trunk/docs/ReleaseNotes.rst

- New :doc:`abseil-redundant-strcat-calls
   being passed to another ``absl::StrCat`` or ``absl::StrAppend``.
 
 - New :doc:`abseil-str-cat-append
   <clang-tidy/checks/abseil-str-cat-append>` check.
 
   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.
 
   Checks for uses of nested namespaces in the form of
   ``namespace a { namespace b { ... }}`` and offers change to
   syntax introduced in C++17 standard: ``namespace a::b { ... }``.
 
 - New :doc:`modernize-deprecated-ios-base-aliases
- New :doc:`readability-uppercase-literal-suffix
   suffix, and suggests to make the suffix uppercase. The list of destination
   suffixes can be optionally provided.
 
 - New alias :doc:`cert-dcl16-c
   <clang-tidy/checks/cert-dcl16-c>` to :doc:`readability-uppercase-literal-suffix
   <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-tidy/checks/readability-uppercase-literal-suffix>`
   added.
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 
 Improvements to modularize
 --------------------------
 
 The improvements are...

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

+.. 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/trunk/docs/clang-tidy/checks/list.rst

.. toctree::
    cert-msc51-cpp
    cert-oop11-cpp (redirects to performance-move-constructor-init) <cert-oop11-cpp>
    cppcoreguidelines-avoid-goto
    cppcoreguidelines-avoid-magic-numbers (redirects to readability-magic-numbers) <cppcoreguidelines-avoid-magic-numbers>
    cppcoreguidelines-c-copy-assignment-signature (redirects to misc-unconventional-assign-operator) <cppcoreguidelines-c-copy-assignment-signature>
    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
    cppcoreguidelines-pro-bounds-pointer-arithmetic
    cppcoreguidelines-pro-type-const-cast
    cppcoreguidelines-pro-type-cstyle-cast
    cppcoreguidelines-pro-type-member-init
    cppcoreguidelines-pro-type-reinterpret-cast
.. toctree::
    llvm-header-guard
    llvm-include-order
    llvm-namespace-comment
    llvm-twine-local
    misc-definitions-in-headers
    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
    misc-unconventional-assign-operator
    misc-uniqueptr-reset-release
    misc-unused-alias-decls
    misc-unused-parameters
    misc-unused-using-decls

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

+.. 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.

clang-tools-extra/trunk/test/clang-tidy/misc-non-private-member-variables-in-classes.cpp

+// RUN: %check_clang_tidy -check-suffixes=PUBLIC,NONPRIVATE,PROTECTED %s misc-non-private-member-variables-in-classes %t
+// RUN: %check_clang_tidy -check-suffixes=PUBLIC,NONPRIVATE,PROTECTED %s misc-non-private-member-variables-in-classes %t -- -config='{CheckOptions: [{key: misc-non-private-member-variables-in-classes.IgnorePublicMemberVariables, value: 0}, {key: misc-non-private-member-variables-in-classes.IgnoreClassesWithAllMemberVariablesBeingPublic, value: 0}]}' --
+// RUN: %check_clang_tidy -check-suffixes=PUBLIC,PROTECTED %s misc-non-private-member-variables-in-classes %t -- -config='{CheckOptions: [{key: misc-non-private-member-variables-in-classes.IgnorePublicMemberVariables, value: 0}, {key: misc-non-private-member-variables-in-classes.IgnoreClassesWithAllMemberVariablesBeingPublic, value: 1}]}' --
+// RUN: %check_clang_tidy -check-suffixes=PUBLIC,PROTECTED %s cppcoreguidelines-non-private-member-variables-in-classes %t -- --
+// RUN: %check_clang_tidy -check-suffixes=PROTECTED %s misc-non-private-member-variables-in-classes %t -- -config='{CheckOptions: [{key: misc-non-private-member-variables-in-classes.IgnorePublicMemberVariables, value: 1}, {key: misc-non-private-member-variables-in-classes.IgnoreClassesWithAllMemberVariablesBeingPublic, value: 0}]}' --
+// RUN: %check_clang_tidy -check-suffixes=PROTECTED %s misc-non-private-member-variables-in-classes %t -- -config='{CheckOptions: [{key: misc-non-private-member-variables-in-classes.IgnorePublicMemberVariables, value: 1}, {key: misc-non-private-member-variables-in-classes.IgnoreClassesWithAllMemberVariablesBeingPublic, value: 1}]}' --
+
+//----------------------------------------------------------------------------//
+
+// Only data, do not warn
+
+struct S0 {
+  int S0_v0;
+
+public:
+  int S0_v1;
+
+protected:
+  int S0_v2;
+
+private:
+  int S0_v3;
+};
+
+class S1 {
+  int S1_v0;
+
+public:
+  int S1_v1;
+
+protected:
+  int S1_v2;
+
+private:
+  int S1_v3;
+};
+
+//----------------------------------------------------------------------------//
+
+// All functions are static, do not warn.
+
+struct S2 {
+  static void S2_m0();
+  int S2_v0;
+
+public:
+  static void S2_m1();
+  int S2_v1;
+
+protected:
+  static void S2_m3();
+  int S2_v2;
+
+private:
+  static void S2_m4();
+  int S2_v3;
+};
+
+class S3 {
+  static void S3_m0();
+  int S3_v0;
+
+public:
+  static void S3_m1();
+  int S3_v1;
+
+protected:
+  static void S3_m3();
+  int S3_v2;
+
+private:
+  static void S3_m4();
+  int S3_v3;
+};
+
+//============================================================================//
+
+// union != struct/class. do not diagnose.
+
+union U0 {
+  void U0_m0();
+  int U0_v0;
+
+public:
+  void U0_m1();
+  int U0_v1;
+
+protected:
+  void U0_m2();
+  int U0_v2;
+
+private:
+  void U0_m3();
+  int U0_v3;
+};
+
+//============================================================================//
+
+// Has non-static method with default visibility.
+
+struct S4 {
+  void S4_m0();
+
+  int S4_v0;
+  // CHECK-MESSAGES-PUBLIC: :[[@LINE-1]]:7: warning: member variable 'S4_v0' has public visibility
+public:
+  int S4_v1;
+  // CHECK-MESSAGES-PUBLIC: :[[@LINE-1]]:7: warning: member variable 'S4_v1' has public visibility
+protected:
+  int S4_v2;
+  // CHECK-MESSAGES-PROTECTED: :[[@LINE-1]]:7: warning: member variable 'S4_v2' has protected visibility
+private:
+  int S4_v3;
+};
+
+class S5 {
+  void S5_m0();
+
+  int S5_v0;
+
+public:
+  int S5_v1;
+  // CHECK-MESSAGES-PUBLIC: :[[@LINE-1]]:7: warning: member variable 'S5_v1' has public visibility
+protected:
+  int S5_v2;
+  // CHECK-MESSAGES-PROTECTED: :[[@LINE-1]]:7: warning: member variable 'S5_v2' has protected visibility
+private:
+  int S5_v3;
+};
+
+//----------------------------------------------------------------------------//
+
+// Has non-static method with public visibility.
+
+struct S6 {
+  int S6_v0;
+  // CHECK-MESSAGES-PUBLIC: :[[@LINE-1]]:7: warning: member variable 'S6_v0' has public visibility
+public:
+  void S6_m0();
+  int S6_v1;
+  // CHECK-MESSAGES-PUBLIC: :[[@LINE-1]]:7: warning: member variable 'S6_v1' has public visibility
+protected:
+  int S6_v2;
+  // CHECK-MESSAGES-PROTECTED: :[[@LINE-1]]:7: warning: member variable 'S6_v2' has protected visibility
+private:
+  int S6_v3;
+};
+
+class S7 {
+  int S7_v0;
+
+public:
+  void S7_m0();
+  int S7_v1;
+  // CHECK-MESSAGES-PUBLIC: :[[@LINE-1]]:7: warning: member variable 'S7_v1' has public visibility
+protected:
+  int S7_v2;
+  // CHECK-MESSAGES-PROTECTED: :[[@LINE-1]]:7: warning: member variable 'S7_v2' has protected visibility
+private:
+  int S7_v3;
+};
+
+//----------------------------------------------------------------------------//
+
+// Has non-static method with protected visibility.
+
+struct S8 {
+  int S8_v0;
+  // CHECK-MESSAGES-PUBLIC: :[[@LINE-1]]:7: warning: member variable 'S8_v0' has public visibility
+public:
+  int S8_v1;
+  // CHECK-MESSAGES-PUBLIC: :[[@LINE-1]]:7: warning: member variable 'S8_v1' has public visibility
+protected:
+  void S8_m0();
+  int S8_v2;
+  // CHECK-MESSAGES-PROTECTED: :[[@LINE-1]]:7: warning: member variable 'S8_v2' has protected visibility
+private:
+  int S8_v3;
+};
+
+class S9 {
+  int S9_v0;
+
+public:
+  int S9_v1;
+  // CHECK-MESSAGES-PUBLIC: :[[@LINE-1]]:7: warning: member variable 'S9_v1' has public visibility
+protected:
+  void S9_m0();
+  int S9_v2;
+  // CHECK-MESSAGES-PROTECTED: :[[@LINE-1]]:7: warning: member variable 'S9_v2' has protected visibility
+private:
+  int S9_v3;
+};
+
+//----------------------------------------------------------------------------//
+
+// Has non-static method with private visibility.
+
+struct S10 {
+  int S10_v0;
+  // CHECK-MESSAGES-PUBLIC: :[[@LINE-1]]:7: warning: member variable 'S10_v0' has public visibility
+public:
+  int S10_v1;
+  // CHECK-MESSAGES-PUBLIC: :[[@LINE-1]]:7: warning: member variable 'S10_v1' has public visibility
+protected:
+  int S10_v2;
+  // CHECK-MESSAGES-PROTECTED: :[[@LINE-1]]:7: warning: member variable 'S10_v2' has protected visibility
+private:
+  void S10_m0();
+  int S10_v3;
+};
+
+class S11 {
+  int S11_v0;
+
+public:
+  int S11_v1;
+  // CHECK-MESSAGES-PUBLIC: :[[@LINE-1]]:7: warning: member variable 'S11_v1' has public visibility
+protected:
+  int S11_v2;
+  // CHECK-MESSAGES-PROTECTED: :[[@LINE-1]]:7: warning: member variable 'S11_v2' has protected visibility
+private:
+  void S11_m0();
+  int S11_v3;
+};
+
+//============================================================================//
+
+// Static variables are ignored.
+// Has non-static methods and static variables.
+
+struct S12 {
+  void S12_m0();
+  static int S12_v0;
+
+public:
+  void S12_m1();
+  static int S12_v1;
+
+protected:
+  void S12_m2();
+  static int S12_v2;
+
+private:
+  void S12_m3();
+  static int S12_v3;
+};
+
+class S13 {
+  void S13_m0();
+  static int S13_v0;
+
+public:
+  void S13_m1();
+  static int S13_v1;
+
+protected:
+  void S13_m2();
+  static int S13_v2;
+
+private:
+  void S13_m3();
+  static int S13_v3;
+};
+
+struct S14 {
+  void S14_m0();
+  int S14_v0;
+  // CHECK-MESSAGES-NONPRIVATE: :[[@LINE-1]]:7: warning: member variable 'S14_v0' has public visibility
+
+public:
+  void S14_m1();
+  int S14_v1;
+  // CHECK-MESSAGES-NONPRIVATE: :[[@LINE-1]]:7: warning: member variable 'S14_v1' has public visibility
+
+protected:
+  void S14_m2();
+
+private:
+  void S14_m3();
+};
+
+class S15 {
+  void S15_m0();
+
+public:
+  void S15_m1();
+  int S15_v1;
+  // CHECK-MESSAGES-NONPRIVATE: :[[@LINE-1]]:7: warning: member variable 'S15_v1' has public visibility
+
+protected:
+  void S15_m2();
+
+private:
+  void S15_m3();
+};
+
+//----------------------------------------------------------------------------//
+
+template <typename T>
+struct S97 {
+  void method();
+  T S97_v0;
+  // CHECK-MESSAGES-NONPRIVATE: :[[@LINE-1]]:5: warning: member variable 'S97_v0' has public visibility
+};
+
+template struct S97<char *>;
+
+template <>
+struct S97<double> {
+  void method();
+  double S97d_v0;
+  // CHECK-MESSAGES-NONPRIVATE: :[[@LINE-1]]:10: warning: member variable 'S97d_v0' has public visibility
+};
+
+//----------------------------------------------------------------------------//
+
+#define FIELD(x) int x;
+
+// Do diagnose fields originating from macros.
+struct S98 {
+  void method();
+  FIELD(S98_v0);
+  // CHECK-MESSAGES-NONPRIVATE: :[[@LINE-1]]:9: warning: member variable 'S98_v0' has public visibility
+};
+
+//----------------------------------------------------------------------------//
+
+// Don't look in descendant classes.
+class S99 {
+  void method();
+
+  struct S99_0 {
+    int S99_S0_v0;
+  };
+
+public:
+  struct S99_1 {
+    int S99_S0_v0;
+  };
+
+protected:
+  struct S99_2 {
+    int S99_S0_v0;
+  };
+
+private:
+  struct S99_3 {
+    int S99_S0_v0;
+  };
+};
+
+//----------------------------------------------------------------------------//
+
+// Only diagnose once, don't let the inheritance fool you.
+struct S100 {
+  int S100_v0;
+  // CHECK-MESSAGES-NONPRIVATE: :[[@LINE-1]]:7: warning: member variable 'S100_v0' has public visibility
+  void m0();
+};
+struct S101_default_inheritance : S100 {
+  int S101_v0;
+  // CHECK-MESSAGES-NONPRIVATE: :[[@LINE-1]]:7: warning: member variable 'S101_v0' has public visibility
+  void m1();
+};
+struct S102_public_inheritance : public S100 {
+  int S102_v0;
+  // CHECK-MESSAGES-NONPRIVATE: :[[@LINE-1]]:7: warning: member variable 'S102_v0' has public visibility
+  void m1();
+};
+struct S103_protected_inheritance : protected S100 {
+  int S103_v0;
+  // CHECK-MESSAGES-NONPRIVATE: :[[@LINE-1]]:7: warning: member variable 'S103_v0' has public visibility
+  void m1();
+};
+struct S104_private_inheritance : private S100 {
+  int S104_v0;
+  // CHECK-MESSAGES-NONPRIVATE: :[[@LINE-1]]:7: warning: member variable 'S104_v0' has public visibility
+  void m1();
+};