[clang-tools-extra] r325029 - [clangd] Fixed findDefinitions to always return absolute paths.

[Ilya Biryukov via cfe-commits]

Author: ibiryukov
Date: Tue Feb 13 09:47:16 2018
New Revision: 325029

URL: http://llvm.org/viewvc/llvm-project?rev=325029&view=rev
Log:
[clangd] Fixed findDefinitions to always return absolute paths.

Relative paths could be returned in some cases, e.g. when relative
path is used in compilation arguments. This led to crash when trying
to convert the path to URI.

Modified:
    clang-tools-extra/trunk/clangd/XRefs.cpp
    clang-tools-extra/trunk/unittests/clangd/TestFS.cpp
    clang-tools-extra/trunk/unittests/clangd/TestFS.h
    clang-tools-extra/trunk/unittests/clangd/XRefsTests.cpp

Modified: clang-tools-extra/trunk/clangd/XRefs.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/XRefs.cpp?rev=325029&r1=325028&r2=325029&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/XRefs.cpp (original)
+++ clang-tools-extra/trunk/clangd/XRefs.cpp Tue Feb 13 09:47:16 2018
@@ -11,6 +11,7 @@
 #include "URI.h"
 #include "clang/Index/IndexDataConsumer.h"
 #include "clang/Index/IndexingAction.h"
+#include "llvm/Support/Path.h"
 namespace clang {
 namespace clangd {
 using namespace llvm;
@@ -138,10 +139,17 @@ getDeclarationLocation(ParsedAST &AST, c
   Range R = {Begin, End};
   Location L;
 
-  StringRef FilePath = F->tryGetRealPathName();
+  SmallString<64> FilePath = F->tryGetRealPathName();
   if (FilePath.empty())
     FilePath = F->getName();
-  L.uri.file = FilePath;
+  if (!llvm::sys::path::is_absolute(FilePath)) {
+    if (!SourceMgr.getFileManager().makeAbsolutePath(FilePath)) {
+      log("Could not turn relative path to absolute: " + FilePath);
+      return llvm::None;
+    }
+  }
+
+  L.uri.file = FilePath.str();
   L.range = R;
   return L;
 }

Modified: clang-tools-extra/trunk/unittests/clangd/TestFS.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/TestFS.cpp?rev=325029&r1=325028&r2=325029&view=diff
==============================================================================
--- clang-tools-extra/trunk/unittests/clangd/TestFS.cpp (original)
+++ clang-tools-extra/trunk/unittests/clangd/TestFS.cpp Tue Feb 13 09:47:16 2018
@@ -33,8 +33,10 @@ MockFSProvider::getTaggedFileSystem(Path
   return make_tagged(FS, Tag);
 }
 
-MockCompilationDatabase::MockCompilationDatabase()
-    : ExtraClangFlags({"-ffreestanding"}) {} // Avoid implicit stdc-predef.h.
+MockCompilationDatabase::MockCompilationDatabase(bool UseRelPaths)
+    : ExtraClangFlags({"-ffreestanding"}), UseRelPaths(UseRelPaths) {
+  // -ffreestanding avoids implicit stdc-predef.h.
+}
 
 llvm::Optional<tooling::CompileCommand>
 MockCompilationDatabase::getCompileCommand(PathRef File) const {
@@ -42,10 +44,10 @@ MockCompilationDatabase::getCompileComma
     return llvm::None;
 
   auto CommandLine = ExtraClangFlags;
+  auto FileName = llvm::sys::path::filename(File);
   CommandLine.insert(CommandLine.begin(), "clang");
-  CommandLine.insert(CommandLine.end(), File.str());
-  return {tooling::CompileCommand(llvm::sys::path::parent_path(File),
-                                  llvm::sys::path::filename(File),
+  CommandLine.insert(CommandLine.end(), UseRelPaths ? FileName : File);
+  return {tooling::CompileCommand(llvm::sys::path::parent_path(File), FileName,
                                   std::move(CommandLine), "")};
 }
 

Modified: clang-tools-extra/trunk/unittests/clangd/TestFS.h
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/TestFS.h?rev=325029&r1=325028&r2=325029&view=diff
==============================================================================
--- clang-tools-extra/trunk/unittests/clangd/TestFS.h (original)
+++ clang-tools-extra/trunk/unittests/clangd/TestFS.h Tue Feb 13 09:47:16 2018
@@ -37,12 +37,15 @@ public:
 // A Compilation database that returns a fixed set of compile flags.
 class MockCompilationDatabase : public GlobalCompilationDatabase {
 public:
-  MockCompilationDatabase();
+  /// When \p UseRelPaths is true, uses relative paths in compile commands.
+  /// When \p UseRelPaths is false, uses absoulte paths.
+  MockCompilationDatabase(bool UseRelPaths = false);
 
   llvm::Optional<tooling::CompileCommand>
   getCompileCommand(PathRef File) const override;
 
   std::vector<std::string> ExtraClangFlags;
+  const bool UseRelPaths;
 };
 
 // Returns an absolute (fake) test directory for this OS.

Modified: clang-tools-extra/trunk/unittests/clangd/XRefsTests.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/XRefsTests.cpp?rev=325029&r1=325028&r2=325029&view=diff
==============================================================================
--- clang-tools-extra/trunk/unittests/clangd/XRefsTests.cpp (original)
+++ clang-tools-extra/trunk/unittests/clangd/XRefsTests.cpp Tue Feb 13 09:47:16 2018
@@ -8,6 +8,7 @@
 //===----------------------------------------------------------------------===//
 #include "Annotations.h"
 #include "ClangdUnit.h"
+#include "Compiler.h"
 #include "Matchers.h"
 #include "TestFS.h"
 #include "XRefs.h"
@@ -37,6 +38,11 @@ using testing::Field;
 using testing::Matcher;
 using testing::UnorderedElementsAreArray;
 
+class IgnoreDiagnostics : public DiagnosticsConsumer {
+  void onDiagnosticsReady(
+      PathRef File, Tagged<std::vector<DiagWithFixIts>> Diagnostics) override {}
+};
+
 // FIXME: this is duplicated with FileIndexTests. Share it.
 ParsedAST build(StringRef Code) {
   auto TestFile = getVirtualTestFilePath("Foo.cpp");
@@ -227,6 +233,30 @@ TEST(GoToDefinition, All) {
   }
 }
 
+TEST(GoToDefinition, RelPathsInCompileCommand) {
+  Annotations SourceAnnotations(R"cpp(
+[[int foo]];
+int baz = f^oo;
+)cpp");
+
+  IgnoreDiagnostics DiagConsumer;
+  MockCompilationDatabase CDB(/*UseRelPaths=*/true);
+  MockFSProvider FS;
+  ClangdServer Server(CDB, DiagConsumer, FS, /*AsyncThreadsCount=*/0,
+                      /*StorePreambleInMemory=*/true);
+
+  auto FooCpp = getVirtualTestFilePath("foo.cpp");
+  FS.Files[FooCpp] = "";
+
+  Server.addDocument(FooCpp, SourceAnnotations.code());
+  auto Locations = Server.findDefinitions(FooCpp, SourceAnnotations.point());
+  EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+
+  EXPECT_THAT(Locations->Value,
+              ElementsAre(Location{URIForFile{FooCpp.str()},
+                                   SourceAnnotations.range()}));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang