[clangd] Add path mappings functionality

[wwagner19]

Summary

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.

[wwagner19]

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!

clang-tools-extra/clangd/PathMapping.h

42

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.

clang-tools-extra/clangd/tool/ClangdMain.cpp

381

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

[sammccall]

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

clang-tools-extra/clangd/PathMapping.cpp

29

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

30

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

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

51

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

123

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

135

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

142

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)

154

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

163

auto here is just string, I think?

165

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.

169

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

clang-tools-extra/clangd/PathMapping.h

21

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.

30

Because there's room for confusion, I'd prefer a struct with named fields over pair<string, string>

32

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

42

This seems fine. Alternatively you could implement a dummy transport and wrap it, but it's probably too much work to justify

clang-tools-extra/clangd/test/Inputs/path-mappings/definition.jsonrpc

26 ↗

(On Diff #208306)

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.

47 ↗

(On Diff #208306)

This is unfortunately not enough context to be a precise assertion here - that URI could appear in the output for other reasons. See the other *.test for how to make a tighter assertion on the output (Mostly matching the JSONRPC id and then CHECK-NEXT).

And the assertion shouldn't need a wildcard in it, the client path is under your control.

47 ↗

(On Diff #208306)

it'd be good to check some of the other output e.g. filename from the diagnostics notification.

clang-tools-extra/clangd/test/path-mappings.test

12 ↗

(On Diff #208306)

the client side of the mapping doesn't need to exist at all on clangd's side, so don't put %t in it.

I'd suggest making the client a windows path, especially since we're mostly developing on unix. This will cover more of the interesting cases.

clang-tools-extra/clangd/tool/ClangdMain.cpp

381

This seems fine, though it is a bit odd to do half the parsing (split on comma) here, and half elsewhere. I'd suggest just making this a cl::opt<string> and passing the whole string to the parse function.

clang-tools-extra/clangd/unittests/PathMappingTests.cpp

41

Windows paths need to be specifiable as `C:\foo\bar".

We should have some tests for each combination of client/server style, I think. (Though if we need an #ifdef because clangd only handles "native" server paths, that's fine).

[wwagner19]

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

clang-tools-extra/clangd/PathMapping.cpp

30

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.

51

yup makes sense!

165

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

clang-tools-extra/clangd/PathMapping.h

21

Agreed, sounds better

32

Way better :)

clang-tools-extra/clangd/test/Inputs/path-mappings/definition.jsonrpc

26 ↗

(On Diff #208306)

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 "remote" file on disk, and having the draft RPC file simply include that, i'll have another go at it.

[wwagner19]

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.

[sammccall]

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.

clang-tools-extra/clangd/PathMapping.cpp

28

by value, not by ref

30

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.

131

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

147

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

194

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

198

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

consumeError(Uri.takeError());

207

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

clang-tools-extra/clangd/PathMapping.h

28

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

35

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

42

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.

53–57

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)

clang-tools-extra/clangd/unittests/PathMappingTests.cpp

140

nit: please write the mapping as C:\ - it's probably redundant with the tests above, but it's clearer that it's on purpose

145

The server path here isn't unix-style - that would be /foo/ or maybe C:/foo (windows path with unix-style slashes).

You could call this URI style, but I don't think this is something we'd want to (deliberately) support and we shouldn't have tests for it.

[sammccall]

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

[wwagner19]

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

[wwagner19]

clang-tools-extra/clangd/PathMapping.cpp

147

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.

207

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.

clang-tools-extra/clangd/PathMapping.h

53–57

much much better that way, thanks

[sammccall]

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)

clang-tools-extra/clangd/PathMapping.cpp

147

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.

207

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<std::string> 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 :-)

[wwagner19]

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!

[wwagner19]

clang-tools-extra/clangd/PathMapping.cpp

147

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.

[wwagner19]

Hey @sammccall, any update here?

[sammccall]

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

[wwagner19]

The last diff was broken, this most recent one

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

[wwagner19]

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 -U999999 into here.

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

[sammccall]

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

clang-tools-extra/clangd/PathMapping.cpp

26

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

Then this is just applyPathMappings, so merged the two.

clang-tools-extra/clangd/PathMapping.h

40

changed to client=server to match the flag syntax

clang-tools-extra/clangd/tool/ClangdMain.cpp

343

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)

654

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

clang-tools-extra/clangd/PathMapping.h

 //===--- PathMapping.h - apply path mappings to LSP messages -===//
 //
 // 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 "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/JSON.h"
 #include "llvm/Support/raw_ostream.h"
 #include <memory>
-#include <tuple>
+#include <string>
 #include <vector>
 
 namespace clang {
 namespace clangd {
 
 class Transport;
 
 /// PathMappings are a collection of paired client and server paths.
 /// These pairs are used to alter file:// URIs appearing in inbound and outbound
 /// LSP messages, as the client's environment may have source files or
-/// dependencies at different locations than the server.
+/// dependencies at different locations than the server. Therefore, both
+/// paths are stored as they appear in file URI bodies, e.g. /usr/include or
+/// /C:/config
 ///
 /// For example, if the mappings were {{"/home/user", "/workarea"}}, then
-/// an inbound LSP message would have file:///home/user/foo.cpp remapped to
-/// file:///workarea/foo.cpp, and the same would happen for replies (in the
-/// opposite order).
+/// a client-to-server LSP message would have file:///home/user/foo.cpp
+/// remapped to file:///workarea/foo.cpp, and the same would happen for replies
+/// (in the opposite order).
 struct PathMapping {
-  PathMapping() {}
-  PathMapping(llvm::StringRef ClientPath, llvm::StringRef ServerPath)
-      : ClientPath(ClientPath), ServerPath(ServerPath) {}
   std::string ClientPath;
   std::string ServerPath;
+  enum class Direction { ClientToServer, ServerToClient };
 };
 using PathMappings = std::vector<PathMapping>;
 
 llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const PathMapping &M);
 
-/// Parse the command line \pRawPathMappings (e.g. "/client=/server") into
+/// Parse the command line \p RawPathMappings (e.g. "/client=/server") into
 /// pairs. Returns an error if the mappings are malformed, i.e. not absolute or
 /// not a proper pair.
 llvm::Expected<PathMappings> parsePathMappings(llvm::StringRef RawPathMappings);
 
-/// Returns a modified \pS with the first matching path in \pPathMappings
+/// Returns a modified \p S with the first matching path in \p Mappings
 /// substituted, if applicable
-llvm::Optional<std::string> doPathMapping(llvm::StringRef S, bool IsIncoming,
+llvm::Optional<std::string> doPathMapping(llvm::StringRef S,
+                                          PathMapping::Direction Dir,
                                           const PathMappings &Mappings);
 
-/// Applies the \pMappings to all the file:// URIs in \pParams.
-/// \pIsIncoming affects which direction the mappings are applied.
-/// NOTE: The first matching mapping will be applied, otherwise \pParams will be
-/// untouched.
-void applyPathMappings(llvm::json::Value &Params, bool IsIncoming,
+/// Applies the \p Mappings to all the file:// URIs in \p Params.
+/// NOTE: The first matching mapping will be applied, otherwise \p Params will
+/// be untouched.
+void applyPathMappings(llvm::json::Value &Params, PathMapping::Direction Dir,
                        const PathMappings &Mappings);
 
-/// Creates a wrapping transport over \pTransp that applies the \pMappings to
+/// Creates a wrapping transport over \p Transp that applies the \p Mappings to
 /// all inbound and outbound LSP messages. All calls are then delegated to the
-/// regular \pTransp (e.g. XPC, JSON).
+/// regular transport (e.g. XPC, JSON).
 std::unique_ptr<Transport>
 createPathMappingTransport(std::unique_ptr<Transport> Transp,
                            PathMappings Mappings);
 
 } // namespace clangd
 } // namespace clang

clang-tools-extra/clangd/PathMapping.cpp

 //===--- PathMapping.cpp - apply path mappings to LSP messages -===//
 //
 // 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 "Logger.h"
 #include "Transport.h"
 #include "URI.h"
 #include "llvm/ADT/None.h"
-#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
-#include "llvm/ADT/SmallString.h"
-#include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Errno.h"
-#include "llvm/Support/JSON.h"
 #include "llvm/Support/Path.h"
-#include "llvm/Support/raw_ostream.h"
 #include <algorithm>
-#include <memory>
 #include <tuple>
-#include <vector>
 
 namespace clang {
 namespace clangd {
 namespace {
 using MapperFunc = llvm::function_ref<llvm::Optional<std::string>(
-    llvm::StringRef, bool, const PathMappings &)>;
+    llvm::StringRef, PathMapping::Direction, const PathMappings &)>;
 
-// Recursively apply the \pMF to all string keys/values in \pV
-void recursivelyMap(llvm::json::Value &V, bool IsIncoming,
+// Recursively apply the MF to all string keys/values in V
+void recursivelyMap(llvm::json::Value &V, PathMapping::Direction Dir,
                     const PathMappings &Mappings, const MapperFunc &MF) {
   using Kind = llvm::json::Value::Kind;
-  const Kind &K = V.kind();
+  Kind K = V.kind();
   if (K == Kind::Object) {
     llvm::json::Object *Obj = V.getAsObject();
     llvm::json::Object MappedObj;
     // 1. Map all the Keys
     for (auto &KV : *Obj) {
       if (llvm::Optional<std::string> MappedKey =
-              MF(KV.first.str(), IsIncoming, Mappings)) {
+              MF(KV.first.str(), Dir, Mappings)) {
         MappedObj.try_emplace(std::move(*MappedKey), std::move(KV.second));
       } else {
         MappedObj.try_emplace(std::move(KV.first), std::move(KV.second));
       }
     }
     *Obj = std::move(MappedObj);
     // 2. Map all the values
     for (auto &KV : *Obj) {
-      recursivelyMap(KV.second, IsIncoming, Mappings, MF);
+      recursivelyMap(KV.second, Dir, Mappings, MF);
     }
   } else if (K == Kind::Array) {
     for (llvm::json::Value &Val : *V.getAsArray()) {
-      recursivelyMap(Val, IsIncoming, Mappings, MF);
+      recursivelyMap(Val, Dir, Mappings, MF);
     }
   } else if (K == Kind::String) {
     if (llvm::Optional<std::string> Mapped =
-            MF(*V.getAsString(), IsIncoming, Mappings)) {
+            MF(*V.getAsString(), Dir, Mappings)) {
       V = std::move(*Mapped);
     }
   }
 }
 
 class PathMappingMessageHandler : public Transport::MessageHandler {
 public:
   PathMappingMessageHandler(MessageHandler &Handler,
                             const PathMappings &Mappings)
       : WrappedHandler(Handler), Mappings(Mappings) {}
 
   bool onNotify(llvm::StringRef Method, llvm::json::Value Params) override {
-    applyPathMappings(Params, /*IsIncoming=*/true, Mappings);
+    applyPathMappings(Params, PathMapping::Direction::ClientToServer, Mappings);
     return WrappedHandler.onNotify(Method, std::move(Params));
   }
 
   bool onCall(llvm::StringRef Method, llvm::json::Value Params,
               llvm::json::Value ID) override {
-    applyPathMappings(Params, /*IsIncoming=*/true, Mappings);
+    applyPathMappings(Params, PathMapping::Direction::ClientToServer, Mappings);
     return WrappedHandler.onCall(Method, std::move(Params), std::move(ID));
   }
 
   bool onReply(llvm::json::Value ID,
                llvm::Expected<llvm::json::Value> Result) override {
     if (Result) {
-      applyPathMappings(*Result, /*IsIncoming=*/true, Mappings);
+      applyPathMappings(*Result, PathMapping::Direction::ClientToServer,
+                        Mappings);
     }
     return WrappedHandler.onReply(std::move(ID), std::move(Result));
   }
 
 private:
   Transport::MessageHandler &WrappedHandler;
   const PathMappings &Mappings;
 };
 
 // Apply path mappings to all LSP messages by intercepting all params/results
 // and then delegating to the normal transport
 class PathMappingTransport : public Transport {
 public:
   PathMappingTransport(std::unique_ptr<Transport> Transp, PathMappings Mappings)
       : WrappedTransport(std::move(Transp)), Mappings(std::move(Mappings)) {}
 
   void notify(llvm::StringRef Method, llvm::json::Value Params) override {
-    applyPathMappings(Params, /*IsIncoming=*/false, Mappings);
+    applyPathMappings(Params, PathMapping::Direction::ServerToClient, Mappings);
     WrappedTransport->notify(Method, std::move(Params));
   }
 
   void call(llvm::StringRef Method, llvm::json::Value Params,
             llvm::json::Value ID) override {
-    applyPathMappings(Params, /*IsIncoming=*/false, Mappings);
+    applyPathMappings(Params, PathMapping::Direction::ServerToClient, Mappings);
     WrappedTransport->call(Method, std::move(Params), std::move(ID));
   }
 
   void reply(llvm::json::Value ID,
              llvm::Expected<llvm::json::Value> Result) override {
     if (Result) {
-      applyPathMappings(*Result, /*IsIncoming=*/false, Mappings);
+      applyPathMappings(*Result, PathMapping::Direction::ServerToClient,
+                        Mappings);
     }
     WrappedTransport->reply(std::move(ID), std::move(Result));
   }
 
   llvm::Error loop(MessageHandler &Handler) override {
     PathMappingMessageHandler WrappedHandler(Handler, Mappings);
     return WrappedTransport->loop(WrappedHandler);
   }
 
 private:
   std::unique_ptr<Transport> WrappedTransport;
   PathMappings Mappings;
 };
 
 inline llvm::Error make_string_error(const llvm::Twine &Message) {
   return llvm::make_error<llvm::StringError>(Message,
                                              llvm::inconvertibleErrorCode());
 }
 
-// Returns whether a path mapping should take place for \pOrigPath
-// i.e. \pMappingPath is a proper sub-path of \p OrigPath
+// 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 \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
+// Converts a unix/windows path to the path portion of a file URI
+// e.g. "C:\foo" -> "/C:/foo"
 llvm::Expected<std::string> parsePath(llvm::StringRef Path) {
   namespace path = llvm::sys::path;
-  std::string ParsedPath;
   if (path::is_absolute(Path, path::Style::posix)) {
     return Path;
   } else if (path::is_absolute(Path, path::Style::windows)) {
     std::string Converted = path::convert_to_slash(Path, path::Style::windows);
     if (Converted.front() != '/') {
       Converted = "/" + Converted;
     }
     return Converted;
   }
   return make_string_error("Path not absolute: " + Path);
 }
 
 } // namespace
 
 llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const PathMapping &M) {
   return OS << M.ClientPath << ", " << M.ServerPath;
 }
 
 llvm::Expected<PathMappings>
 parsePathMappings(llvm::StringRef RawPathMappings) {
   llvm::StringRef ClientPath, ServerPath, PathPair, Rest = RawPathMappings;
   PathMappings ParsedMappings;
   while (!Rest.empty()) {
     std::tie(PathPair, Rest) = Rest.split(",");
     std::tie(ClientPath, ServerPath) = PathPair.split("=");
     if (ClientPath.empty() || ServerPath.empty()) {
       return make_string_error("Not a valid path mapping pair: " + PathPair);
     }
     llvm::Expected<std::string> ParsedClientPath = parsePath(ClientPath);
     if (!ParsedClientPath) {
       return ParsedClientPath.takeError();
     }
     llvm::Expected<std::string> ParsedServerPath = parsePath(ServerPath);
     if (!ParsedServerPath) {
       return ParsedServerPath.takeError();
     }
-    ParsedMappings.emplace_back(std::move(*ParsedClientPath),
-                                std::move(*ParsedServerPath));
+    ParsedMappings.push_back(
+        {std::move(*ParsedClientPath), std::move(*ParsedServerPath)});
   }
   return ParsedMappings;
 }
 
-llvm::Optional<std::string> doPathMapping(llvm::StringRef S, bool IsIncoming,
+llvm::Optional<std::string> doPathMapping(llvm::StringRef S,
+                                          PathMapping::Direction Dir,
                                           const PathMappings &Mappings) {
+  // Retrun early to optimize for the common case, wherein S is not a file URI
   if (!S.startswith("file://"))
     return llvm::None;
   auto Uri = URI::parse(S);
   if (!Uri) {
+    llvm::consumeError(Uri.takeError());
     return llvm::None;
   }
   for (const auto &Mapping : Mappings) {
-    const std::string &From =
-        IsIncoming ? Mapping.ClientPath : Mapping.ServerPath;
-    const std::string &To =
-        IsIncoming ? Mapping.ServerPath : Mapping.ClientPath;
+    const std::string &From = Dir == PathMapping::Direction::ClientToServer
+                                  ? Mapping.ClientPath
+                                  : Mapping.ServerPath;
+    const std::string &To = Dir == PathMapping::Direction::ClientToServer
+                                ? Mapping.ServerPath
+                                : Mapping.ClientPath;
     if (mappingMatches(Uri->body(), From)) {
-      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());
+      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();
     }
   }
   return llvm::None;
 }
 
-void applyPathMappings(llvm::json::Value &Params, bool IsIncoming,
+void applyPathMappings(llvm::json::Value &Params, PathMapping::Direction Dir,
                        const PathMappings &Mappings) {
-  recursivelyMap(Params, IsIncoming, Mappings, doPathMapping);
+  recursivelyMap(Params, Dir, Mappings, doPathMapping);
 }
 
 std::unique_ptr<Transport>
 createPathMappingTransport(std::unique_ptr<Transport> Transp,
                            PathMappings Mappings) {
   return std::make_unique<PathMappingTransport>(std::move(Transp), Mappings);
 }
 
 } // namespace clangd
 } // namespace clang

clang-tools-extra/clangd/tool/ClangdMain.cpp

opt<bool> EnableTestScheme{
     init(false),
     Hidden,
 };
 
 static llvm::cl::opt<std::string> PathMappingsArg(
     "path-mappings",
     cat(Protocol),
     llvm::cl::desc(
         "Comma separated list of '<client_path>=<server_path>' pairs "
         "that can be used to map between file locations on the client "
         "and and a remote "
         "location where clangd is running,"
         "e.g. "
         "/home/project=/workarea/project,/home/project/.includes=/opt/include"
-        "opt/include"),
+        ),
     llvm::cl::init(""));
 
 opt<Path> InputMirrorFile{
     "input-mirror-file",
     cat(Protocol),
     desc("Mirror all LSP input to the specified file. Useful for debugging"),
     init(""),
     Hidden,
opt<OffsetEncoding> ForceOffsetEncoding{
     desc("Force the offsetEncoding used for character positions. "
          "This bypasses negotiation via client capabilities"),
     values(
         clEnumValN(OffsetEncoding::UTF8, "utf-8", "Offsets are in UTF-8 bytes"),
         clEnumValN(OffsetEncoding::UTF16, "utf-16",
                    "Offsets are in UTF-16 code units"),
         clEnumValN(OffsetEncoding::UTF32, "utf-32",
                    "Offsets are in unicode codepoints")),
     init(OffsetEncoding::UnsupportedEncoding),
 };
 
 opt<bool> PrettyPrint{
     "pretty",
     cat(Protocol),
     desc("Pretty-print JSON output"),
     init(false),
 };
TransportLayer = newJSONTransport(
         stdin, llvm::outs(),
         InputMirrorStream ? InputMirrorStream.getPointer() : nullptr,
         PrettyPrint, InputStyle);
   }
   if (!PathMappingsArg.empty()) {
     auto Mappings = parsePathMappings(PathMappingsArg);
     if (!Mappings) {
       auto Err = Mappings.takeError();
       llvm::errs() << llvm::toString(std::move(Err)) << "\n";
       return 1;
     }
     TransportLayer =
         createPathMappingTransport(std::move(TransportLayer), *Mappings);
   }
   // Create an empty clang-tidy option.
   std::mutex ClangTidyOptMu;
   std::unique_ptr<tidy::ClangTidyOptionsProvider>

clang-tools-extra/clangd/unittests/PathMappingTests.cpp

TEST(ParsePathMappingTests, WindowsPath) {
   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<PathMappings> 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
TEST(ParsePathMappingTests, ParsesMultiple) {
   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, bool IsIncoming) {
+                  llvm::StringRef RawMappings, PathMapping::Direction Dir) {
   llvm::Expected<PathMappings> Mappings = parsePathMappings(RawMappings);
   if (!Mappings)
     return false;
-  llvm::Optional<std::string> MappedPath =
-      doPathMapping(Orig, IsIncoming, *Mappings);
+  llvm::Optional<std::string> 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", "", 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) {
   auto Params = llvm::json::parse(R"({
     "textDocument": {"uri": "file:///home/foo.cpp"},
     "position": {"line": 0, "character": 0}
   })");
   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);
 }
 
 TEST(ApplyPathMappingTests, MapsAllMatchingPaths) {
   // Handles nested objects and array values
   auto Params = llvm::json::parse(R"({
     "rootUri": {"uri": "file:///home/foo.cpp"},
     "workspaceFolders": ["file:///home/src", "file:///tmp"]
   })");
   auto ExpectedParams = llvm::json::parse(R"({
     "rootUri": {"uri": "file:///workarea/foo.cpp"},
     "workspaceFolders": ["file:///workarea/src", "file:///tmp"]
   })");
   auto Mappings = parsePathMappings("/home=/workarea");
   ASSERT_TRUE(bool(Params) && bool(ExpectedParams) && bool(Mappings));
-  applyPathMappings(*Params, /*IsIncoming=*/true, *Mappings);
+  applyPathMappings(*Params, PathMapping::Direction::ClientToServer, *Mappings);
   EXPECT_EQ(*Params, *ExpectedParams);
 }
 
 TEST(ApplyPathMappingTests, MapsOutbound) {
   auto Params = llvm::json::parse(R"({
     "id": 1,
     "result": [
       {"uri": "file:///opt/include/foo.h"},
       {"uri": "file:///workarea/src/foo.cpp"}]
   })");
   auto ExpectedParams = llvm::json::parse(R"({
     "id": 1,
     "result": [
       {"uri": "file:///home/.includes/foo.h"},
       {"uri": "file:///home/src/foo.cpp"}]
   })");
   auto Mappings =
       parsePathMappings("/home=/workarea,/home/.includes=/opt/include");
   ASSERT_TRUE(bool(Params) && bool(ExpectedParams) && bool(Mappings));
-  applyPathMappings(*Params, /*IsIncoming=*/false, *Mappings);
+  applyPathMappings(*Params, PathMapping::Direction::ServerToClient, *Mappings);
   EXPECT_EQ(*Params, *ExpectedParams);
 }
 
 TEST(ApplyPathMappingTests, MapsKeys) {
   auto Params = llvm::json::parse(R"({
     "changes": {
       "file:///home/foo.cpp": {"newText": "..."},
       "file:///home/src/bar.cpp": {"newText": "..."}
     }
   })");
   auto ExpectedParams = llvm::json::parse(R"({
     "changes": {
       "file:///workarea/foo.cpp": {"newText": "..."},
       "file:///workarea/src/bar.cpp": {"newText": "..."}
     }
   })");
   auto Mappings = parsePathMappings("/home=/workarea");
   ASSERT_TRUE(bool(Params) && bool(ExpectedParams) && bool(Mappings));
-  applyPathMappings(*Params, /*IsIncoming=*/true, *Mappings);
+  applyPathMappings(*Params, PathMapping::Direction::ClientToServer, *Mappings);
   EXPECT_EQ(*Params, *ExpectedParams);
 }
 
 } // namespace
 } // namespace clangd
 } // namespace clang