[PATCH] D34146: [clangd] Allow to override contents of the file during completion.

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL305291: [clangd] Allow to override contents of the file 
during completion. (authored by ibiryukov).

Changed prior to commit:
  https://reviews.llvm.org/D34146?vs=102328=102331#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D34146

Files:
  clang-tools-extra/trunk/clangd/ClangdServer.cpp
  clang-tools-extra/trunk/clangd/ClangdServer.h
  clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp

Index: clang-tools-extra/trunk/clangd/ClangdServer.h
===
--- clang-tools-extra/trunk/clangd/ClangdServer.h
+++ clang-tools-extra/trunk/clangd/ClangdServer.h
@@ -152,8 +152,15 @@
   /// Force \p File to be reparsed using the latest contents.
   void forceReparse(PathRef File);
 
-  /// Run code completion for \p File at \p Pos.
-  Tagged codeComplete(PathRef File, Position Pos);
+  /// Run code completion for \p File at \p Pos. If \p OverridenContents is not
+  /// None, they will used only for code completion, i.e. no diagnostics update
+  /// will be scheduled and a draft for \p File will not be updated.
+  /// If \p OverridenContents is None, contents of the current draft for \p File
+  /// will be used.
+  /// This method should only be called for currently tracked files.
+  Tagged
+  codeComplete(PathRef File, Position Pos,
+   llvm::Optional OverridenContents = llvm::None);
 
   /// Run formatting for \p Rng inside \p File.
   std::vector formatRange(PathRef File, Range Rng);
Index: clang-tools-extra/trunk/clangd/ClangdServer.cpp
===
--- clang-tools-extra/trunk/clangd/ClangdServer.cpp
+++ clang-tools-extra/trunk/clangd/ClangdServer.cpp
@@ -184,17 +184,29 @@
   addDocument(File, getDocument(File));
 }
 
-Tagged ClangdServer::codeComplete(PathRef File,
-   Position Pos) {
-  auto FileContents = DraftMgr.getDraft(File);
-  assert(FileContents.Draft && "codeComplete is called for non-added document");
+Tagged
+ClangdServer::codeComplete(PathRef File, Position Pos,
+   llvm::Optional OverridenContents) {
+  std::string DraftStorage;
+  if (!OverridenContents) {
+auto FileContents = DraftMgr.getDraft(File);
+assert(FileContents.Draft &&
+   "codeComplete is called for non-added document");
+
+DraftStorage = std::move(*FileContents.Draft);
+OverridenContents = DraftStorage;
+  }
 
   std::vector Result;
   auto TaggedFS = FSProvider->getTaggedFileSystem();
-  Units.runOnUnitWithoutReparse(
-  File, *FileContents.Draft, *CDB, PCHs, TaggedFS.Value, [&](ClangdUnit ) {
-Result = Unit.codeComplete(*FileContents.Draft, Pos, TaggedFS.Value);
-  });
+  // It would be nice to use runOnUnitWithoutReparse here, but we can't
+  // guarantee the correctness of code completion cache here if we don't do the
+  // reparse.
+  Units.runOnUnit(File, *OverridenContents, *CDB, PCHs, TaggedFS.Value,
+  [&](ClangdUnit ) {
+Result = Unit.codeComplete(*OverridenContents, Pos,
+   TaggedFS.Value);
+  });
   return make_tagged(std::move(Result), TaggedFS.Tag);
 }
 
Index: clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp
@@ -398,5 +398,69 @@
   EXPECT_EQ(Server.codeComplete(FooCpp, Position{0, 0}).Tag, FS->Tag);
 }
 
+class ClangdCompletionTest : public ClangdVFSTest {
+protected:
+  bool ContainsItem(std::vector const , StringRef Name) {
+for (const auto  : Items) {
+  if (Item.insertText == Name)
+return true;
+}
+return false;
+  }
+};
+
+TEST_F(ClangdCompletionTest, CheckContentsOverride) {
+  MockFSProvider *FS;
+
+  ClangdServer Server(llvm::make_unique(),
+  llvm::make_unique(),
+  getAndMove(llvm::make_unique(), FS),
+  /*RunSynchronously=*/false);
+
+  auto FooCpp = getVirtualTestFilePath("foo.cpp");
+  const auto SourceContents = R"cpp(
+int aba;
+int b =   ;
+)cpp";
+
+  const auto OverridenSourceContents = R"cpp(
+int cbc;
+int b =   ;
+)cpp";
+  // Complete after '=' sign. We need to be careful to keep the SourceContents'
+  // size the same.
+  // We complete on the 3rd line (2nd in zero-based numbering), because raw
+  // string literal of the SourceContents starts with a newline(it's easy to
+  // miss).
+  Position CompletePos = {2, 8};
+  FS->Files[FooCpp] = SourceContents;
+
+  Server.addDocument(FooCpp, SourceContents);
+
+  {
+auto CodeCompletionResults1 =
+Server.codeComplete(FooCpp, CompletePos, None).Value;
+

[PATCH] D34146: [clangd] Allow to override contents of the file during completion.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/ClangdServer.cpp:190
+   llvm::Optional OverridenContents) {
+  std::string DraftStorage;
+  if (!OverridenContents) {

This is the relevant line that changed from the previous review.


https://reviews.llvm.org/D34146



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34146: [clangd] Allow to override contents of the file during completion.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov created this revision.

This is a reapplied r305280 with a fix to the crash found by build bots
(StringRef to an out-of-scope local std::string).


https://reviews.llvm.org/D34146

Files:
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  unittests/clangd/ClangdTests.cpp

Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -398,5 +398,69 @@
   EXPECT_EQ(Server.codeComplete(FooCpp, Position{0, 0}).Tag, FS->Tag);
 }
 
+class ClangdCompletionTest : public ClangdVFSTest {
+protected:
+  bool ContainsItem(std::vector const , StringRef Name) {
+for (const auto  : Items) {
+  if (Item.insertText == Name)
+return true;
+}
+return false;
+  }
+};
+
+TEST_F(ClangdCompletionTest, CheckContentsOverride) {
+  MockFSProvider *FS;
+
+  ClangdServer Server(llvm::make_unique(),
+  llvm::make_unique(),
+  getAndMove(llvm::make_unique(), FS),
+  /*RunSynchronously=*/false);
+
+  auto FooCpp = getVirtualTestFilePath("foo.cpp");
+  const auto SourceContents = R"cpp(
+int aba;
+int b =   ;
+)cpp";
+
+  const auto OverridenSourceContents = R"cpp(
+int cbc;
+int b =   ;
+)cpp";
+  // Complete after '=' sign. We need to be careful to keep the SourceContents'
+  // size the same.
+  // We complete on the 3rd line (2nd in zero-based numbering), because raw
+  // string literal of the SourceContents starts with a newline(it's easy to
+  // miss).
+  Position CompletePos = {2, 8};
+  FS->Files[FooCpp] = SourceContents;
+
+  Server.addDocument(FooCpp, SourceContents);
+
+  {
+auto CodeCompletionResults1 =
+Server.codeComplete(FooCpp, CompletePos, None).Value;
+EXPECT_TRUE(ContainsItem(CodeCompletionResults1, "aba"));
+EXPECT_FALSE(ContainsItem(CodeCompletionResults1, "cbc"));
+  }
+
+  {
+auto CodeCompletionResultsOverriden =
+Server
+.codeComplete(FooCpp, CompletePos,
+  StringRef(OverridenSourceContents))
+.Value;
+EXPECT_TRUE(ContainsItem(CodeCompletionResultsOverriden, "cbc"));
+EXPECT_FALSE(ContainsItem(CodeCompletionResultsOverriden, "aba"));
+  }
+
+  {
+auto CodeCompletionResults2 =
+Server.codeComplete(FooCpp, CompletePos, None).Value;
+EXPECT_TRUE(ContainsItem(CodeCompletionResults2, "aba"));
+EXPECT_FALSE(ContainsItem(CodeCompletionResults2, "cbc"));
+  }
+}
+
 } // namespace clangd
 } // namespace clang
Index: clangd/ClangdServer.h
===
--- clangd/ClangdServer.h
+++ clangd/ClangdServer.h
@@ -152,8 +152,15 @@
   /// Force \p File to be reparsed using the latest contents.
   void forceReparse(PathRef File);
 
-  /// Run code completion for \p File at \p Pos.
-  Tagged codeComplete(PathRef File, Position Pos);
+  /// Run code completion for \p File at \p Pos. If \p OverridenContents is not
+  /// None, they will used only for code completion, i.e. no diagnostics update
+  /// will be scheduled and a draft for \p File will not be updated.
+  /// If \p OverridenContents is None, contents of the current draft for \p File
+  /// will be used.
+  /// This method should only be called for currently tracked files.
+  Tagged
+  codeComplete(PathRef File, Position Pos,
+   llvm::Optional OverridenContents = llvm::None);
 
   /// Run formatting for \p Rng inside \p File.
   std::vector formatRange(PathRef File, Range Rng);
Index: clangd/ClangdServer.cpp
===
--- clangd/ClangdServer.cpp
+++ clangd/ClangdServer.cpp
@@ -184,17 +184,29 @@
   addDocument(File, getDocument(File));
 }
 
-Tagged ClangdServer::codeComplete(PathRef File,
-   Position Pos) {
-  auto FileContents = DraftMgr.getDraft(File);
-  assert(FileContents.Draft && "codeComplete is called for non-added document");
+Tagged
+ClangdServer::codeComplete(PathRef File, Position Pos,
+   llvm::Optional OverridenContents) {
+  std::string DraftStorage;
+  if (!OverridenContents) {
+auto FileContents = DraftMgr.getDraft(File);
+assert(FileContents.Draft &&
+   "codeComplete is called for non-added document");
+
+DraftStorage = std::move(*FileContents.Draft);
+OverridenContents = DraftStorage;
+  }
 
   std::vector Result;
   auto TaggedFS = FSProvider->getTaggedFileSystem();
-  Units.runOnUnitWithoutReparse(
-  File, *FileContents.Draft, *CDB, PCHs, TaggedFS.Value, [&](ClangdUnit ) {
-Result = Unit.codeComplete(*FileContents.Draft, Pos, TaggedFS.Value);
-  });
+  // It would be nice to use runOnUnitWithoutReparse here, but we can't
+  // guarantee the correctness of code completion cache here if we don't do the
+  // reparse.
+