[PATCH] D56903: [clangd] Suggest adding missing includes for incomplete type diagnostics.

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

In D56903#1365487 <https://reviews.llvm.org/D56903#1365487>, @sammccall wrote:

> This looks pretty good! My main concern is latency (and variability of latency) in practice.
> Particularly:
>
> - we should avoid paying for fuzzyfind and fetching hundreds of results when we want exact-match
> - limit the damage in degenerate cases: should we limit to e.g. 5 index queries per TU?
>
>   Actually, now that I think about it - if we can see a forward declaration, can't we do a lookup() instead of a fuzzyFind? Is the problem that this doesn't generalize to the no-declaration case (where it looks like a typo)?


I switched to use lookup requests for incomplete type diagnostics. And yes, the typo errors need to use FuzzyFind or lookup by names as no USR is available.

> If we had an operation that looked like lookup() but worked by qname, would we design the feature the same way?
>  In particular, would we want to batch the requests to the index (lookup takes a set of IDs)? This would affect the code structure a bit. But it would make the feature cost basically O(1) instead of O(errs)...

As chatted offline, we decided to leave the batching optimization as a TODO in this patch. See inline comment for more detailed response regarding performance.



================
Comment at: clangd/ClangdUnit.h:91
+        IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS,
+        const SymbolIndex *Index);
 
----------------
sammccall wrote:
> Index is a reasonable (if surprising) param here, but I think we need to explicitly enable the include-fixing behavior. Even the very minimal hard-coded list of clang-tidy checks caused issues :-( And this is going to result in new network RPCs in some configs.
> 
> I'd suggest keeping the Index param, and wrapping the "use include fixer?" policy along with the clang-tidy options in D55256 as some sort of "diagnostic options" struct. (Though be wary of name clash with the one in Diagnostics.h, sigh).
How about `ParseOptions`?


================
Comment at: clangd/IncludeFixer.cpp:39
+                                   const clang::Diagnostic &Info) const {
+  if (isIncompleteTypeDiag(Info.getID())) {
+    // Incomplete type diagnostics should have a QualType argument for the
----------------
sammccall wrote:
> can you add a trace to this function so we know what the impact on latency is?
Added a tracer in `fixInCompleteType` to avoid noise from unsupported diagnostics.


================
Comment at: clangd/IncludeFixer.cpp:66
+  vlog("Trying to fix include for incomplete type {0}", IncompleteType);
+  FuzzyFindRequest Req;
+  Req.AnyScope = false;
----------------
ilya-biryukov wrote:
> sammccall wrote:
> > limit?
> Why do we use fuzzyFind and not `lookup` here?
> Are there cases when we can't construct `SymbolID` for the `TagDecl`?
Switched to use lookup. The typo diagnostics (i.e. undefined symbol) can't use lookup as there is no declaration, but we can definitely use lookup for incomplete types in this patch.


================
Comment at: clangd/IncludeFixer.cpp:74
+  llvm::Optional<Symbol> Matched;
+  Index.fuzzyFind(Req, [&](const Symbol &Sym) {
+    // FIXME: support multiple matched symbols.
----------------
sammccall wrote:
> so issuing a bunch of fuzzy finds in sequence is clearly not ideal from a performance standpoint.
> Any ideas on what we might do better, or how we can limit the worst case?
> (not sure we need to do any better in this patch, but might be worth comments)
As you suggested, batching requests from all diagnostics would definitely help. SymbolIndex already supports batch lookup by IDs, but we would need to extend SymbolIndex to support lookup by Names for the typo errors that we are handling in D57021. In order to batch index requests from all diagnostics, the existing logic around handling and storing diagnostics might also need refactoring. I added a TODO for this. We can revisit if the performance turned out to be a big problem.

Another thing we could do is adding cache for index results. For example, for the code copy-paste case, one incomplete/undefined symbol can cause multiple errors. Caching should save us some unnecessary index queries.

For this patch, I added a limit on the number of index requests in IncludeFixer (5 for now), which would prevent us from sending too many index requests when building AST.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56903