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

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:169
 
-std::vector<Diag> generateMissingIncludeDiagnostics(
+using MissingIncludeEdits = llvm::MapVector<include_cleaner::Header, TextEdit>;
+MissingIncludeEdits generateMissingIncludeEdits(
----------------
kadircet wrote:
> what do we gain by preserving the insertion order here exactly?
> 
> in `generateMissingIncludeDiagnostics`, we already traverse using the order of `MissingIncludeDiagInfo`s 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 ?
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.


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:330
+  llvm::DenseMap<include_cleaner::Header,
+                 llvm::SetVector<include_cleaner::Symbol>>
+      HeaderToSymbols;
----------------
kadircet wrote:
> 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
you're right, done.


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:353
+    ChangeAnnotationIdentifier ID = AddAllMissingID + std::to_string(I++);
+    AddAllMissing.Edits.push_back(Edit);
+    AddAllMissing.Edits.back().annotationId = ID;
----------------
kadircet wrote:
> we might generate multiple insertions to same location here e.g. insert `a.h` and `b.h` at the top of the file. [LSP](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textEditArray) says that order reported by the server will be preserved. hence we should actually make sure our edits are ordered by spelling.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149437/new/

https://reviews.llvm.org/D149437