[clangd] Emit ChangeAnnotation label and description for include-cleaner diagnostics.

[hokein]

VSCode UI:

• missing-include

• unused-include

[kadircet]

clang-tools-extra/clangd/IncludeCleaner.cpp

445

what about just taking in llvm::ArrayRef<const Inclusion *> UnusedIncludes and having a TextEdit dropInclude(const Inclusion&); that we can use in here & inside generateUnusedIncludeDiagnostics? That way we can just use the spelling inside the inclusion to generate the label, without all the extra logic.

485

i think rather than a single diag info, we should actually store a set of them now, right? as we want to surface not only the "first" use of a header, but all. (possibly with some trimming).

514

i don't think line information is as useful (as there's no structured link to it). what about something like:

label: `Include "foo.h"`
description: `Provides 'X', 'Y', 'Z', ...`

550

sorry for missing it during last review, but this should actually be if (!UnusedIncludes.empty() && !MissingIncludeDiags.empty()) as even if we have only 1 fix for each category, we should still show the fix all. probably it deserves a fix in a separate patch.

[hokein]

clang-tools-extra/clangd/IncludeCleaner.cpp

550

good catch, send a fix in https://reviews.llvm.org/D149822. (it turns out more tricky than we thought, only changing the if is not enough, I added some unittests for it).

[hokein]

address comments and polish implementations.

[hokein]

I have polished the implementation about the fix-all for missing-include diagnostics, please take a look on the new code.

[kadircet]

thanks, mostly LG. I think we need to be a little careful when generating insertions for all the missing includes though.

clang-tools-extra/clangd/IncludeCleaner.cpp

166–170

what do we gain by preserving the insertion order here exactly?

in generateMissingIncludeDiagnostics, we already traverse using the order of MissingIncludeDiagInfos and inside addAllMissingIncludes the order we preserved here (diagnostic order) doesn't really make much sense, we probably should generate fixes in the order of insertion location, or header spelling.

so what about just using a DenseMap ?

183

nit: maybe inline into the call site

309

we might still have symbols with the same name (ns1::Foo vs ns2::Foo), they'll show up the same in the final annotation.

since we're already sorting by name, we might as well deduplicate after sorting and store just a SmallVector here

332

we might generate multiple insertions to same location here e.g. insert a.h and b.h at the top of the file. LSP says that order reported by the server will be preserved. hence we should actually make sure our edits are ordered by spelling.

clang-tools-extra/clangd/IncludeCleaner.h

44 ↗

(On Diff #522067)

can you put definition out-of-line ?

clang-tools-extra/include-cleaner/include/clang-include-cleaner/Types.h

71 ↗

(On Diff #522067)

// Unqualified name of the symbol

[hokein]

rebase, and address review comments

clang-tools-extra/clangd/IncludeCleaner.cpp

166–170

The reason was that we need a deterministic-order for visting MissingIncludeEdits, otherwise that batch-fix lit test might fail from time to time.

particularly, in addAllMissingIncludes, we generate a Fix by iterating MissingIncludeEdits, we construct the Fix.Edits, and Fix.Annotations.

• For Fix.Edits, as you pointed out in another comment, we can sort it by (range, newText), to make the case where multiple #includes insertion are at the same place better;
• For Fix.Annotations, this is a bit hard to do a sort afterward as we construct the ID + Annotation, but luckily the final order doesn't matter there, we only need a deterministic order to make lit test happy.

The other alternative is to switch MissingIncludeEdits to a vector<{Header, TextEdit}> sorted by the TextEdit, but in generateMissingIncludeDiagnostics, we need to lookup from a Header, then this means we have to do a linear scan which doesn't feel good.

309

you're right, done.

332

Good catch, this seems like an existing problem today. Sorting these edits by spelling header is one solution,

I'm planning to do it in this patch, added a FIXME.

clang-tools-extra/clangd/IncludeCleaner.cpp

case include_cleaner::Symbol::Macro:
     return Sym.macro().Name->getName().str();
   case include_cleaner::Symbol::Declaration:
     return llvm::dyn_cast<NamedDecl>(&Sym.declaration())
         ->getQualifiedNameAsString();
   }
   llvm_unreachable("Unknown symbol kind");
 }
 
-std::vector<Diag> generateMissingIncludeDiagnostics(
+struct MissingIncludeDiag {
+  Diag D;
+  const MissingIncludeDiagInfo * DInfo = nullptr;
+};
+std::vector<MissingIncludeDiag> generateMissingIncludeDiagnostics(
     ParsedAST &AST, llvm::ArrayRef<MissingIncludeDiagInfo> MissingIncludes,
     llvm::StringRef Code) {
-  std::vector<Diag> Result;
+  std::vector<MissingIncludeDiag> Result;
   const Config &Cfg = Config::current();
   if (Cfg.Diagnostics.MissingIncludes != Config::IncludesPolicy::Strict ||
       Cfg.Diagnostics.SuppressAll ||
       Cfg.Diagnostics.Suppress.contains("missing-includes")) {
     return Result;
   }
 
   const SourceManager &SM = AST.getSourceManager();
   const FileEntry *MainFile = SM.getFileEntryForID(SM.getMainFileID());
 
   auto FileStyle = format::getStyle(
       format::DefaultFormatStyle, AST.tuPath(), format::DefaultFallbackStyle,
       Code, &SM.getFileManager().getVirtualFileSystem());
   if (!FileStyle) {
     elog("Couldn't infer style", FileStyle.takeError());
     FileStyle = format::getLLVMStyle();
   }
 
for (const auto &SymbolWithMissingInclude : MissingIncludes) {
     // include is present in a PP-disabled region, or spelling of the header
     // turns out to be the same as one of the unresolved includes in the
     // main file.
     std::optional<tooling::Replacement> Replacement = HeaderIncludes.insert(
         HeaderRef.trim("\"<>"), Angled, tooling::IncludeDirective::Include);
     if (!Replacement.has_value())
       continue;
 
-    Diag &D = Result.emplace_back();
+    auto & R = Result.emplace_back();
+    R.DInfo = &SymbolWithMissingInclude;
+    auto & D = R.D;
     D.Message =
         llvm::formatv("No header providing \"{0}\" is directly included",
                       getSymbolName(SymbolWithMissingInclude.Symbol));
     D.Name = "missing-includes";
     D.Source = Diag::DiagSource::Clangd;
     D.File = AST.tuPath();
     D.InsideMainFile = true;
     D.Severity = DiagnosticsEngine::Warning;

 include_cleaner::Includes
 convertIncludes(const SourceManager &SM,
                 const llvm::ArrayRef<Inclusion> Includes) {
   include_cleaner::Includes ConvertedIncludes;
   for (const Inclusion &Inc : Includes) {
     include_cleaner::Include TransformedInc;
     llvm::StringRef WrittenRef = llvm::StringRef(Inc.Written);
     TransformedInc.Spelled = WrittenRef.trim("\"<>");
     TransformedInc.HashLocation =
         SM.getComposedLoc(SM.getMainFileID(), Inc.HashOffset);
     TransformedInc.Line = Inc.HashLine + 1;
     TransformedInc.Angled = WrittenRef.starts_with("<");
     auto FE = SM.getFileManager().getFile(Inc.Resolved);
     if (!FE) {
       elog("IncludeCleaner: Failed to get an entry for resolved path {0}: {1}",
            Inc.Resolved, FE.getError().message());
       continue;
     }
     TransformedInc.Resolved = *FE;
     ConvertedIncludes.add(std::move(TransformedInc));
   }
   return ConvertedIncludes;
 }
 
 std::string spellHeader(ParsedAST &AST, const FileEntry *MainFile,
                         include_cleaner::Header Provider) {
   if (Provider.kind() == include_cleaner::Header::Physical) {
     if (auto CanonicalPath = getCanonicalPath(Provider.physical()->getLastRef(),
                                               AST.getSourceManager())) {
       std::string SpelledHeader =
           llvm::cantFail(URI::includeSpelling(URI::create(*CanonicalPath)));
       if (!SpelledHeader.empty())
         return SpelledHeader;
     }
   }
   return include_cleaner::spellHeader(
       Provider, AST.getPreprocessor().getHeaderSearchInfo(), MainFile);
 }
 
 std::vector<const Inclusion *>
llvm::stable_sort(MissingIncludes, [](const MissingIncludeDiagInfo &LHS,
     return true;
   });
   MissingIncludes.erase(llvm::unique(MissingIncludes), MissingIncludes.end());
   std::vector<const Inclusion *> UnusedIncludes =
       getUnused(AST, Used, /*ReferencedPublicHeaders*/ {});
   return {std::move(UnusedIncludes), std::move(MissingIncludes)};
 }
 
-Fix removeAllUnusedIncludes(llvm::ArrayRef<Diag> UnusedIncludes) {
+Fix removeAllUnusedIncludes(llvm::ArrayRef<Diag> UnusedIncludes,
+                            llvm::StringRef Code) {
   assert(!UnusedIncludes.empty());
-
   Fix RemoveAll;
   RemoveAll.Message = "remove all unused includes";
   for (const auto &Diag : UnusedIncludes) {
     assert(Diag.Fixes.size() == 1 && "Expected exactly one fix.");
     RemoveAll.Edits.insert(RemoveAll.Edits.end(),
          Diag.Fixes.front().Edits.begin(),
          Diag.Fixes.front().Edits.end());
   }
 
-  // TODO(hokein): emit a suitable text for the label.
-  ChangeAnnotation Annotation = {/*label=*/"",
-                                 /*needsConfirmation=*/true,
-                                 /*description=*/""};
+  llvm::SmallVector<llvm::StringRef> Lines;
+  Code.split(Lines, "\n");
   static const ChangeAnnotationIdentifier RemoveAllUnusedID =
       "RemoveAllUnusedIncludes";
   for (unsigned I = 0; I < RemoveAll.Edits.size(); ++I) {
     ChangeAnnotationIdentifier ID = RemoveAllUnusedID + std::to_string(I);
+    unsigned StartLine = RemoveAll.Edits[I].range.start.line;
+    assert(StartLine < Lines.size() && "Out of bound line number!");
+
     RemoveAll.Edits[I].annotationId = ID;
-    RemoveAll.Annotations.push_back({ID, Annotation});
+    RemoveAll.Annotations.push_back(
+        {ID, ChangeAnnotation{
+                 /*label=*/llvm::formatv("Remove '{0}'", Lines[StartLine]),
+                 /*needsConfirmation=*/true,
+                 /*description=*/""}
+        });
   }
   return RemoveAll;
 }
-Fix addAllMissingIncludes(llvm::ArrayRef<Diag> MissingIncludeDiags) {
+Fix addAllMissingIncludes(
+    llvm::ArrayRef<MissingIncludeDiag> MissingIncludeDiags,
+    llvm::StringRef Code) {
   assert(!MissingIncludeDiags.empty());
 
   Fix AddAllMissing;
   AddAllMissing.Message = "add all missing includes";
   // A map to deduplicate the edits with the same new text.
   // newText (#include "my_missing_header.h") -> TextEdit.
-  llvm::StringMap<TextEdit> Edits;
+  llvm::StringMap<std::pair<TextEdit, const MissingIncludeDiag*>> Edits;
   for (const auto &Diag : MissingIncludeDiags) {
-    assert(Diag.Fixes.size() == 1 && "Expected exactly one fix.");
-    for (const auto& Edit : Diag.Fixes.front().Edits) {
-      Edits.try_emplace(Edit.newText, Edit);
+    assert(Diag.D.Fixes.size() == 1 && "Expected exactly one fix.");
+    for (const auto& Edit : Diag.D.Fixes.front().Edits) {
+      Edits.try_emplace(Edit.newText, std::make_pair(Edit, &Diag));
     }
   }
-  // FIXME(hokein): emit used symbol reference in the annotation.
-  ChangeAnnotation Annotation = {/*label=*/"",
-                                 /*needsConfirmation=*/true,
-                                 /*description=*/""};
+  llvm::SmallVector<llvm::StringRef> Lines;
+  Code.split(Lines, "\n");
+
   static const ChangeAnnotationIdentifier AddAllMissingID =
       "AddAllMissingIncludes";
   unsigned I = 0;
-  for (auto &It : Edits) {
+  for (auto &[NewText, EditAndDiag] : Edits) {
     ChangeAnnotationIdentifier ID = AddAllMissingID + std::to_string(I++);
-    AddAllMissing.Edits.push_back(std::move(It.getValue()));
+    AddAllMissing.Edits.push_back(std::move(EditAndDiag.first));
     AddAllMissing.Edits.back().annotationId = ID;
 
-    AddAllMissing.Annotations.push_back({ID, Annotation});
+    unsigned StartLine = EditAndDiag.second->D.Range.start.line;
+    assert(StartLine < Lines.size() && "Out of bound line number!");
+
+    AddAllMissing.Annotations.push_back(
+        {ID,
+         ChangeAnnotation{
+             /*label=*/llvm::formatv("Add '{0}' for symbol '{1}'",
+                                     NewText.rtrim("\n"),
+                                     EditAndDiag.second->DInfo->Symbol),
+             /*needsConfirmation=*/true,
+             /*description=*/
+             llvm::formatv("Line {0}: '{1}'", StartLine, Lines[StartLine])}});
   }
   return AddAllMissing;
 }
 Fix fixAll(const Fix& RemoveAllUnused, const Fix& AddAllMissing) {
   Fix FixAll;
   FixAll.Message = "fix all includes";
 
   for (const auto &F : RemoveAllUnused.Edits)

 
 std::vector<Diag> generateIncludeCleanerDiagnostic(
     ParsedAST &AST, const IncludeCleanerFindings &Findings,
     llvm::StringRef Code) {
   std::vector<Diag> UnusedIncludes = generateUnusedIncludeDiagnostics(
       AST.tuPath(), Findings.UnusedIncludes, Code);
   std::optional<Fix> RemoveAllUnused;;
   if (UnusedIncludes.size() > 1)
-    RemoveAllUnused = removeAllUnusedIncludes(UnusedIncludes);
+    RemoveAllUnused = removeAllUnusedIncludes(UnusedIncludes, Code);
 
-  std::vector<Diag> MissingIncludeDiags = generateMissingIncludeDiagnostics(
+  auto MissingIncludeDiags = generateMissingIncludeDiagnostics(
       AST, Findings.MissingIncludes, Code);
   std::optional<Fix> AddAllMissing;
   if (MissingIncludeDiags.size() > 1)
-    AddAllMissing = addAllMissingIncludes(MissingIncludeDiags);
+    AddAllMissing = addAllMissingIncludes(MissingIncludeDiags, Code);
 
   std::optional<Fix> FixAll;
   if (RemoveAllUnused && AddAllMissing)
     FixAll = fixAll(*RemoveAllUnused, *AddAllMissing);
 
   auto AddBatchFix = [](const std::optional<Fix> &F, clang::clangd::Diag *Out) {
     if (!F) return;
     Out->Fixes.push_back(*F);
   };
   for (auto &Diag : MissingIncludeDiags) {
-    AddBatchFix(AddAllMissing, &Diag);
-    AddBatchFix(FixAll, &Diag);
+    AddBatchFix(AddAllMissing, &Diag.D);
+    AddBatchFix(FixAll, &Diag.D);
   }
   for (auto &Diag : UnusedIncludes) {
     AddBatchFix(RemoveAllUnused, &Diag);
     AddBatchFix(FixAll, &Diag);
   }
 
-  auto Result = std::move(MissingIncludeDiags);
-  llvm::move(UnusedIncludes,
-             std::back_inserter(Result));
+  std::vector<Diag> Result;
+  for (auto& Diag : MissingIncludeDiags)
+    Result.push_back(std::move(Diag.D));
+  llvm::move(UnusedIncludes, std::back_inserter(Result));
   return Result;
 }
 
 std::vector<Diag> issueIncludeCleanerDiagnostics(ParsedAST &AST,
                                                  llvm::StringRef Code) {
   // Interaction is only polished for C/CPP.
   if (AST.getLangOpts().ObjC)
     return {};

clang-tools-extra/clangd/test/include-cleaner-batch-fix.test

 # CHECK-NEXT:      "command": "clangd.applyFix",
 # CHECK-NEXT:      "title": "Apply fix: #include {{.*}}foo.h{{.*}}"
 # CHECK-NEXT:    },
 # CHECK-NEXT:    {
 # CHECK-NEXT:      "arguments": [
 # CHECK-NEXT:        {
 # CHECK-NEXT:          "changeAnnotations": {
 # CHECK-NEXT:            "AddAllMissingIncludes0": {
-# CHECK-NEXT:              "label": "",
+# CHECK-NEXT:              "description": "Line 2: {{.*}}",
+# CHECK-NEXT:              "label": "Add {{.*}}",
 # CHECK-NEXT:              "needsConfirmation": true
 # CHECK-NEXT:            },
 # CHECK-NEXT:            "AddAllMissingIncludes1": {
-# CHECK-NEXT:              "label": "",
+# CHECK-NEXT:              "description": "Line 2: {{.*}}",
+# CHECK-NEXT:              "label": "Add {{.*}}",
 # CHECK-NEXT:              "needsConfirmation": true
 # CHECK-NEXT:            }
 # CHECK-NEXT:          },
 # CHECK-NEXT:          "documentChanges": [
 # CHECK-NEXT:            {
 # CHECK-NEXT:              "edits": [
 # CHECK-NEXT:                {
 # CHECK-NEXT:                  "annotationId": "AddAllMissingIncludes0",

 # CHECK-NEXT:      "command": "clangd.applyFix",
 # CHECK-NEXT:      "title": "Apply fix: add all missing includes"
 # CHECK-NEXT:    },
 # CHECK-NEXT:    {
 # CHECK-NEXT:      "arguments": [
 # CHECK-NEXT:        {
 # CHECK-NEXT:          "changeAnnotations": {
 # CHECK-NEXT:            "AddAllMissingIncludes0": {
-# CHECK-NEXT:              "label": "",
+# CHECK-NEXT:              "description": "Line 2: ' Foo* foo; Bar* bar;'",
+# CHECK-NEXT:              "label": "Add {{.*}}bar.h{{.*}} for symbol 'Bar'",
 # CHECK-NEXT:              "needsConfirmation": true
 # CHECK-NEXT:            },
 # CHECK-NEXT:            "AddAllMissingIncludes1": {
-# CHECK-NEXT:              "label": "",
+# CHECK-NEXT:              "description": "Line 2: ' Foo* foo; Bar* bar;'",
+# CHECK-NEXT:              "label": "Add {{.*}}foo.h{{.*}} for symbol 'Foo'",
 # CHECK-NEXT:              "needsConfirmation": true
 # CHECK-NEXT:            },
 # CHECK-NEXT:            "RemoveAllUnusedIncludes0": {
-# CHECK-NEXT:              "label": "",
+# CHECK-NEXT:              "label": "Remove {{.*}}all1.h{{.*}}",
 # CHECK-NEXT:              "needsConfirmation": true
 # CHECK-NEXT:            },
 # CHECK-NEXT:            "RemoveAllUnusedIncludes1": {
-# CHECK-NEXT:              "label": "",
+# CHECK-NEXT:              "label": "Remove {{.*}}all2.h{{.*}}",
 # CHECK-NEXT:              "needsConfirmation": true
 # CHECK-NEXT:            }
 # CHECK-NEXT:          },
 # CHECK-NEXT:          "documentChanges": [
 # CHECK-NEXT:            {
 # CHECK-NEXT:              "edits": [
 # CHECK-NEXT:                {
 # CHECK-NEXT:                  "annotationId": "RemoveAllUnusedIncludes0",

 # CHECK-NEXT:      "command": "clangd.applyFix",
 # CHECK-NEXT:      "title": "Apply fix: remove #include directive"
 # CHECK-NEXT:    },
 # CHECK-NEXT:    {
 # CHECK-NEXT:      "arguments": [
 # CHECK-NEXT:        {
 # CHECK-NEXT:          "changeAnnotations": {
 # CHECK-NEXT:            "RemoveAllUnusedIncludes0": {
-# CHECK-NEXT:              "label": "",
+# CHECK-NEXT:              "label": "Remove {{.*}}",
 # CHECK-NEXT:              "needsConfirmation": true
 # CHECK-NEXT:            },
 # CHECK-NEXT:            "RemoveAllUnusedIncludes1": {
-# CHECK-NEXT:              "label": "",
+# CHECK-NEXT:              "label": "Remove {{.*}}",
 # CHECK-NEXT:              "needsConfirmation": true
 # CHECK-NEXT:            }
 # CHECK-NEXT:          },
 # CHECK-NEXT:          "documentChanges": [
 # CHECK-NEXT:            {
 # CHECK-NEXT:              "edits": [
 # CHECK-NEXT:                {
 # CHECK-NEXT:                  "annotationId": "RemoveAllUnusedIncludes0",

 # CHECK-NEXT:      "command": "clangd.applyFix",
 # CHECK-NEXT:      "title": "Apply fix: remove all unused includes"
 # CHECK-NEXT:    },
 # CHECK-NEXT:    {
 # CHECK-NEXT:      "arguments": [
 # CHECK-NEXT:        {
 # CHECK-NEXT:          "changeAnnotations": {
 # CHECK-NEXT:            "AddAllMissingIncludes0": {
-# CHECK-NEXT:              "label": "",
+# CHECK-NEXT:              "description": "Line 2: {{.*}}",
+# CHECK-NEXT:              "label": "Add {{.*}}",
 # CHECK-NEXT:              "needsConfirmation": true
 # CHECK-NEXT:            },
 # CHECK-NEXT:            "AddAllMissingIncludes1": {
-# CHECK-NEXT:              "label": "",
+# CHECK-NEXT:              "description": "Line 2: {{.*}}",
+# CHECK-NEXT:              "label": "Add {{.*}}",
 # CHECK-NEXT:              "needsConfirmation": true
 # CHECK-NEXT:            },
 # CHECK-NEXT:            "RemoveAllUnusedIncludes0": {
-# CHECK-NEXT:              "label": "",
+# CHECK-NEXT:              "label": "Remove {{.*}}",
 # CHECK-NEXT:              "needsConfirmation": true
 # CHECK-NEXT:            },
 # CHECK-NEXT:            "RemoveAllUnusedIncludes1": {
-# CHECK-NEXT:              "label": "",
+# CHECK-NEXT:              "label": "Remove {{.*}}",
 # CHECK-NEXT:              "needsConfirmation": true
 # CHECK-NEXT:            }
 # CHECK-NEXT:          },
 # CHECK-NEXT:          "documentChanges": [
 # CHECK-NEXT:            {
 # CHECK-NEXT:              "edits": [
 # CHECK-NEXT:                {
 # CHECK-NEXT:                  "annotationId": "RemoveAllUnusedIncludes0",