[clang-tidy] Fix assert in modernize-loop-convert

[njames93]

Summary

I'm unable to figure out a reproduction of this bug as it emerged when using clangd(On a ReleaseWithAssertions build) making it very hard to figure out the exact code that caught it.
However this is a pretty obvious check to include.

[aaron.ballman]

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

316–317

It's really strange to me that we're even getting to this point in the check -- the only way for the assertion to fail is for the member call expression to be on something without a name. The cases I can think of for that would be something like foo.operator+(RHS) or something similarly nonsensical within this context (we're looking for things named begin or end). I think it'd make more sense to handle this at the matcher level (or early in the call chain) so that we never get here.

I think having a test case would be really useful to trying to understand what changes are appropriate. I don't think these changes are wrong so much as I wonder if we're in the wrong place to make them (and we'll hit other confused code elsewhere).

549

This doesn't seem necessary? Calling NamedDecl::getName() already asserts that the name is an identifier.

[njames93]

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

316–317

It is very strange. At the matcher level we are looking for calls to methods named begin or end or (c/r) variants.
I'm gonna try and put some debug prints and see if I can figure out the code actually causing the assert in the first place.

In the mean time, I have a patch in the works that moves a lot of the logic into the matchers and this whole function is removed in there, however a few creases still need to be ironed out in there.

549

I put that in there to figure out which getName assertion was firing as the call stack was optimized away on RelWithAssert builds. Probably can be removed.

[njames93]

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

316–317

So I put some debugprints in there and managed to find the cause of the crash

// llvm/lib/Option/OptTable.cpp:150
for (StringSet<>::const_iterator I = PrefixesUnion.begin(),
                                 E = PrefixesUnion.end(); I != E; ++I) {
  StringRef Prefix = I->getKey();
  for (StringRef::const_iterator C = Prefix.begin(), CE = Prefix.end();
                                 C != CE; ++C)
    if (!is_contained(PrefixChars, *C))
      PrefixChars.push_back(*C);
}

However I'm still not 100% sure of the cause of the crash. Its nothing to do with begin() being a member of the bast class StringMap. I tried testing with inherited begin/end methods and no crash.
My best guess is Prefix.begin() returns StringMapIterator<>, that class contains a conversion operator to StringMapConstIterator<>. Conversion operators names aren't Identifiers so that would explain the assert.
If that is the case, the actual issue is in digThroughConstructors being fooled by the CXXMemberCallExpr that's just a conversion operator.

The other patch in the works doesn't use that function so the bug is not present there, maybe its easier to just get that ready. WDYT?

[aaron.ballman]

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

316–317

The other patch in the works doesn't use that function so the bug is not present there, maybe its easier to just get that ready. WDYT?

I think that makes sense.

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

static const Expr *getContainerFromBeginEndCall(const Expr *Init, bool IsBegin,
   const auto *TheCall =
       dyn_cast_or_null<CXXMemberCallExpr>(digThroughConstructors(Init));
   if (!TheCall || TheCall->getNumArgs() != 0)
     return nullptr;
 
   const auto *Member = dyn_cast<MemberExpr>(TheCall->getCallee());
   if (!Member)
     return nullptr;
+  if (!Member->getMemberDecl()->getDeclName().isIdentifier())
+    return nullptr;
   StringRef Name = Member->getMemberDecl()->getName();
   if (!Name.consume_back(IsBegin ? "begin" : "end"))
     return nullptr;
   if (IsReverse && !Name.consume_back("r"))
     return nullptr;
   if (!Name.empty() && !Name.equals("c"))
     return nullptr;
 
void LoopConvertCheck::doConversion(
     const ForStmt *Loop, RangeDescriptor Descriptor) {
   std::string VarName;
   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());
+    assert(AliasVar->getDeclName().isIdentifier());
     VarName = 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;