[clangd] Drop impossible completions (unavailable or inaccessible)

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

Reviewers: ilya-biryukov

Subscribers: cfe-commits

Differential Revision: https://reviews.llvm.org/D39836

llvm-svn: 318925

diff --git a/clang-tools-extra/clangd/ClangdLSPServer.cpp b/clang-tools-extra/clangd/ClangdLSPServer.cpp
index 80352ee..0513b79 100644 (file)
--- a/clang-tools-extra/clangd/ClangdLSPServer.cpp
+++ b/clang-tools-extra/clangd/ClangdLSPServer.cpp
@@ -236,14 +236,12 @@ void ClangdLSPServer::onSwitchSourceHeader(Ctx C,
 
 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) {

diff --git a/clang-tools-extra/clangd/ClangdLSPServer.h b/clang-tools-extra/clangd/ClangdLSPServer.h
index 1c2ede9..4516334 100644 (file)
--- a/clang-tools-extra/clangd/ClangdLSPServer.h
+++ b/clang-tools-extra/clangd/ClangdLSPServer.h
@@ -31,7 +31,8 @@ public:
   /// 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);
 

diff --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp
index dfe94b3..618d69f 100644 (file)
--- a/clang-tools-extra/clangd/ClangdServer.cpp
+++ b/clang-tools-extra/clangd/ClangdServer.cpp
@@ -169,11 +169,14 @@ ClangdScheduler::~ClangdScheduler() {
     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()),

diff --git a/clang-tools-extra/clangd/ClangdServer.h b/clang-tools-extra/clangd/ClangdServer.h
index bd95d00..6e30440 100644 (file)
--- a/clang-tools-extra/clangd/ClangdServer.h
+++ b/clang-tools-extra/clangd/ClangdServer.h
@@ -182,9 +182,6 @@ public:
   /// 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.
@@ -213,7 +210,7 @@ public:
                DiagnosticsConsumer &DiagConsumer,
                FileSystemProvider &FSProvider, unsigned AsyncThreadsCount,
                bool StorePreamblesInMemory,
-               clangd::CodeCompleteOptions CodeCompleteOpts,
+               const clangd::CodeCompleteOptions &CodeCompleteOpts,
                clangd::Logger &Logger,
                llvm::Optional<StringRef> ResourceDir = llvm::None);
 

diff --git a/clang-tools-extra/clangd/ClangdUnit.cpp b/clang-tools-extra/clangd/ClangdUnit.cpp
index 5ccfc2b..bd205e0 100644 (file)
--- a/clang-tools-extra/clangd/ClangdUnit.cpp
+++ b/clang-tools-extra/clangd/ClangdUnit.cpp
@@ -436,7 +436,12 @@ public:
                                   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;
@@ -806,22 +811,6 @@ bool invokeCodeComplete(std::unique_ptr<CodeCompleteConsumer> Consumer,
 
 } // 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;

diff --git a/clang-tools-extra/clangd/ClangdUnit.h b/clang-tools-extra/clangd/ClangdUnit.h
index 6babf55..22a1b5a 100644 (file)
--- a/clang-tools-extra/clangd/ClangdUnit.h
+++ b/clang-tools-extra/clangd/ClangdUnit.h
@@ -255,16 +255,6 @@ private:
 };
 
 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;
 
@@ -276,7 +266,7 @@ struct CodeCompleteOptions {
   /// 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;
@@ -289,6 +279,10 @@ struct CodeCompleteOptions {
   /// 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;

diff --git a/clang-tools-extra/clangd/tool/ClangdMain.cpp b/clang-tools-extra/clangd/tool/ClangdMain.cpp
index 81512c2..e209fe0 100644 (file)
--- a/clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ b/clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -42,8 +42,18 @@ static llvm::cl::opt<unsigned>
 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"),
@@ -157,9 +167,12 @@ int main(int argc, char *argv[]) {
   // 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");

diff --git a/clang-tools-extra/test/clangd/completion-items-kinds.test b/clang-tools-extra/test/clangd/completion-items-kinds.test
index 03bd96f..1dc1ee5 100644 (file)
--- a/clang-tools-extra/test/clangd/completion-items-kinds.test
+++ b/clang-tools-extra/test/clangd/completion-items-kinds.test
@@ -10,15 +10,11 @@ Content-Length: 220
 Content-Length: 148\r
 \r
 {"jsonrpc":"2.0","id":1,"method":"textDocument/completion","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":4,"character":7}}}\r
-Content-Length: 58\r
 # CHECK: {"id":1,"jsonrpc":"2.0","result":{"isIncomplete":false,"items":\r
 #\r
 # Keyword\r
 # CHECK-DAG: {"filterText":"int","insertText":"int","insertTextFormat":1,"kind":14,"label":"int","sortText":"000050int"}\r
 #\r
-# Code pattern\r
-# CHECK-DAG: {"filterText":"static_cast","insertText":"static_cast<${1:type}>(${2:expression})","insertTextFormat":2,"kind":15,"label":"static_cast<type>(expression)","sortText":"000040static_cast"}\r
-#\r
 # Struct\r
 # CHECK-DAG: {"filterText":"Struct","insertText":"Struct","insertTextFormat":1,"kind":7,"label":"Struct","sortText":"000050Struct"}\r
 #\r
@@ -32,5 +28,15 @@ Content-Length: 58
 # CHECK-DAG: {"detail":"int","filterText":"function","insertText":"function()","insertTextFormat":1,"kind":3,"label":"function()","sortText":"000012function"}\r
 #\r
 # CHECK-SAME: ]}}\r
+Content-Length: 146\r
+\r
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///main.cpp","languageId":"cpp","version":1,"text":"whi"}}}\r
+Content-Length: 148\r
+\r
+{"jsonrpc":"2.0","id":2,"method":"textDocument/completion","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":1,"character":3}}}\r
+# Code pattern (unfortunately there are none in expression context)\r
+# CHECK-DAG: {"filterText":"namespace","insertText":"namespace ${1:identifier}{${2:declarations}\n}","insertTextFormat":2,"kind":15,"label":"namespace identifier{declarations}","sortText":"000040namespace"}\r
+#\r
+Content-Length: 58\r
 \r
 {"jsonrpc":"2.0","id":3,"method":"shutdown","params":null}\r

diff --git a/clang-tools-extra/test/clangd/completion-priorities.test b/clang-tools-extra/test/clangd/completion-priorities.test
index 349c1fb..a1705dd 100644 (file)
--- a/clang-tools-extra/test/clangd/completion-priorities.test
+++ b/clang-tools-extra/test/clangd/completion-priorities.test
@@ -61,28 +61,10 @@ Content-Length: 151
 # CHECK-NEXT:        "kind": 2,\r
 # CHECK-NEXT:        "label": "pub()",\r
 # CHECK-NEXT:        "sortText": "000034pub"\r
-# CHECK-NEXT:      },\r
-# priv() and prot() are at the end of the list\r
-# CHECK-NEXT:      {\r
-#      CHECK:        "detail": "void",\r
-#      CHECK:        "filterText": "priv",\r
-# CHECK-NEXT:        "insertText": "priv",\r
-# CHECK-NEXT:        "insertTextFormat": 1,\r
-# CHECK-NEXT:        "kind": 2,\r
-# CHECK-NEXT:        "label": "priv()",\r
-# CHECK-NEXT:        "sortText": "200034priv"\r
-# CHECK-NEXT:      },\r
-# CHECK-NEXT:      {\r
-# CHECK-NEXT:        "detail": "void",\r
-# CHECK-NEXT:        "filterText": "prot",\r
-# CHECK-NEXT:        "insertText": "prot",\r
-# CHECK-NEXT:        "insertTextFormat": 1,\r
-# CHECK-NEXT:        "kind": 2,\r
-# CHECK-NEXT:        "label": "prot()",\r
-# CHECK-NEXT:        "sortText": "200034prot"\r
 # CHECK-NEXT:      }\r
-# CHECK-NEXT:    ]\r
-# CHECK-NEXT:  }\r
+#  CHECK-NOT:        "label": "priv()",\r
+#  CHECK-NOT:        "label": "prot()",\r
+#      CHECK:    ]\r
 Content-Length: 58\r
 \r
 {"jsonrpc":"2.0","id":4,"method":"shutdown","params":null}\r

diff --git a/clang-tools-extra/test/clangd/completion-qualifiers.test b/clang-tools-extra/test/clangd/completion-qualifiers.test
index d684401..cd93a43 100644 (file)
--- a/clang-tools-extra/test/clangd/completion-qualifiers.test
+++ b/clang-tools-extra/test/clangd/completion-qualifiers.test
@@ -13,7 +13,7 @@ Content-Length: 151
 # CHECK-NEXT:  "result": {\r
 # CHECK-NEXT:    "isIncomplete": false,\r
 # CHECK-NEXT:    "items": [\r
-# Eligible const functions are at the top of the list.\r
+# Eligible functions are at the top of the list.\r
 # CHECK-NEXT:      {\r
 # CHECK-NEXT:        "detail": "int",\r
 # CHECK-NEXT:        "filterText": "bar",\r
@@ -32,18 +32,9 @@ Content-Length: 151
 # CHECK-NEXT:        "label": "Foo::foo() const",\r
 # CHECK-NEXT:        "sortText": "000037foo"\r
 # CHECK-NEXT:      },\r
-# Ineligible non-const function is at the bottom of the list.\r
-# CHECK-NEXT:      {\r
-#      CHECK:        "detail": "int",\r
-#      CHECK:        "filterText": "foo",\r
-# CHECK-NEXT:        "insertText": "foo",\r
-# CHECK-NEXT:        "insertTextFormat": 1,\r
-# CHECK-NEXT:        "kind": 2,\r
-# CHECK-NEXT:        "label": "foo() const",\r
-# CHECK-NEXT:        "sortText": "200035foo"\r
-# CHECK-NEXT:      }\r
-# CHECK-NEXT:    ]\r
-# CHECK-NEXT:  }\r
+# Ineligible private functions are not present.\r
+#  CHECK-NOT:        "label": "foo() const",\r
+#      CHECK:    ]\r
 Content-Length: 44\r
 \r
 {"jsonrpc":"2.0","id":4,"method":"shutdown"}\r

diff --git a/clang-tools-extra/unittests/clangd/ClangdTests.cpp b/clang-tools-extra/unittests/clangd/ClangdTests.cpp
index 570e7d1..44c8fde 100644 (file)
--- a/clang-tools-extra/unittests/clangd/ClangdTests.cpp
+++ b/clang-tools-extra/unittests/clangd/ClangdTests.cpp
@@ -788,6 +788,8 @@ struct ClassWithMembers {
   int method();
 
   int field;
+private:
+  int private_field;
 };
 
 int test() {
@@ -828,7 +830,10 @@ int test() {
       // 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));
@@ -889,18 +894,26 @@ int test() {
     }
   };
 
-  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 {};