[clangd] Don't show base class versions of members as completions.

[sammccall]

Summary

These are available via qualifiers, but signal to noise level is low.
Keep required quailifier machinery around though, for cross-ns completion.

[ioeric]

Keep required quailifier machinery around though, for cross-ns completion.

Do we have cross-ns completion in sema?

clangd/CodeComplete.cpp

732

Are there hidden results that are neither declarations nor class members and do not require qualifier?

[sammccall]

In D53571#1274082, @ioeric wrote:

Keep required quailifier machinery around though, for cross-ns completion.

Do we have cross-ns completion in sema?

No, I don't think so. I meant RequiredQualifier etc.
Unfortunately I've got no idea whether there are !QualifierIsInformative cases in Sema we still care about, so I don't know whether that logic is safe to remove...

clangd/CodeComplete.cpp

732

I don't know. I thought a namespace-scope thing shadowed by a member was such a case, but it doesn't seem to generate results at all.

But that (hypothetical) example illustrates a point: I do think we would want to show ns::foo() if it was shadowed by MyClass()::foo(). The difference is that they're probably fundamentally different things that share a name, while MySubClass::foo() is usually just a strictly better version of MyBaseClass::foo() as far as a caller is concerned.

[sammccall]

Remove misleading change to test.

[ioeric]

The diff seems to be wrong. Please rebase.

clangd/CodeComplete.cpp

732

Fair enough. Thanks!

[sammccall]

Rebase

clangd/ClangdLSPServer.h

void setExtraFlagsForFile(PathRef File,
                               std::vector<std::string> ExtraFlags);
 
     /// Set the compile commands directory to \p P.
     /// Only valid for directory-based CDB, no-op and error log on InMemoryCDB;
     void setCompileCommandsDir(Path P);
 
     /// Returns a CDB that should be used to get compile commands for the
     /// current instance of ClangdLSPServer.
-    GlobalCompilationDatabase &getCDB();
+    GlobalCompilationDatabase &getCDB() { return *CDB; }
 
   private:
     CompilationDB(std::unique_ptr<GlobalCompilationDatabase> CDB,
-                  std::unique_ptr<CachingCompilationDb> CachingCDB,
                   bool IsDirectoryBased)
-        : CDB(std::move(CDB)), CachingCDB(std::move(CachingCDB)),
-          IsDirectoryBased(IsDirectoryBased) {}
+        : CDB(std::move(CDB)), IsDirectoryBased(IsDirectoryBased) {}
 
     // if IsDirectoryBased is true, an instance of InMemoryCDB.
     // If IsDirectoryBased is false, an instance of DirectoryBasedCDB.
     // unique_ptr<GlobalCompilationDatabase> CDB;
     std::unique_ptr<GlobalCompilationDatabase> CDB;
-    // Non-null only for directory-based CDB
-    std::unique_ptr<CachingCompilationDb> CachingCDB;
     bool IsDirectoryBased;
   };
 
   // Most code should not deal with Transport directly.
   // MessageHandler deals with incoming messages, use call() etc for outgoing.
   clangd::Transport &Transp;
   class MessageHandler;
   std::unique_ptr<MessageHandler> MsgHandler;

clangd/ClangdLSPServer.cpp

 
 void ClangdLSPServer::reparseOpenedFiles() {
   for (const Path &FilePath : DraftMgr.getActiveFiles())
     Server->addDocument(FilePath, *DraftMgr.getDraft(FilePath),
                         WantDiagnostics::Auto);
 }
 
 ClangdLSPServer::CompilationDB ClangdLSPServer::CompilationDB::makeInMemory() {
-  return CompilationDB(llvm::make_unique<InMemoryCompilationDb>(), nullptr,
+  return CompilationDB(llvm::make_unique<InMemoryCompilationDb>(),
                        /*IsDirectoryBased=*/false);
 }
 
 ClangdLSPServer::CompilationDB
 ClangdLSPServer::CompilationDB::makeDirectoryBased(
     Optional<Path> CompileCommandsDir) {
   auto CDB = llvm::make_unique<DirectoryBasedGlobalCompilationDatabase>(
       std::move(CompileCommandsDir));
-  auto CachingCDB = llvm::make_unique<CachingCompilationDb>(*CDB);
-  return CompilationDB(std::move(CDB), std::move(CachingCDB),
+  return CompilationDB(std::move(CDB),
                        /*IsDirectoryBased=*/true);
 }
 
 void ClangdLSPServer::CompilationDB::invalidate(PathRef File) {
   if (!IsDirectoryBased)
     static_cast<InMemoryCompilationDb *>(CDB.get())->invalidate(File);
-  else
-    CachingCDB->invalidate(File);
 }
 
 bool ClangdLSPServer::CompilationDB::setCompilationCommandForFile(
     PathRef File, tooling::CompileCommand CompilationCommand) {
   if (IsDirectoryBased) {
     elog("Trying to set compile command for {0} while using directory-based "
          "compilation database",
          File);
     return false;
   }
   return static_cast<InMemoryCompilationDb *>(CDB.get())
       ->setCompilationCommandForFile(File, std::move(CompilationCommand));
 }
 
 void ClangdLSPServer::CompilationDB::setExtraFlagsForFile(
     PathRef File, std::vector<std::string> ExtraFlags) {
   if (!IsDirectoryBased) {
     elog("Trying to set extra flags for {0} while using in-memory compilation "
          "database",
          File);
     return;
   }
   static_cast<DirectoryBasedGlobalCompilationDatabase *>(CDB.get())
       ->setExtraFlagsForFile(File, std::move(ExtraFlags));
-  CachingCDB->invalidate(File);
 }
 
 void ClangdLSPServer::CompilationDB::setCompileCommandsDir(Path P) {
   if (!IsDirectoryBased) {
     elog("Trying to set compile commands dir while using in-memory compilation "
          "database");
     return;
   }
   static_cast<DirectoryBasedGlobalCompilationDatabase *>(CDB.get())
       ->setCompileCommandsDir(P);
-  CachingCDB->clear();
-}
-
-GlobalCompilationDatabase &ClangdLSPServer::CompilationDB::getCDB() {
-  if (CachingCDB)
-    return *CachingCDB;
-  return *CDB;
 }
 
 } // namespace clangd
 } // namespace clang

clangd/CodeComplete.cpp

void ProcessCodeCompleteResults(class Sema &S, CodeCompletionContext Context,
     }
     // Record the completion context.
     CCSema = &S;
     CCContext = Context;
 
     // Retain the results we might want.
     for (unsigned I = 0; I < NumResults; ++I) {
       auto &Result = InResults[I];
-      // Drop hidden items which cannot be found by lookup after completion.
-      // Exception: some items can be named by using a qualifier.
-      if (Result.Hidden && (!Result.Qualifier || Result.QualifierIsInformative))
+      // Class members that are shadowed by subclasses are usually noise.
+      if (Result.Hidden && Result.Declaration &&
+          Result.Declaration->isCXXClassMember())
         continue;
       if (!Opts.IncludeIneligibleResults &&
           (Result.Availability == CXAvailability_NotAvailable ||
            Result.Availability == CXAvailability_NotAccessible))
         continue;
       if (Result.Declaration &&
           !Context.getBaseType().isNull() // is this a member-access context?
           && isBlacklistedMember(*Result.Declaration))

clangd/GlobalCompilationDatabase.h

private:
 
   /// Stores extra flags per file.
   llvm::StringMap<std::vector<std::string>> ExtraFlagsForFile;
   /// Used for command argument pointing to folder where compile_commands.json
   /// is located.
   llvm::Optional<Path> CompileCommandsDir;
 };
 
-/// A wrapper around GlobalCompilationDatabase that caches the compile commands.
-/// Note that only results of getCompileCommand are cached.
-class CachingCompilationDb : public GlobalCompilationDatabase {
-public:
-  explicit CachingCompilationDb(const GlobalCompilationDatabase &InnerCDB);
-
-  /// Gets compile command for \p File from cache or CDB if it's not in the
-  /// cache.
-  llvm::Optional<tooling::CompileCommand>
-  getCompileCommand(PathRef File) const override;
-
-  /// Forwards to the inner CDB. Results of this function are not cached.
-  tooling::CompileCommand getFallbackCommand(PathRef File) const override;
-
-  /// Removes an entry for \p File if it's present in the cache.
-  void invalidate(PathRef File);
-
-  /// Removes all cached compile commands.
-  void clear();
-
-private:
-  const GlobalCompilationDatabase &InnerCDB;
-  mutable std::mutex Mut;
-  mutable llvm::StringMap<llvm::Optional<tooling::CompileCommand>>
-      Cached; /* GUARDED_BY(Mut) */
-};
-
 /// Gets compile args from an in-memory mapping based on a filepath. Typically
 /// used by clients who provide the compile commands themselves.
 class InMemoryCompilationDb : public GlobalCompilationDatabase {
 public:
   /// Gets compile command for \p File from the stored mapping.
   llvm::Optional<tooling::CompileCommand>
   getCompileCommand(PathRef File) const override;
 

clangd/GlobalCompilationDatabase.cpp

if (CompileCommandsDir)
     return getCDBInDirLocked(*CompileCommandsDir);
   for (auto Path = path::parent_path(File); !Path.empty();
        Path = path::parent_path(Path))
     if (auto CDB = getCDBInDirLocked(Path))
       return CDB;
   return nullptr;
 }
 
-CachingCompilationDb::CachingCompilationDb(
-    const GlobalCompilationDatabase &InnerCDB)
-    : InnerCDB(InnerCDB) {}
-
-Optional<tooling::CompileCommand>
-CachingCompilationDb::getCompileCommand(PathRef File) const {
-  std::unique_lock<std::mutex> Lock(Mut);
-  auto It = Cached.find(File);
-  if (It != Cached.end())
-    return It->second;
-
-  Lock.unlock();
-  Optional<tooling::CompileCommand> Command = InnerCDB.getCompileCommand(File);
-  Lock.lock();
-  return Cached.try_emplace(File, std::move(Command)).first->getValue();
-}
-
-tooling::CompileCommand
-CachingCompilationDb::getFallbackCommand(PathRef File) const {
-  return InnerCDB.getFallbackCommand(File);
-}
-
-void CachingCompilationDb::invalidate(PathRef File) {
-  std::unique_lock<std::mutex> Lock(Mut);
-  Cached.erase(File);
-}
-
-void CachingCompilationDb::clear() {
-  std::unique_lock<std::mutex> Lock(Mut);
-  Cached.clear();
-}
-
 Optional<tooling::CompileCommand>
 InMemoryCompilationDb::getCompileCommand(PathRef File) const {
   std::lock_guard<std::mutex> Lock(Mutex);
   auto It = Commands.find(File);
   if (It == Commands.end())
     return None;
   return It->second;
 }

unittests/clangd/CodeCompleteTests.cpp

auto Results = completions(R"cpp(
         int bar() const;
       };
       class Bar : public Foo {
         int foo() const;
       };
       void test() { Bar().^ }
   )cpp");
   EXPECT_THAT(Results.Completions,
-              HasSubsequence(AllOf(Qualifier(""), Named("bar")),
-                             AllOf(Qualifier("Foo::"), Named("foo"))));
+              Contains(AllOf(Qualifier(""), Named("bar"))));
+  // Hidden members are not shown.
   EXPECT_THAT(Results.Completions,
-              Not(Contains(AllOf(Qualifier(""), Named("foo"))))); // private
+              Not(Contains(AllOf(Qualifier("Foo::"), Named("foo")))));
+  // Private members are not shown.
+  EXPECT_THAT(Results.Completions,
+              Not(Contains(AllOf(Qualifier(""), Named("foo")))));
 }
 
 TEST(CompletionTest, InjectedTypename) {
   // These are suppressed when accessed as a member...
   EXPECT_THAT(completions("struct X{}; void foo(){ X().^ }").Completions,
               Not(Has("X")));
   EXPECT_THAT(completions("struct X{ void foo(){ this->^ } };").Completions,
               Not(Has("X")));