[clangd] Drop impossible completions (unavailable or inaccessible)

[sammccall]

Summary

(There must be some reason why D38077 didn't just do this, but I don't get it!)

[ilya-biryukov]

I personally think they're useful to have anyway, and they don't add much noise when they're at the end of completions list.
I sometimes want to complete an item I know is there and then fix accessibility. Do you think they add too much noise?

I think I'm almost alone in this camp, though. (I remember @klimek saying he thinks they're sometimes useful too, but I may be mistaken)

[sammccall]

In D39836#920306, @ilya-biryukov wrote:

I personally think they're useful to have anyway, and they don't add much noise when they're at the end of completions list.
I sometimes want to complete an item I know is there and then fix accessibility. Do you think they add too much noise?

For me the noise definitely outweighs the value, e.g. when I'm trying to see all the available functions. There are costs beyond noise:

• they bulk up tests and debugging output
• they complicate the scoring scheme - we *never* want them to appear above a legal completion, so a 1-dimensional score needs special handling as here.
• compute: we spend time scoring them, computing their strings, writing them over the wire, and then the client has to process them

[klimek]

In D39836#920313, @sammccall wrote:

In D39836#920306, @ilya-biryukov wrote:

I personally think they're useful to have anyway, and they don't add much noise when they're at the end of completions list.
I sometimes want to complete an item I know is there and then fix accessibility. Do you think they add too much noise?

For me the noise definitely outweighs the value, e.g. when I'm trying to see all the available functions. There are costs beyond noise:

• they bulk up tests and debugging output
• they complicate the scoring scheme - we *never* want them to appear above a legal completion, so a 1-dimensional score needs special handling as here.
• compute: we spend time scoring them, computing their strings, writing them over the wire, and then the client has to process them

I do think they are useful to me, but I don't know what the percentages of folks who want them vs. not are, and whether people have actually tried multiple schemes.
I think it's more likely that we get bug reports to put them in if we don't show them than the other way round, so I think taking them out is a good way to get that feedback :)

[ilya-biryukov]

This is probably one of the things that I'd like to be configurable.

In D39836#920313, @sammccall wrote:

• they bulk up tests and debugging output

I'd argue that the more you put into tests, the more potential errors you'll see. E.g. if we're not showing private members, we might miss some presentation issues that only those exhibit. And you can always leave them out in tests in case you only want to test the public ones (though that's not a good idea, probably).
As for debugging, I don't LSP is any good as a debugging format anyway, we probably want some different form of output.

• they complicate the scoring scheme - we *never* want them to appear above a legal completion, so a 1-dimensional score needs special handling as here.

We could always use a 2-dimensional score (accessible, sort_score). I'd love to have some editor support to grey-out some items (not only inaccessible, deprecated items could have been somehow signalled in completion UI as well), probably requires some changes to LSP too and not a priority, though.

• compute: we spend time scoring them, computing their strings, writing them over the wire, and then the client has to process them

I don't think it matters much, given that there aren't many items that are inaccessible compared to the total number of completion items when it's slow (they are all class members, but the biggest portion of items come from namespaces). And after-dot completion is usually fast (and when it's slow it's because reparse is slow, not because we take too much time to process the items).

That being said, I don't think there's a chance I'll argue my way through this. Let's turn them off and see if someone (besides me) complains. LGTM.

[arphaman]

I would prefer to make this behaviour configurable.

[sammccall]

So I probably should have started from the other end, here :-)

I'd really like to make the completion retrieval and ranking more flexible. In particular

• incorporating results from other sources (indexes: both in-memory and external services).
• combining more quality signals like usage count and fuzzy-match-strength in a non-lexicographic-sort way

The biggest difficulty in *supporting* unusable functions is maintaining the invariant that all unusable functions are ranked lower than usable ones - all code that deals with ranking has to deal with this special case, e.g. by making score a tuple instead of a single number.

If the current approach of "give them a penalty" is enough, knowing that in the future it may lead to e.g. a very widely used but inaccessible protected function being ranked highly, then that seems fine to me too. A wider configuration space means testing is more work, but happy to live with it. What do you think?

(With my user-hat on, configurable is fine, though I do strongly feel they should be off by default, and it seems unlikely many users will change the defaults.)

[arphaman]

In D39836#920587, @sammccall wrote:

So I probably should have started from the other end, here :-)

I'd really like to make the completion retrieval and ranking more flexible. In particular

• incorporating results from other sources (indexes: both in-memory and external services).
• combining more quality signals like usage count and fuzzy-match-strength in a non-lexicographic-sort way

The biggest difficulty in *supporting* unusable functions is maintaining the invariant that all unusable functions are ranked lower than usable ones - all code that deals with ranking has to deal with this special case, e.g. by making score a tuple instead of a single number.

That sounds good to me. Please keep in mind that not all clients might want to take advantage of these things as they might have their own fuzz-match logic and external result injection.

If the current approach of "give them a penalty" is enough, knowing that in the future it may lead to e.g. a very widely used but inaccessible protected function being ranked highly, then that seems fine to me too. A wider configuration space means testing is more work, but happy to live with it. What do you think?

(With my user-hat on, configurable is fine, though I do strongly feel they should be off by default, and it seems unlikely many users will change the defaults.)

I'd be ok with off by default, as long as it's possible to turn it on :)

[sammccall]

In D39836#920977, @arphaman wrote:

In D39836#920587, @sammccall wrote:

So I probably should have started from the other end, here :-)

I'd really like to make the completion retrieval and ranking more flexible. In particular

• incorporating results from other sources (indexes: both in-memory and external services).
• combining more quality signals like usage count and fuzzy-match-strength in a non-lexicographic-sort way

The biggest difficulty in *supporting* unusable functions is maintaining the invariant that all unusable functions are ranked lower than usable ones - all code that deals with ranking has to deal with this special case, e.g. by making score a tuple instead of a single number.

That sounds good to me. Please keep in mind that not all clients might want to take advantage of these things as they might have their own fuzz-match logic

Yup! My understanding of the protocol is clients will generally trigger completion after the dot, and then filter client-side *unless* the result set is incomplete.
So fuzzy-match filtering/scoring would only be triggered if we limit the number of results (proposed as optional in D39852) or if completion is explicitly triggered mid-identifier (maybe we should allow turning this off too).

and external result injection.

Absolutely. To be concrete, i'm thinking about three overlaid:

1. a static generated index that clangd consumes (e.g. generated by index-while-building and located on disk, or a hosted index of google's monorepo)
2. a dynamically generated index of symbols in files that clangd has seen. (Likely to be dirty with respect to 1)
3. context-sensitive completion results as today

1 would definitely be optional.
2 isn't really external... we might want to handle global symbols with 2 and not 3 as now, but that's an implementation detail.

If the current approach of "give them a penalty" is enough, knowing that in the future it may lead to e.g. a very widely used but inaccessible protected function being ranked highly, then that seems fine to me too. A wider configuration space means testing is more work, but happy to live with it. What do you think?

(With my user-hat on, configurable is fine, though I do strongly feel they should be off by default, and it seems unlikely many users will change the defaults.)

I'd be ok with off by default, as long as it's possible to turn it on :)

Sure, I'll update the patch.

For these things that are (I assume) more "user preference" than project- or integration-specific, I wonder whether command-line flags are the right thing. Configuration in ~/.clangd is a can of worms, but might be the right thing in the long-run.

[sammccall]

Add an option to allow impossible completions.
While here, remove 'helpful' constructors that take some subset of the params.

[sammccall]

(trying to resync the diff to head)

[sammccall]

PTAL - this is now optional. The results will be hidden by default, but can be turned on with a flag.

[ilya-biryukov]

clangd/tool/ClangdMain.cpp

169 ↗

(On Diff #123819)

We should also set IncludeCodePatterns = EnableSnippets here. May be worth adding a test that they are in the completion results.

[sammccall]

clangd/tool/ClangdMain.cpp

169 ↗

(On Diff #123819)

Good catch!
Unfortunately our previous attempt this failed because static_cast is not a code pattern (probably it should be).

• Added a test to completion-items-kinds.
• Added documentation to the flag to indicate it does this
• Changed the CodePatterns default to true (equivalent to setting it here, and keeps the defaults in one place)
• set flag defaults based on CompleteOption defaults so they're in one place

[sammccall]

Address review comments

[ilya-biryukov]

LGTM

clang-tools-extra/trunk/clangd/ClangdLSPServer.h

 /// This class provides implementation of an LSP server, glueing the JSON
 /// dispatch and ClangdServer together.
 class ClangdLSPServer : private DiagnosticsConsumer, private ProtocolCallbacks {
 public:
   /// If \p CompileCommandsDir has a value, compile_commands.json will be
   /// loaded only from \p CompileCommandsDir. Otherwise, clangd will look
   /// for compile_commands.json in all parent directories of each file.
   ClangdLSPServer(JSONOutput &Out, unsigned AsyncThreadsCount,
-                  bool StorePreamblesInMemory, bool SnippetCompletions,
+                  bool StorePreamblesInMemory,
+                  const clangd::CodeCompleteOptions &CCOpts,
                   llvm::Optional<StringRef> ResourceDir,
                   llvm::Optional<Path> CompileCommandsDir);
 
   /// Run LSP server loop, receiving input for it from \p In. \p In must be
   /// opened in binary mode. Output will be written using Out variable passed to
   /// class constructor. This method must not be executed more than once for
   /// each instance of ClangdLSPServer.
   ///

clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp

void ClangdLSPServer::onSwitchSourceHeader(Ctx C,
                                            TextDocumentIdentifier &Params) {
   llvm::Optional<Path> Result = Server.switchSourceHeader(Params.uri.file);
   std::string ResultUri;
   C.reply(Result ? URI::fromFile(*Result).uri : "");
 }
 
 ClangdLSPServer::ClangdLSPServer(JSONOutput &Out, unsigned AsyncThreadsCount,
                                  bool StorePreamblesInMemory,
-                                 bool SnippetCompletions,
+                                 const clangd::CodeCompleteOptions &CCOpts,
                                  llvm::Optional<StringRef> ResourceDir,
                                  llvm::Optional<Path> CompileCommandsDir)
     : Out(Out), CDB(/*Logger=*/Out, std::move(CompileCommandsDir)),
       Server(CDB, /*DiagConsumer=*/*this, FSProvider, AsyncThreadsCount,
-             StorePreamblesInMemory,
-             clangd::CodeCompleteOptions(
-                 /*EnableSnippetsAndCodePatterns=*/SnippetCompletions),
+             StorePreamblesInMemory, CCOpts,
              /*Logger=*/Out, ResourceDir) {}
 
 bool ClangdLSPServer::run(std::istream &In) {
   assert(!IsDone && "Run was called before");
 
   // Set up JSONRPCDispatcher.
   JSONRPCDispatcher Dispatcher(
       [](RequestContext Ctx, llvm::yaml::MappingNode *Params) {

clang-tools-extra/trunk/clangd/ClangdServer.h

 /// definition.
 class ClangdServer {
 public:
   /// Creates a new ClangdServer instance.
   /// To process parsing requests asynchronously, ClangdServer will spawn \p
   /// AsyncThreadsCount worker threads. However, if \p AsyncThreadsCount is 0,
   /// all requests will be processed on the calling thread.
   ///
-  /// When \p SnippetCompletions is true, completion items will be presented
-  /// with embedded snippets. Otherwise, plaintext items will be presented.
-  ///
   /// ClangdServer uses \p FSProvider to get an instance of vfs::FileSystem for
   /// each parsing request. Results of code completion and diagnostics also
   /// include a tag, that \p FSProvider returns along with the vfs::FileSystem.
   ///
   /// The value of \p ResourceDir will be used to search for internal headers
   /// (overriding defaults and -resource-dir compiler flag). If \p ResourceDir
   /// is None, ClangdServer will call CompilerInvocation::GetResourcePath() to
   /// obtain the standard resource directory.
public:
   /// \p StorePreamblesInMemory defines whether the Preambles generated by
   /// clangd are stored in-memory or on disk.
   ///
   /// Various messages are logged using \p Logger.
   ClangdServer(GlobalCompilationDatabase &CDB,
                DiagnosticsConsumer &DiagConsumer,
                FileSystemProvider &FSProvider, unsigned AsyncThreadsCount,
                bool StorePreamblesInMemory,
-               clangd::CodeCompleteOptions CodeCompleteOpts,
+               const clangd::CodeCompleteOptions &CodeCompleteOpts,
                clangd::Logger &Logger,
                llvm::Optional<StringRef> ResourceDir = llvm::None);
 
   /// Set the root path of the workspace.
   void setRootPath(PathRef RootPath);
 
   /// Add a \p File to the list of tracked C++ files or update the contents if
   /// \p File is already tracked. Also schedules parsing of the AST for it on a

clang-tools-extra/trunk/clangd/ClangdServer.cpp

if (RunSynchronously)
     Done = true;
   } // unlock Mutex
   RequestCV.notify_all();
 
   for (auto &Worker : Workers)
     Worker.join();
 }
 
-ClangdServer::ClangdServer(
-    GlobalCompilationDatabase &CDB, DiagnosticsConsumer &DiagConsumer,
-    FileSystemProvider &FSProvider, unsigned AsyncThreadsCount,
-    bool StorePreamblesInMemory, clangd::CodeCompleteOptions CodeCompleteOpts,
-    clangd::Logger &Logger, llvm::Optional<StringRef> ResourceDir)
+ClangdServer::ClangdServer(GlobalCompilationDatabase &CDB,
+                           DiagnosticsConsumer &DiagConsumer,
+                           FileSystemProvider &FSProvider,
+                           unsigned AsyncThreadsCount,
+                           bool StorePreamblesInMemory,
+                           const clangd::CodeCompleteOptions &CodeCompleteOpts,
+                           clangd::Logger &Logger,
+                           llvm::Optional<StringRef> ResourceDir)
     : Logger(Logger), CDB(CDB), DiagConsumer(DiagConsumer),
       FSProvider(FSProvider),
       ResourceDir(ResourceDir ? ResourceDir->str() : getStandardResourceDir()),
       PCHs(std::make_shared<PCHContainerOperations>()),
       StorePreamblesInMemory(StorePreamblesInMemory),
       CodeCompleteOpts(CodeCompleteOpts), WorkScheduler(AsyncThreadsCount) {}
 
 void ClangdServer::setRootPath(PathRef RootPath) {

clang-tools-extra/trunk/clangd/ClangdUnit.h

private:
   std::shared_ptr<const PreambleData> LatestAvailablePreamble;
   /// Utility class, required by clang.
   std::shared_ptr<PCHContainerOperations> PCHs;
   /// Used for logging various messages.
   clangd::Logger &Logger;
 };
 
 struct CodeCompleteOptions {
-  CodeCompleteOptions() = default;
-
-  /// Uses default values for all flags, but sets EnableSnippets and
-  /// IncludeCodePatterns to the value of EnableSnippetsAndCodePatterns.
-  explicit CodeCompleteOptions(bool EnableSnippetsAndCodePatterns);
-
-  CodeCompleteOptions(bool EnableSnippets, bool IncludeCodePatterns,
-                      bool IncludeMacros, bool IncludeGlobals,
-                      bool IncludeBriefComments);
-
   /// Returns options that can be passed to clang's completion engine.
   clang::CodeCompleteOptions getClangCompleteOpts() const;
 
   /// When true, completion items will contain expandable code snippets in
   /// completion (e.g.  `return ${1:expression}` or `foo(${1:int a}, ${2:int
   /// b})).
   bool EnableSnippets = false;
 
   /// Add code patterns to completion results.
   /// If EnableSnippets is false, this options is ignored and code patterns will
   /// always be omitted.
-  bool IncludeCodePatterns = false;
+  bool IncludeCodePatterns = true;
 
   /// Add macros to code completion results.
   bool IncludeMacros = true;
 
   /// Add globals to code completion results.
   bool IncludeGlobals = true;
 
   /// Add brief comments to completion items, if available.
   /// FIXME(ibiryukov): it looks like turning this option on significantly slows
   /// down completion, investigate if it can be made faster.
   bool IncludeBriefComments = true;
 
+  /// Include results that are not legal completions in the current context.
+  /// For example, private members are usually inaccessible.
+  bool IncludeIneligibleResults = false;
+
   /// Limit the number of results returned (0 means no limit).
   /// If more results are available, we set CompletionList.isIncomplete.
   size_t Limit = 0;
 };
 
 /// Get code completions at a specified \p Pos in \p FileName.
 CompletionList
 codeComplete(PathRef FileName, const tooling::CompileCommand &Command,

clang-tools-extra/trunk/clangd/ClangdUnit.cpp

CompletionItemsCollector(const clangd::CodeCompleteOptions &CodeCompleteOpts,
         Allocator(std::make_shared<clang::GlobalCodeCompletionAllocator>()),
         CCTUInfo(Allocator) {}
 
   void ProcessCodeCompleteResults(Sema &S, CodeCompletionContext Context,
                                   CodeCompletionResult *Results,
                                   unsigned NumResults) override final {
     std::priority_queue<CompletionCandidate> Candidates;
     for (unsigned I = 0; I < NumResults; ++I) {
-      Candidates.emplace(Results[I]);
+      auto &Result = Results[I];
+      if (!ClangdOpts.IncludeIneligibleResults &&
+          (Result.Availability == CXAvailability_NotAvailable ||
+           Result.Availability == CXAvailability_NotAccessible))
+        continue;
+      Candidates.emplace(Result);
       if (ClangdOpts.Limit && Candidates.size() > ClangdOpts.Limit) {
         Candidates.pop();
         Items.isIncomplete = true;
       }
     }
     while (!Candidates.empty()) {
       auto &Candidate = Candidates.top();
       const auto *CCS = Candidate.Result->CreateCodeCompletionString(
bool invokeCodeComplete(std::unique_ptr<CodeCompleteConsumer> Consumer,
 
   Action.EndSourceFile();
 
   return true;
 }
 
 } // namespace
 
-clangd::CodeCompleteOptions::CodeCompleteOptions(
-    bool EnableSnippetsAndCodePatterns)
-    : CodeCompleteOptions() {
-  EnableSnippets = EnableSnippetsAndCodePatterns;
-  IncludeCodePatterns = EnableSnippetsAndCodePatterns;
-}
-
-clangd::CodeCompleteOptions::CodeCompleteOptions(bool EnableSnippets,
-                                                 bool IncludeCodePatterns,
-                                                 bool IncludeMacros,
-                                                 bool IncludeGlobals,
-                                                 bool IncludeBriefComments)
-    : EnableSnippets(EnableSnippets), IncludeCodePatterns(IncludeCodePatterns),
-      IncludeMacros(IncludeMacros), IncludeGlobals(IncludeGlobals),
-      IncludeBriefComments(IncludeBriefComments) {}
-
 clang::CodeCompleteOptions
 clangd::CodeCompleteOptions::getClangCompleteOpts() const {
   clang::CodeCompleteOptions Result;
   Result.IncludeCodePatterns = EnableSnippets && IncludeCodePatterns;
   Result.IncludeMacros = IncludeMacros;
   Result.IncludeGlobals = IncludeGlobals;
   Result.IncludeBriefComments = IncludeBriefComments;
 

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

 static llvm::cl::opt<unsigned>
     WorkerThreadsCount("j",
                        llvm::cl::desc("Number of async workers used by clangd"),
                        llvm::cl::init(getDefaultAsyncThreadsCount()));
 
 static llvm::cl::opt<bool> EnableSnippets(
     "enable-snippets",
     llvm::cl::desc(
-        "Present snippet completions instead of plaintext completions"),
-    llvm::cl::init(false));
+        "Present snippet completions instead of plaintext completions. "
+        "This also enables code pattern results." /* FIXME: should it? */),
+    llvm::cl::init(clangd::CodeCompleteOptions().EnableSnippets));
+
+// FIXME: Flags are the wrong mechanism for user preferences.
+// We should probably read a dotfile or similar.
+static llvm::cl::opt<bool> IncludeIneligibleResults(
+    "include-ineligible-results",
+    llvm::cl::desc(
+        "Include ineligible completion results (e.g. private members)"),
+    llvm::cl::init(clangd::CodeCompleteOptions().IncludeIneligibleResults),
+    llvm::cl::Hidden);
 
 static llvm::cl::opt<bool>
     PrettyPrint("pretty", llvm::cl::desc("Pretty-print JSON output"),
                 llvm::cl::init(false));
 
 static llvm::cl::opt<PCHStorageFlag> PCHStorage(
     "pch-storage",
     llvm::cl::desc("Storing PCHs in memory increases memory usages, but may "
int main(int argc, char *argv[]) {
 
   llvm::Optional<StringRef> ResourceDirRef = None;
   if (!ResourceDir.empty())
     ResourceDirRef = ResourceDir;
 
   // Change stdin to binary to not lose \r\n on windows.
   llvm::sys::ChangeStdinToBinary();
 
+  clangd::CodeCompleteOptions CCOpts;
+  CCOpts.EnableSnippets = EnableSnippets;
+  CCOpts.IncludeIneligibleResults = IncludeIneligibleResults;
   // Initialize and run ClangdLSPServer.
   ClangdLSPServer LSPServer(Out, WorkerThreadsCount, StorePreamblesInMemory,
-                            EnableSnippets, ResourceDirRef,
+                            CCOpts, ResourceDirRef,
                             CompileCommandsDirPath);
   constexpr int NoShutdownRequestErrorCode = 1;
   llvm::set_thread_name("clangd.main");
   return LSPServer.run(std::cin) ? 0 : NoShutdownRequestErrorCode;
 }

clang-tools-extra/trunk/test/clangd/completion-items-kinds.test

 # RUN: clangd -enable-snippets -run-synchronously < %s | FileCheck %s
 # It is absolutely vital that this file has CRLF line endings.
 #
 Content-Length: 125
 
 {"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
 Content-Length: 220
 
 {"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///main.cpp","languageId":"cpp","version":1,"text":"#define MACRO X\nint variable;\nstruct Struct {};\n int function();\nint X = "}}}
 Content-Length: 148
 
 {"jsonrpc":"2.0","id":1,"method":"textDocument/completion","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":4,"character":7}}}
-Content-Length: 58
 # CHECK: {"id":1,"jsonrpc":"2.0","result":{"isIncomplete":false,"items":
 #
 # Keyword
 # CHECK-DAG: {"filterText":"int","insertText":"int","insertTextFormat":1,"kind":14,"label":"int","sortText":"000050int"}
 #
-# Code pattern
-# CHECK-DAG: {"filterText":"static_cast","insertText":"static_cast<${1:type}>(${2:expression})","insertTextFormat":2,"kind":15,"label":"static_cast<type>(expression)","sortText":"000040static_cast"}
-#
 # Struct
 # CHECK-DAG: {"filterText":"Struct","insertText":"Struct","insertTextFormat":1,"kind":7,"label":"Struct","sortText":"000050Struct"}
 #
 # Macro
 # CHECK-DAG: {"filterText":"MACRO","insertText":"MACRO","insertTextFormat":1,"kind":1,"label":"MACRO","sortText":"000070MACRO"}
 #
 # Variable
 # CHECK-DAG: {"detail":"int","filterText":"variable","insertText":"variable","insertTextFormat":1,"kind":6,"label":"variable","sortText":"000012variable"}
 #
 # Function
 # CHECK-DAG: {"detail":"int","filterText":"function","insertText":"function()","insertTextFormat":1,"kind":3,"label":"function()","sortText":"000012function"}
 #
 # CHECK-SAME: ]}}
+Content-Length: 146
+
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///main.cpp","languageId":"cpp","version":1,"text":"whi"}}}
+Content-Length: 148
+
+{"jsonrpc":"2.0","id":2,"method":"textDocument/completion","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":1,"character":3}}}
+# Code pattern (unfortunately there are none in expression context)
+# CHECK-DAG: {"filterText":"namespace","insertText":"namespace ${1:identifier}{${2:declarations}\n}","insertTextFormat":2,"kind":15,"label":"namespace identifier{declarations}","sortText":"000040namespace"}
+#
+Content-Length: 58
 
 {"jsonrpc":"2.0","id":3,"method":"shutdown","params":null}

clang-tools-extra/trunk/test/clangd/completion-priorities.test

 # CHECK-NEXT:      {
 # CHECK-NEXT:        "detail": "void",
 # CHECK-NEXT:        "filterText": "pub",
 # CHECK-NEXT:        "insertText": "pub",
 # CHECK-NEXT:        "insertTextFormat": 1,
 # CHECK-NEXT:        "kind": 2,
 # CHECK-NEXT:        "label": "pub()",
 # CHECK-NEXT:        "sortText": "000034pub"
-# CHECK-NEXT:      },
-# priv() and prot() are at the end of the list
-# CHECK-NEXT:      {
-#      CHECK:        "detail": "void",
-#      CHECK:        "filterText": "priv",
-# CHECK-NEXT:        "insertText": "priv",
-# CHECK-NEXT:        "insertTextFormat": 1,
-# CHECK-NEXT:        "kind": 2,
-# CHECK-NEXT:        "label": "priv()",
-# CHECK-NEXT:        "sortText": "200034priv"
-# CHECK-NEXT:      },
-# CHECK-NEXT:      {
-# CHECK-NEXT:        "detail": "void",
-# CHECK-NEXT:        "filterText": "prot",
-# CHECK-NEXT:        "insertText": "prot",
-# CHECK-NEXT:        "insertTextFormat": 1,
-# CHECK-NEXT:        "kind": 2,
-# CHECK-NEXT:        "label": "prot()",
-# CHECK-NEXT:        "sortText": "200034prot"
-# CHECK-NEXT:      }
-# CHECK-NEXT:    ]
-# CHECK-NEXT:  }
+# CHECK-NEXT:      }
+#  CHECK-NOT:        "label": "priv()",
+#  CHECK-NOT:        "label": "prot()",
+#      CHECK:    ]
 Content-Length: 58
 
 {"jsonrpc":"2.0","id":4,"method":"shutdown","params":null}
 Content-Length: 33
 
 {"jsonrpc":"2.0":"method":"exit"}

clang-tools-extra/trunk/test/clangd/completion-qualifiers.test

 # RUN: clangd -pretty -run-synchronously < %s | FileCheck -strict-whitespace %s
 Content-Length: 125
 
 {"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
 Content-Length: 297
 
 {"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///main.cpp","languageId":"cpp","version":1,"text":"class Foo {\n  public:\n    int foo() const;\n    int bar() const;\n};\n\nclass Bar : public Foo {\n  int foo() const;\n};\n\nvoid test() {\n  Bar().\n}"}}}
 Content-Length: 151
 
 {"jsonrpc":"2.0","id":2,"method":"textDocument/completion","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":11,"character":8}}}
 #      CHECK:  "id": 2,
 # CHECK-NEXT:  "jsonrpc": "2.0",
 # CHECK-NEXT:  "result": {
 # CHECK-NEXT:    "isIncomplete": false,
 # CHECK-NEXT:    "items": [
-# Eligible const functions are at the top of the list.
+# Eligible functions are at the top of the list.
 # CHECK-NEXT:      {
 # CHECK-NEXT:        "detail": "int",
 # CHECK-NEXT:        "filterText": "bar",
 # CHECK-NEXT:        "insertText": "bar",
 # CHECK-NEXT:        "insertTextFormat": 1,
 # CHECK-NEXT:        "kind": 2,
 # CHECK-NEXT:        "label": "bar() const",
 # CHECK-NEXT:        "sortText": "000037bar"
 # CHECK-NEXT:      },
 # CHECK-NEXT:      {
 # CHECK-NEXT:        "detail": "int",
 # CHECK-NEXT:        "filterText": "foo",
 # CHECK-NEXT:        "insertText": "foo",
 # CHECK-NEXT:        "insertTextFormat": 1,
 # CHECK-NEXT:        "kind": 2,
 # CHECK-NEXT:        "label": "Foo::foo() const",
 # CHECK-NEXT:        "sortText": "000037foo"
 # CHECK-NEXT:      },
-# Ineligible non-const function is at the bottom of the list.
-# CHECK-NEXT:      {
-#      CHECK:        "detail": "int",
-#      CHECK:        "filterText": "foo",
-# CHECK-NEXT:        "insertText": "foo",
-# CHECK-NEXT:        "insertTextFormat": 1,
-# CHECK-NEXT:        "kind": 2,
-# CHECK-NEXT:        "label": "foo() const",
-# CHECK-NEXT:        "sortText": "200035foo"
-# CHECK-NEXT:      }
-# CHECK-NEXT:    ]
-# CHECK-NEXT:  }
+# Ineligible private functions are not present.
+#  CHECK-NOT:        "label": "foo() const",
+#      CHECK:    ]
 Content-Length: 44
 
 {"jsonrpc":"2.0","id":4,"method":"shutdown"}
 Content-Length: 33
 
 {"jsonrpc":"2.0":"method":"exit"}

clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp

 
 struct GlobalClass {};
 
 struct ClassWithMembers {
   /// Doc for method.
   int method();
 
   int field;
+private:
+  int private_field;
 };
 
 int test() {
   struct LocalClass {};
 
   /// Doc for local_var.
   int local_var;
 
/// For after-dot completion we must always get consistent results.
                          .codeComplete(FooCpp, MemberCompletion.MarkerPos,
                                        StringRef(MemberCompletion.Text))
                          .get()
                          .Value;
 
       // Class members. The only items that must be present in after-dor
       // completion.
       EXPECT_TRUE(ContainsItem(Results, MethodItemText));
+      EXPECT_TRUE(ContainsItem(Results, MethodItemText));
       EXPECT_TRUE(ContainsItem(Results, "field"));
+      EXPECT_EQ(Opts.IncludeIneligibleResults,
+                ContainsItem(Results, "private_field"));
       // Global items.
       EXPECT_FALSE(ContainsItem(Results, "global_var"));
       EXPECT_FALSE(ContainsItem(Results, GlobalFuncItemText));
       EXPECT_FALSE(ContainsItem(Results, "GlobalClass"));
       // A macro.
       EXPECT_FALSE(ContainsItem(Results, "MACRO"));
       // Local items.
       EXPECT_FALSE(ContainsItem(Results, "LocalClass"));
// Global completion differs based on the Opts that were passed.
       EXPECT_EQ(
           Opts.IncludeBriefComments,
           ContainsItemPred(Results, [](clangd::CompletionItem const &Item) {
             return !Item.documentation.empty();
           }));
     }
   };
 
-  for (bool IncludeMacros : {true, false})
-    for (bool IncludeGlobals : {true, false})
-      for (bool IncludeBriefComments : {true, false})
-        for (bool EnableSnippets : {true, false})
+  clangd::CodeCompleteOptions CCOpts;
+  for (bool IncludeMacros : {true, false}){
+    CCOpts.IncludeMacros = IncludeMacros;
+    for (bool IncludeGlobals : {true, false}){
+      CCOpts.IncludeGlobals = IncludeGlobals;
+      for (bool IncludeBriefComments : {true, false}){
+        CCOpts.IncludeBriefComments = IncludeBriefComments;
+        for (bool EnableSnippets : {true, false}){
+          CCOpts.EnableSnippets = EnableSnippets;
           for (bool IncludeCodePatterns : {true, false}) {
-            TestWithOpts(clangd::CodeCompleteOptions(
-                /*EnableSnippets=*/EnableSnippets,
-                /*IncludeCodePatterns=*/IncludeCodePatterns,
-                /*IncludeMacros=*/IncludeMacros,
-                /*IncludeGlobals=*/IncludeGlobals,
-                /*IncludeBriefComments=*/IncludeBriefComments));
-          }
+            CCOpts.IncludeCodePatterns = IncludeCodePatterns;
+            for (bool IncludeIneligibleResults : {true, false}) {
+              CCOpts.IncludeIneligibleResults = IncludeIneligibleResults;
+              TestWithOpts(CCOpts);
+            }
+          }
+        }
+      }
+    }
 }
 
 class ClangdThreadingTest : public ClangdVFSTest {};
 
 TEST_F(ClangdThreadingTest, StressTest) {
   // Without 'static' clang gives an error for a usage inside TestDiagConsumer.
   static const unsigned FilesCount = 5;
   const unsigned RequestsCount = 500;