[clang-tidy] Do not trigger cppcoreguidelines-avoid-const-or-ref-data-members on lambda captures

[carlosgalvezp]

Summary

Lambdas are implemented as regular classes internally,
and the captured variables end up as members there.
Do not diagnose those - the check should cover only
regular classes and structs.

[Eugene.Zelenko]

Please mention changes in Release Notes.

[carlosgalvezp]

In D131780#3719881, @Eugene.Zelenko wrote:

Please mention changes in Release Notes.

Thanks for the quick review, @Eugene.Zelenko !

This check was newly added a couple of days ago (as per the "New checks" section in the Release Notes). I thought the "Changes to exiting checks" section was only meant for checks already existing prior to the new release, to avoid adding excessive clutter to the documentation. New checks typically take a few iterations to polish when testing on larger projects.

What do you think?

[carlosgalvezp]

Test also capture by reference.

[Eugene.Zelenko]

In D131780#3720896, @carlosgalvezp wrote:

In D131780#3719881, @Eugene.Zelenko wrote:

Please mention changes in Release Notes.

Thanks for the quick review, @Eugene.Zelenko !

This check was newly added a couple of days ago (as per the "New checks" section in the Release Notes). I thought the "Changes to exiting checks" section was only meant for checks already existing prior to the new release, to avoid adding excessive clutter to the documentation. New checks typically take a few iterations to polish when testing on larger projects.

What do you think?

Sorry, I didn't know history. Sure, it doesn't make sense for checks added in main.

[njames93]

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

191

Can you add some cases with implicit capture (using [=] and [&])

[carlosgalvezp]

Added unit test for implicit capture (ref and value)

[njames93]

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

191

I should have been more clear, you need to actually use the variables inside the lambda to implicitly capture them.

[carlosgalvezp]

Update implicit lambda captures to use the captured variables
inside the lambdas.

[carlosgalvezp]

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

191

Thanks, didn't know that!

It's actually quite interesting, implicit lambda captures never trigger an error:
https://godbolt.org/z/cErf4jv8E

But it's probably good to keep the test anyway in case the lambda implementation changes.

[carlosgalvezp]

Fix typo

[njames93]

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

191

What's more interesting about that is the error message that was emitted.

<source>:6:16: warning: member '' of type 'const int' is const qualified [cppcoreguidelines-avoid-const-or-ref-data-members]
    auto y3 = [x]{};

However this fix should accidentally fix the issue of unnamed members.

[carlosgalvezp]

Thanks for the review!

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

 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace cppcoreguidelines {
+namespace {
+
+AST_MATCHER(FieldDecl, isMemberOfLambda) {
+  return Node.getParent()->isLambda();
+}
+
+} // namespace
 
 void AvoidConstOrRefDataMembersCheck::registerMatchers(MatchFinder *Finder) {
-  Finder->addMatcher(
-      fieldDecl(hasType(hasCanonicalType(referenceType()))).bind("ref"), this);
-  Finder->addMatcher(
-      fieldDecl(hasType(qualType(isConstQualified()))).bind("const"), this);
+  Finder->addMatcher(fieldDecl(unless(isMemberOfLambda()),
+                               hasType(hasCanonicalType(referenceType())))
+                         .bind("ref"),
+                     this);
+  Finder->addMatcher(fieldDecl(unless(isMemberOfLambda()),
+                               hasType(qualType(isConstQualified())))
+                         .bind("const"),
+                     this);
 }
 
 void AvoidConstOrRefDataMembersCheck::check(
     const MatchFinder::MatchResult &Result) {
   if (const auto *MatchedDecl = Result.Nodes.getNodeAs<FieldDecl>("ref"))
     diag(MatchedDecl->getLocation(), "member %0 of type %1 is a reference")
         << MatchedDecl << MatchedDecl->getType();
   if (const auto *MatchedDecl = Result.Nodes.getNodeAs<FieldDecl>("const"))
     diag(MatchedDecl->getLocation(), "member %0 of type %1 is const qualified")
         << MatchedDecl << MatchedDecl->getType();
 }
 
 } // namespace cppcoreguidelines
 } // namespace tidy
 } // namespace clang

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

struct TemplatedRef {
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: member 't' of type 'int &' is a reference
 };
 
 TemplatedOk<int> t1{};
 TemplatedConst<const int> t2{123};
 TemplatedConstRef<const int &> t3{123};
 TemplatedRefRef<int &&> t4{123};
 TemplatedRef<int &> t5{t1.t};
+
+// Lambdas capturing const or ref members should not trigger warnings
+void lambdas()
+{
+  int x1{123};
+  const int x2{123};
+  const int& x3{123};
+  int&& x4{123};
+  int& x5{x1};
+
+  auto v1 = [x1]{};
+  auto v2 = [x2]{};
+  auto v3 = [x3]{};
+  auto v4 = [x4]{};
+  auto v5 = [x5]{};
+
+  auto r1 = [&x1]{};
+  auto r2 = [&x2]{};
+  auto r3 = [&x3]{};
+  auto r4 = [&x4]{};
+  auto r5 = [&x5]{};
+
+  auto iv = [=]{
+    auto c1 = x1;
+    auto c2 = x2;
+    auto c3 = x3;
+    auto c4 = x4;
+    auto c5 = x5;
+  };
+
+  auto ir = [&]{
+    auto c1 = x1;
+    auto c2 = x2;
+    auto c3 = x3;
+    auto c4 = x4;
+    auto c5 = x5;
+  };
+}