[clangd] Add error handling (elog) in code completion.

[njames93]

clang-tools-extra/clangd/CodeComplete.cpp

187–188

This can stay as 1 line, The condition variable in an if has lifetime throughout the then/else branch.

[sammccall]

clang-tools-extra/clangd/CodeComplete.cpp

192

I think this is too noisy.

We can hit this in "normal" cases of certain projects.
For better or worse, we don't currently filter out index results where we know the responsible header and can't include it (because it's not under any include path entry). In these cases toHeaderFile() returns an error.

Since this can happen in the normal case for *each* completion candidate (default 100), and completion requests are very frequent, this could dominate log output in affected projects.

It *might* be OK at vlog? I get the desire to not silence errors here. I think the question is what are the error cases we're *trying* to call out loudly. Maybe we can separate out the "shouldn't happen" vs the "fairly expected" cases.
(Even then there's the prospect that this is either going to not fire or fire dozens of times, which is a little sad)

[adamcz]

address comments

[adamcz]

clang-tools-extra/clangd/CodeComplete.cpp

192

Originally I didn't even log here. We do this expansion at other places and they may log already, I don't think it's critical to log here. Kadir pointed out error needs to be handled or it will assert()-fail (that one place returns Expected rather than Optional, I missed that), which is the motivation for this change.

I made it vlog, but if you think it's too spammy I can remove it and just ignore the error (by this time explicitly, so it doesn't crash). I don't think we'll see these errors often - this only happens when we failed to parse the URI or recognize the scheme, which basically can only happen with remote index and some kind of version skew or corrupt data, so I'm fine with vlog() I think.

[sammccall]

clang-tools-extra/clangd/CodeComplete.cpp

192

Oh wow, I totally missed that the Expected was unhandled before!

Yeah let's go with vlog.
(If you like, downgrading the log() to vlog() for the callsite in CodeComplete.cpp seems like a good idea)

clang-tools-extra/clangd/CodeComplete.cpp

size_t overloadSet(const CodeCompleteOptions &Opts, llvm::StringRef FileName,
                      IncludeInserter *Inserter) const {
     if (!Opts.BundleOverloads.getValueOr(false))
       return 0;
 
     // Depending on the index implementation, we can see different header
     // strings (literal or URI) mapping to the same file. We still want to
     // bundle those, so we must resolve the header to be included here.
     std::string HeaderForHash;
-    if (Inserter)
-      if (auto Header = headerToInsertIfAllowed(Opts))
-        if (auto HeaderFile = toHeaderFile(*Header, FileName))
+    if (Inserter) {
+      if (auto Header = headerToInsertIfAllowed(Opts)) {
+        if (auto HeaderFile = toHeaderFile(*Header, FileName)) {
           if (auto Spelled =
                   Inserter->calculateIncludePath(*HeaderFile, FileName))
             HeaderForHash = *Spelled;
+        } else {
+          vlog("Code completion header path manipulation failed {0}",
+               HeaderFile.takeError());
+        }
+      }
+    }
 
     llvm::SmallString<256> Scratch;
     if (IndexResult) {
       switch (IndexResult->SymInfo.Kind) {
       case index::SymbolKind::ClassMethod:
       case index::SymbolKind::InstanceMethod:
       case index::SymbolKind::StaticMethod:
 #ifndef NDEBUG