[clang-tidy] Fix a crash in modernize-loop-convert around conversion operators

[Szelethus]

Summary

modernize-loop-convert checks and fixes when a loop that iterates over the elements of a container can be rewritten from a for(...; ...; ...) style into the "new" C++11 for-range format. For that, it needs to parse the elements of that loop, like its init-statement, such as ItType it = cont.begin(). modernize-loop-convert checks whether the loop variable is initialized by a begin() member function.

When an iterator is initialized with a conversion operator (e.g. for (const_iterator it = non_const_container.begin(); ...), attempts to retrieve the name of the initializer expression resulted in an assert, as conversion operators don't have a valid IdentifierInfo.

I fixed this by making digThroughConstructors dig through conversion operators as well.

[Szelethus]

Clarify the summary.
Delete unnecessary includes.
More fitting iterator names in the test files.

[steakhal]

Looks great.

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

154–155

I think it's called conversion member functions.

clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.h

278

[Szelethus]

I'll intend to land this by friday unless there are objections!

[Eugene.Zelenko]

Please wait for @aaron.ballman approval.

[aaron.ballman]

LGTM modulo comments from @steakhal!

[whisperity]

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

154–168

Perhaps it would be nice if an example was added to the comment here which involves said conversion.

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

278

This is not specific to Qt, the GUI framework, right?

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

 /// Determine whether Init appears to be an initializing an iterator.
 ///
 /// If it is, returns the object whose begin() or end() method is called, and
 /// the output parameter isArrow is set to indicate whether the initialization
 /// is called via . or ->.
 static const Expr *getContainerFromBeginEndCall(const Expr *Init, bool IsBegin,
                                                 bool *IsArrow, bool IsReverse) {
   // FIXME: Maybe allow declaration/initialization outside of the for loop.
-  const auto *TheCall =
-      dyn_cast_or_null<CXXMemberCallExpr>(digThroughConstructors(Init));
+  const auto *TheCall = dyn_cast_or_null<CXXMemberCallExpr>(
+      digThroughConstructorsConversions(Init));
   if (!TheCall || TheCall->getNumArgs() != 0)
     return nullptr;
 
   const auto *Member = dyn_cast<MemberExpr>(TheCall->getCallee());
   if (!Member)
     return nullptr;
   StringRef Name = Member->getMemberDecl()->getName();
   if (!Name.consume_back(IsBegin ? "begin" : "end"))

clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.h

 private:
   Level CurrentLevel;
 };
 
 // The main computational result of ForLoopIndexVisitor.
 typedef llvm::SmallVector<Usage, 8> UsageResult;
 
 // General functions used by ForLoopIndexUseVisitor and LoopConvertCheck.
-const Expr *digThroughConstructors(const Expr *E);
+const Expr *digThroughConstructorsConversions(const Expr *E);
 bool areSameExpr(ASTContext *Context, const Expr *First, const Expr *Second);
 const DeclRefExpr *getDeclRef(const Expr *E);
 bool areSameVariable(const ValueDecl *First, const ValueDecl *Second);
 
 /// Discover usages of expressions consisting of index or iterator
 /// access.
 ///
 /// Given an index variable, recursively crawls a for loop to discover if the

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

bool DeclFinderASTVisitor::VisitTypeLoc(TypeLoc TL) {
   if (const IdentifierInfo *Ident = QType.getBaseTypeIdentifier()) {
     if (Ident->getName() == Name) {
       Found = true;
       return false;
     }
   }
   return true;
 }
 
-/// Look through conversion/copy constructors to find the explicit
-/// initialization expression, returning it is found.
+/// Look through conversion/copy constructors and member functions to find the
+/// explicit initialization expression, returning it is found.
 ///
 /// The main idea is that given
 ///   vector<int> v;
 /// we consider either of these initializations
 ///   vector<int>::iterator it = v.begin();
 ///   vector<int>::iterator it(v.begin());
+///   vector<int>::const_iterator it(v.begin());
 /// and retrieve `v.begin()` as the expression used to initialize `it` but do
 /// not include
 ///   vector<int>::iterator it;
 ///   vector<int>::iterator it(v.begin(), 0); // if this constructor existed
 /// as being initialized from `v.begin()`
-const Expr *digThroughConstructors(const Expr *E) {
+const Expr *digThroughConstructorsConversions(const Expr *E) {
   if (!E)
     return nullptr;
   E = E->IgnoreImplicit();
   if (const auto *ConstructExpr = dyn_cast<CXXConstructExpr>(E)) {
     // The initial constructor must take exactly one parameter, but base class
     // and deferred constructors can take more.
     if (ConstructExpr->getNumArgs() != 1 ||
         ConstructExpr->getConstructionKind() != CXXConstructExpr::CK_Complete)
       return nullptr;
     E = ConstructExpr->getArg(0);
     if (const auto *Temp = dyn_cast<MaterializeTemporaryExpr>(E))
       E = Temp->getSubExpr();
-    return digThroughConstructors(E);
+    return digThroughConstructorsConversions(E);
   }
+  // If this is a conversion (as iterators commonly convert into their const
+  // iterator counterparts), dig through that as well.
+  if (const auto *ME = dyn_cast<CXXMemberCallExpr>(E))
+    if (const auto *D = dyn_cast<CXXConversionDecl>(ME->getMethodDecl()))
+      return digThroughConstructorsConversions(ME->getImplicitObjectArgument());
   return E;
 }
 
 /// Returns true when two Exprs are equivalent.
 bool areSameExpr(ASTContext *Context, const Expr *First, const Expr *Second) {
   if (!First || !Second)
     return false;
 
static bool isAliasDecl(ASTContext *Context, const Decl *TheDecl,
   if (!VDecl)
     return false;
   if (!VDecl->hasInit())
     return false;
 
   bool OnlyCasts = true;
   const Expr *Init = VDecl->getInit()->IgnoreParenImpCasts();
   if (isa_and_nonnull<CXXConstructExpr>(Init)) {
-    Init = digThroughConstructors(Init);
+    Init = digThroughConstructorsConversions(Init);
     OnlyCasts = false;
   }
   if (!Init)
     return false;
 
   // Check that the declared type is the same as (or a reference to) the
   // container type.
   if (!OnlyCasts) {

clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-loop-convert/structures.h

struct iterator {
     bool operator!=(const iterator &other);
     void insert(value_type);
     value_type X;
   };
   iterator begin();
   iterator end();
 };
 
+struct Q {
+  typedef int value_type;
+  struct const_iterator {
+    value_type &operator*();
+    const value_type &operator*() const;
+    const_iterator &operator++();
+    bool operator!=(const const_iterator &other);
+    void insert(value_type);
+    value_type X;
+  };
+  struct iterator {
+    operator const_iterator() const;
+  };
+  iterator begin();
+  iterator end();
+};
+
 struct U {
   struct iterator {
     Val& operator*();
     const Val& operator*()const;
     iterator& operator ++();
     bool operator!=(const iterator &other);
     Val *operator->();
   };

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

void f() {
   T Tt;
   for (T::iterator It = Tt.begin(), E = Tt.end(); It != E; ++It) {
     printf("I found %d\n", *It);
   }
   // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
   // CHECK-FIXES: for (int & It : Tt)
   // CHECK-FIXES-NEXT: printf("I found %d\n", It);
 
+  // Do not crash because of Qq.begin() converting. Q::iterator converts with a
+  // conversion operator, which has no name, to Q::const_iterator.
+  Q Qq;
+  for (Q::const_iterator It = Qq.begin(), E = Qq.end(); It != E; ++It) {
+    printf("I found %d\n", *It);
+  }
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int & It : Qq)
+  // CHECK-FIXES-NEXT: printf("I found %d\n", It);
+
   T *Pt;
   for (T::iterator It = Pt->begin(), E = Pt->end(); It != E; ++It) {
     printf("I found %d\n", *It);
   }
   // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
   // CHECK-FIXES: for (int & It : *Pt)
   // CHECK-FIXES-NEXT: printf("I found %d\n", It);