[clangd] Fix modernize-loop-convert "multiple diag in flight" crash.

[hokein]

Summary

this maybe not ideal, but it is trivial and does fix the crash.

Fixes https://github.com/clangd/clangd/issues/156.

[sammccall]

Ah, nice work!
Agree it's a hack (and how many more instances are lurking) but I don't know how better to fix it and it's been reported 3 times...

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

529

I'm not sure we need to mention clangd here - I'd expect standalone clang-tidy to be able to hit the same condition?
At least logically I think the bug exists either way.

clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp

275

Does the bug repro with clang-tidy? If so it'd be nice to make this a clang-tidy test.

[hokein]

refine the fix.

[hokein]

In D78715#1999236, @sammccall wrote:

Ah, nice work!
Agree it's a hack (and how many more instances are lurking) but I don't know how better to fix it and it's been reported 3 times...

this is the only place in clang-tidy calling getTypeInfo.

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

529

yeah logically, but it doesn't trigger the assertion somehow (haven't dig into it). removed it.

I realized that the code below may change Descriptor.ElemType, refined the fix (we store the fixits, and emit the diagnostic at the end of the function).

clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp

275

unfortunately, not for standalone clang-tidy :(

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

 //===--- LoopConvertCheck.cpp - clang-tidy---------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "LoopConvertCheck.h"

 
 /// 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) {
+  // getTypeInfo may emit a diagnostic, we have to call it before constructing
+  // the Diag to avoid the "multiple-diagnostic in flight" crash in clangd.
+  auto TypeWidth = Context->getTypeInfo(Descriptor.ElemType).Width;
   auto Diag = diag(Loop->getForLoc(), "use range-based for loop instead");
 
   std::string VarName;
   bool VarNameFromAlias = (Usages.size() == 1) && AliasDecl;
   bool AliasVarIsRef = false;
   bool CanCopy = true;
 
   if (VarNameFromAlias) {
void LoopConvertCheck::doConversion(
 
   QualType Type = Context->getAutoDeductType();
   if (!Descriptor.ElemType.isNull() && Descriptor.ElemType->isFundamentalType())
     Type = Descriptor.ElemType.getUnqualifiedType();
 
   // If the new variable name is from the aliased variable, then the reference
   // type for the new variable should only be used if the aliased variable was
   // declared as a reference.
   bool IsCheapToCopy =
       !Descriptor.ElemType.isNull() &&
       Descriptor.ElemType.isTriviallyCopyableType(*Context) &&
       // TypeInfo::Width is in bits.
-      Context->getTypeInfo(Descriptor.ElemType).Width <= 8 * MaxCopySize;
+      TypeWidth <= 8 * MaxCopySize;
   bool UseCopy = CanCopy && ((VarNameFromAlias && !AliasVarIsRef) ||
                              (Descriptor.DerefByConstRef && IsCheapToCopy));
 
   if (!UseCopy) {
     if (Descriptor.DerefByConstRef) {
       Type = Context->getLValueReferenceType(Context->getConstType(Type));
     } else if (Descriptor.DerefByValue) {
       if (!IsCheapToCopy)

clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp

EXPECT_THAT(
                    "use a trailing return type for this function"),
               DiagSource(Diag::ClangTidy),
               DiagName("modernize-use-trailing-return-type"),
               // Verify that we don't have "[check-name]" suffix in the message.
               WithFix(FixMessage(
                   "use a trailing return type for this function")))));
 }
 
+TEST(DiagnosticTest, NoMultipleDiagnosticInFlight) {
+  Annotations Main(R"cpp(
+    template <typename T> struct Foo {
+      T *begin();
+      T *end();
+    };
+    struct LabelInfo {
+      int a;
+      bool b;
+    };
+
+    void f() {
+      Foo<LabelInfo> label_info_map;
+      [[for]] (auto it = label_info_map.begin(); it != label_info_map.end(); ++it) {
+        auto S = *it;
+      }
+    }
+  )cpp");
+  TestTU TU = TestTU::withCode(Main.code());
+  TU.ClangTidyChecks = "modernize-loop-convert";
+  EXPECT_THAT(
+      TU.build().getDiagnostics(),
+      UnorderedElementsAre(::testing::AllOf(
+          Diag(Main.range(), "use range-based for loop instead"),
+          DiagSource(Diag::ClangTidy), DiagName("modernize-loop-convert"))));
+}
+
 TEST(DiagnosticTest, ClangTidySuppressionComment) {
   Annotations Main(R"cpp(
     int main() {
       int i = 3;
       double d = 8 / i;  // NOLINT
       // NOLINTNEXTLINE
       double e = 8 / i;
       #define BAD 8 / i