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

[William Wagner via Phabricator via cfe-commits]

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

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



================
Comment at: clang-tools-extra/clangd/PathMapping.cpp:30
+  const auto &K = V.kind();
+  if (K == Kind::Object) {
+    for (auto &KV : *V.getAsObject()) {
----------------
sammccall wrote:
> object keys may be file uris too. (see `WorkspaceEdit.changes`)
> 
> This case is going to be a bit annoying to handle, I think :-(
Indeed, it seems so. It doesn't look like `json::Object` has any key removal functionality (although I could very well be reading this wrong). If so, then I guess I can just create a new `json::Object`, moving over the old values, and replacing the keys if necessary. 


================
Comment at: clang-tools-extra/clangd/PathMapping.cpp:51
+    llvm::json::Value MappedParams =
+        doPathMapping(Params, /*IsIncoming=*/true, Mappings);
+    return WrappedHandler.onNotify(Method, std::move(MappedParams));
----------------
sammccall wrote:
> not a really big deal, but we're forcing a copy here - should doPathMapping mutate its argument and return void instead?
yup makes sense!


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

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


================
Comment at: clang-tools-extra/clangd/PathMapping.h:21
+
+/// PathMappings are a collection of paired host and remote paths.
+/// These pairs are used to alter file:// URIs appearing in inbound and outbound
----------------
sammccall wrote:
> hmm, the host/remote terminology is a bit confusing to me.
> I guess the host is the machine where clangd is running, and remote is the other end, but  from the user's point of view this is a configuration where the clangd is remote.
> 
> What about calling these client/server paths? The language client/server roles are defined in the spec and these terms are likely to make some sense to users.
Agreed, sounds better


================
Comment at: clang-tools-extra/clangd/PathMapping.h:32
+
+/// Parse the command line \pRawPathMappings (e.g. "/host|/remote") into
+/// pairs. Returns an error if the mappings are malformed, i.e. not absolute or
----------------
sammccall wrote:
> I'd suggest `=` as a delimiter instead, it's more evocative and more common.
> 
> The order is tricky, I suspect `/client/path=/server/path` is going to be more intuitive
Way better :)


================
Comment at: clang-tools-extra/clangd/test/Inputs/path-mappings/definition.jsonrpc:26
+---
+{
+  "jsonrpc": "2.0",
----------------
sammccall wrote:
> This test seems to have unneccesary moving parts. Was it copied from the background index test?
> 
> One header file on disk and one draft file sent over RPC should be enough. A compilation database shouldn't be necessary I think.
> 
> You shouldn't need to splice URIs, because the input and output paths are virtual and fully under your control (that's the point of this patch, :)). So the test should be able to run on windows.
> 
> I think this test can actually be a standard one where the JSON-RPC calls and assertions go in the *.test file.
This was copied from the background test, I felt a bit uneasy about how complicated it got, but I had a bit of trouble getting a simpler one going. You're right though, I can't see why this wouldn't work with a "remote" file on disk, and having the draft RPC file simply include that, i'll have another go at it.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D64305