[clangd] Provide excuses for bad code completions, based on diagnostics. C++ API only.

[sammccall]

Summary

In some circumstances we provide bad completions or no completions, because of
problems in the code. This can be puzzling and opaque to users.
If we can tell the user that this is the case and why, they'll be happy.
Experiments with go language servers have shown this to be a big win.

Two major problems:

• Sema doesn't provide the information we need
• LSP provides no protocol mechanims, and editors don't have UI for this

This patch attempts to guess the information, looking at diagnostics on the line.
Other heuristics are possible (e.g. using completion context). It's unclear how
useful or successful they will be. This is mostly a quick hack to test viability.

This is exposed as an extension of the C++ API only (not bound to LSP).
The idea is to test viability with a non-public editor that happens to have the
right UI already (and doesn't use LSP), and experiment with approaches.
If something fairly simple works well, we'll keep this and expose an LSP extension.
If it's not useful, we should drop this idea.

[ilya-biryukov]

Since we're showing the diagnostics in the editors anyway, how crucial do we think it is to actually add that to the procotol?
Having more concrete reasons for misbehaving completions sounds more useful, though, e.g. "cannot complete members of the incomplete type", etc.

Totally fine to have this in the C++ API for experiments, of course.

• LSP provides no protocol mechanims, and editors don't have UI for this

Have we tried returning an error in addition to the results? How do the clients behave in that case?

clangd/CodeComplete.cpp

674

It took some time to figure out what "excuse" means here.
Maybe be more straightforward in naming/comments about what this class does? I.e. finds a last diagnostic on the same line.
It's private to this file, so it's easy to rename if we'll want to expand the heuristics.

686

Maybe use a getLineNumber helper from the SourceManager instead?
It needs a FileID, but it shouldn't be hard to get one.

1040

Maybe replace an out parameter with returning a struct of two members?
Is there any use for calling code completion without providing an "excuse"?

[kadircet]

clangd/CodeComplete.cpp

700

There are also a lot of cases where we can't find an include file(usually due to confused compile commands) and fail miserably. Maybe in addition to checking current line, we could also check for lines that are starting with #include or diags of type diag::err_pp_file_not_found ?

[ilya-biryukov]

clangd/CodeComplete.cpp

700

Good point!
It's even more complicated than that: some of those missing files might be in preamble.
This will lead to preamble being incomplete. The problem with that is that DiagnosticsConsumer we create for completion will not see those errors (we have a separate consumer instance for preamble).
So we also need to a way to keep the reason why preamble is broken in the preamble itself.

[sammccall]

All these things are good ideas - I'm not addressing comments yet because I'm waiting for feedback from the team that's trying this out - maybe this needs patches, or maybe it'll never work.

clangd/ClangdLSPServer.cpp

 void ClangdLSPServer::onCompletion(const TextDocumentPositionParams &Params,
                                    Callback<CompletionList> Reply) {
   Server->codeComplete(Params.textDocument.uri.file(), Params.position, CCOpts,
                        Bind(
                            [this](decltype(Reply) Reply,
                                   llvm::Expected<CodeCompleteResult> List) {
                              if (!List)
                                return Reply(List.takeError());
+                             log("completion excuse: {0}", List->Excuse);
                              CompletionList LSPList;
                              LSPList.isIncomplete = List->HasMore;
                              for (const auto &R : List->Completions) {
                                CompletionItem C = R.render(CCOpts);
                                C.kind = adjustKindToCapability(
                                    C.kind, SupportedCompletionItemKinds);
                                LSPList.items.push_back(std::move(C));
                              }

clangd/CodeComplete.h

struct CodeCompletion {
   // Serialize this to an LSP completion item. This is a lossy operation.
   CompletionItem render(const CodeCompleteOptions &) const;
 };
 raw_ostream &operator<<(raw_ostream &, const CodeCompletion &);
 struct CodeCompleteResult {
   std::vector<CodeCompletion> Completions;
   bool HasMore = false;
   CodeCompletionContext::Kind Context = CodeCompletionContext::CCC_Other;
+
+  // Our best guess at why completions might be poor (human-readable string).
+  // Only set if we suspect completions are poor *and* we know why.
+  std::string Excuse;
 };
 raw_ostream &operator<<(raw_ostream &, const CodeCompleteResult &);
 
 /// A speculative and asynchronous fuzzy find index request (based on cached
 /// request) that can be sent before parsing sema. This would reduce completion
 /// latency if the speculation succeeds.
 struct SpeculativeFuzzyFind {
   /// A cached request from past code completions.

clangd/CodeComplete.cpp

static bool isBlacklistedMember(const NamedDecl &D) {
   auto NameKind = D.getDeclName().getNameKind();
   if (NameKind == DeclarationName::CXXOperatorName ||
       NameKind == DeclarationName::CXXLiteralOperatorName ||
       NameKind == DeclarationName::CXXConversionFunctionName)
     return true;
   return false;
 }
 
+// Tries to come up with a convincing excuse for our bad completion results,
+// based on the diagnostics near the completion point.
+class ExcuseMaker : public DiagnosticConsumer {
+  unsigned StartOfLine, Offset;
+  std::string &Excuse;
+  SmallString<256> ExcuseBuf;
+
+ public:
+   ExcuseMaker(StringRef Code, unsigned Offset, std::string &Excuse)
+       : Offset(Offset), Excuse(Excuse) {
+     assert(Offset <= Code.size());
+     for (StartOfLine = Offset;
+          StartOfLine > 0 && Code[StartOfLine - 1] != '\n'; --StartOfLine)
+       ;
+   }
+
+   void HandleDiagnostic(DiagnosticsEngine::Level Level,
+                         const clang::Diagnostic &Diag) override {
+     // We accept >= error diagnostics, before the cursor but on the same line.
+     if (Level < DiagnosticsEngine::Error || !Diag.hasSourceManager())
+       return;
+     auto& SM = Diag.getSourceManager();
+     auto Loc = SM.getDecomposedLoc(Diag.getLocation());
+     if (Loc.first != SM.getMainFileID())
+       return;
+     unsigned DiagLoc = Loc.second;
+     if (DiagLoc < StartOfLine || DiagLoc > Offset)
+       return;
+     ExcuseBuf.clear();
+     Diag.FormatDiagnostic(ExcuseBuf);
+   }
+
+   ~ExcuseMaker() { Excuse = ExcuseBuf.str(); }
+};
+
+
 // The CompletionRecorder captures Sema code-complete output, including context.
 // It filters out ignored results (but doesn't apply fuzzy-filtering yet).
 // It doesn't do scoring or conversion to CompletionItem yet, as we want to
 // merge with index results first.
 // Generally the fields and methods of this object should only be used from
 // within the callback.
 struct CompletionRecorder : public CodeCompleteConsumer {
   CompletionRecorder(const CodeCompleteOptions &Opts,
struct SemaCompleteInput {
   std::shared_ptr<PCHContainerOperations> PCHs;
 };
 
 // Invokes Sema code completion on a file.
 // If \p Includes is set, it will be updated based on the compiler invocation.
 bool semaCodeComplete(std::unique_ptr<CodeCompleteConsumer> Consumer,
                       const clang::CodeCompleteOptions &Options,
                       const SemaCompleteInput &Input,
-                      IncludeStructure *Includes = nullptr) {
+                      IncludeStructure *Includes = nullptr,
+                      std::string *Excuse = nullptr) {
   trace::Span Tracer("Sema completion");
   std::vector<const char *> ArgStrs;
   for (const auto &S : Input.Command.CommandLine)
     ArgStrs.push_back(S.c_str());
 
   if (Input.VFS->setCurrentWorkingDirectory(Input.Command.Directory)) {
     log("Couldn't set working directory");
     // We run parsing anyway, our lit-tests rely on results for non-existing
     // working dirs.
   }
 
+  auto Offset = positionToOffset(Input.Contents, Input.Pos);
+  if (!Offset) {
+    elog("Code completion position was invalid {0}", Offset.takeError());
+    return false;
+  }
+  std::unique_ptr<DiagnosticConsumer> DiagConsumer(
+      Excuse ? new ExcuseMaker(Input.Contents, *Offset, *Excuse)
+             : (DiagnosticConsumer *)new IgnoreDiagnostics());
   IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS = Input.VFS;
   if (Input.Preamble && Input.Preamble->StatCache)
     VFS = Input.Preamble->StatCache->getConsumingFS(std::move(VFS));
-  IgnoreDiagnostics DummyDiagsConsumer;
   auto CI = createInvocationFromCommandLine(
       ArgStrs,
       CompilerInstance::createDiagnostics(new DiagnosticOptions,
-                                          &DummyDiagsConsumer, false),
+                                          DiagConsumer.get(), false),
       VFS);
   if (!CI) {
     elog("Couldn't create CompilerInvocation");
     return false;
   }
   auto &FrontendOpts = CI->getFrontendOpts();
   FrontendOpts.DisableFree = false;
   FrontendOpts.SkipFunctionBodies = true;
   CI->getLangOpts()->CommentOpts.ParseAllComments = true;
   // Disable typo correction in Sema.
   CI->getLangOpts()->SpellChecking = false;
   // Setup code completion.
   FrontendOpts.CodeCompleteOpts = Options;
   FrontendOpts.CodeCompletionAt.FileName = Input.FileName;
-  auto Offset = positionToOffset(Input.Contents, Input.Pos);
-  if (!Offset) {
-    elog("Code completion position was invalid {0}", Offset.takeError());
-    return false;
-  }
   std::tie(FrontendOpts.CodeCompletionAt.Line,
            FrontendOpts.CodeCompletionAt.Column) =
       offsetToClangLineColumn(Input.Contents, *Offset);
 
   std::unique_ptr<llvm::MemoryBuffer> ContentsBuffer =
       llvm::MemoryBuffer::getMemBufferCopy(Input.Contents, Input.FileName);
   // The diagnostic options must be set before creating a CompilerInstance.
   CI->getDiagnosticOpts().IgnoreWarnings = true;
   // We reuse the preamble whether it's valid or not. This is a
   // correctness/performance tradeoff: building without a preamble is slow, and
   // completion is latency-sensitive.
   // However, if we're completing *inside* the preamble section of the draft,
   // overriding the preamble will break sema completion. Fortunately we can just
   // skip all includes in this case; these completions are really simple.
   bool CompletingInPreamble =
       ComputePreambleBounds(*CI->getLangOpts(), ContentsBuffer.get(), 0).Size >
       *Offset;
   // NOTE: we must call BeginSourceFile after prepareCompilerInstance. Otherwise
   // the remapped buffers do not get freed.
   auto Clang = prepareCompilerInstance(
       std::move(CI),
       (Input.Preamble && !CompletingInPreamble) ? &Input.Preamble->Preamble
                                                 : nullptr,
       std::move(ContentsBuffer), std::move(Input.PCHs), std::move(VFS),
-      DummyDiagsConsumer);
+      *DiagConsumer);
   Clang->getPreprocessorOpts().SingleFileParseMode = CompletingInPreamble;
   Clang->setCodeCompletionConsumer(Consumer.release());
 
   SyntaxOnlyAction Action;
   if (!Action.BeginSourceFile(*Clang, Clang->getFrontendOpts().Inputs[0])) {
     log("BeginSourceFile() failed when running codeComplete for {0}",
         Input.FileName);
     return false;
auto RecorderOwner = llvm::make_unique<CompletionRecorder>(Opts, [&]() {
       log("Code complete: sema context {0}, query scopes [{1}] (AnyScope={2})",
           getCompletionKindString(Recorder->CCContext.getKind()),
           llvm::join(QueryScopes.begin(), QueryScopes.end(), ","), AllScopes);
     });
 
     Recorder = RecorderOwner.get();
 
     semaCodeComplete(std::move(RecorderOwner), Opts.getClangCompleteOpts(),
-                     SemaCCInput, &Includes);
+                     SemaCCInput, &Includes, &Output.Excuse);
 
     SPAN_ATTACH(Tracer, "sema_results", NSema);
     SPAN_ATTACH(Tracer, "index_results", NIndex);
     SPAN_ATTACH(Tracer, "merged_results", NBoth);
     SPAN_ATTACH(Tracer, "returned_results", int64_t(Output.Completions.size()));
     SPAN_ATTACH(Tracer, "incomplete", Output.HasMore);
     log("Code complete: {0} results from Sema, {1} from Index, "
         "{2} matched, {3} returned{4}.",

unittests/clangd/CodeCompleteTests.cpp

)cpp",
                              {cls("nx::Clangd1"), cls("nx::Clangd2")}, Opts);
   // Although Clangd1 is from another namespace, Sema tells us it's in-scope and
   // needs no qualifier.
   EXPECT_THAT(Results.Completions,
               UnorderedElementsAre(AllOf(Qualifier(""), Named("Clangd1")),
                                    AllOf(Qualifier("nx::"), Named("Clangd2"))));
 }
 
+TEST(CompletionTest, Excuses) {
+  struct {
+    const char *Code;
+    const char *Excuse;
+  } Tests[] = {
+      {
+          R"cpp(
+                class X;
+                X* x;
+                int main() {
+                  x->^
+                }
+          )cpp",
+          "member access into incomplete type 'X'",
+      },
+      {
+          R"cpp(
+                // Error is on same line.
+                template <class> struct X;
+                X<int> x; ^
+          )cpp",
+          "implicit instantiation of undefined template 'X<int>'",
+      },
+      {
+          R"cpp(
+                // Error is on previous line.
+                template <class> struct X;
+                X<int> x;
+                ^
+          )cpp",
+          "",
+      },
+      {
+          R"cpp(
+                // Just a warning (declaration does not declare anything).
+                int; x^
+          )cpp",
+          "",
+      },
+  };
+  for (const auto &Test : Tests)
+    EXPECT_EQ(completions(Test.Code).Excuse, Test.Excuse) << Test.Code;
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang