[clangd] Avoid duplicates in findDefinitions response

Summary:
When compile_commands.json contains some source files expressed as
relative paths, we can get duplicate responses to findDefinitions.  The
responses only differ by the URI, which are different versions of the
same file:

    "result": [
        {
            ...
            "uri": "file:///home/emaisin/src/ls-interact/cpp-test/build/../src/first.h"
        },
        {
            ...
            "uri": "file:///home/emaisin/src/ls-interact/cpp-test/src/first.h"
        }
    ]

In getAbsoluteFilePath, we try to obtain the realpath of the FileEntry
by calling tryGetRealPathName.  However, this can fail and return an
empty string.  It may be bug a bug in clang, but in any case we should
fall back to computing it ourselves if it happens.

I changed getAbsoluteFilePath so that if tryGetRealPathName succeeds, we
return right away (a real path is always absolute).  Otherwise, we try
to build an absolute path, as we did before, but we also call
VFS->getRealPath to make sure to get the canonical path (e.g. without
any ".." in it).

Reviewers: malaperle

Subscribers: hokein, ilya-biryukov, ioeric, MaskRay, jkorous, cfe-commits

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

llvm-svn: 339483

3 files changed, 75 insertions, 16 deletions

diff --git a/clang-tools-extra/unittests/clangd/TestFS.cpp b/clang-tools-extra/unittests/clangd/TestFS.cpp
index b3081d6430b..d3112b92278 100644
--- a/clang-tools-extra/unittests/clangd/TestFS.cpp
+++ b/clang-tools-extra/unittests/clangd/TestFS.cpp
@@ -32,8 +32,10 @@ buildTestFS(llvm::StringMap<std::string> const &Files,
   return MemFS;
 }
 
-MockCompilationDatabase::MockCompilationDatabase(bool UseRelPaths)
-    : ExtraClangFlags({"-ffreestanding"}), UseRelPaths(UseRelPaths) {
+MockCompilationDatabase::MockCompilationDatabase(StringRef Directory,
+                                                 StringRef RelPathPrefix)
+    : ExtraClangFlags({"-ffreestanding"}), Directory(Directory),
+      RelPathPrefix(RelPathPrefix) {
   // -ffreestanding avoids implicit stdc-predef.h.
 }
 
@@ -42,12 +44,24 @@ MockCompilationDatabase::getCompileCommand(PathRef File) const {
   if (ExtraClangFlags.empty())
     return None;
 
-  auto CommandLine = ExtraClangFlags;
   auto FileName = sys::path::filename(File);
+
+  // Build the compile command.
+  auto CommandLine = ExtraClangFlags;
   CommandLine.insert(CommandLine.begin(), "clang");
-  CommandLine.insert(CommandLine.end(), UseRelPaths ? FileName : File);
-  return {tooling::CompileCommand(sys::path::parent_path(File), FileName,
-                                  std::move(CommandLine), "")};
+  if (RelPathPrefix.empty()) {
+    // Use the absolute path in the compile command.
+    CommandLine.push_back(File);
+  } else {
+    // Build a relative path using RelPathPrefix.
+    SmallString<32> RelativeFilePath(RelPathPrefix);
+    llvm::sys::path::append(RelativeFilePath, FileName);
+    CommandLine.push_back(RelativeFilePath.str());
+  }
+
+  return {tooling::CompileCommand(
+      Directory != StringRef() ? Directory : sys::path::parent_path(File),
+      FileName, std::move(CommandLine), "")};
 }
 
 const char *testRoot() {
diff --git a/clang-tools-extra/unittests/clangd/TestFS.h b/clang-tools-extra/unittests/clangd/TestFS.h
index 9c2a15e63d3..a0efb755d00 100644
--- a/clang-tools-extra/unittests/clangd/TestFS.h
+++ b/clang-tools-extra/unittests/clangd/TestFS.h
@@ -40,15 +40,22 @@ public:
 // A Compilation database that returns a fixed set of compile flags.
 class MockCompilationDatabase : public GlobalCompilationDatabase {
 public:
-  /// When \p UseRelPaths is true, uses relative paths in compile commands.
-  /// When \p UseRelPaths is false, uses absoulte paths.
-  MockCompilationDatabase(bool UseRelPaths = false);
+  /// If \p Directory is not empty, use that as the Directory field of the
+  /// CompileCommand.
+  ///
+  /// If \p RelPathPrefix is not empty, use that as a prefix in front of the
+  /// source file name, instead of using an absolute path.
+  MockCompilationDatabase(StringRef Directory = StringRef(),
+                          StringRef RelPathPrefix = StringRef());
 
   llvm::Optional<tooling::CompileCommand>
   getCompileCommand(PathRef File) const override;
 
   std::vector<std::string> ExtraClangFlags;
-  const bool UseRelPaths;
+
+private:
+  StringRef Directory;
+  StringRef RelPathPrefix;
 };
 
 // Returns an absolute (fake) test directory for this OS.
diff --git a/clang-tools-extra/unittests/clangd/XRefsTests.cpp b/clang-tools-extra/unittests/clangd/XRefsTests.cpp
index 3383dd54341..b08af32df21 100644
--- a/clang-tools-extra/unittests/clangd/XRefsTests.cpp
+++ b/clang-tools-extra/unittests/clangd/XRefsTests.cpp
@@ -312,27 +312,65 @@ TEST(GoToDefinition, All) {
 }
 
 TEST(GoToDefinition, RelPathsInCompileCommand) {
+  // The source is in "/clangd-test/src".
+  // We build in "/clangd-test/build".
+
   Annotations SourceAnnotations(R"cpp(
+#include "header_in_preamble.h"
 int [[foo]];
-int baz = f^oo;
+#include "header_not_in_preamble.h"
+int baz = f$p1^oo + bar_pre$p2^amble + bar_not_pre$p3^amble;
+)cpp");
+
+  Annotations HeaderInPreambleAnnotations(R"cpp(
+int [[bar_preamble]];
+)cpp");
+
+  Annotations HeaderNotInPreambleAnnotations(R"cpp(
+int [[bar_not_preamble]];
 )cpp");
 
+  // Make the compilation paths appear as ../src/foo.cpp in the compile
+  // commands.
+  SmallString<32> RelPathPrefix("..");
+  llvm::sys::path::append(RelPathPrefix, "src");
+  std::string BuildDir = testPath("build");
+  MockCompilationDatabase CDB(BuildDir, RelPathPrefix);
+
   IgnoreDiagnostics DiagConsumer;
-  MockCompilationDatabase CDB(/*UseRelPaths=*/true);
   MockFSProvider FS;
   ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
 
-  auto FooCpp = testPath("foo.cpp");
+  // Fill the filesystem.
+  auto FooCpp = testPath("src/foo.cpp");
   FS.Files[FooCpp] = "";
+  auto HeaderInPreambleH = testPath("src/header_in_preamble.h");
+  FS.Files[HeaderInPreambleH] = HeaderInPreambleAnnotations.code();
+  auto HeaderNotInPreambleH = testPath("src/header_not_in_preamble.h");
+  FS.Files[HeaderNotInPreambleH] = HeaderNotInPreambleAnnotations.code();
 
-  Server.addDocument(FooCpp, SourceAnnotations.code());
   runAddDocument(Server, FooCpp, SourceAnnotations.code());
+
+  // Go to a definition in main source file.
   auto Locations =
-      runFindDefinitions(Server, FooCpp, SourceAnnotations.point());
+      runFindDefinitions(Server, FooCpp, SourceAnnotations.point("p1"));
   EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
-
   EXPECT_THAT(*Locations, ElementsAre(Location{URIForFile{FooCpp},
                                                SourceAnnotations.range()}));
+
+  // Go to a definition in header_in_preamble.h.
+  Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("p2"));
+  EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+  EXPECT_THAT(*Locations,
+              ElementsAre(Location{URIForFile{HeaderInPreambleH},
+                                   HeaderInPreambleAnnotations.range()}));
+
+  // Go to a definition in header_not_in_preamble.h.
+  Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("p3"));
+  EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+  EXPECT_THAT(*Locations,
+              ElementsAre(Location{URIForFile{HeaderNotInPreambleH},
+                                   HeaderNotInPreambleAnnotations.range()}));
 }
 
 TEST(Hover, All) {