[clang-tidy] Add cppcoreguidelines-avoid-const-or-ref-data-members check

[carlosgalvezp]

Summary

Flags uses of const-qualified and reference data members in structs.
Implements rule C.12 of C++ Core Guidelines.

[carlosgalvezp]

Fix FIXME

[carlosgalvezp]

Move logic from check to registerMatchers

[dfrib]

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-const-or-ref-data-members.cpp

26

Differentiate between lvalue reference and rvalue reference members using these terms instead of "ref" and "refref". E.g.:

• RefMember -> LvalueRefMember
• RefRefMember -> RvalueRefMember

(apply to all cases below).

41

ConstAndRefMembers

101

Should we test for array types also? E.g.:

template<int N> using Array = int[N];

struct ConstArrayMember {
  const Array<1> c;
};

struct LvalueRefArrayMember {
  Array<2>& lvr;   
};

struct ConstRefArrayMember {
  Array<3> const& clvr;
};

struct LvalueRefArrayMember {
  Array<4>&& rvr;   
};

122

Consider expanding with the the non-const lvalue ref case, e.g.

TemplatedOk<int> t4{};
TemplateRef<int&> t5{t4.t};

[carlosgalvezp]

Fix review comments

[carlosgalvezp]

This comment was removed by carlosgalvezp.

[Eugene.Zelenko]

clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.h

30

Please add isLanguageVersionSupported.

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-avoid-const-or-ref-data-members.rst

11

Usually such links are placed at the end.

[carlosgalvezp]

Address review comments.
Rebase on latest main.

clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.h

30

Done. I generated this check via add_new_check.py - should we add this there as well to avoid similar issues in the future (separate commit)?

I see some checks implement this function and some others don't, so it's not totally clear to me when is needed.

[carlosgalvezp]

Remove copy-paste comment

[carlosgalvezp]

Kind ping to reviewers :)

[LegalizeAdulthood]

Tests and docs have moved to subdirectories by module name.

Please rebase onto HEAD and:

• move test/clang-tidy/checkers/cppcoreguidelines-avoid-const-or-ref-data-members.cpp to test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp
• move docs/clang-tidy/checks/cppcoreguidelines-avoid-const-or-ref-data-members.rst to docs/clang-tidy/checks/cppcoreguidelines/avoid-const-or-ref-data-members.rst
• make the target test-clang-extra to validate your tests still pass
• make the target docs-clang-tools-html to validate your documentation links are correct

[carlosgalvezp]

Rebase and fix directory structure for doc and test.

[carlosgalvezp]

Fix test name

[carlosgalvezp]

@LegalizeAdulthood I've addressed your comments, thanks for the clear instructions! One thing I didn't manage to do is build the target docs-clang-tools-html, it says it doesn't exist. I've enabled LLVM_BUILD_DOCS=ON in the CMake call - do you know if I need to enable something else?

PS: also the test-clang-extra doesn't exist, I typically run check-clang-tools-extra.

[LegalizeAdulthood]

clang-tools-extra/docs/ReleaseNotes.rst

126

Sort new checks by check name.

127

This link is wrong, please install Sphinx and build the target docs-clang-tools-html to validate links

[carlosgalvezp]

Address review comments.

[carlosgalvezp]

@LegalizeAdulthood Thanks for the review! I've now rebased and addressed your comments. I've also verify the docs with the command you suggested, I was missing -DLLVM_ENABLE_SPHINX in the cmake command.

[carlosgalvezp]

@LegalizeAdulthood I've addressed your comments, is there anything else that should be fixed before landing the patch?

[carlosgalvezp]

• Rebase onto latest main.
• Remove references to std::reference_wrapper as alternative suggestions, as per: https://github.com/isocpp/CppCoreGuidelines/issues/1919

[carlosgalvezp]

Ping to reviewers

[carlosgalvezp]

Rebase onto latest main.

[carlosgalvezp]

@njames93 Would you mind reviewing? Thanks!

[njames93]

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp

103–104 ↗

(On Diff #448323)

I feel like this could potentially confuse users. Maybe we should walk the alias to show where the type was declared to be const/reference.
Or, not a fan of this choice, don't diagnose these unless the user opts into this behaviour.

136–157 ↗

(On Diff #448323)

This whole block displeases me. Warning on the instantiation isn't a great idea and could confuse users.
Would again need some expansion notes to explain why the warning is being triggered, especially when if for 99% of the uses one of these structs has a T which isn't const or a reference.
Failing that just disabling the check in template instantiations would also fix the issue.

[carlosgalvezp]

Great feedback, thanks! I had some ideas on how to go around the issues, looking forward to your thoughts.

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-const-or-ref-data-members.cpp

103–104 ↗

(On Diff #448323)

Sounds reasonable! I see that Clang compiler does not print an "alias stack" in detail, for example here:

https://godbolt.org/z/8sqE4fM1v

For the sake of consistency and simplicity, would it make sense to print something similar here? Example:

"warning: member 'c' of type 'ConstType' (aka 'const int') is const qualified"

136–157 ↗

(On Diff #448323)

I agree with the sentiment 100%. Unfortunately it's currently the only way to test this kind of code in clang-tidy AFAIK, see for example other existing checks:

https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/test/clang-tidy/checkers/google/readability-casting.cpp#L332

I fully agree on needing expansion notes, just like the Clang compiler does. A very similar question about it was recently posted:

https://discourse.llvm.org/t/clang-tidy-diagnostics-for-template-code/62909

In that sense I believe it would make sense to try and focus the efforts into implementing a good centralized solution instead of having it duplicated on different checks.

Until then, I believe it should be fairly easy to clarify the actual type as proposed in the above comment for aliases.

What do you think?

[carlosgalvezp]

• Display type information in the diagnostic.
• Rebase onto latest main.

[carlosgalvezp]

@njames93 I've updated the patch with extra type information, let me know if you think it's good enough!

[carlosgalvezp]

@njames93 Friendly ping.

[carlosgalvezp]

Rebase

[bigdavedev]

LGTM. All comments have been addressed. Any improvements to the diagnostics can be made down the line depending on how users feel about them.

clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidConstOrRefDataMembersCheck.h

+//===--- AvoidConstOrRefDataMembersCheck.h - clang-tidy ---------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_AVOIDCONSTORREFDATAMEMBERSCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_AVOIDCONSTORREFDATAMEMBERSCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace cppcoreguidelines {
+
+/// FIXME: Write a short description.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-avoid-const-or-ref-data-members.html
+class AvoidConstOrRefDataMembersCheck : public ClangTidyCheck {
+public:
+  AvoidConstOrRefDataMembersCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace cppcoreguidelines
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_AVOIDCONSTORREFDATAMEMBERSCHECK_H

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

+//===--- AvoidConstOrRefDataMembersCheck.cpp - clang-tidy -----------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "AvoidConstOrRefDataMembersCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace cppcoreguidelines {
+
+void AvoidConstOrRefDataMembersCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(fieldDecl().bind("x"), this);
+}
+
+void AvoidConstOrRefDataMembersCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  const auto *MatchedDecl = Result.Nodes.getNodeAs<FieldDecl>("x");
+  if (MatchedDecl->getType().getTypePtr()->isReferenceType())
+    diag(MatchedDecl->getLocation(), "member %0 is a reference") << MatchedDecl;
+  if (MatchedDecl->getType().isConstQualified())
+    diag(MatchedDecl->getLocation(), "member %0 is const qualified")
+        << MatchedDecl;
+}
+
+} // namespace cppcoreguidelines
+} // namespace tidy
+} // namespace clang

clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   FrontendOpenMP
   Support
   )
 
 add_clang_library(clangTidyCppCoreGuidelinesModule
+  AvoidConstOrRefDataMembersCheck.cpp
   AvoidGotoCheck.cpp
   AvoidNonConstGlobalVariablesCheck.cpp
   CppCoreGuidelinesTidyModule.cpp
   InitVariablesCheck.cpp
   InterfacesGlobalInitCheck.cpp
   MacroUsageCheck.cpp
   NarrowingConversionsCheck.cpp
   NoMallocCheck.cpp

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

 //===-- CppCoreGuidelinesTidyModule.cpp - clang-tidy ----------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "../misc/NonPrivateMemberVariablesInClassesCheck.h"
 #include "../misc/UnconventionalAssignOperatorCheck.h"
 #include "../modernize/AvoidCArraysCheck.h"
 #include "../modernize/UseOverrideCheck.h"
 #include "../readability/MagicNumbersCheck.h"
+#include "AvoidConstOrRefDataMembersCheck.h"
 #include "AvoidGotoCheck.h"
 #include "AvoidNonConstGlobalVariablesCheck.h"
 #include "InitVariablesCheck.h"
 #include "InterfacesGlobalInitCheck.h"
 #include "MacroUsageCheck.h"
 #include "NarrowingConversionsCheck.h"
 #include "NoMallocCheck.h"
 #include "OwningMemoryCheck.h"

 namespace cppcoreguidelines {
 
 /// A module containing checks of the C++ Core Guidelines
 class CppCoreGuidelinesModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<modernize::AvoidCArraysCheck>(
         "cppcoreguidelines-avoid-c-arrays");
+    CheckFactories.registerCheck<AvoidConstOrRefDataMembersCheck>(
+        "cppcoreguidelines-avoid-const-or-ref-data-members");
     CheckFactories.registerCheck<AvoidGotoCheck>(
         "cppcoreguidelines-avoid-goto");
     CheckFactories.registerCheck<readability::MagicNumbersCheck>(
         "cppcoreguidelines-avoid-magic-numbers");
     CheckFactories.registerCheck<AvoidNonConstGlobalVariablesCheck>(
         "cppcoreguidelines-avoid-non-const-global-variables");
     CheckFactories.registerCheck<modernize::UseOverrideCheck>(
         "cppcoreguidelines-explicit-virtual-functions");

clang-tools-extra/docs/ReleaseNotes.rst

- New :doc:`bugprone-shared-ptr-array-mismatch <clang-tidy/checks/bugprone-shared-ptr-array-mismatch>` check.
   Finds initializations of C++ shared pointers to non-array type that are initialized with an array.
 
 - New :doc:`bugprone-unchecked-optional-access
   <clang-tidy/checks/bugprone-unchecked-optional-access>` check.
 
    Warns when the code is unwrapping a `std::optional<T>`, `absl::optional<T>`,
    or `base::Optional<T>` object without assuring that it contains a value.
 
+- New :doc:`cppcoreguidelines-avoid-const-or-ref-data-members
+  <clang-tidy/checks/cppcoreguidelines-avoid-const-or-ref-data-members>` check.
+
+  Warns when a struct or class uses const or reference (lvalue or rvalue) data members.
+
 - New :doc:`modernize-macro-to-enum
   <clang-tidy/checks/modernize-macro-to-enum>` check.
 
   Replaces groups of adjacent macros with an unscoped anonymous enum.
 
 - New :doc:`portability-std-allocator-const <clang-tidy/checks/portability-std-allocator-const>` check.
 
   Report use of ``std::vector<const T>`` (and similar containers of const

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-avoid-const-or-ref-data-members.rst

+.. title:: clang-tidy - cppcoreguidelines-avoid-const-or-ref-data-members
+
+cppcoreguidelines-avoid-const-or-ref-data-members
+=================================================
+
+This check warns when structs or classes have const-qualified or reference
+(lvalue or rvalue) data members. Having such members is rarely useful, and
+makes the class only copy-constructible but not copy-assignable.
+
+The check implements
+`C.12 of C++ Core Guidelines <https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#c12-dont-make-data-members-const-or-references>`_.
+
+Further reading:
+`Data members: Never const <https://quuxplusone.github.io/blog/2022/01/23/dont-const-all-the-things/#data-members-never-const>`_.
+
+Examples:
+
+.. code-block:: c++
+
+  // Bad, const-qualified member
+  struct Const {
+    const int x;
+  }
+
+  // Good:
+  class Foo {
+   public:
+    int get() const { return x; }
+   private:
+    int x;
+  };
+
+  // Bad, lvalue reference member
+  struct Ref {
+    int& x;
+  };
+
+  // Good:
+  struct Foo {
+    int* x;                         // Alternative 1
+    gsl::not_null<int> x;           // Alternative 2
+    std::reference_wrapper<int> x;  // Alternative 3
+  };
+
+  // Bad, rvalue reference member
+  struct RefRef {
+    int&& x;
+  };

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

.. csv-table::
    `clang-analyzer-osx.cocoa.MissingSuperCall <clang-analyzer-osx.cocoa.MissingSuperCall.html>`_,
    `clang-analyzer-osx.cocoa.NonNilReturnValue <clang-analyzer-osx.cocoa.NonNilReturnValue.html>`_,
    `clang-analyzer-osx.cocoa.RunLoopAutoreleaseLeak <clang-analyzer-osx.cocoa.RunLoopAutoreleaseLeak.html>`_,
    `clang-analyzer-valist.CopyToSelf <clang-analyzer-valist.CopyToSelf.html>`_,
    `clang-analyzer-valist.Uninitialized <clang-analyzer-valist.Uninitialized.html>`_,
    `clang-analyzer-valist.Unterminated <clang-analyzer-valist.Unterminated.html>`_,
    `concurrency-mt-unsafe <concurrency-mt-unsafe.html>`_,
    `concurrency-thread-canceltype-asynchronous <concurrency-thread-canceltype-asynchronous.html>`_,
+   `cppcoreguidelines-avoid-const-or-ref-data-members <cppcoreguidelines-avoid-const-or-ref-data-members.html>`_,
    `cppcoreguidelines-avoid-goto <cppcoreguidelines-avoid-goto.html>`_,
    `cppcoreguidelines-avoid-non-const-global-variables <cppcoreguidelines-avoid-non-const-global-variables.html>`_,
    `cppcoreguidelines-init-variables <cppcoreguidelines-init-variables.html>`_, "Yes"
    `cppcoreguidelines-interfaces-global-init <cppcoreguidelines-interfaces-global-init.html>`_,
    `cppcoreguidelines-macro-usage <cppcoreguidelines-macro-usage.html>`_,
    `cppcoreguidelines-narrowing-conversions <cppcoreguidelines-narrowing-conversions.html>`_,
    `cppcoreguidelines-no-malloc <cppcoreguidelines-no-malloc.html>`_,
    `cppcoreguidelines-owning-memory <cppcoreguidelines-owning-memory.html>`_,

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-const-or-ref-data-members.cpp

+// RUN: %check_clang_tidy %s cppcoreguidelines-avoid-const-or-ref-data-members %t
+namespace std {
+template <typename T>
+struct reference_wrapper {};
+} // namespace std
+
+namespace gsl {
+template <typename T>
+struct not_null {};
+} // namespace gsl
+
+struct Ok {
+  int i;
+  int *p;
+  const int *pc;
+  std::reference_wrapper<int> r;
+  gsl::not_null<int> n;
+};
+
+struct ConstMember {
+  const int c;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: member 'c' is const qualified [cppcoreguidelines-avoid-const-or-ref-data-members]
+};
+
+struct RefMember {
+  int &r;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'r' is a reference
+};
+
+struct RefRefMember {
+  int &&rr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: member 'rr' is a reference
+};
+
+struct ConstRefMember {
+  const int &r;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: member 'r' is a reference
+};
+
+struct ConstAndRefMember {
+  int const c;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: member 'c' is const qualified
+  int &r;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'r' is a reference
+  int &&rr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: member 'rr' is a reference
+};
+
+struct Foo {};
+
+struct Ok2 {
+  Foo i;
+  Foo *p;
+  const Foo *pc;
+  std::reference_wrapper<Foo> r;
+  gsl::not_null<Foo> n;
+};
+
+struct ConstMember2 {
+  const Foo c;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: member 'c' is const qualified [cppcoreguidelines-avoid-const-or-ref-data-members]
+};
+
+struct RefMember2 {
+  Foo &r;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'r' is a reference
+};
+
+struct RefRefMember2 {
+  Foo &&rr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: member 'rr' is a reference
+};
+
+struct ConstRefMember2 {
+  const Foo &r;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: member 'r' is a reference
+};
+
+struct ConstAndRefMember2 {
+  const Foo c;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: member 'c' is const qualified
+  Foo &r;
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: member 'r' is a reference
+  Foo &&rr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: member 'rr' is a reference
+};
+
+using ConstType = const int;
+using RefType = int &;
+using RefRefType = int &&;
+
+struct WithAlias {
+  ConstType c;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: member 'c' is const qualified
+  RefType r;
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: member 'r' is a reference
+  RefRefType rr;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: member 'rr' is a reference
+};
+
+template <typename T>
+struct TemplatedConst {
+  T t;
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: member 't' is const qualified
+};
+
+template <typename T>
+struct TemplatedRef {
+  T t;
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: member 't' is a reference
+};
+
+template <typename T>
+struct TemplatedOk {
+  T t;
+};
+
+TemplatedConst<const int> t1{123};
+TemplatedRef<const int &> t2{123};
+TemplatedRef<int &&> t3{123};
+TemplatedOk<int> t4{};