Page MenuHomePhabricator

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

Authored by MyDeveloperDay on Mar 19 2019, 3:58 AM.

Details

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;

Diff Detail

Event Timeline

MyDeveloperDay created this revision.Mar 19 2019, 3:58 AM

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

MyDeveloperDay planned changes to this revision.Mar 19 2019, 7:50 AM

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

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

MyDeveloperDay updated this revision to Diff 191311.EditedMar 19 2019, 8:32 AM

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.

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

MyDeveloperDay planned changes to this revision.Mar 19 2019, 8:38 AM
Eugene.Zelenko added inline comments.
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.

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 marked 5 inline comments as done.Mar 19 2019, 11:57 AM
MyDeveloperDay added inline comments.
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 marked an inline comment as done.Apr 5 2019, 2:37 AM

friendly ping

alexfh added a comment.Apr 5 2019, 5:01 AM

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 added a comment.Apr 5 2019, 5:02 AM

friendly ping

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