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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay created this revision.
MyDeveloperDay added reviewers: JonasToth, aaron.ballman, alexfh, michaelplatings.
MyDeveloperDay added a project: clang-tools-extra.
Herald added subscribers: jdoerfert, xazax.hun.

While testing clang-tidy for D59251: [Documentation] Proposal for plan to change variable names <https://reviews.llvm.org/D59251> 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;




https://reviews.llvm.org/D59540

Files:
  clang-tidy/readability/IdentifierNamingCheck.cpp
  test/clang-tidy/readability-identifier-naming.cpp


Index: test/clang-tidy/readability-identifier-naming.cpp
===================================================================
--- test/clang-tidy/readability-identifier-naming.cpp
+++ test/clang-tidy/readability-identifier-naming.cpp
@@ -501,3 +501,13 @@
 // 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;
+}
Index: clang-tidy/readability/IdentifierNamingCheck.cpp
===================================================================
--- clang-tidy/readability/IdentifierNamingCheck.cpp
+++ clang-tidy/readability/IdentifierNamingCheck.cpp
@@ -787,10 +787,22 @@
   }
 
   if (const auto *DeclRef = Result.Nodes.getNodeAs<DeclRefExpr>("declRef")) {
-    SourceRange Range = DeclRef->getNameInfo().getSourceRange();
-    addUsage(NamingCheckFailures, DeclRef->getDecl(), Range,
-             Result.SourceManager);
-    return;
+    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")) {


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D59540.191269.patch
Type: text/x-patch
Size: 1727 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190319/2f50fa31/attachment.bin>