[PATCH] D64305: [clangd] Add path mappings functionality

[William Wagner via Phabricator via cfe-commits]

wwagner19 created this revision.
wwagner19 added reviewers: sammccall, ilya-biryukov.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, 
mgorny.
Herald added a project: clang.

Add path mappings to clangd which translate file URIs on inbound and outbound 
LSP messages. This mapping allows clangd to run in a remote environment (e.g. 
docker), where the source files and dependencies may be at different locations 
than the host. See 
http://lists.llvm.org/pipermail/clangd-dev/2019-January/000231.htm for more.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D64305

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/PathMapping.cpp
  clang-tools-extra/clangd/PathMapping.h
  clang-tools-extra/clangd/test/Inputs/path-mappings/compile_commands.json
  clang-tools-extra/clangd/test/Inputs/path-mappings/definition.jsonrpc
  clang-tools-extra/clangd/test/Inputs/path-mappings/remote/foo.cpp
  clang-tools-extra/clangd/test/Inputs/path-mappings/remote/foo.h
  clang-tools-extra/clangd/test/path-mappings.test
  clang-tools-extra/clangd/tool/ClangdMain.cpp
  clang-tools-extra/clangd/unittests/CMakeLists.txt
  clang-tools-extra/clangd/unittests/PathMappingTests.cpp

Index: clang-tools-extra/clangd/unittests/PathMappingTests.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/unittests/PathMappingTests.cpp
@@ -0,0 +1,132 @@
+//===-- PathMappingTests.cpp  *- C++ -*---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "PathMapping.h"
+#include "llvm/Support/JSON.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+using ::testing::ElementsAre;
+using ::testing::Pair;
+
+TEST(ParsePathMappingTests, ParseFailed) {
+  auto FailedParse = [](const std::vector &RawMappings) {
+auto Mappings = parsePathMappings(RawMappings);
+if (!Mappings) {
+  consumeError(Mappings.takeError());
+  return true;
+}
+return false;
+  };
+  // uneven mappings
+  EXPECT_TRUE(FailedParse({"/home/myuser1|"}));
+  // mappings need to be absolute
+  EXPECT_TRUE(FailedParse({"home/project|/workarea/project"}));
+  // improper delimiter
+  EXPECT_TRUE(FailedParse({"/home||/workarea"}));
+  // no delimiter
+  EXPECT_TRUE(FailedParse({"/home"}));
+}
+
+TEST(ParsePathMappingTests, AllowsWindowsAndUnixPaths) {
+  std::vector RawPathMappings = {
+  "/C:/home/project|/workarea/project",
+  "/home/project/.includes|/C:/opt/include"};
+  auto Parsed = parsePathMappings(RawPathMappings);
+  ASSERT_TRUE(bool(Parsed));
+  EXPECT_THAT(*Parsed,
+  ElementsAre(Pair("/C:/home/project", "/workarea/project"),
+  Pair("/home/project/.includes", "/C:/opt/include")));
+}
+
+TEST(ParsePathMappingTests, ParsesCorrectly) {
+  std::vector RawPathMappings = {
+  "/home/project|/workarea/project",
+  "/home/project/.includes|/opt/include"};
+  auto Parsed = parsePathMappings(RawPathMappings);
+  ASSERT_TRUE(bool(Parsed));
+  EXPECT_THAT(*Parsed,
+  ElementsAre(Pair("/home/project", "/workarea/project"),
+  Pair("/home/project/.includes", "/opt/include")));
+}
+
+TEST(DoPathMappingTests, PreservesOriginalParams) {
+  auto Params = llvm::json::parse(R"({
+"textDocument": {"uri": "file:///home/foo.cpp"},
+"position": {"line": 0, "character": 0}
+  })");
+  ASSERT_TRUE(bool(Params));
+  auto MappedParams =
+  doPathMapping(*Params, /*IsIncoming=*/true, /*Mappings=*/{});
+  EXPECT_EQ(MappedParams, *Params);
+}
+
+TEST(DoPathMappingTests, MapsUsingFirstMatch) {
+  auto Params = llvm::json::parse(R"({
+"textDocument": {"uri": "file:///home/project/foo.cpp"},
+"position": {"line": 0, "character": 0}
+})");
+  auto ExpectedParams = llvm::json::parse(R"({
+"textDocument": {"uri": "file:///workarea1/project/foo.cpp"},
+"position": {"line": 0, "character": 0}
+})");
+  ASSERT_TRUE(bool(Params) && bool(ExpectedParams));
+  PathMappings Mappings{{"/home", "/workarea1"}, {"/home", "/workarea2"}};
+  auto MappedParams = doPathMapping(*Params, /*IsIncoming=*/true, Mappings);
+  EXPECT_EQ(MappedParams, *ExpectedParams);
+}
+
+TEST(DoPathMappingTests, MapsOutgoing) {
+  auto Params = llvm::json::parse(R"({
+"result": "file:///opt/include/foo.h"
+})");
+  auto ExpectedParams = llvm::json::parse(R"({
+"result": "file:///home/project/.includes/foo.h"
+})");
+  ASSERT_TRUE(bool(Params) && bool(ExpectedParams));
+  PathMappings Mappings{{"/home/project/.includes", "/opt/include"}};
+  auto MappedParams = doPathMapping(*Params, /*IsIncoming=*/false, Ma

[PATCH] D64305: [clangd] Add path mappings functionality

[William Wagner via Phabricator via cfe-commits]

wwagner19 marked 2 inline comments as done.
wwagner19 added a comment.
Herald added a subscriber: ormris.

Hey,

This is my first proposed change to LLVM, so sorry if I messed anything up. The 
proposed changes here follow from discussion on clangd-dev (from janruary 
 and from 
june  ).
It seems like a rather large one, but fear not, most of the code is simply 
tests and wrapper code.

Happy to hear any feedback, thanks!




Comment at: clang-tools-extra/clangd/PathMapping.h:42
+/// untouched.
+llvm::json::Value doPathMapping(const llvm::json::Value &Params,
+bool IsIncoming, const PathMappings &Mappings);

Ideally this wouldn't be in the public interface, but I wanted to  unit test it 
and wasn't sure of a way to do that cleanly - other than putting it in the 
header.



Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:291
+   "opt/include"),
+llvm::cl::CommaSeparated);
 

Comma separated list here obviously limits the path mapping file paths, but 
there was precedent for this already (in `--QueryDriverGlobs`) and it seemed 
simplest. 

Also,a command-line argument felt the most straightforward, as I'm not aware of 
any clangd project settings file (please lmk if there is one :) ). Users can 
set up custom path mappings by using e.g. vscode workspace `settings.json`, 
coc.nvim `coc-settings.json`


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64305/new/

https://reviews.llvm.org/D64305



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

[PATCH] D64305: [clangd] Add path mappings functionality

[William Wagner via Phabricator via cfe-commits]

wwagner19 marked 9 inline comments as done.
wwagner19 added a comment.

Thanks for all the feedback, Sam! I'll try and get an updated patch up sometime 
tomorrow.




Comment at: clang-tools-extra/clangd/PathMapping.cpp:30
+  const auto &K = V.kind();
+  if (K == Kind::Object) {
+for (auto &KV : *V.getAsObject()) {

sammccall wrote:
> object keys may be file uris too. (see `WorkspaceEdit.changes`)
> 
> This case is going to be a bit annoying to handle, I think :-(
Indeed, it seems so. It doesn't look like `json::Object` has any key removal 
functionality (although I could very well be reading this wrong). If so, then I 
guess I can just create a new `json::Object`, moving over the old values, and 
replacing the keys if necessary. 



Comment at: clang-tools-extra/clangd/PathMapping.cpp:51
+llvm::json::Value MappedParams =
+doPathMapping(Params, /*IsIncoming=*/true, Mappings);
+return WrappedHandler.onNotify(Method, std::move(MappedParams));

sammccall wrote:
> not a really big deal, but we're forcing a copy here - should doPathMapping 
> mutate its argument and return void instead?
yup makes sense!



Comment at: clang-tools-extra/clangd/PathMapping.cpp:165
+  const auto &To = IsIncoming ? Mapping.second : Mapping.first;
+  if (Uri->body().startswith(From)) {
+std::string MappedBody = Uri->body();

sammccall wrote:
> This is simplistic I'm afraid: it's not going to work at all on windows 
> (where paths have `\` but URIs have `/`), and it's going to falsely match 
> "/foo-bar/baz" against a mapping for "/foo".
> 
> `llvm::sys::fs::replace_path_prefix` is probably the beginning of a solution. 
> If I'm reading correctly the first two args need to have the same slash style 
> and OldPrefix should have its trailing slash.
> 
> I'd actually suggest pulling out the function to map one string, and 
> unit-testing that, to catch all the filename cases.
> 
> Then the json::Value traversal tests should be focused on testing the places 
> where a string can appear in JSON, not all the different contents of the 
> string.
Ah yea good catch, this will be a bit more tricky then. I was originally just 
imagining the users using strictly URI syntax in the `--path-mappings`, but 
that's doesn't seem very friendly in hindsight. So just to clarify, we should:
1. Allow the users to specify windows style paths (e.g. C:\foo) and posix style 
paths
2. Allow the inter-op of both, i.e. `--path-mappings="C:\foo=/bar"`

IIUC, file URIs will always have the forward-slash syntax, so this may require 
storing the windows-style path mapping in forward-slash style. I can try and 
get this going tomorrow. Although, one tricky thing might be trying to figure 
out if a path is indeed windows-style (in a unix environment where _WIN32 isn't 
defined). 



Comment at: clang-tools-extra/clangd/PathMapping.h:21
+
+/// PathMappings are a collection of paired host and remote paths.
+/// These pairs are used to alter file:// URIs appearing in inbound and 
outbound

sammccall wrote:
> hmm, the host/remote terminology is a bit confusing to me.
> I guess the host is the machine where clangd is running, and remote is the 
> other end, but  from the user's point of view this is a configuration where 
> the clangd is remote.
> 
> What about calling these client/server paths? The language client/server 
> roles are defined in the spec and these terms are likely to make some sense 
> to users.
Agreed, sounds better



Comment at: clang-tools-extra/clangd/PathMapping.h:32
+
+/// Parse the command line \pRawPathMappings (e.g. "/host|/remote") into
+/// pairs. Returns an error if the mappings are malformed, i.e. not absolute or

sammccall wrote:
> I'd suggest `=` as a delimiter instead, it's more evocative and more common.
> 
> The order is tricky, I suspect `/client/path=/server/path` is going to be 
> more intuitive
Way better :)



Comment at: 
clang-tools-extra/clangd/test/Inputs/path-mappings/definition.jsonrpc:26
+---
+{
+  "jsonrpc": "2.0",

sammccall wrote:
> This test seems to have unneccesary moving parts. Was it copied from the 
> background index test?
> 
> One header file on disk and one draft file sent over RPC should be enough. A 
> compilation database shouldn't be necessary I think.
> 
> You shouldn't need to splice URIs, because the input and output paths are 
> virtual and fully under your control (that's the point of this patch, :)). So 
> the test should be able to run on windows.
> 
> I think this test can actually be a standard one where the JSON-RPC calls and 
> assertions go in the *.test file.
This was copied from the background test, I felt a bit uneasy about how 
complicated it got, but I had a bit of trouble getting a simpler one going. 
You're right though, I can't see why this wouldn't work w

[PATCH] D64305: [clangd] Add path mappings functionality

[William Wagner via Phabricator via cfe-commits]

wwagner19 updated this revision to Diff 236293.
wwagner19 added a comment.

The last diff was broken, this most recent one

- Changes IsIncoming boolean to an enum
- Refactors the matching path logic


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64305/new/

https://reviews.llvm.org/D64305

Files:
  clang-tools-extra/clangd/PathMapping.cpp


Index: clang-tools-extra/clangd/PathMapping.cpp
===
--- clang-tools-extra/clangd/PathMapping.cpp
+++ clang-tools-extra/clangd/PathMapping.cpp
@@ -128,21 +128,6 @@
  llvm::inconvertibleErrorCode());
 }
 
-// Returns whether a path mapping should take place for OrigPath
-// i.e. MappingPath is a proper sub-path of  OrigPath
-bool mappingMatches(llvm::StringRef OrigPath, llvm::StringRef MappingPath) {
-  namespace path = llvm::sys::path;
-  auto OrigPathItr = path::begin(OrigPath, path::Style::posix);
-  auto OrigPathEnd = path::end(OrigPath);
-  auto MappingPathItr = path::begin(MappingPath, path::Style::posix);
-  auto MappingPathEnd = path::end(MappingPath);
-  if (std::distance(OrigPathItr, OrigPathEnd) <
-  std::distance(MappingPathItr, MappingPathEnd)) {
-return false;
-  }
-  return std::equal(MappingPathItr, MappingPathEnd, OrigPathItr);
-}
-
 // Converts a unix/windows path to the path portion of a file URI
 // e.g. "C:\foo" -> "/C:/foo"
 llvm::Expected parsePath(llvm::StringRef Path) {
@@ -207,11 +192,11 @@
 const std::string &To = Dir == PathMapping::Direction::ClientToServer
 ? Mapping.ServerPath
 : Mapping.ClientPath;
-if (mappingMatches(Uri->body(), From)) {
-  std::string MappedBody = std::move(Uri->body());
-  MappedBody.replace(MappedBody.find(From), From.length(), To);
-  auto MappedUri = URI(Uri->scheme(), Uri->authority(), 
MappedBody.c_str());
-  return MappedUri.toString();
+llvm::StringRef Body = Uri->body();
+if (Body.consume_front(From) && (Body.empty() || Body.front() == '/')) {
+  std::string MappedBody = (To + Body).str();
+  return URI(Uri->scheme(), Uri->authority(), MappedBody.c_str())
+  .toString();
 }
   }
   return llvm::None;


Index: clang-tools-extra/clangd/PathMapping.cpp
===
--- clang-tools-extra/clangd/PathMapping.cpp
+++ clang-tools-extra/clangd/PathMapping.cpp
@@ -128,21 +128,6 @@
  llvm::inconvertibleErrorCode());
 }
 
-// Returns whether a path mapping should take place for OrigPath
-// i.e. MappingPath is a proper sub-path of  OrigPath
-bool mappingMatches(llvm::StringRef OrigPath, llvm::StringRef MappingPath) {
-  namespace path = llvm::sys::path;
-  auto OrigPathItr = path::begin(OrigPath, path::Style::posix);
-  auto OrigPathEnd = path::end(OrigPath);
-  auto MappingPathItr = path::begin(MappingPath, path::Style::posix);
-  auto MappingPathEnd = path::end(MappingPath);
-  if (std::distance(OrigPathItr, OrigPathEnd) <
-  std::distance(MappingPathItr, MappingPathEnd)) {
-return false;
-  }
-  return std::equal(MappingPathItr, MappingPathEnd, OrigPathItr);
-}
-
 // Converts a unix/windows path to the path portion of a file URI
 // e.g. "C:\foo" -> "/C:/foo"
 llvm::Expected parsePath(llvm::StringRef Path) {
@@ -207,11 +192,11 @@
 const std::string &To = Dir == PathMapping::Direction::ClientToServer
 ? Mapping.ServerPath
 : Mapping.ClientPath;
-if (mappingMatches(Uri->body(), From)) {
-  std::string MappedBody = std::move(Uri->body());
-  MappedBody.replace(MappedBody.find(From), From.length(), To);
-  auto MappedUri = URI(Uri->scheme(), Uri->authority(), MappedBody.c_str());
-  return MappedUri.toString();
+llvm::StringRef Body = Uri->body();
+if (Body.consume_front(From) && (Body.empty() || Body.front() == '/')) {
+  std::string MappedBody = (To + Body).str();
+  return URI(Uri->scheme(), Uri->authority(), MappedBody.c_str())
+  .toString();
 }
   }
   return llvm::None;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64305: [clangd] Add path mappings functionality

[William Wagner via Phabricator via cfe-commits]

wwagner19 updated this revision to Diff 236295.
wwagner19 added a comment.

To be honest, I'm not sure how to remedy this. So I just rebased all my commits 
into one and dropped the `git show HEAD -U99` into here.

Please let me know if you need me to fix anything / open a new diff.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64305/new/

https://reviews.llvm.org/D64305

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/PathMapping.cpp
  clang-tools-extra/clangd/PathMapping.h
  clang-tools-extra/clangd/test/Inputs/path-mappings/server/foo.h
  clang-tools-extra/clangd/test/path-mappings.test
  clang-tools-extra/clangd/tool/ClangdMain.cpp
  clang-tools-extra/clangd/unittests/CMakeLists.txt
  clang-tools-extra/clangd/unittests/PathMappingTests.cpp

Index: clang-tools-extra/clangd/unittests/PathMappingTests.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/unittests/PathMappingTests.cpp
@@ -0,0 +1,216 @@
+//===-- PathMappingTests.cpp  *- C++ -*---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "PathMapping.h"
+#include "llvm/Support/JSON.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+#include 
+namespace clang {
+namespace clangd {
+namespace {
+using ::testing::ElementsAre;
+MATCHER_P2(Mapping, ClientPath, ServerPath, "") {
+  return arg.ClientPath == ClientPath && arg.ServerPath == ServerPath;
+}
+
+bool failedParse(llvm::StringRef RawMappings) {
+  llvm::Expected Mappings = parsePathMappings(RawMappings);
+  if (!Mappings) {
+consumeError(Mappings.takeError());
+return true;
+  }
+  return false;
+}
+
+TEST(ParsePathMappingTests, WindowsPath) {
+  // Relative path to C drive
+  EXPECT_TRUE(failedParse(R"(C:a=/root)"));
+  EXPECT_TRUE(failedParse(R"(\C:a=/root)"));
+  // Relative path to current drive.
+  EXPECT_TRUE(failedParse(R"(\a=/root)"));
+  // Absolute paths
+  llvm::Expected ParsedMappings =
+  parsePathMappings(R"(C:\a=/root)");
+  ASSERT_TRUE(bool(ParsedMappings));
+  EXPECT_THAT(*ParsedMappings, ElementsAre(Mapping("/C:/a", "/root")));
+  // Absolute UNC path
+  ParsedMappings = parsePathMappings(R"(\\Server\C$=/root)");
+  ASSERT_TRUE(bool(ParsedMappings));
+  EXPECT_THAT(*ParsedMappings, ElementsAre(Mapping("//Server/C$", "/root")));
+}
+
+TEST(ParsePathMappingTests, UnixPath) {
+  // Relative unix path
+  EXPECT_TRUE(failedParse("a/b=/root"));
+  // Absolute unix path
+  llvm::Expected ParsedMappings = parsePathMappings("/A/b=/root");
+  ASSERT_TRUE(bool(ParsedMappings));
+  EXPECT_THAT(*ParsedMappings, ElementsAre(Mapping("/A/b", "/root")));
+  // Aboslute unix path w/ backslash
+  ParsedMappings = parsePathMappings(R"(/a/b\\ar=/root)");
+  ASSERT_TRUE(bool(ParsedMappings));
+  EXPECT_THAT(*ParsedMappings, ElementsAre(Mapping(R"(/a/b\\ar)", "/root")));
+}
+
+TEST(ParsePathMappingTests, ImproperFormat) {
+  // uneven mappings
+  EXPECT_TRUE(failedParse("/home/myuser1="));
+  // mappings need to be absolute
+  EXPECT_TRUE(failedParse("home/project=/workarea/project"));
+  // duplicate delimiter
+  EXPECT_TRUE(failedParse("/home==/workarea"));
+  // no delimiter
+  EXPECT_TRUE(failedParse("/home"));
+  // improper delimiter
+  EXPECT_TRUE(failedParse("/home,/workarea"));
+}
+
+TEST(ParsePathMappingTests, ParsesMultiple) {
+  std::string RawPathMappings =
+  "/home/project=/workarea/project,/home/project/.includes=/opt/include";
+  auto Parsed = parsePathMappings(RawPathMappings);
+  ASSERT_TRUE(bool(Parsed));
+  EXPECT_THAT(*Parsed,
+  ElementsAre(Mapping("/home/project", "/workarea/project"),
+  Mapping("/home/project/.includes", "/opt/include")));
+}
+
+bool mapsProperly(llvm::StringRef Orig, llvm::StringRef Expected,
+  llvm::StringRef RawMappings, PathMapping::Direction Dir) {
+  llvm::Expected Mappings = parsePathMappings(RawMappings);
+  if (!Mappings)
+return false;
+  llvm::Optional MappedPath = doPathMapping(Orig, Dir, *Mappings);
+  std::string Actual = MappedPath ? *MappedPath : Orig.str();
+  EXPECT_STREQ(Expected.str().c_str(), Actual.c_str());
+  return Expected == Actual;
+}
+
+TEST(DoPathMappingTests, PreservesOriginal) {
+  // Preserves original path when no mapping
+  EXPECT_TRUE(mapsProperly("file:///home", "file:///home", "",
+   PathMapping::Direction::ClientToServer));
+}
+
+TEST(DoPathMappingTests, UsesFirstMatch) {
+  EXPECT_TRUE(mapsProperly("file:///home/foo.cpp", "file:///workarea1/foo.cpp",
+   "/home=/workarea1,/home=/workarea2",
+   PathMapping::Direction::ClientToServer));
+}
+
+TEST(DoPathMappingTests, IgnoresSubstrings

[PATCH] D64305: [clangd] Add path mappings functionality

[William Wagner via Phabricator via cfe-commits]

wwagner19 updated this revision to Diff 227643.
wwagner19 added a comment.
Herald added a subscriber: usaxena95.

Unfortunately, I had to take a bit of a hiatus there, but i'm back a few months 
later with an updated patch incorporating all of @sammccall 's feedback! 
Notably,

- Windows paths are now accounted for, basically we first try to parse a unix 
path, and fall back to windows if possible. After, windows paths are converted 
to forward-slash notation, so the prefix stuff can work.
- Mapping LSP jsonrpc keys is now done, albeit a bit awkward due to no delete 
key/value API
- The lit test is improved, as it no longer relies on background index and 
tests windows client path

As for the validity of this feature, I am well aware of vscode's remote 
feature, but it is still essential for vim/emacs/etc. editors, IMO.

Please take a look when you have a chance, thanks.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64305/new/

https://reviews.llvm.org/D64305

Files:
  clang-tools-extra/clangd/PathMapping.cpp
  clang-tools-extra/clangd/PathMapping.h
  clang-tools-extra/clangd/test/Inputs/path-mappings/compile_commands.json
  clang-tools-extra/clangd/test/Inputs/path-mappings/definition.jsonrpc
  clang-tools-extra/clangd/test/Inputs/path-mappings/remote/foo.cpp
  clang-tools-extra/clangd/test/Inputs/path-mappings/remote/foo.h
  clang-tools-extra/clangd/test/Inputs/path-mappings/server/foo.h
  clang-tools-extra/clangd/test/path-mappings.test
  clang-tools-extra/clangd/tool/ClangdMain.cpp
  clang-tools-extra/clangd/unittests/PathMappingTests.cpp

Index: clang-tools-extra/clangd/unittests/PathMappingTests.cpp
===
--- clang-tools-extra/clangd/unittests/PathMappingTests.cpp
+++ clang-tools-extra/clangd/unittests/PathMappingTests.cpp
@@ -10,121 +10,200 @@
 #include "llvm/Support/JSON.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
-
+#include 
 namespace clang {
 namespace clangd {
 namespace {
 using ::testing::ElementsAre;
-using ::testing::Pair;
-
-TEST(ParsePathMappingTests, ParseFailed) {
-  auto FailedParse = [](const std::vector &RawMappings) {
-auto Mappings = parsePathMappings(RawMappings);
-if (!Mappings) {
-  consumeError(Mappings.takeError());
-  return true;
-}
-return false;
-  };
+MATCHER_P2(Mapping, ClientPath, ServerPath, "") {
+  return arg.ClientPath == ClientPath && arg.ServerPath == ServerPath;
+}
+
+bool failedParse(llvm::StringRef RawMappings) {
+  llvm::Expected Mappings = parsePathMappings(RawMappings);
+  if (!Mappings) {
+consumeError(Mappings.takeError());
+return true;
+  }
+  return false;
+}
+
+TEST(ParsePathMappingTests, WindowsPath) {
+  // Relative path to C drive
+  EXPECT_TRUE(failedParse(R"(C:a=/root)"));
+  EXPECT_TRUE(failedParse(R"(\C:a=/root)"));
+  // Relative path to current drive.
+  EXPECT_TRUE(failedParse(R"(\a=/root)"));
+  // Absolute paths
+  llvm::Expected ParsedMappings =
+  parsePathMappings(R"(C:\a=/root)");
+  ASSERT_TRUE(bool(ParsedMappings));
+  EXPECT_THAT(*ParsedMappings, ElementsAre(Mapping("/C:/a", "/root")));
+  // Absolute UNC path
+  ParsedMappings = parsePathMappings(R"(\\Server\C$=/root)");
+  ASSERT_TRUE(bool(ParsedMappings));
+  EXPECT_THAT(*ParsedMappings, ElementsAre(Mapping("//Server/C$", "/root")));
+}
+
+TEST(ParsePathMappingTests, UnixPath) {
+  // Relative unix path
+  EXPECT_TRUE(failedParse("a/b=/root"));
+  // Absolute unix path
+  llvm::Expected ParsedMappings = parsePathMappings("/A/b=/root");
+  ASSERT_TRUE(bool(ParsedMappings));
+  EXPECT_THAT(*ParsedMappings, ElementsAre(Mapping("/A/b", "/root")));
+  // Aboslute unix path w/ backslash
+  ParsedMappings = parsePathMappings(R"(/a/b\\ar=/root)");
+  ASSERT_TRUE(bool(ParsedMappings));
+  EXPECT_THAT(*ParsedMappings, ElementsAre(Mapping(R"(/a/b\\ar)", "/root")));
+}
+
+TEST(ParsePathMappingTests, ImproperFormat) {
   // uneven mappings
-  EXPECT_TRUE(FailedParse({"/home/myuser1|"}));
+  EXPECT_TRUE(failedParse("/home/myuser1="));
   // mappings need to be absolute
-  EXPECT_TRUE(FailedParse({"home/project|/workarea/project"}));
-  // improper delimiter
-  EXPECT_TRUE(FailedParse({"/home||/workarea"}));
+  EXPECT_TRUE(failedParse("home/project=/workarea/project"));
+  // duplicate delimiter
+  EXPECT_TRUE(failedParse("/home==/workarea"));
   // no delimiter
-  EXPECT_TRUE(FailedParse({"/home"}));
+  EXPECT_TRUE(failedParse("/home"));
+  // improper delimiter
+  EXPECT_TRUE(failedParse("/home,/workarea"));
 }
 
-TEST(ParsePathMappingTests, AllowsWindowsAndUnixPaths) {
-  std::vector RawPathMappings = {
-  "/C:/home/project|/workarea/project",
-  "/home/project/.includes|/C:/opt/include"};
+TEST(ParsePathMappingTests, ParsesMultiple) {
+  std::string RawPathMappings =
+  "/home/project=/workarea/project,/home/project/.includes=/opt/include";
   auto Parsed = parsePathMappings(RawPathMappings);
   ASSERT_TRUE(bool(Parsed));
   EXPECT_THAT(*Parsed,
-  E

[PATCH] D64305: [clangd] Add path mappings functionality

[William Wagner via Phabricator via cfe-commits]

wwagner19 updated this revision to Diff 229721.
wwagner19 marked 5 inline comments as done.
wwagner19 added a comment.

Thanks for the second review Sam, I addressed most of your comments, notably:

- Changed the bool IsIncoming to an enum
- Fixed the "doxygen" comments,
- Removed some redundant incudes/variables
- Switched `replace_path_prefix` to string replace


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64305/new/

https://reviews.llvm.org/D64305

Files:
  clang-tools-extra/clangd/PathMapping.cpp
  clang-tools-extra/clangd/PathMapping.h
  clang-tools-extra/clangd/tool/ClangdMain.cpp
  clang-tools-extra/clangd/unittests/PathMappingTests.cpp

Index: clang-tools-extra/clangd/unittests/PathMappingTests.cpp
===
--- clang-tools-extra/clangd/unittests/PathMappingTests.cpp
+++ clang-tools-extra/clangd/unittests/PathMappingTests.cpp
@@ -82,12 +82,11 @@
 }
 
 bool mapsProperly(llvm::StringRef Orig, llvm::StringRef Expected,
-  llvm::StringRef RawMappings, bool IsIncoming) {
+  llvm::StringRef RawMappings, PathMapping::Direction Dir) {
   llvm::Expected Mappings = parsePathMappings(RawMappings);
   if (!Mappings)
 return false;
-  llvm::Optional MappedPath =
-  doPathMapping(Orig, IsIncoming, *Mappings);
+  llvm::Optional MappedPath = doPathMapping(Orig, Dir, *Mappings);
   std::string Actual = MappedPath ? *MappedPath : Orig.str();
   EXPECT_STREQ(Expected.str().c_str(), Actual.c_str());
   return Expected == Actual;
@@ -95,48 +94,54 @@
 
 TEST(DoPathMappingTests, PreservesOriginal) {
   // Preserves original path when no mapping
-  EXPECT_TRUE(mapsProperly("file:///home", "file:///home", "", true));
+  EXPECT_TRUE(mapsProperly("file:///home", "file:///home", "",
+   PathMapping::Direction::ClientToServer));
 }
 
 TEST(DoPathMappingTests, UsesFirstMatch) {
   EXPECT_TRUE(mapsProperly("file:///home/foo.cpp", "file:///workarea1/foo.cpp",
-   "/home=/workarea1,/home=/workarea2", true));
+   "/home=/workarea1,/home=/workarea2",
+   PathMapping::Direction::ClientToServer));
 }
 
 TEST(DoPathMappingTests, IgnoresSubstrings) {
   // Doesn't map substrings that aren't a proper path prefix
   EXPECT_TRUE(mapsProperly("file://home/foo-bar.cpp", "file://home/foo-bar.cpp",
-   "/home/foo=/home/bar", true));
+   "/home/foo=/home/bar",
+   PathMapping::Direction::ClientToServer));
 }
 
 TEST(DoPathMappingTests, MapsOutgoingPaths) {
   // When IsIncoming is false (i.e.a  response), map the other way
   EXPECT_TRUE(mapsProperly("file:///workarea/foo.cpp", "file:///home/foo.cpp",
-   "/home=/workarea", false));
+   "/home=/workarea",
+   PathMapping::Direction::ServerToClient));
 }
 
 TEST(DoPathMappingTests, OnlyMapFileUris) {
   EXPECT_TRUE(mapsProperly("test:///home/foo.cpp", "test:///home/foo.cpp",
-   "/home=/workarea", true));
+   "/home=/workarea",
+   PathMapping::Direction::ClientToServer));
 }
 
 TEST(DoPathMappingTests, RespectsCaseSensitivity) {
   EXPECT_TRUE(mapsProperly("file:///HOME/foo.cpp", "file:///HOME/foo.cpp",
-   "/home=/workarea", true));
+   "/home=/workarea",
+   PathMapping::Direction::ClientToServer));
 }
 
 TEST(DoPathMappingTests, MapsWindowsPaths) {
   // Maps windows properly
   EXPECT_TRUE(mapsProperly("file:///C:/home/foo.cpp",
-   "file:///C:/workarea/foo.cpp", "C:/home=C:/workarea",
-   true));
+   "file:///C:/workarea/foo.cpp", R"(C:\home=C:\workarea)",
+   PathMapping::Direction::ClientToServer));
 }
 
 TEST(DoPathMappingTests, MapsWindowsUnixInterop) {
   // Path mappings with a windows-style client path and unix-style server path
-  EXPECT_TRUE(mapsProperly("file:///C:/home/foo.cpp",
-   "file:///C:/workarea/foo.cpp",
-   "C:/home=/C:/workarea", true));
+  EXPECT_TRUE(mapsProperly(
+  "file:///C:/home/foo.cpp", "file:///workarea/foo.cpp",
+  R"(C:\home=/workarea)", PathMapping::Direction::ClientToServer));
 }
 
 TEST(ApplyPathMappingTests, PreservesOriginalParams) {
@@ -147,7 +152,7 @@
   ASSERT_TRUE(bool(Params));
   llvm::json::Value ExpectedParams = *Params;
   PathMappings Mappings;
-  applyPathMappings(*Params, /*IsIncoming=*/true, Mappings);
+  applyPathMappings(*Params, PathMapping::Direction::ClientToServer, Mappings);
   EXPECT_EQ(*Params, ExpectedParams);
 }
 
@@ -163,7 +168,7 @@
   })");
   auto Mappings = parsePathMappings("/home=/workarea");
   ASSERT_TRUE(bool(Params) && bool(ExpectedParams) && bool(Mappings));
-  applyPathMappings(*Para

[PATCH] D64305: [clangd] Add path mappings functionality

[William Wagner via Phabricator via cfe-commits]

wwagner19 marked 9 inline comments as done and an inline comment as not done.
wwagner19 added inline comments.



Comment at: clang-tools-extra/clangd/PathMapping.cpp:153
+// Converts \pPath to a posix-style absolute, i.e. if it's a windows path
+// then the backward-slash notation will be converted to forward slash
+llvm::Expected parsePath(llvm::StringRef Path) {

sammccall wrote:
> "posix-style" doesn't really describe representing `c:\foo` as `/c:/foo`. 
> That's really *just* a URI thing AFAIK.
> 
> Something like "Converts a unix/windows path to the path part of a file URI".
> But in that case, I think the implementation is just 
> `URI::createFile(Path).body()`. Does that pass tests?
Oh I did not realize `createFile` was a thing, however looking at the way it's 
implemented now, won't that throw an assert if a non-absolute path is passed 
in? If so, is that desirable at all?

IIUC, if I were to use that API, then wouldn't it make more sense for it to 
return an `llvm::Expected`? If we want to consolidate the logic to one place, 
I'd be happy to try and refactor the signature.



Comment at: clang-tools-extra/clangd/PathMapping.cpp:214
+  llvm::SmallString<256> MappedPath(Uri->body());
+  llvm::sys::path::replace_path_prefix(MappedPath, From, To);
+  auto MappedUri = URI(Uri->scheme(), Uri->authority(), 
MappedPath.c_str());

sammccall wrote:
> Sorry, I know I suggested replace_path_prefix, but now that the mappings 
> consist of paths in their URL form, I think the old string::replace version 
> is what you want :-/
Will do, I like string replace better anyway! 

I'm not a huge fan of the `mappingMatches` function, and would prefer a simple 
string `startswith(from)`, but the only way I may see that working is by 
appending "/" to all the path mappings internally - which would prevent `/foo` 
matching `/foo-bar` - but appending a "/" would break directory-based file 
URIs, I believe.



Comment at: clang-tools-extra/clangd/PathMapping.h:56
 /// untouched.
-llvm::json::Value doPathMapping(const llvm::json::Value &Params,
-bool IsIncoming, const PathMappings &Mappings);
+void applyPathMappings(llvm::json::Value &Params, bool IsIncoming,
+   const PathMappings &Mappings);

sammccall wrote:
> nit: the sense of the bool is pretty arbitrary here, prefer a two value enum?
> 
> e.g. `enum class PathMapping::Direction { ServerToClient, ClientToServer }`
> 
> (Reusing the "server" and "client" concept rather than adding 
> "incoming/outgoing" seems a little simpler, though up to you)
much much better that way, thanks


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64305/new/

https://reviews.llvm.org/D64305



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

[PATCH] D64305: [clangd] Add path mappings functionality

[William Wagner via Phabricator via cfe-commits]

wwagner19 updated this revision to Diff 229969.
wwagner19 marked 2 inline comments as done.
wwagner19 added a comment.

Awesome! I am not an LLVM committer, so if you could commit on my behalf that'd 
be great- although I'm not sure how LLVM handles squashing/merging, continuous 
integration, etc., so please let me know if I need to do anything else (aside 
from the code of course).

Once again, thanks for all the help - I learned a lot!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64305/new/

https://reviews.llvm.org/D64305

Files:
  clang-tools-extra/clangd/PathMapping.cpp


Index: clang-tools-extra/clangd/PathMapping.cpp
===
--- clang-tools-extra/clangd/PathMapping.cpp
+++ clang-tools-extra/clangd/PathMapping.cpp
@@ -128,21 +128,6 @@
  llvm::inconvertibleErrorCode());
 }
 
-// Returns whether a path mapping should take place for OrigPath
-// i.e. MappingPath is a proper sub-path of  OrigPath
-bool mappingMatches(llvm::StringRef OrigPath, llvm::StringRef MappingPath) {
-  namespace path = llvm::sys::path;
-  auto OrigPathItr = path::begin(OrigPath, path::Style::posix);
-  auto OrigPathEnd = path::end(OrigPath);
-  auto MappingPathItr = path::begin(MappingPath, path::Style::posix);
-  auto MappingPathEnd = path::end(MappingPath);
-  if (std::distance(OrigPathItr, OrigPathEnd) <
-  std::distance(MappingPathItr, MappingPathEnd)) {
-return false;
-  }
-  return std::equal(MappingPathItr, MappingPathEnd, OrigPathItr);
-}
-
 // Converts a unix/windows path to the path portion of a file URI
 // e.g. "C:\foo" -> "/C:/foo"
 llvm::Expected parsePath(llvm::StringRef Path) {
@@ -207,11 +192,11 @@
 const std::string &To = Dir == PathMapping::Direction::ClientToServer
 ? Mapping.ServerPath
 : Mapping.ClientPath;
-if (mappingMatches(Uri->body(), From)) {
-  std::string MappedBody = std::move(Uri->body());
-  MappedBody.replace(MappedBody.find(From), From.length(), To);
-  auto MappedUri = URI(Uri->scheme(), Uri->authority(), 
MappedBody.c_str());
-  return MappedUri.toString();
+llvm::StringRef Body = Uri->body();
+if (Body.consume_front(From) && (Body.empty() || Body.front() == '/')) {
+  std::string MappedBody = (To + Body).str();
+  return URI(Uri->scheme(), Uri->authority(), MappedBody.c_str())
+  .toString();
 }
   }
   return llvm::None;


Index: clang-tools-extra/clangd/PathMapping.cpp
===
--- clang-tools-extra/clangd/PathMapping.cpp
+++ clang-tools-extra/clangd/PathMapping.cpp
@@ -128,21 +128,6 @@
  llvm::inconvertibleErrorCode());
 }
 
-// Returns whether a path mapping should take place for OrigPath
-// i.e. MappingPath is a proper sub-path of  OrigPath
-bool mappingMatches(llvm::StringRef OrigPath, llvm::StringRef MappingPath) {
-  namespace path = llvm::sys::path;
-  auto OrigPathItr = path::begin(OrigPath, path::Style::posix);
-  auto OrigPathEnd = path::end(OrigPath);
-  auto MappingPathItr = path::begin(MappingPath, path::Style::posix);
-  auto MappingPathEnd = path::end(MappingPath);
-  if (std::distance(OrigPathItr, OrigPathEnd) <
-  std::distance(MappingPathItr, MappingPathEnd)) {
-return false;
-  }
-  return std::equal(MappingPathItr, MappingPathEnd, OrigPathItr);
-}
-
 // Converts a unix/windows path to the path portion of a file URI
 // e.g. "C:\foo" -> "/C:/foo"
 llvm::Expected parsePath(llvm::StringRef Path) {
@@ -207,11 +192,11 @@
 const std::string &To = Dir == PathMapping::Direction::ClientToServer
 ? Mapping.ServerPath
 : Mapping.ClientPath;
-if (mappingMatches(Uri->body(), From)) {
-  std::string MappedBody = std::move(Uri->body());
-  MappedBody.replace(MappedBody.find(From), From.length(), To);
-  auto MappedUri = URI(Uri->scheme(), Uri->authority(), MappedBody.c_str());
-  return MappedUri.toString();
+llvm::StringRef Body = Uri->body();
+if (Body.consume_front(From) && (Body.empty() || Body.front() == '/')) {
+  std::string MappedBody = (To + Body).str();
+  return URI(Uri->scheme(), Uri->authority(), MappedBody.c_str())
+  .toString();
 }
   }
   return llvm::None;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64305: [clangd] Add path mappings functionality

[William Wagner via Phabricator via cfe-commits]

wwagner19 marked 2 inline comments as done.
wwagner19 added inline comments.



Comment at: clang-tools-extra/clangd/PathMapping.cpp:153
+// Converts \pPath to a posix-style absolute, i.e. if it's a windows path
+// then the backward-slash notation will be converted to forward slash
+llvm::Expected parsePath(llvm::StringRef Path) {

sammccall wrote:
> wwagner19 wrote:
> > sammccall wrote:
> > > "posix-style" doesn't really describe representing `c:\foo` as `/c:/foo`. 
> > > That's really *just* a URI thing AFAIK.
> > > 
> > > Something like "Converts a unix/windows path to the path part of a file 
> > > URI".
> > > But in that case, I think the implementation is just 
> > > `URI::createFile(Path).body()`. Does that pass tests?
> > Oh I did not realize `createFile` was a thing, however looking at the way 
> > it's implemented now, won't that throw an assert if a non-absolute path is 
> > passed in? If so, is that desirable at all?
> > 
> > IIUC, if I were to use that API, then wouldn't it make more sense for it to 
> > return an `llvm::Expected`? If we want to consolidate the logic to one 
> > place, I'd be happy to try and refactor the signature.
> I think allowing relative paths to be specified in path mappings is 
> needlessly confusing. Clangd flags are typically specified in a config file 
> somewhere, and that config often doesn't know about the working directory.
> 
> We don't want to hit the assert, so I'd suggest testing if it's absolute 
> first, and returning an error if not.
So even with checking for absolute paths before calling `createFile`, it still 
won't work quite right. Currently, `createFile`, and consequently 
`FileSystemScheme().uriFromAbsolutePath(AbsolutePath)`, converts paths 
differently depending on the environment Clangd is running on (via WIN32 or 
some other means).

e.g. if we had mapping `C:\home=/workarea` and Clangd built/running on linux, 
then the `replace_path_prefix` by default would use posix style, which won't 
replace the `\`. This may not be **too** useful in practice, but it's a small 
price to pay for windows-unix-interop, I feel.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64305/new/

https://reviews.llvm.org/D64305



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

[PATCH] D64305: [clangd] Add path mappings functionality

[William Wagner via Phabricator via cfe-commits]

wwagner19 added a comment.

Hey @sammccall, any update here?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64305/new/

https://reviews.llvm.org/D64305



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