[CodeComplete] an option to suppress endings for header completion

[richard9404]

Summary

Header completion in clang always apppends endings to the results, i.e. / for folders and " (or >) for files. But a language-neutral client (like many vim lsp plugins) does not know a subsequent completion is expected, while the auto-inserted / prevents the user from triggering one by manually typing a /. And "/> might collide with auto pair insertion in some clients.

#include "f"
          ^ completion triggered here
       => folder/  <- client is not aware that another completion is expected
       => file.h"  <- '"' collides with the ending '"' inserted by the client

This commit adds an option to suppress the endings, and adds / as a completion trigger in clangd, which makes header completion more similar to object member completion. The downside is that clangd will not be able to distinguish folders from files when setting the completion item kinds.

[kadircet]

clang-tools-extra/clangd/ClangdLSPServer.cpp

1088

i think instead of regex, it would be better to just perform a linear search to make sure line looks something like this(up to the Offset):

{whitespaces}#{whitespaces}include{whitespaces}{",<}{not {",>}}

IIRC, regex had a bad performance and we would like to trigger as quickly as possible. @sammccall might've more context about this one.

1090

i suppose this if should only enclose the StartOfLine = 0; statement, not the rest?

1092

as i mentioned above, the line doesn't have to start with #, and we cannot expect arbitrary stuff after include, we must make sure current point is inside quotes(") or brackets(<)

[sammccall]

Thanks for digging through this!

Adding an option doesn't avoid a decision about what the behavior will be. >99% of users will use the default for something as fine-grained as this.
So we need to find a default that works well for most people. And if it's good enough to be the default, it's probably good enough to be the only behavior - supporting multiple has high costs and benefits few people.

As for behavior, it seems there's a few different changes/aspects worth looking at:

Inserting trailing quotes after filenames

The advantages:

• don't have to type the quote, or remember whether it's > or "
• makes the file/directory distinction in the completion list obvious

The disadvantages:

• looks funny in completion list

(I haven't included issues if the line already has more text and a closing quote here, as we have to consider this case carefully either way)

My take here is that inserting the trailing quote is somewhat useful and people quickly get used to it. What do you think?

Inserting slashes after directories

The current workflow (with the slash inserted) is:

1. start typing directory name
2. select completion item
3. hit enter or similar
4. start typing filename, or hit ctrl-space etc

The proposed workflow is:

1. start typing directory name
2. select completion
3. hit slash

This is certainly a better workflow, especially you don't know the first character of the next path segment (well, at least for US keyboard layout users where slash is easy to reach).
My main concerns are a) may not be available in all editors, b) users may not know about it and use it.

1. start typing directory name
2. select completion
3. hit enter or similar
4. hit slash

which is pretty clumsy. One option would be to check whether the client has commitCharacter support. If so, omit the slash, and send slash as a commitCharacter. Otherwise include the slash... This obviously solves the editor but not the user problem.

Handling the rest of the line

We may not be completing at the end of the line. This happens if the user completes in the middle of the file path, or the editor has inserted a matching quote, etc.

Now we need to decide how much of the line we're going to replace. Various API contracts mean this should be the same for every completion item in a given context. We can re-insert text that already exists in the replaced range (e.g. closing quotes), but the cursor will be placed on the right of it.

Let's look at #include "a/b^c" where c might be empty. (There are other cases, but I think this is the most important).
Our main options are:

1. replace b, c, and " with the completion (and quote)
2. replace b and c with the completion
3. replace b with the completion

Option 3 is dominated by option 2 I think - it tends to produce nonsense tokens. So only 1 and 2 are worth considering.

Advantages of replacing the closing quote:

• is consistent with inserting closing quotes elsewhere, which seems like useful behavior elsewhere
• puts us at the end of the line after the completion, ready to hit enter

Disadvantages of replacing the closing quote:

• less jarring when completing a directory, as it doesn't eat the closing quote

Are there others?
This one seems like a wash to me, and I'm comfortable with the current behavior. In particular I don't see the conflict between inserting quotes and editors auto-pairing quotes that you mention. Can you elaborate?

---

I don't want to jump to a conclusion, but I would at least figure that the most important part of this change/issue with current behavior is the workflow around completing a directory name. Is that right?

[richard9404]

I think we should first think about what are participating in a modularised development environment. In terms of code completion, there are usually four layers:

• a "backend" which knows everything about the language, e.g. ClangSema
• a "frontend" which follows some well-known protocol(s), e.g. clangd
• an adapter which deals with all kinds of quirks of the frontend and editor and ensures everyone's happy, e.g. vscode-clangd or some magic configuration files for vim lsp plugins
• an editor which the user is accustomed to but knows little about the language, e.g. vscode or vim with some lsp plugin

Now, the completion behaviours we care about.

the default behaviour
I think ClangSema, as a language backend library, targets more about frontend developers rather than end users, and should provide rich but "pure" information to ensure flexibility. It doesn't seem necessary for it the decide which behaviour is more advantageous, especially for some end user oriented behaviours. In this specific case, I suppose it is more reasonable to either provide an option or return the raw info and let the upper levels to decide what to do.

the slashes
Different editors, under different user settings, have differences in completion behaviour, which are not always distinguishable from LSP's ClientCapabilities, e.g.:

• whether some key needs to be hit to commit an completion: If hitting Enter (or something else) is mandatory and commitCharacters are not supported, it should be better for clangd to include the ending slashes in the completion results; but if committing is implicit, it seems more reasonable to let the user hit / to trigger another completion, and users of these kinds of editors should be unlikely to hit Enter after selecting a completion item
• is it easy to forcibly trigger a completion: manually triggering completion is uncommon and most editors would bind it to some multi-key shortcuts, which makes the completion less fluent, or the user will need to <Backspace>->/ if another completion is needed immediately after a directory completion

I think clangd should choose a behaviour suitable for the most usual circumstance, while trying to support more cases via options (or init params) and leaving the decision to the adapters.

completion with trailing stuff
When the suffix of a completion item matches with the prefix of the content following the completion point, things get complicated. The backend and language server are able to decide the range that should be replaced, and everything works perfectly as long as the editor correctly support textEdit. But an editor without textEdit support will turn to insertText, and it has to decide whether the "duplication" should be eliminated, which is impossible for a language-natural editor to implement accurately. The editor will either keep them in all cases, or merge them in all cases, producing something like #include "dir/^.h" -> #include "dir/foo.h".h" or ret = Func1(ns::^); -> ret = Func1(ns::Func2();. This is indeed a tricky issue.

clang-tools-extra/clangd/ClangdLSPServer.cpp

 #include "clang/Tooling/Core/Replacement.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/Path.h"
+#include "llvm/Support/Regex.h"
 #include "llvm/Support/ScopedPrinter.h"
 
 namespace clang {
 namespace clangd {
 namespace {
 /// Transforms a tweak into a code action that would apply it if executed.
 /// EXPECTS: T.prepare() was called and returned true.
 CodeAction toCodeAction(const ClangdServer::TweakRef &T, const URIForFile &File,
llvm::json::Object Result{
              llvm::json::Object{
                  {"firstTriggerCharacter", "\n"},
                  {"moreTriggerCharacter", {}},
              }},
             {"codeActionProvider", true},
             {"completionProvider",
              llvm::json::Object{
                  {"resolveProvider", false},
-                 // We do extra checks for '>' and ':' in completion to only
-                 // trigger on '->' and '::'.
-                 {"triggerCharacters", {".", ">", ":"}},
+                 // We do extra checks for '>', ':' and '/' in completion to
+                 // only trigger on '->', '::' and '#include .../'.
+                 {"triggerCharacters", {".", ">", ":", "/"}},
              }},
             {"signatureHelpProvider",
              llvm::json::Object{
                  {"triggerCharacters", {"(", ","}},
              }},
             {"declarationProvider", true},
             {"definitionProvider", true},
             {"documentHighlightProvider", true},
std::vector<Fix> ClangdLSPServer::getFixes(llvm::StringRef File,
 
   return FixItsIter->second;
 }
 
 bool ClangdLSPServer::shouldRunCompletion(
     const CompletionParams &Params) const {
   llvm::StringRef Trigger = Params.context.triggerCharacter;
   if (Params.context.triggerKind != CompletionTriggerKind::TriggerCharacter ||
-      (Trigger != ">" && Trigger != ":"))
+      (Trigger != ">" && Trigger != ":" && Trigger != "/"))
     return true;
 
   auto Code = DraftMgr.getDraft(Params.textDocument.uri.file());
   if (!Code)
     return true; // completion code will log the error for untracked doc.
 
   // A completion request is sent when the user types '>' or ':', but we only
   // want to trigger on '->' and '::'. We check the preceeding character to make
bool ClangdLSPServer::shouldRunCompletion(
   }
   if (*Offset < 2)
     return false;
 
   if (Trigger == ">")
     return (*Code)[*Offset - 2] == '-'; // trigger only on '->'.
   if (Trigger == ":")
     return (*Code)[*Offset - 2] == ':'; // trigger only on '::'.
+
+  if (Trigger == "/") { // trigger only on '#include.../'
+    auto StartOfLine = Code->rfind('\n', *Offset);
+    if (StartOfLine == llvm::StringRef::npos) {
+      StartOfLine = 0;
+      llvm::Regex IncPattern("^#[[:space:]]*include.*");
+      return IncPattern.match(
+          llvm::StringRef(Code->data() + StartOfLine, *Offset - StartOfLine));
+    }
+  }
   assert(false && "unhandled trigger character");
   return true;
 }
 
 void ClangdLSPServer::onHighlightingsReady(
     PathRef File, std::vector<HighlightingToken> Highlightings) {
   publishSemanticHighlighting(
       {{URIForFile::canonicalize(File, /*TUPath=*/File)},

clang-tools-extra/clangd/CodeComplete.h

struct CodeCompleteOptions {
 
   /// Whether to include index symbols that are not defined in the scopes
   /// visible from the code completion point. This applies in contexts without
   /// explicit scope qualifiers.
   ///
   /// Such completions can insert scope qualifiers.
   bool AllScopes = false;
 
+  /// Whether to include the '/', '"' and '>' endings when completing included
+  /// files.
+  bool IncludedFileEndings = true;
+
   /// Whether to use the clang parser, or fallback to text-based completion
   /// (using identifiers in the current file and symbol indexes).
   enum CodeCompletionParse {
     /// Block until we can run the parser (e.g. preamble is built).
     /// Return an error if this fails.
     AlwaysParse,
     /// Run the parser if inputs (preamble) are ready.
     /// Otherwise, use text-based completion.

clang-tools-extra/clangd/CodeComplete.cpp

   // formatting of the comments.
   Result.IncludeBriefComments = false;
 
   // When an is used, Sema is responsible for completing the main file,
   // the index can provide results from the preamble.
   // Tell Sema not to deserialize the preamble to look for results.
   Result.LoadExternal = !Index;
   Result.IncludeFixIts = IncludeFixIts;
+  Result.IncludedFileEndings = IncludedFileEndings;
 
   return Result;
 }
 
 CompletionPrefix guessCompletionPrefix(llvm::StringRef Content,
                                        unsigned Offset) {
   assert(Offset <= Content.size());
   StringRef Rest = Content.take_front(Offset);

clang-tools-extra/clangd/test/initialize-params.test

 # RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
 # Test initialize request parameters with rootUri
 {"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootUri":"test:///workspace","capabilities":{},"trace":"off"}}
 #      CHECK:  "id": 0,
 # CHECK-NEXT:  "jsonrpc": "2.0",
 # CHECK-NEXT:  "result": {
 # CHECK-NEXT:    "capabilities": {
 # CHECK-NEXT:      "codeActionProvider": true,
 # CHECK-NEXT:      "completionProvider": {
 # CHECK-NEXT:        "resolveProvider": false,
 # CHECK-NEXT:        "triggerCharacters": [
 # CHECK-NEXT:          ".",
 # CHECK-NEXT:          ">",
-# CHECK-NEXT:          ":"
+# CHECK-NEXT:          ":",
+# CHECK-NEXT:          "/"
 # CHECK-NEXT:        ]
 # CHECK-NEXT:      },
 # CHECK-NEXT:      "declarationProvider": true,
 # CHECK-NEXT:      "definitionProvider": true,
 # CHECK-NEXT:      "documentFormattingProvider": true,
 # CHECK-NEXT:      "documentHighlightProvider": true,
 # CHECK-NEXT:      "documentOnTypeFormattingProvider": {
 # CHECK-NEXT:        "firstTriggerCharacter": "\n",

clang-tools-extra/clangd/tool/ClangdMain.cpp

static llvm::cl::list<std::string> QueryDriverGlobs(
     llvm::cl::CommaSeparated);
 
 static llvm::cl::list<std::string> TweakList(
     "tweaks",
     llvm::cl::desc(
         "Specify a list of Tweaks to enable (only for clangd developers)."),
     llvm::cl::Hidden, llvm::cl::CommaSeparated);
 
+static llvm::cl::opt<bool> CompleteIncludedFilesWithEndings(
+    "complete-included-files-with-endings",
+    llvm::cl::desc("Complete included files with endings, which are '/' for "
+                   "folders, and '\"' (or '>') for files (default=true)"),
+    llvm::cl::init(true));
+
 namespace {
 
 /// \brief Supports a test URI scheme with relaxed constraints for lit tests.
 /// The path in a test URI will be combined with a platform-specific fake
 /// directory to form an absolute path. For example, test:///a.cpp is resolved
 /// C:\clangd-test\a.cpp on Windows and /clangd-test/a.cpp on Unix.
 class TestScheme : public URIScheme {
 public:
int main(int argc, char *argv[]) {
   if (!HeaderInsertionDecorators) {
     CCOpts.IncludeIndicator.Insert.clear();
     CCOpts.IncludeIndicator.NoInsert.clear();
   }
   CCOpts.SpeculativeIndexRequest = Opts.StaticIndex;
   CCOpts.EnableFunctionArgSnippets = EnableFunctionArgSnippets;
   CCOpts.AllScopes = AllScopesCompletion;
   CCOpts.RunParser = CodeCompletionParse;
+  CCOpts.IncludedFileEndings = CompleteIncludedFilesWithEndings;
 
   RealFileSystemProvider FSProvider;
   // Initialize and run ClangdLSPServer.
   // Change stdin to binary to not lose \r\n on windows.
   llvm::sys::ChangeStdinToBinary();
 
   std::unique_ptr<Transport> TransportLayer;
   if (getenv("CLANGD_AS_XPC_SERVICE")) {

clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp

                              R"cpp(
         #include "^"
       )cpp");
   EXPECT_THAT(Results.Completions,
               AllOf(Has("sub/", CompletionItemKind::Folder),
                     Has("bar.h\"", CompletionItemKind::File)));
 }
 
+TEST(CompletionTest, IncludedCompletionWithoutEndings) {
+  MockFSProvider FS;
+  MockCompilationDatabase CDB;
+  std::string Subdir = testPath("sub");
+  std::string SearchDirArg = (Twine("-I") + Subdir).str();
+  CDB.ExtraClangFlags = {SearchDirArg.c_str()};
+  std::string BarHeader = testPath("sub/bar.h");
+  FS.Files[BarHeader] = "";
+  IgnoreDiagnostics DiagConsumer;
+  ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+  clangd::CodeCompleteOptions CCOpts;
+  CCOpts.IncludedFileEndings = false;
+  auto Results = completions(Server,
+                             R"cpp(
+        #include "^"
+      )cpp",
+                             {}, CCOpts);
+  EXPECT_THAT(Results.Completions, AllOf(Has("sub"), Has("bar.h")));
+}
+
 TEST(CompletionTest, NoCrashAtNonAlphaIncludeHeader) {
   auto Results = completions(
       R"cpp(
         #include "./^"
       )cpp");
   EXPECT_TRUE(Results.Completions.empty());
 }
 

clang/include/clang/Driver/CC1Options.td

 def no_code_completion_globals : Flag<["-"], "no-code-completion-globals">,
   HelpText<"Do not include global declarations in code-completion results.">;
 def no_code_completion_ns_level_decls : Flag<["-"], "no-code-completion-ns-level-decls">,
   HelpText<"Do not include declarations inside namespaces (incl. global namespace) in the code-completion results.">;
 def code_completion_brief_comments : Flag<["-"], "code-completion-brief-comments">,
   HelpText<"Include brief documentation comments in code-completion results.">;
 def code_completion_with_fixits : Flag<["-"], "code-completion-with-fixits">,
   HelpText<"Include code completion results which require small fix-its.">;
+def no_code_completion_included_file_endings : Flag<["-"], "no-code-completion-included-file-endings">,
+  HelpText<"Do not include '/', '\"' and '>' endings when completing included files.">;
 def disable_free : Flag<["-"], "disable-free">,
   HelpText<"Disable freeing of memory on exit">;
 def discard_value_names : Flag<["-"], "discard-value-names">,
   HelpText<"Discard value names in LLVM IR">;
 def load : Separate<["-"], "load">, MetaVarName<"<dsopath>">,
   HelpText<"Load the named plugin (dynamic shared object)">;
 def plugin : Separate<["-"], "plugin">, MetaVarName<"<name>">,
   HelpText<"Use the named plugin action instead of the default action (use \"help\" to list available options)">;

clang/include/clang/Sema/CodeCompleteConsumer.h

   bool includeFixIts() const { return CodeCompleteOpts.IncludeFixIts; }
 
   /// Hint whether to load data from the external AST in order to provide
   /// full results. If false, declarations from the preamble may be omitted.
   bool loadExternal() const {
     return CodeCompleteOpts.LoadExternal;
   }
 
+  /// Whether to include the '/', '"' and '>' endings when completing included
+  /// files.
+  bool includedFileEndings() const {
+    return CodeCompleteOpts.IncludedFileEndings;
+  }
+
   /// Deregisters and destroys this code-completion consumer.
   virtual ~CodeCompleteConsumer();
 
   /// \name Code-completion filtering
   /// Check if the result should be filtered out.
   virtual bool isResultFilteredOut(StringRef Filter,
                                    CodeCompletionResult Results) {
     return false;

clang/include/clang/Sema/CodeCompleteOptions.h

public:
   /// If false, namespace-level declarations and macros from the preamble may be
   /// omitted.
   unsigned LoadExternal : 1;
 
   /// Include results after corrections (small fix-its), e.g. change '.' to '->'
   /// on member access, etc.
   unsigned IncludeFixIts : 1;
 
+  /// Whether to include the '/', '"' and '>' endings when completing included
+  /// files.
+  unsigned IncludedFileEndings : 1;
+
   CodeCompleteOptions()
       : IncludeMacros(0), IncludeCodePatterns(0), IncludeGlobals(1),
-        IncludeNamespaceLevelDecls(1), IncludeBriefComments(0),
-        LoadExternal(1), IncludeFixIts(0) {}
+        IncludeNamespaceLevelDecls(1), IncludeBriefComments(0), LoadExternal(1),
+        IncludeFixIts(0), IncludedFileEndings(1) {}
 };
 
 } // namespace clang
 
 #endif
 

clang/lib/Frontend/CompilerInvocation.cpp

   Opts.CodeCompleteOpts.IncludeGlobals
     = !Args.hasArg(OPT_no_code_completion_globals);
   Opts.CodeCompleteOpts.IncludeNamespaceLevelDecls
     = !Args.hasArg(OPT_no_code_completion_ns_level_decls);
   Opts.CodeCompleteOpts.IncludeBriefComments
     = Args.hasArg(OPT_code_completion_brief_comments);
   Opts.CodeCompleteOpts.IncludeFixIts
     = Args.hasArg(OPT_code_completion_with_fixits);
+  Opts.CodeCompleteOpts.IncludedFileEndings
+    = !Args.hasArg(OPT_no_code_completion_included_file_endings);
 
   Opts.OverrideRecordLayoutsFile
     = Args.getLastArgValue(OPT_foverride_record_layout_EQ);
   Opts.AuxTriple = Args.getLastArgValue(OPT_aux_triple);
   Opts.StatsFile = Args.getLastArgValue(OPT_stats_file);
 
   if (const Arg *A = Args.getLastArg(OPT_arcmt_check,
                                      OPT_arcmt_modify,

clang/lib/Sema/SemaCodeComplete.cpp

                         CodeCompleter->getCodeCompletionTUInfo(),
                         CodeCompletionContext::CCC_IncludedFile);
   llvm::DenseSet<StringRef> SeenResults; // To deduplicate results.
 
   // Helper: adds one file or directory completion result.
   auto AddCompletion = [&](StringRef Filename, bool IsDirectory) {
     SmallString<64> TypedChunk = Filename;
     // Directory completion is up to the slash, e.g. <sys/
-    TypedChunk.push_back(IsDirectory ? '/' : Angled ? '>' : '"');
+    if (CodeCompleter->includedFileEndings())
+      TypedChunk.push_back(IsDirectory ? '/' : Angled ? '>' : '"');
     auto R = SeenResults.insert(TypedChunk);
     if (R.second) { // New completion
       const char *InternedTyped = Results.getAllocator().CopyString(TypedChunk);
       *R.first = InternedTyped; // Avoid dangling StringRef.
       CodeCompletionBuilder Builder(CodeCompleter->getAllocator(),
                                     CodeCompleter->getCodeCompletionTUInfo());
       Builder.AddTypedTextChunk(InternedTyped);
       // The result is a "Pattern", which is pretty opaque.

clang/test/CodeCompletion/included-files-endings.cpp

+// RUN: rm -rf %t && mkdir %t && cp %s %t/main.cc && mkdir %t/foo && mkdir %t/sys
+// RUN: touch %t/foobar.h && touch %t/foo/bar.h && touch %t/sys/foosys.h
+
+// With endings.
+#include "foobar.h"
+// RUN: %clang -fsyntax-only -isystem %t/sys -Xclang -code-completion-at=%t/main.cc:5:13 %t/main.cc | FileCheck -check-prefix=CHECK-1 %s
+// CHECK-1: foo/
+// CHECK-1: foobar.h"
+
+// System headers with endings.
+#include <foosys.h>
+// RUN: %clang -fsyntax-only -isystem %t/sys -Xclang -code-completion-at=%t/main.cc:11:14 %t/main.cc | FileCheck -check-prefix=CHECK-2 %s
+// CHECK-2: sys.h>
+
+// Without endings.
+#include "foobar.h"
+// RUN: %clang -fsyntax-only -isystem %t/sys -Xclang -code-completion-at=%t/main.cc:16:13 -Xclang -no-code-completion-included-file-endings %t/main.cc | FileCheck -check-prefix=CHECK-3 %s
+// CHECK-3: foo
+// CHECK-3: foobar.h
+
+// System headers without endings.
+#include <foosys.h>
+// RUN: %clang -fsyntax-only -isystem %t/sys -Xclang -code-completion-at=%t/main.cc:22:14 -Xclang -no-code-completion-included-file-endings %t/main.cc | FileCheck -check-prefix=CHECK-4 %s
+// CHECK-4: sys.h