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

[Sam McCall via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGc69ae835d0e0: [clangd] Add path mappings functionality 
(authored by sammccall).

Changed prior to commit:
  https://reviews.llvm.org/D64305?vs=236295=236548#toc

Repository:
  rG LLVM Github Monorepo

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, 

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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks! The latest snapshot looks good. Landing it now with a few minor tweaks 
mentioned below.

(And trivial local style things, we generally prefer to drop braces on simple 
if statements etc.)




Comment at: clang-tools-extra/clangd/PathMapping.cpp:26
+void recursivelyMap(llvm::json::Value , PathMapping::Direction Dir,
+const PathMappings , const MapperFunc ) {
+  using Kind = llvm::json::Value::Kind;

Removed the MapperFunc argument here as it's always doPathMapping.

Then this is just applyPathMappings, so merged the two.



Comment at: clang-tools-extra/clangd/PathMapping.h:40
+
+llvm::raw_ostream <<(llvm::raw_ostream , const PathMapping );
+

changed to client=server to match the flag syntax



Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:358
+llvm::cl::desc(
+"Comma separated list of '=' pairs "
+"that can be used to map between file locations on the client "

I extended this doc a bit to clarify what "client" and "server" paths mean, and 
explain the first-match-wins.
(I don't think the example reflects first-match-wins, so I reversed the order)



Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:675
+  auto Err = Mappings.takeError();
+  llvm::errs() << llvm::toString(std::move(Err)) << "\n";
+  return 1;

changed to elog("{0}", Mappings.takeError());


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 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, 

[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  = 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  = 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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Sorry, I was out for a bit and this fell off my radar.
It looks good, but the last snapshot seems to have reverted some earlier 
changes (book vs enum, some doxygen comments etc). Do you know what's up there?

Either way I should be able to apply this on Monday


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

[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 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  = 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  = 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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

LG.  A couple of optional comments to address as you see fit.

It looks like you're not an LLVM committer yet, do you want me to commit this 
for you?
(Usually it's reasonable to ask for this after landing a couple of patches: 
https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access)




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) {

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.



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());

wwagner19 wrote:
> 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.
Yeah, I see what you mean.
My favorite way to structure this code would probably be to always **strip** 
trailing slashes from the mappings, and combine the check-for-match and 
apply-match for a single entry:

```optional doPathMapping(StringRef Path, StringRef From, 
StringRef To) {
  if (Path.consume_front(From) && (Path.empty() || Path.startswith('/'))
return (To + Path).str()
  return None;
}```

Totally up to you, your tests look good :-)


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 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 ,
-bool IsIncoming, const PathMappings );
+void applyPathMappings(llvm::json::Value , bool IsIncoming,
+   const PathMappings );

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 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));
-  

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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks, this looks a lot better.
Main thing that was unclear to me is the fact that the paths in the mappings 
are now URI-paths, so "/C:/foo" on windows.

This looks like the right idea as it ensures much of the path-mapping code gets 
to ignore slashes and such.
Docs/naming could reflect it a bit better.




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

wwagner19 wrote:
> 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. 
Sorry, you're right - I'll fix `json::Object`.
Nevertheless I think the copy-into-new-object approach is the clearest way to 
deal with renames of keys that may otherwise collide in the intermediate state.



Comment at: clang-tools-extra/clangd/PathMapping.cpp:36
   using Kind = llvm::json::Value::Kind;
-  const auto  = V.kind();
+  const Kind  = V.kind();
   if (K == Kind::Object) {

by value, not by ref



Comment at: clang-tools-extra/clangd/PathMapping.cpp:137
 
+// Returns whether a path mapping should take place for \pOrigPath
+// i.e. \pMappingPath is a proper sub-path of \p OrigPath

this isn't a doxygen comment, please omit \p



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) {

"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?



Comment at: clang-tools-extra/clangd/PathMapping.cpp:201
+  const PathMappings ) {
+  if (!S.startswith("file://"))
+return llvm::None;

nit: add a comment like "bail out quickly in the common case"? to make it clear 
this is (only) a performance optimization



Comment at: clang-tools-extra/clangd/PathMapping.cpp:205
+  if (!Uri) {
+return llvm::None;
+  }

you need to consume the error, or it'll assert

consumeError(Uri.takeError());



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());

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 :-/



Comment at: clang-tools-extra/clangd/PathMapping.h:26
+/// dependencies at different locations than the server.
 ///
 /// For example, if the mappings were {{"/home/user", "/workarea"}}, then

add a comment: `Therefore, both paths are represented as in file URI bodies, 
e.g. /etc/passwd or /C:/config.sys`



Comment at: clang-tools-extra/clangd/PathMapping.h:33
+  PathMapping() {}
+  PathMapping(llvm::StringRef ClientPath, llvm::StringRef ServerPath)
+  : ClientPath(ClientPath), ServerPath(ServerPath) {}

nit: you can probably drop these constructors and use `{}` aggregate 
initialization, up to you.



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

nit: please `\p RawPathMappings` with a space, or drop the doxygen tag 
entirely. These are read more often in the code than in rendered documentation, 
and `\pFoo` is hard to read.



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

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" 

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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

(also welcome back and thanks for picking this up!)


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 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 ) {
-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,
-  

[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  = V.kind();
+  if (K == Kind::Object) {
+for (auto  : *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  = 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 with a 

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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks for putting this together. Overall it looks pretty good!

Main issues to address are:

- file path handling should manage windows paths and have tests for this
  - the lit test can be simplified quite a lot I think




Comment at: clang-tools-extra/clangd/PathMapping.cpp:29
+  using Kind = llvm::json::Value::Kind;
+  const auto  = V.kind();
+  if (K == Kind::Object) {

V.kind() is an enum, using `const auto&` here is confusing. It's just `Kind`?



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

object keys may be file uris too. (see `WorkspaceEdit.changes`)

This case is going to be a bit annoying to handle, I think :-(



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));

not a really big deal, but we're forcing a copy here - should doPathMapping 
mutate its argument and return void instead?



Comment at: clang-tools-extra/clangd/PathMapping.cpp:123
+parsePathMappings(const std::vector ) {
+  if (!RawPathMappings.size()) {
+return make_string_error("Must provide at least one path mapping");

I'm not sure why this needs to be an error



Comment at: clang-tools-extra/clangd/PathMapping.cpp:135
+  return make_string_error("Path mapping not absolute: " + HostPath);
+} else if (!llvm::sys::path::is_absolute(RemotePath)) {
+  return make_string_error("Path mapping not absolute: " + RemotePath);

will an absolute windows path `C:\foo` be treated as absolute on a unix machine?
(I know the reverse is true)



Comment at: clang-tools-extra/clangd/PathMapping.cpp:142
+  llvm::raw_string_ostream OS(S);
+  OS << "Parsed path mappings: ";
+  for (const auto  : ParsedMappings)

Please leave this to the caller to log, if necessary.

(I think we're likely to start logging argv/config on startup, so in general 
having every flag logged separately probably isn't desirable)



Comment at: clang-tools-extra/clangd/PathMapping.cpp:154
+  recursivelyMap(
+  MappedParams, [, IsIncoming](llvm::StringRef S) -> std::string {
+if (!S.startswith("file://"))

again, copying every string in the json seems excessive. can we return 
optional and only populate it when we match?



Comment at: clang-tools-extra/clangd/PathMapping.cpp:163
+for (const auto  : Mappings) {
+  const auto  = IsIncoming ? Mapping.first : Mapping.second;
+  const auto  = IsIncoming ? Mapping.second : Mapping.first;

auto here is just string, I think?



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

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.



Comment at: clang-tools-extra/clangd/PathMapping.cpp:169
+auto MappedUri = URI(Uri->scheme(), Uri->authority(), MappedBody);
+vlog("Mapped {0} file path from {1} to {2}",
+ IsIncoming ? "incoming" : "outgoing", Uri->toString(),

This is too verbose for vlog. If you think it's important to keep, it should be 
dlog.



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

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.



Comment at: 

[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 ,
+bool IsIncoming, const PathMappings );

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 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 ) {
+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, Mappings);
+