[clang-tidy] [PR41119] readability-identifier-naming incorrectly fixes lambda capture

[MyDeveloperDay]

Summary

While testing clang-tidy for D59251: [Documentation] Proposal for plan to change variable names I found that a lambda capture could get incorrectly get transformed by readability-identifier-naming when run with -fix

The following code:

bool foo() {
  bool Columns=false;
  auto ptr=[&] {
          return Columns;
        }();
  return true;
}

would get transformed to (replace [&] with [columns]

bool foo() {
  bool columns=false;
  auto ptr=[columns] {
          return columns;
        }();
  return true;
}

This is caused by the presence of a declrefexpr in the LambdaExpr, seeming having the location of the [&] (line 3 column 13), but also having the Name "Columns"

-DeclRefExpr <col:13> 'bool' lvalue Var 0x55f0145f1c80 'Columns' 'bool'

LambdaExpr <line:3:12, line:5:9> '(lambda at line:3:12)'
                  |-CXXRecordDecl <line:3:12> col:12 implicit class definition
                  | |-DefinitionData lambda pass_in_registers trivially_copyable can_const_default_init
                  | | |-DefaultConstructor
                  | | |-CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param
                  | | |-MoveConstructor exists simple trivial needs_implicit
                  | | |-CopyAssignment trivial has_const_param needs_implicit implicit_has_const_param
                  | | |-MoveAssignment
                  | | `-Destructor simple irrelevant trivial
                  | |-FieldDecl <line:4:18> col:18 implicit 'bool &'
                  | |-CXXMethodDecl <line:3:14, line:5:9> line:3:12 used operator() 'auto () const -> bool' inline
                  | | `-CompoundStmt <col:16, line:5:9>
                  | |   `-ReturnStmt <line:4:11, col:18>
                  | |     `-ImplicitCastExpr <col:18> 'bool' <LValueToRValue>
                  | |       `-DeclRefExpr <col:18> 'bool' lvalue Var 0x55f0145f1c80 'Columns' 'bool'
                  | `-CXXDestructorDecl <line:3:12> col:12 implicit referenced ~ 'void () noexcept' inline default trivial
                  |-DeclRefExpr <col:13> 'bool' lvalue Var 0x55f0145f1c80 'Columns' 'bool'
                  `-CompoundStmt <col:16, line:5:9>
                    `-ReturnStmt <line:4:11, col:18>
                      `-ImplicitCastExpr <col:18> 'bool' <LValueToRValue>
                        `-DeclRefExpr <col:18> 'bool' lvalue Var 0x55f0145f1c80 'Columns' 'bool'

The following code tries to detect the DeclRefExpr in the LambdaExpr, and not add this to the usage map

This issue is logged as https://bugs.llvm.org/show_bug.cgi?id=41119

I have retested this against lib/Clang/Format and this issue is resolved.

// Don't use this Format, if the difference between the longest and shortest
// element in a column exceeds a threshold to avoid excessive spaces.
if ([&] {
      for (unsigned i = 0; i < columns - 1; ++i)
        if (format.ColumnSizes[i] - minSizeInColumn[i] > 10)
          return true;
      return false;
    }())
  continue;

[JonasToth]

What happens on [=]() ..., direct capture [&Columns]()... and [Columns]()...?

[MyDeveloperDay]

In D59540#1434837, @JonasToth wrote:

What happens on [=]() ..., direct capture [&Columns]()... and [Columns]()...?

Thanks for the review... in answer to your question.. not great!

[MyDeveloperDay]

Minor modification to improve the [=] case

[&Columns] and [Columns] are not yet fixed and will not be correctly renamed to [&columns] and [columns]

But at least they are left unaltered

I think the real problem here is how identifier works, it uses source ranges, so its not like its comparing "Columns" with "columns" it just seeing [&] occupying the same taken up with the DeclRef (which is the [Columns] case is correct)

As a potential solution I wonder if I have to look at the text of the SourceRange and not replace '&' or '=' and in the case of &Column increment the begin location.

[alexfh]

Thanks for fixing this! Could you expand the test a bit? See the inline comment.

test/clang-tidy/readability-identifier-naming.cpp

506

If the formatting is not critical for the logic of the test, please clang-format the new test code.

509

Please add more tests with

1. by value automatic captures
2. manual captures by value
3. manual captures by reference
4. nested lambdas capturing the same variable

A bit more nested code inside the lambda would also be interesting (where the use of the variable would be wrapped in a couple of compound statements).

[Eugene.Zelenko]

clang-tidy/readability/IdentifierNamingCheck.cpp

790–805

Type is not obvious, so auto should not be used.

797

Type is not obvious, so auto should not be used.

[MyDeveloperDay]

Address review comments
This may not be a more satisfactory solution but it at least now covers the other cases raised by @JonasToth
Add more tests around this area.

[MyDeveloperDay]

test/clang-tidy/readability-identifier-naming.cpp

509

Think i've covered the cases you wanted here, if you can think of another drop the code into a comment and I'll add it.

[MyDeveloperDay]

friendly ping

[alexfh]

Thanks, looks better now, but still a few comments, mostly nits.

clang-tidy/readability/IdentifierNamingCheck.cpp

701

Please use early return:

if (Parents.empty())
  return false;

702

Same here:

if (!ST)
  return false;

710

And here just return ST && isa<LambdaExpr>(ST);

790–805

Let's drop this variable. The code is going to be simpler without it:

if (isLambda(DeclRef, Result.Context)) {
  StringRef CaptureText = ...;
  if (CaptureText != "=" && CaptureText != "&") {
    addUsage(...);
    return;
  }
}

792

Lexer::getSourceText returns a StringRef, there's no need to copy its contents.

792

nit: CaptureText

795

I find it easier to understand, if negation is propagated through the parentheses:

if (captureText != "=" && captureText != "&")

test/clang-tidy/readability-identifier-naming.cpp

506

nit: The formatting is still off here, e.g. missing spaces around the equals sign.

508

Let's make the CHECK-FIXES lines unique to reduce the chance of mixing them up and to make identifying broken test cases easier. One way to do this is to make the variable names distinct, another way is to add unique trailing comments both in the code and in the CHECK-FIXES patterns.

[alexfh]

In D59540#1456011, @MyDeveloperDay wrote:

friendly ping

Sorry for the delay. Feel free to ping earlier. And more frequently :)

clang-tidy/readability/IdentifierNamingCheck.cpp

return addUsage(Failures,
                                                        Decl->getNameAsString()),
                   Range, SourceMgr);
 }
 
 void IdentifierNamingCheck::check(const MatchFinder::MatchResult &Result) {
   if (const auto *Decl =
           Result.Nodes.getNodeAs<CXXConstructorDecl>("classRef")) {
     if (Decl->isImplicit())
       return;
 
     addUsage(NamingCheckFailures, Decl->getParent(),
              Decl->getNameInfo().getSourceRange());
 
     for (const auto *Init : Decl->inits()) {
       if (!Init->isWritten() || Init->isInClassMemberInitializer())
         continue;
       if (const auto *FD = Init->getAnyMember())
         addUsage(NamingCheckFailures, FD,
                  SourceRange(Init->getMemberLocation()));
       // Note: delegating constructors and base class initializers are handled
       // via the "typeLoc" matcher.
     }
     return;
   }
 
   if (const auto *Decl =
if (const auto *Decl = Result.Nodes.getNodeAs<UsingDecl>("using")) {
     for (const auto &Shadow : Decl->shadows()) {
       addUsage(NamingCheckFailures, Shadow->getTargetDecl(),
                Decl->getNameInfo().getSourceRange());
     }
     return;
   }
 
   if (const auto *DeclRef = Result.Nodes.getNodeAs<DeclRefExpr>("declRef")) {
+    const auto &Parents = Result.Context->getParents(*DeclRef);
+    bool LambdaDeclRef = false;
+
+    if (!Parents.empty()) {
+      const Stmt *ST = Parents[0].get<Stmt>();
+
+      if (ST && isa<LambdaExpr>(ST))
+        LambdaDeclRef = true;
+    }
+
+    if (!LambdaDeclRef) {
       SourceRange Range = DeclRef->getNameInfo().getSourceRange();
       addUsage(NamingCheckFailures, DeclRef->getDecl(), Range,
                Result.SourceManager);
       return;
     }
+  }
 
   if (const auto *Decl = Result.Nodes.getNodeAs<NamedDecl>("decl")) {
     if (!Decl->getIdentifier() || Decl->getName().empty() || Decl->isImplicit())
       return;
 
     // Fix type aliases in value declarations
     if (const auto *Value = Result.Nodes.getNodeAs<ValueDecl>("decl")) {
       if (const auto *Typedef =

test/clang-tidy/readability-identifier-naming.cpp

 // CHECK-MESSAGES: :[[@LINE-1]]:59: warning: invalid case style for constant pointer parameter 'constant_ptr'
 // CHECK-FIXES: {{^}}void MyPoiterFunction(int * p_normal_pointer, int * const cp_ConstantPtr){{{$}}
     int * l_PointerA;
     int * const pointer_b = nullptr;
 // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: invalid case style for local constant pointer 'pointer_b'
 // CHECK-FIXES: {{^}}    int * const lc_PointerB = nullptr;{{$}}
 }
 
+
+bool Foo() {
+  bool Columns=false;
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: invalid case style for local variable 'Columns'
+// CHECK-FIXES: {{^}}  bool columns=false;
+  auto ptr=[&]{return Columns;}();
+// CHECK-FIXES: {{^}}  auto ptr=[&]{return columns;}();
+  return Columns;
+// CHECK-FIXES: {{^}}  return columns;
+}