[clang-tidy] Fix RenamerClangTidy checks breaking lambda captures.

[njames93]

Summary

Currently these checks will overwrite the default capture token (=|&) if any default captures refer to decls that need fixing.
There was an a previous patch (D59540) that tried to address this shortfall but it went about it trying to detect if it was inside a lambda then comparing the textual representation.
Learning from the previous its obvious that we need someway to tag a DeclRefExpr.
I've settled on RefersToDefaultCapture for now, but there could be a case to make it IsImplicit, for when a DeclRefExpr isn't actually written.
If anyone can think of good examples when this could happen I'd definitely support that instead.
Right now I've only addressed lambdas as I don't know anything about ObjectiveC Blocks, If this makes sense there I'd be happy to extend it.

[njames93]

Where would be a good place for testing refersToDefaultCapture support?

clang/lib/Sema/SemaLambda.cpp

1619

Correct me if I'm wrong, but there shouldn't be a case where the assert in cast fails.

clang/lib/Serialization/ASTWriterDecl.cpp

2285–2286

I have no idea about BitcodeWriter, is this correct or should I just be using the literal abbreviation?

[njames93]

Add a test case using init capture.

[njames93]

Ping?

[njames93]

Ping?

[aaron.ballman]

In D97653#2593287, @njames93 wrote:

Where would be a good place for testing refersToDefaultCapture support?

Thank you for your patience while I thought about this a bit more, sorry for the delay in reviewing it!

I think it probably makes sense to add testing for it in clang\test\AST with an AST dumping test (so the text node dumper would probably need to be updated to mention that the DeclRefExpr is an implicit capture).

clang/lib/Sema/SemaLambda.cpp

1619

Hmmm, could it be an unresolved lookup expr in the case of a dependent capture? Or a MemberExpr in the case of capturing an anonymous structure or union member capture?

Because we're trying to determine if a DeclRefExpr was implicitly captured, would it be better to use "implicit capture" in the identifier rather than "default capture"?

[njames93]

In D97653#2675005, @aaron.ballman wrote:

In D97653#2593287, @njames93 wrote:

Where would be a good place for testing refersToDefaultCapture support?

Thank you for your patience while I thought about this a bit more, sorry for the delay in reviewing it!

I think it probably makes sense to add testing for it in clang\test\AST with an AST dumping test (so the text node dumper would probably need to be updated to mention that the DeclRefExpr is an implicit capture).

That sounds like a good place to start, thanks.

clang/lib/Sema/SemaLambda.cpp

1619

Can't capture anonymous members so thats a non issue. I'm not entirely sure about dependent, but the codepaths where isVariableCapture is true always seem to result in either an ExprError or DeclRefExpr.

I do agree with the name though.

[njames93]

Use implicit capture instead of default capture.

@aaron.ballman, Unfortunately the AST dump tests don't work as implicit captures don't appear to show up in the textual representation.

[aaron.ballman]

In D97653#2681458, @njames93 wrote:

Use implicit capture instead of default capture.

@aaron.ballman, Unfortunately the AST dump tests don't work as implicit captures don't appear to show up in the textual representation.

Yeah, I was suggesting to add that as part of this patch so it'd be testable. We currently print something like -DeclRefExpr <col:14> 'int' lvalue Var 0x55b0ddc7f808 'b' 'int' and I think if we changed TextNodeDumper::VisitDeclRefExpr() to also print out RefersToEnclosingVariableOrCapture and RefersToImplicitCapture only when they're set, we could output something like: -DeclRefExpr <col:14> 'int' lvalue Var 0x55b0ddc7f808 'b' 'int' implicit_capture or -DeclRefExpr <col:14> 'int' lvalue Var 0x55b0ddc7f808 'b' 'int' explicit_capture, which would be useful (in addition to making your changes testable). WDYT?

clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp

void RenamerClangTidyCheck::check(const MatchFinder::MatchResult &Result) {
   if (const auto *Decl = Result.Nodes.getNodeAs<UsingDecl>("using")) {
     for (const auto *Shadow : Decl->shadows())
       addUsage(Shadow->getTargetDecl(), Decl->getNameInfo().getSourceRange(),
                Result.SourceManager);
     return;
   }
 
   if (const auto *DeclRef = Result.Nodes.getNodeAs<DeclRefExpr>("declRef")) {
+    if (DeclRef->refersToDefaultCapture())
+      return;
     SourceRange Range = DeclRef->getNameInfo().getSourceRange();
     addUsage(DeclRef->getDecl(), Range, Result.SourceManager);
     return;
   }
 
   if (const auto *MemberRef =
           Result.Nodes.getNodeAs<MemberExpr>("memberExpr")) {
     SourceRange Range = MemberRef->getMemberNameInfo().getSourceRange();

clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp

 struct async_obj {
 public:
   never_suspend operator co_await() const noexcept;
 };
 
 task ImplicitDeclTest(async_obj &a_object) {
   co_await a_object;  // CHECK-MESSAGES-NOT: warning: invalid case style for local variable
 }
+
+// CHECK-MESSAGES: :[[@LINE+2]]:21: warning: invalid case style for parameter 'Param'
+// CHECK-MESSAGES: :[[@LINE+1]]:32: warning: invalid case style for parameter 'Param2'
+void LambdaTest(int Param, int Param2) {
+  (void)[Param]() { return Param; };
+  (void)[&Param]() { return Param; };
+  (void)[=]() { return Param; };
+  (void)[&]() { return Param; };
+  (void)[=]() { return Param + Param2; };
+  (void)[=, &Param]() { return Param + Param2; };
+  // CHECK-FIXES: void LambdaTest(int a_param, int a_param2) {
+  // CHECK-FIXES-NEXT:   (void)[a_param]() { return a_param; };
+  // CHECK-FIXES-NEXT:   (void)[&a_param]() { return a_param; };
+  // CHECK-FIXES-NEXT:   (void)[=]() { return a_param; };
+  // CHECK-FIXES-NEXT:   (void)[&]() { return a_param; };
+  // CHECK-FIXES-NEXT:   (void)[=]() { return a_param + a_param2; };
+  // CHECK-FIXES-NEXT:   (void)[=, &a_param]() { return a_param + a_param2; };
+}

clang/include/clang/AST/Expr.h

public:
   }
 
   /// Does this DeclRefExpr refer to an enclosing local or a captured
   /// variable?
   bool refersToEnclosingVariableOrCapture() const {
     return DeclRefExprBits.RefersToEnclosingVariableOrCapture;
   }
 
+  bool refersToDefaultCapture() const {
+    return DeclRefExprBits.RefersToDefaultCapture;
+  }
+
+  bool setRefersToDefaultCapture(bool V = true) {
+    return DeclRefExprBits.RefersToDefaultCapture = V;
+  }
+
   static bool classof(const Stmt *T) {
     return T->getStmtClass() == DeclRefExprClass;
   }
 
   // Iterators
   child_range children() {
     return child_range(child_iterator(), child_iterator());
   }

clang/include/clang/AST/Stmt.h

class DeclRefExprBitfields {
     unsigned : NumExprBits;
 
     unsigned HasQualifier : 1;
     unsigned HasTemplateKWAndArgsInfo : 1;
     unsigned HasFoundDecl : 1;
     unsigned HadMultipleCandidates : 1;
     unsigned RefersToEnclosingVariableOrCapture : 1;
     unsigned NonOdrUseReason : 2;
+    unsigned RefersToDefaultCapture : 1;
 
     /// The location of the declaration name itself.
     SourceLocation Loc;
   };
 
 
   class FloatingLiteralBitfields {
     friend class FloatingLiteral;

clang/lib/AST/ASTImporter.cpp

ExpectedStmt ASTNodeImporter::VisitDeclRefExpr(DeclRefExpr *E) {
   }
 
   auto *ToE = DeclRefExpr::Create(
       Importer.getToContext(), ToQualifierLoc, ToTemplateKeywordLoc, ToDecl,
       E->refersToEnclosingVariableOrCapture(), ToLocation, ToType,
       E->getValueKind(), ToFoundD, ToResInfo, E->isNonOdrUse());
   if (E->hadMultipleCandidates())
     ToE->setHadMultipleCandidates(true);
+  if (E->refersToDefaultCapture())
+    ToE->setRefersToDefaultCapture(true);
   return ToE;
 }
 
 ExpectedStmt ASTNodeImporter::VisitImplicitValueInitExpr(ImplicitValueInitExpr *E) {
   ExpectedType TypeOrErr = import(E->getType());
   if (!TypeOrErr)
     return TypeOrErr.takeError();
 

clang/lib/AST/Expr.cpp

DeclRefExpr::DeclRefExpr(const ASTContext &Ctx, ValueDecl *D,
                          NonOdrUseReason NOUR)
     : Expr(DeclRefExprClass, T, VK, OK_Ordinary), D(D), DNLoc(LocInfo) {
   DeclRefExprBits.HasQualifier = false;
   DeclRefExprBits.HasTemplateKWAndArgsInfo = false;
   DeclRefExprBits.HasFoundDecl = false;
   DeclRefExprBits.HadMultipleCandidates = false;
   DeclRefExprBits.RefersToEnclosingVariableOrCapture =
       RefersToEnclosingVariableOrCapture;
+  DeclRefExprBits.RefersToDefaultCapture = false;
   DeclRefExprBits.NonOdrUseReason = NOUR;
   DeclRefExprBits.Loc = L;
   setDependence(computeDependence(this, Ctx));
 }
 
 DeclRefExpr::DeclRefExpr(const ASTContext &Ctx,
                          NestedNameSpecifierLoc QualifierLoc,
                          SourceLocation TemplateKWLoc, ValueDecl *D,
new (getTrailingObjects<NestedNameSpecifierLoc>())
         NestedNameSpecifierLoc(QualifierLoc);
   DeclRefExprBits.HasFoundDecl = FoundD ? 1 : 0;
   if (FoundD)
     *getTrailingObjects<NamedDecl *>() = FoundD;
   DeclRefExprBits.HasTemplateKWAndArgsInfo
     = (TemplateArgs || TemplateKWLoc.isValid()) ? 1 : 0;
   DeclRefExprBits.RefersToEnclosingVariableOrCapture =
       RefersToEnclosingVariableOrCapture;
+  DeclRefExprBits.RefersToDefaultCapture = false;
   DeclRefExprBits.NonOdrUseReason = NOUR;
   if (TemplateArgs) {
     auto Deps = TemplateArgumentDependence::None;
     getTrailingObjects<ASTTemplateKWAndArgsInfo>()->initializeFrom(
         TemplateKWLoc, *TemplateArgs, getTrailingObjects<TemplateArgumentLoc>(),
         Deps);
     assert(!(Deps & TemplateArgumentDependence::Dependent) &&
            "built a DeclRefExpr with dependent template args");

clang/lib/Sema/SemaLambda.cpp

ExprResult Sema::BuildCaptureInit(const Capture &Cap,
   // make a copy, even for a "copy capture".
   if (IsOpenMPMapping)
     return Init;
 
   if (Init.isInvalid())
     return ExprError();
 
   Expr *InitExpr = Init.get();
+  if (Cap.isVariableCapture() && ImplicitCaptureLoc.isValid())
+    cast<DeclRefExpr>(InitExpr)->setRefersToDefaultCapture(true);
   InitializedEntity Entity = InitializedEntity::InitializeLambdaCapture(
       Name, Cap.getCaptureType(), Loc);
   InitializationKind InitKind =
       InitializationKind::CreateDirect(Loc, Loc, Loc);
   InitializationSequence InitSeq(*this, Entity, InitKind, InitExpr);
   return InitSeq.Perform(*this, Entity, InitKind, InitExpr);
 }
 

clang/lib/Serialization/ASTReaderStmt.cpp

 void ASTStmtReader::VisitDeclRefExpr(DeclRefExpr *E) {
   VisitExpr(E);
 
   E->DeclRefExprBits.HasQualifier = Record.readInt();
   E->DeclRefExprBits.HasFoundDecl = Record.readInt();
   E->DeclRefExprBits.HasTemplateKWAndArgsInfo = Record.readInt();
   E->DeclRefExprBits.HadMultipleCandidates = Record.readInt();
   E->DeclRefExprBits.RefersToEnclosingVariableOrCapture = Record.readInt();
+  E->DeclRefExprBits.RefersToDefaultCapture = Record.readInt();
   E->DeclRefExprBits.NonOdrUseReason = Record.readInt();
   unsigned NumTemplateArgs = 0;
   if (E->hasTemplateKWAndArgsInfo())
     NumTemplateArgs = Record.readInt();
 
   if (E->hasQualifier())
     new (E->getTrailingObjects<NestedNameSpecifierLoc>())
         NestedNameSpecifierLoc(Record.readNestedNameSpecifierLoc());
while (true) {
     case EXPR_PREDEFINED:
       S = PredefinedExpr::CreateEmpty(
           Context,
           /*HasFunctionName*/ Record[ASTStmtReader::NumExprFields]);
       break;
 
     case EXPR_DECL_REF:
       S = DeclRefExpr::CreateEmpty(
           Context,
           /*HasQualifier=*/Record[ASTStmtReader::NumExprFields],
           /*HasFoundDecl=*/Record[ASTStmtReader::NumExprFields + 1],
           /*HasTemplateKWAndArgsInfo=*/Record[ASTStmtReader::NumExprFields + 2],
-        /*NumTemplateArgs=*/Record[ASTStmtReader::NumExprFields + 2] ?
-          Record[ASTStmtReader::NumExprFields + 6] : 0);
+          /*NumTemplateArgs=*/
+          Record[ASTStmtReader::NumExprFields + 2]
+              ? Record[ASTStmtReader::NumExprFields + 7]
+              : 0);
       break;
 
     case EXPR_INTEGER_LITERAL:
       S = IntegerLiteral::Create(Context, Empty);
       break;
 
     case EXPR_FIXEDPOINT_LITERAL:
       S = FixedPointLiteral::Create(Context, Empty);

clang/lib/Serialization/ASTWriterDecl.cpp

void ASTWriter::WriteDeclAbbrevs() {
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 3)); //GetValueKind
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 3)); //GetObjectKind
   //DeclRefExpr
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); //HasQualifier
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); //GetDeclFound
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); //ExplicitTemplateArgs
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); //HadMultipleCandidates
   Abv->Add(BitCodeAbbrevOp(0)); // RefersToEnclosingVariableOrCapture
+  Abv->Add(
+      BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // RefersToDefaultCapture
   Abv->Add(BitCodeAbbrevOp(0)); // NonOdrUseReason
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // DeclRef
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // Location
   DeclRefExprAbbrev = Stream.EmitAbbrev(std::move(Abv));
 
   // Abbreviation for EXPR_INTEGER_LITERAL
   Abv = std::make_shared<BitCodeAbbrev>();
   Abv->Add(BitCodeAbbrevOp(serialization::EXPR_INTEGER_LITERAL));

clang/lib/Serialization/ASTWriterStmt.cpp

 void ASTStmtWriter::VisitDeclRefExpr(DeclRefExpr *E) {
   VisitExpr(E);
 
   Record.push_back(E->hasQualifier());
   Record.push_back(E->getDecl() != E->getFoundDecl());
   Record.push_back(E->hasTemplateKWAndArgsInfo());
   Record.push_back(E->hadMultipleCandidates());
   Record.push_back(E->refersToEnclosingVariableOrCapture());
+  Record.push_back(E->refersToDefaultCapture());
   Record.push_back(E->isNonOdrUse());
 
   if (E->hasTemplateKWAndArgsInfo()) {
     unsigned NumTemplateArgs = E->getNumTemplateArgs();
     Record.push_back(NumTemplateArgs);
   }
 
   DeclarationName::NameKind nk = (E->getDecl()->getDeclName().getNameKind());