[clang-tidy] Fix wrong code generation for `modernize-loop-convert` with structured bindings.

[AMS21]

Summary

Fixes llvm#62951

[AMS21]

Remove debug code

[PiotrZSL]

Overall looks fine.

clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp

545–547

this could be merged, try dyn_cast

[AMS21]

Use dyn_cast

[AMS21]

If there are no more problems, I would kindly ask for someone to push this on my behalf :)

clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp

 
 /// Computes the changes needed to convert a given for loop, and
 /// applies them.
 void LoopConvertCheck::doConversion(
     ASTContext *Context, const VarDecl *IndexVar,
     const ValueDecl *MaybeContainer, const UsageResult &Usages,
     const DeclStmt *AliasDecl, bool AliasUseRequired, bool AliasFromForInit,
     const ForStmt *Loop, RangeDescriptor Descriptor) {
-  std::string VarName;
+  std::string VarNameOrStructuredBinding;
   bool VarNameFromAlias = (Usages.size() == 1) && AliasDecl;
   bool AliasVarIsRef = false;
   bool CanCopy = true;
   std::vector<FixItHint> FixIts;
   if (VarNameFromAlias) {
     const auto *AliasVar = cast<VarDecl>(AliasDecl->getSingleDecl());
-    VarName = AliasVar->getName().str();
+
+    // Handle structured bindings
+    if (const auto *AliasDecompositionDecl =
+            dyn_cast<DecompositionDecl>(AliasDecl->getSingleDecl())) {
+      VarNameOrStructuredBinding = "[";
+
+      assert(!AliasDecompositionDecl->bindings().empty() && "No bindings");
+      for (const BindingDecl *Binding : AliasDecompositionDecl->bindings()) {
+        VarNameOrStructuredBinding += Binding->getName().str() + ", ";
+      }
+
+      VarNameOrStructuredBinding.erase(VarNameOrStructuredBinding.size() - 2,
+                                       2);
+      VarNameOrStructuredBinding += "]";
+    } else {
+      VarNameOrStructuredBinding = AliasVar->getName().str();
 
       // Use the type of the alias if it's not the same
       QualType AliasVarType = AliasVar->getType();
       assert(!AliasVarType.isNull() && "Type in VarDecl is null");
       if (AliasVarType->isReferenceType()) {
         AliasVarType = AliasVarType.getNonReferenceType();
         AliasVarIsRef = true;
       }
       if (Descriptor.ElemType.isNull() ||
           !Context->hasSameUnqualifiedType(AliasVarType, Descriptor.ElemType))
         Descriptor.ElemType = AliasVarType;
+    }
 
     // We keep along the entire DeclStmt to keep the correct range here.
     SourceRange ReplaceRange = AliasDecl->getSourceRange();
 
     std::string ReplacementText;
     if (AliasUseRequired) {
-      ReplacementText = VarName;
+      ReplacementText = VarNameOrStructuredBinding;
     } else if (AliasFromForInit) {
       // FIXME: Clang includes the location of the ';' but only for DeclStmt's
       // in a for loop's init clause. Need to put this ';' back while removing
       // the declaration of the alias variable. This is probably a bug.
       ReplacementText = ";";
     } else {
       // Avoid leaving empty lines or trailing whitespaces.
       getAliasRange(Context->getSourceManager(), ReplaceRange);
     }
 
     FixIts.push_back(FixItHint::CreateReplacement(
         CharSourceRange::getTokenRange(ReplaceRange), ReplacementText));
     // No further replacements are made to the loop, since the iterator or index
     // was used exactly once - in the initialization of AliasVar.
   } else {
     VariableNamer Namer(&TUInfo->getGeneratedDecls(),
                         &TUInfo->getParentFinder().getStmtToParentStmtMap(),
                         Loop, IndexVar, MaybeContainer, Context, NamingStyle);
-    VarName = Namer.createIndexName();
+    VarNameOrStructuredBinding = Namer.createIndexName();
     // First, replace all usages of the array subscript expression with our new
     // variable.
     for (const auto &Usage : Usages) {
       std::string ReplaceText;
       SourceRange Range = Usage.Range;
       if (Usage.Expression) {
         // If this is an access to a member through the arrow operator, after
         // the replacement it must be accessed through the '.' operator.
-        ReplaceText = Usage.Kind == Usage::UK_MemberThroughArrow ? VarName + "."
-                                                                 : VarName;
+        ReplaceText = Usage.Kind == Usage::UK_MemberThroughArrow
+                          ? VarNameOrStructuredBinding + "."
+                          : VarNameOrStructuredBinding;
         auto Parents = Context->getParents(*Usage.Expression);
         if (Parents.size() == 1) {
           if (const auto *Paren = Parents[0].get<ParenExpr>()) {
             // Usage.Expression will be replaced with the new index variable,
             // and parenthesis around a simple DeclRefExpr can always be
             // removed.
             Range = Paren->getSourceRange();
           } else if (const auto *UOP = Parents[0].get<UnaryOperator>()) {
             // If we are taking the address of the loop variable, then we must
             // not use a copy, as it would mean taking the address of the loop's
             // local index instead.
             // FIXME: This won't catch cases where the address is taken outside
             // of the loop's body (for instance, in a function that got the
             // loop's index as a const reference parameter), or where we take
             // the address of a member (like "&Arr[i].A.B.C").
             if (UOP->getOpcode() == UO_AddrOf)
               CanCopy = false;
           }
         }
       } else {
         // The Usage expression is only null in case of lambda captures (which
         // are VarDecl). If the index is captured by value, add '&' to capture
         // by reference instead.
-        ReplaceText =
-            Usage.Kind == Usage::UK_CaptureByCopy ? "&" + VarName : VarName;
+        ReplaceText = Usage.Kind == Usage::UK_CaptureByCopy
+                          ? "&" + VarNameOrStructuredBinding
+                          : VarNameOrStructuredBinding;
       }
       TUInfo->getReplacedVars().insert(std::make_pair(Loop, IndexVar));
       FixIts.push_back(FixItHint::CreateReplacement(
           CharSourceRange::getTokenRange(Range), ReplaceText));
     }
   }
 
   // Now, we need to construct the new range expression.
if (Descriptor.DerefByConstRef) {
       Type = Context->getLValueReferenceType(Type);
     }
   }
 
   SmallString<128> Range;
   llvm::raw_svector_ostream Output(Range);
   Output << '(';
   Type.print(Output, getLangOpts());
-  Output << ' ' << VarName << " : ";
+  Output << ' ' << VarNameOrStructuredBinding << " : ";
   if (Descriptor.NeedsReverseCall)
     Output << getReverseFunction() << '(';
   if (Descriptor.ContainerNeedsDereference)
     Output << '*';
   Output << Descriptor.ContainerString;
   if (Descriptor.NeedsReverseCall)
     Output << "))";
   else
     Output << ')';
   FixIts.push_back(FixItHint::CreateReplacement(
       CharSourceRange::getTokenRange(ParenRange), Range));
 
   if (Descriptor.NeedsReverseCall && !getReverseHeader().empty()) {
     if (std::optional<FixItHint> Insertion = Inserter.createIncludeInsertion(
             Context->getSourceManager().getFileID(Loop->getBeginLoc()),
             getReverseHeader()))
       FixIts.push_back(*Insertion);
   }
   diag(Loop->getForLoc(), "use range-based for loop instead") << FixIts;
-  TUInfo->getGeneratedDecls().insert(make_pair(Loop, VarName));
+  TUInfo->getGeneratedDecls().insert(
+      make_pair(Loop, VarNameOrStructuredBinding));
 }
 
 /// Returns a string which refers to the container iterated over.
 StringRef LoopConvertCheck::getContainerString(ASTContext *Context,
                                                const ForStmt *Loop,
                                                const Expr *ContainerExpr) {
   StringRef ContainerString;
   ContainerExpr = ContainerExpr->IgnoreParenImpCasts();
   if (isa<CXXThisExpr>(ContainerExpr)) {
     ContainerString = "this";
   } else {
     // For CXXOperatorCallExpr such as vector_ptr->size() we want the class
     // object vector_ptr, but for vector[2] we need the whole expression.
-    if (const auto* E = dyn_cast<CXXOperatorCallExpr>(ContainerExpr))
+    if (const auto *E = dyn_cast<CXXOperatorCallExpr>(ContainerExpr))
       if (E->getOperator() != OO_Subscript)
         ContainerExpr = E->getArg(0);
     ContainerString =
         getStringFromRange(Context->getSourceManager(), Context->getLangOpts(),
                            ContainerExpr->getSourceRange());
   }
 
   return ContainerString;

clang-tools-extra/docs/ReleaseNotes.rst

- Deprecated check-local options `HeaderFileExtensions`
   <clang-tidy/checks/misc/unused-using-decls>` check.
   Global options of the same name should be used instead.
 
 - Improved :doc:`modernize-concat-nested-namespaces
   <clang-tidy/checks/modernize/concat-nested-namespaces>` to fix incorrect fixes when
   using macro between namespace declarations, to fix false positive when using namespace
   with attributes and to support nested inline namespace introduced in c++20.
 
+- Fixed an issue in `modernize-loop-convert
+  <clang-tidy/checks/modernize/modernize-loop-convert>` generating wrong code
+  when using structured bindings.
+
 - In :doc:`modernize-use-default-member-init
   <clang-tidy/checks/modernize/use-default-member-init>` count template
   constructors toward hand written constructors so that they are skipped if more
   than one exists.
 
 - Fixed crash in :doc:`modernize-use-default-member-init
   <clang-tidy/checks/modernize/use-default-member-init>` with array members which
   are value initialized.

clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-structured-binding.cpp

+// RUN: %check_clang_tidy -std=c++17-or-later %s modernize-loop-convert %t
+
+struct S {
+  int a{0};
+  int b{1};
+};
+
+template <typename T, unsigned long Size>
+struct array {
+
+  T *begin() { return data; }
+  const T* cbegin() const { return data; }
+  T *end() { return data + Size; }
+  const T *cend() const { return data + Size; }
+
+  T data[Size];
+};
+
+const int N = 6;
+S Arr[N];
+
+void f() {
+  int Sum = 0;
+
+  for (int I = 0; I < N; ++I) {
+    auto [a, b] = Arr[I];
+    Sum += a + b;
+  }
+  // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead [modernize-loop-convert]
+  // CHECK-FIXES: for (auto [a, b] : Arr) {
+  // CHECK-FIXES-NEXT: Sum += a + b;
+
+  array<S, N> arr;
+  for (auto* It = arr.begin(); It != arr.end(); ++It) {
+    auto [a, b] = *It;
+    Sum = a + b;
+  }
+  // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead [modernize-loop-convert]
+  // CHECK-FIXES: for (auto [a, b] : arr) {
+  // CHECK-FIXES-NEXT: Sum = a + b;
+
+  for (auto* It = arr.cbegin(); It != arr.cend(); ++It) {
+    auto [a, b] = *It;
+    Sum = a + b;
+  }
+  // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead [modernize-loop-convert]
+  // CHECK-FIXES: for (auto [a, b] : arr) {
+  // CHECK-FIXES-NEXT: Sum = a + b;
+}