[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

810–822

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

817

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);

810

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;
  }
}

812

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

812

nit: CaptureText

815

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

static StyleKind findStyleKind(
     const NamedDecl *D,
     const std::vector<llvm::Optional<IdentifierNamingCheck::NamingStyle>>
         &NamingStyles) {
   assert(D && D->getIdentifier() && !D->getName().empty() && !D->isImplicit() &&
          "Decl must be an explicit identifier with a name.");
 
   if (isa<ObjCIvarDecl>(D) && NamingStyles[SK_ObjcIvar])
     return SK_ObjcIvar;
-  
   if (isa<TypedefDecl>(D) && NamingStyles[SK_Typedef])
     return SK_Typedef;
 
   if (isa<TypeAliasDecl>(D) && NamingStyles[SK_TypeAlias])
     return SK_TypeAlias;
 
   if (const auto *Decl = dyn_cast<NamespaceDecl>(D)) {
     if (Decl->isAnonymousNamespace())
if (!Type.isNull() && Type.isConstQualified()) {
       if (NamingStyles[SK_Constant])
         return SK_Constant;
     }
 
     if (Decl->isParameterPack() && NamingStyles[SK_ParameterPack])
       return SK_ParameterPack;
 
     if (!Type.isNull() && Type.getTypePtr()->isAnyPointerType() && NamingStyles[SK_PointerParameter])
-        return SK_PointerParameter;
 
     if (NamingStyles[SK_Parameter])
       return SK_Parameter;
 
     return SK_Invalid;
   }
 
   if (const auto *Decl = dyn_cast<VarDecl>(D)) {
if (const auto *Decl = dyn_cast<VarDecl>(D)) {
     if (Decl->isFileVarDecl() && Type.getTypePtr()->isAnyPointerType() && NamingStyles[SK_GlobalPointer])
       return SK_GlobalPointer;
 
     if (Decl->isFileVarDecl() && NamingStyles[SK_GlobalVariable])
       return SK_GlobalVariable;
 
     if (Decl->isStaticLocal() && NamingStyles[SK_StaticVariable])
       return SK_StaticVariable;
- 
     if (Decl->isLocalVarDecl() && Type.getTypePtr()->isAnyPointerType() && NamingStyles[SK_LocalPointer])
       return SK_LocalPointer;
 
     if (Decl->isLocalVarDecl() && NamingStyles[SK_LocalVariable])
       return SK_LocalVariable;
 
     if (Decl->isFunctionOrMethodVarDecl() && NamingStyles[SK_LocalVariable])
       return SK_LocalVariable;
static void addUsage(IdentifierNamingCheck::NamingCheckFailureMap &Failures,
                      const NamedDecl *Decl, SourceRange Range,
                      SourceManager *SourceMgr = nullptr) {
   return addUsage(Failures,
                   IdentifierNamingCheck::NamingCheckId(Decl->getLocation(),
                                                        Decl->getNameAsString()),
                   Range, SourceMgr);
 }
 
+/// Determine if the DeclRef is direct descentent of a LambdaExpr
+/// ignoring ImplicitCasts.
+static bool isLambda(const DeclRefExpr *DeclRef, ASTContext *Context) {
+  const ASTContext::DynTypedNodeList &Parents = Context->getParents(*DeclRef);
+  if (!Parents.empty()) {
+    const Stmt *ST = Parents[0].get<Stmt>();
+    // Ignore ImplicitCastExpr if we find one.
+    if (ST && isa<ImplicitCastExpr>(ST)) {
+      const ASTContext::DynTypedNodeList &CastParents =
+          Context->getParents(*ST);
+      if (!CastParents.empty())
+        ST = CastParents[0].get<Stmt>();
+    }
+    if (ST && isa<LambdaExpr>(ST))
+      return true;
+  }
+  return false;
+}
+
 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 &Shadow : Decl->shadows()) {
       addUsage(NamingCheckFailures, Shadow->getTargetDecl(),
                Decl->getNameInfo().getSourceRange());
     }
     return;
   }
 
   if (const auto *DeclRef = Result.Nodes.getNodeAs<DeclRefExpr>("declRef")) {
     SourceRange Range = DeclRef->getNameInfo().getSourceRange();
+    bool LambdaDeclRef = isLambda(DeclRef, Result.Context);
+    if (LambdaDeclRef) {
+      std::string captureText =
+          Lexer::getSourceText(CharSourceRange::getTokenRange(Range),
+                               *Result.SourceManager, getLangOpts());
+      if (!(captureText == "=" || captureText == "&"))
+        LambdaDeclRef = false;
+    }
+    if (!LambdaDeclRef) {
       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 LambdaCaptureTest() {
+  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: {{^}}    return columns;
+  }();
+
+  return Columns;
+// CHECK-FIXES: {{^}}  return columns;
+}
+
+bool LambdaCaptureTest1() {
+  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: {{^}}    return columns;
+  }();
+
+  return Columns;
+// CHECK-FIXES: {{^}}  return columns;
+}
+
+bool LambdaCaptureTest2() {
+  bool Columns = false;
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: invalid case style for local variable 'Columns'
+// CHECK-FIXES: {{^}}  bool columns = false;
+  auto ptr = [&Columns]{
+// CHECK-FIXES: {{^}}  auto ptr = [&columns]{
+    return Columns;
+// CHECK-FIXES: {{^}}    return columns;
+  }();
+  return Columns;
+// CHECK-FIXES: {{^}}  return columns;
+}
+
+bool LambdaCaptureTest3() {
+  bool Columns = false;
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: invalid case style for local variable 'Columns'
+// CHECK-FIXES: {{^}}  bool columns = false;
+  auto ptr = [Columns]{
+// CHECK-FIXES: {{^}}  auto ptr = [columns]{
+    return Columns;
+// CHECK-FIXES: {{^}}    return columns;
+  }();
+  return Columns;
+// CHECK-FIXES: {{^}}  return columns;
+}
+
+bool LambdaCaptureTest4() {
+  bool Columns = false;
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: invalid case style for local variable 'Columns'
+// CHECK-FIXES: {{^}}  bool columns = false;
+  auto ptr = [&]{
+    if (true)
+      return Columns;
+// CHECK-FIXES: {{^}}      return columns;
+  }();
+  return Columns;
+// CHECK-FIXES: {{^}}  return columns;
+}
+
+bool LambdaCaptureTest5() {
+  bool Columns = false;
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: invalid case style for local variable 'Columns'
+// CHECK-FIXES: {{^}}  bool columns = false;
+  auto ptr = [](bool Columns) {
+// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: invalid case style for parameter 'Columns'
+// CHECK-FIXES: {{^}}  auto ptr = [](bool a_columns) {
+        return Columns;
+// CHECK-FIXES: {{^}}        return a_columns;
+  };
+  return Columns;
+// CHECK-FIXES: {{^}}  return columns;
+}
+
+bool LambdaCaptureTest6() {
+  bool Columns = false;
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: invalid case style for local variable 'Columns'
+// CHECK-FIXES: {{^}}  bool columns = false;
+  auto ptr = [Columns]() ->bool {
+// CHECK-FIXES: {{^}}  auto ptr = [columns]() ->bool {
+      [=]() -> bool {
+      return Columns;
+// CHECK-FIXES: {{^}}      return columns;
+    };
+    return Columns;
+// CHECK-FIXES: {{^}}    return columns;
+  };
+  return Columns;
+// CHECK-FIXES: {{^}}  return columns;
+}
+
+bool LambdaCaptureTest7() {
+  int  a;
+  bool Columns = false;
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: invalid case style for local variable 'Columns'
+// CHECK-FIXES: {{^}}  bool columns = false;
+  auto ptr = [&Columns, a]{
+// CHECK-FIXES: {{^}}  auto ptr = [&columns, a]{
+    return Columns;
+// CHECK-FIXES: {{^}}    return columns;
+  }();
+  return Columns;
+// CHECK-FIXES: {{^}}  return columns;
+}
+
+bool LambdaCaptureTest8() {
+  int a;
+  bool Columns = false;
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: invalid case style for local variable 'Columns'
+// CHECK-FIXES: {{^}}  bool columns = false;
+  auto ptr = [a, Columns]{
+// CHECK-FIXES: {{^}}  auto ptr = [a, columns]{
+    return Columns;
+// CHECK-FIXES: {{^}}    return columns;
+  }();
+  return Columns;
+// CHECK-FIXES: {{^}}  return columns;
+}
+
+bool LambdaCaptureTest9() {
+  int Rows;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: invalid case style for local variable 'Rows'
+// CHECK-FIXES: {{^}}  int rows;
+  bool Columns = false;
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: invalid case style for local variable 'Columns'
+// CHECK-FIXES: {{^}}  bool columns = false;
+  auto ptr = [Rows, Columns]{
+// CHECK-FIXES: {{^}}  auto ptr = [rows, columns]{
+    return Columns;
+// CHECK-FIXES: {{^}}    return columns;
+  }();
+  return Columns;
+// CHECK-FIXES: {{^}}  return columns;
+}
+
+bool LambdaCaptureTest10() {
+  int Rows;
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: invalid case style for local variable 'Rows'
+// CHECK-FIXES: {{^}}  int rows;
+  bool Columns = false;
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: invalid case style for local variable 'Columns'
+// CHECK-FIXES: {{^}}  bool columns = false;
+  auto ptr = [&Rows, &Columns]{
+// CHECK-FIXES: {{^}}  auto ptr = [&rows, &columns]{
+    return Columns;
+// CHECK-FIXES: {{^}}    return columns;
+  }();
+  return Columns;
+// CHECK-FIXES: {{^}}  return columns;
+}