[clangd] Add -header-insertion=never flag to disable include insertion in code completion

[sammccall]

Summary

One clear use case: use with an editor that reacts poorly to edits above the cursor.

[ioeric]

lg!

clangd/CodeComplete.cpp

1176

Why optional? In the current implementation, it's always initialized.

[sammccall]

clangd/CodeComplete.cpp

1176

Oops, this was left-over from a previous iteration.

[phosek]

Our Mac builders have started failing after this change with the following:

[3145/3502] Building CXX object tools/clang/tools/extra/clangd/tool/CMakeFiles/clangd.dir/ClangdMain.cpp.o
FAILED: tools/clang/tools/extra/clangd/tool/CMakeFiles/clangd.dir/ClangdMain.cpp.o 
/b/s/w/ir/cache/goma/client/gomacc /b/s/w/ir/k/cipd/bin/clang++  -DCLANG_VENDOR="\"Fuchsia \"" -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Itools/clang/tools/extra/clangd/tool -I/b/s/w/ir/k/llvm-project/clang-tools-extra/clangd/tool -I/b/s/w/ir/k/llvm-project/clang/include -Itools/clang/include -I/usr/include/libxml2 -Iinclude -I/b/s/w/ir/k/llvm-project/llvm/include -I/b/s/w/ir/k/llvm-project/clang-tools-extra/clangd/tool/.. -Itools/clang/tools/extra/clangd/tool/.. -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -std=c++11 -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion -fdiagnostics-color -fno-common -Woverloaded-virtual -Wno-nested-anon-types -O3 -gline-tables-only   -UNDEBUG  -fno-exceptions -fno-rtti -MD -MT tools/clang/tools/extra/clangd/tool/CMakeFiles/clangd.dir/ClangdMain.cpp.o -MF tools/clang/tools/extra/clangd/tool/CMakeFiles/clangd.dir/ClangdMain.cpp.o.d -o tools/clang/tools/extra/clangd/tool/CMakeFiles/clangd.dir/ClangdMain.cpp.o -c /b/s/w/ir/k/llvm-project/clang-tools-extra/clangd/tool/ClangdMain.cpp
/b/s/w/ir/k/llvm-project/clang-tools-extra/clangd/tool/ClangdMain.cpp:474:22: error: use of undeclared identifier 'newXPCTransport'
    TransportLayer = newXPCTransport();
                     ^
1 error generated.

I don't understand why since this change hasn't touched the failing line.

[MaskRay]

In D60409#1461579, @phosek wrote:

Our Mac builders have started failing after this change with the following:

[3145/3502] Building CXX object tools/clang/tools/extra/clangd/tool/CMakeFiles/clangd.dir/ClangdMain.cpp.o
FAILED: tools/clang/tools/extra/clangd/tool/CMakeFiles/clangd.dir/ClangdMain.cpp.o 
/b/s/w/ir/cache/goma/client/gomacc /b/s/w/ir/k/cipd/bin/clang++  -DCLANG_VENDOR="\"Fuchsia \"" -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Itools/clang/tools/extra/clangd/tool -I/b/s/w/ir/k/llvm-project/clang-tools-extra/clangd/tool -I/b/s/w/ir/k/llvm-project/clang/include -Itools/clang/include -I/usr/include/libxml2 -Iinclude -I/b/s/w/ir/k/llvm-project/llvm/include -I/b/s/w/ir/k/llvm-project/clang-tools-extra/clangd/tool/.. -Itools/clang/tools/extra/clangd/tool/.. -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -std=c++11 -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion -fdiagnostics-color -fno-common -Woverloaded-virtual -Wno-nested-anon-types -O3 -gline-tables-only   -UNDEBUG  -fno-exceptions -fno-rtti -MD -MT tools/clang/tools/extra/clangd/tool/CMakeFiles/clangd.dir/ClangdMain.cpp.o -MF tools/clang/tools/extra/clangd/tool/CMakeFiles/clangd.dir/ClangdMain.cpp.o.d -o tools/clang/tools/extra/clangd/tool/CMakeFiles/clangd.dir/ClangdMain.cpp.o -c /b/s/w/ir/k/llvm-project/clang-tools-extra/clangd/tool/ClangdMain.cpp
/b/s/w/ir/k/llvm-project/clang-tools-extra/clangd/tool/ClangdMain.cpp:474:22: error: use of undeclared identifier 'newXPCTransport'
    TransportLayer = newXPCTransport();
                     ^
1 error generated.

I don't understand why since this change hasn't touched the failing line.

It has been fixed by rCTE358103

clangd/CodeComplete.h

struct CodeCompleteOptions {
 
   /// Combine overloads into a single completion item where possible.
   bool BundleOverloads = false;
 
   /// Limit the number of results returned (0 means no limit).
   /// If more results are available, we set CompletionList.isIncomplete.
   size_t Limit = 0;
 
+  enum IncludeInsertion {
+    IWYU,
+    NeverInsert,
+  } InsertIncludes = IncludeInsertion::IWYU;
+
   /// A visual indicator to prepend to the completion label to indicate whether
   /// completion result would trigger an #include insertion or not.
   struct IncludeInsertionIndicator {
     std::string Insert = "•";
     std::string NoInsert = " ";
   } IncludeIndicator;
 
   /// Expose origins of completion items in the label (for debugging).

clangd/CodeComplete.cpp

struct CompletionCandidate {
   llvm::StringRef Name; // Used for filtering and sorting.
   // We may have a result from Sema, from the index, or both.
   const CodeCompletionResult *SemaResult = nullptr;
   const Symbol *IndexResult = nullptr;
   llvm::SmallVector<llvm::StringRef, 1> RankedIncludeHeaders;
 
   // Returns a token identifying the overload set this is part of.
   // 0 indicates it's not part of any overload set.
-  size_t overloadSet() const {
+  size_t overloadSet(const CodeCompleteOptions &Opts) const {
+    if (!Opts.BundleOverloads)
+      return 0;
     llvm::SmallString<256> Scratch;
     if (IndexResult) {
       switch (IndexResult->SymInfo.Kind) {
       case index::SymbolKind::ClassMethod:
       case index::SymbolKind::InstanceMethod:
       case index::SymbolKind::StaticMethod:
 #ifndef NDEBUG
         llvm_unreachable("Don't expect members from index in code completion");
 #else
         LLVM_FALLTHROUGH;
 #endif
       case index::SymbolKind::Function:
         // We can't group overloads together that need different #includes.
         // This could break #include insertion.
         return llvm::hash_combine(
             (IndexResult->Scope + IndexResult->Name).toStringRef(Scratch),
-            headerToInsertIfAllowed().getValueOr(""));
+            headerToInsertIfAllowed(Opts).getValueOr(""));
       default:
         return 0;
       }
     }
     assert(SemaResult);
     // We need to make sure we're consistent with the IndexResult case!
     const NamedDecl *D = SemaResult->Declaration;
     if (!D || !D->isFunctionOrFunctionTemplate())
       return 0;
     {
       llvm::raw_svector_ostream OS(Scratch);
       D->printQualifiedName(OS);
     }
     return llvm::hash_combine(Scratch,
-                              headerToInsertIfAllowed().getValueOr(""));
+                              headerToInsertIfAllowed(Opts).getValueOr(""));
   }
 
   // The best header to include if include insertion is allowed.
-  llvm::Optional<llvm::StringRef> headerToInsertIfAllowed() const {
-    if (RankedIncludeHeaders.empty())
+  llvm::Optional<llvm::StringRef>
+  headerToInsertIfAllowed(const CodeCompleteOptions &Opts) const {
+    if (Opts.InsertIncludes == CodeCompleteOptions::NeverInsert ||
+        RankedIncludeHeaders.empty())
       return None;
     if (SemaResult && SemaResult->Declaration) {
       // Avoid inserting new #include if the declaration is found in the current
       // file e.g. the symbol is forward declared.
       auto &SM = SemaResult->Declaration->getASTContext().getSourceManager();
       for (const Decl *RD : SemaResult->Declaration->redecls())
         if (SM.isInMainFile(SM.getExpansionLoc(RD->getBeginLoc())))
           return None;
auto Inserted = [&](llvm::StringRef Header)
         return ResolvedDeclaring.takeError();
       auto ResolvedInserted = toHeaderFile(Header, FileName);
       if (!ResolvedInserted)
         return ResolvedInserted.takeError();
       return std::make_pair(
           Includes.calculateIncludePath(*ResolvedDeclaring, *ResolvedInserted),
           Includes.shouldInsertInclude(*ResolvedDeclaring, *ResolvedInserted));
     };
-    bool ShouldInsert = C.headerToInsertIfAllowed().hasValue();
+    bool ShouldInsert = C.headerToInsertIfAllowed(Opts).hasValue();
     // Calculate include paths and edits for all possible headers.
     for (const auto &Inc : C.RankedIncludeHeaders) {
       if (auto ToInclude = Inserted(Inc)) {
         CodeCompletion::IncludeCandidate Include;
         Include.Header = ToInclude->first;
         if (ToInclude->second && ShouldInsert)
           Include.Insertion = Includes.insert(ToInclude->first);
         Completion.Includes.push_back(std::move(Include));
class CodeCompleteFlow {
   std::vector<std::string> QueryScopes; // Initialized once Sema runs.
   // Initialized once QueryScopes is initialized, if there are scopes.
   llvm::Optional<ScopeDistance> ScopeProximity;
   llvm::Optional<OpaqueType> PreferredType; // Initialized once Sema runs.
   // Whether to query symbols from any scope. Initialized once Sema runs.
   bool AllScopes = false;
   // Include-insertion and proximity scoring rely on the include structure.
   // This is available after Sema has run.
-  llvm::Optional<IncludeInserter> Inserter;  // Available during runWithSema.
+  llvm::Optional<IncludeInserter> Inserter;  // Optional during runWithSema.
   llvm::Optional<URIDistance> FileProximity; // Initialized once Sema runs.
   /// Speculative request based on the cached request and the filter text before
   /// the cursor.
   /// Initialized right before sema run. This is only set if `SpecFuzzyFind` is
   /// set and contains a cached request.
   llvm::Optional<FuzzyFindRequest> SpecReq;
 
 public:
mergeResults(const std::vector<CodeCompletionResult> &SemaResults,
     auto AddToBundles = [&](const CodeCompletionResult *SemaResult,
                             const Symbol *IndexResult) {
       CompletionCandidate C;
       C.SemaResult = SemaResult;
       C.IndexResult = IndexResult;
       if (C.IndexResult)
         C.RankedIncludeHeaders = getRankedIncludes(*C.IndexResult);
       C.Name = IndexResult ? IndexResult->Name : Recorder->getName(*SemaResult);
-      if (auto OverloadSet = Opts.BundleOverloads ? C.overloadSet() : 0) {
+      if (auto OverloadSet = C.overloadSet(Opts)) {
         auto Ret = BundleLookup.try_emplace(OverloadSet, Bundles.size());
         if (Ret.second)
           Bundles.emplace_back();
         Bundles[Ret.first->second].push_back(std::move(C));
       } else {
         Bundles.emplace_back();
         Bundles.back().push_back(std::move(C));
       }

clangd/tool/ClangdMain.cpp

 //===--- ClangdMain.cpp - clangd server loop ------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
-#include "Features.inc"
 #include "ClangdLSPServer.h"
+#include "CodeComplete.h"
+#include "Features.inc"
 #include "Path.h"
 #include "Protocol.h"
 #include "Trace.h"
 #include "Transport.h"
 #include "index/Serialization.h"
 #include "clang/Basic/Version.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/Support/CommandLine.h"
llvm::cl::desc(
         "namespaces) visible from the code completion point. Such completions "
         "can insert scope qualifiers."),
     llvm::cl::init(true));
 
 static llvm::cl::opt<bool> ShowOrigins(
     "debug-origin", llvm::cl::desc("Show origins of completion items"),
     llvm::cl::init(CodeCompleteOptions().ShowOrigins), llvm::cl::Hidden);
 
+static llvm::cl::opt<CodeCompleteOptions::IncludeInsertion> HeaderInsertion(
+    "header-insertion",
+    llvm::cl::desc("Add #include directives when accepting code completions"),
+    llvm::cl::init(CodeCompleteOptions().InsertIncludes),
+    llvm::cl::values(
+        clEnumValN(CodeCompleteOptions::IWYU, "iwyu",
+                   "Include what you use. "
+                   "Insert the owning header for top-level symbols, unless the "
+                   "header is already directly included or the symbol is "
+                   "forward-declared."),
+        clEnumValN(
+            CodeCompleteOptions::NeverInsert, "never",
+            "Never insert #include directives as part of code completion")));
+
 static llvm::cl::opt<bool> HeaderInsertionDecorators(
     "header-insertion-decorators",
     llvm::cl::desc("Prepend a circular dot or space before the completion "
                    "label, depending on whether "
                    "an include line will be inserted or not."),
     llvm::cl::init(true));
 
 static llvm::cl::opt<Path> IndexFile(
int main(int argc, char *argv[]) {
   Opts.StaticIndex = StaticIdx.get();
   Opts.AsyncThreadsCount = WorkerThreadsCount;
 
   clangd::CodeCompleteOptions CCOpts;
   CCOpts.IncludeIneligibleResults = IncludeIneligibleResults;
   CCOpts.Limit = LimitResults;
   CCOpts.BundleOverloads = CompletionStyle != Detailed;
   CCOpts.ShowOrigins = ShowOrigins;
+  CCOpts.InsertIncludes = HeaderInsertion;
   if (!HeaderInsertionDecorators) {
     CCOpts.IncludeIndicator.Insert.clear();
     CCOpts.IncludeIndicator.NoInsert.clear();
   }
   CCOpts.SpeculativeIndexRequest = Opts.StaticIndex;
   CCOpts.EnableFunctionArgSnippets = EnableFunctionArgSnippets;
   CCOpts.AllScopes = AllScopesCompletion;
 

unittests/clangd/CodeCompleteTests.cpp

 
 namespace clang {
 namespace clangd {
 
 namespace {
 using ::llvm::Failed;
 using ::testing::AllOf;
 using ::testing::Contains;
-using ::testing::Each;
 using ::testing::ElementsAre;
 using ::testing::Field;
 using ::testing::HasSubstr;
 using ::testing::IsEmpty;
 using ::testing::Not;
 using ::testing::UnorderedElementsAre;
 
 class IgnoreDiagnostics : public DiagnosticsConsumer {
TEST(CompletionTest, IncludeInsertionPreprocessorIntegrationTests) {
   // Shoten include path based on search dirctory and insert.
   auto Results = completions(Server,
                              R"cpp(
           int main() { ns::^ }
       )cpp",
                              {Sym});
   EXPECT_THAT(Results.Completions,
               ElementsAre(AllOf(Named("X"), InsertInclude("\"bar.h\""))));
+  // Can be disabled via option.
+  CodeCompleteOptions NoInsertion;
+  NoInsertion.InsertIncludes = CodeCompleteOptions::NeverInsert;
+  Results = completions(Server,
+                             R"cpp(
+          int main() { ns::^ }
+      )cpp",
+                             {Sym}, NoInsertion);
+  EXPECT_THAT(Results.Completions,
+              ElementsAre(AllOf(Named("X"), Not(InsertInclude()))));
   // Duplicate based on inclusions in preamble.
   Results = completions(Server,
                         R"cpp(
           #include "sub/bar.h"  // not shortest, so should only match resolved.
           int main() { ns::^ }
       )cpp",
                         {Sym});
   EXPECT_THAT(Results.Completions, ElementsAre(AllOf(Named("X"), Labeled("X"),