[PATCH] D64305: [clangd] Add path mappings functionality
Sam McCall via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Sun Jul 7 21:52:28 PDT 2019
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 &K = 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 &K = V.kind();
+ if (K == Kind::Object) {
+ for (auto &KV : *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<std::string> &RawPathMappings) {
+ 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 &P : 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, [&Mappings, IsIncoming](llvm::StringRef S) -> std::string {
+ if (!S.startswith("file://"))
----------------
again, copying every string in the json seems excessive. can we return optional<string> and only populate it when we match?
================
Comment at: clang-tools-extra/clangd/PathMapping.cpp:163
+ for (const auto &Mapping : Mappings) {
+ const auto &From = IsIncoming ? Mapping.first : Mapping.second;
+ const auto &To = IsIncoming ? Mapping.second : Mapping.first;
----------------
auto here is just string, I think?
================
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();
----------------
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: clang-tools-extra/clangd/PathMapping.h:30
+/// opposite order).
+using PathMappings = std::vector<std::pair<std::string, std::string>>;
+
----------------
Because there's room for confusion, I'd prefer a struct with named fields over `pair<string, string>`
================
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
----------------
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
================
Comment at: clang-tools-extra/clangd/PathMapping.h:42
+/// untouched.
+llvm::json::Value doPathMapping(const llvm::json::Value &Params,
+ bool IsIncoming, const PathMappings &Mappings);
----------------
wwagner19 wrote:
> 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.
This seems fine. Alternatively you could implement a dummy transport and wrap it, but it's probably too much work to justify
================
Comment at: clang-tools-extra/clangd/test/Inputs/path-mappings/definition.jsonrpc:26
+---
+{
+ "jsonrpc": "2.0",
----------------
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.
================
Comment at: clang-tools-extra/clangd/test/Inputs/path-mappings/definition.jsonrpc:47
+}
+# CHECK: "uri": "file://{{.*}}/host/foo.cpp"
+---
----------------
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.
================
Comment at: clang-tools-extra/clangd/test/Inputs/path-mappings/definition.jsonrpc:47
+}
+# CHECK: "uri": "file://{{.*}}/host/foo.cpp"
+---
----------------
sammccall wrote:
> 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.
it'd be good to check some of the other output e.g. filename from the diagnostics notification.
================
Comment at: clang-tools-extra/clangd/test/path-mappings.test:12
+# With path mappings, when we go to definition on foo(), we get back a host file uri
+# RUN: clangd -background-index -background-index-rebuild-period=0 --path-mappings '%t/host|%t/remote' -lit-test < %t/definition.jsonrpc | FileCheck %t/definition.jsonrpc
+
----------------
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.
================
Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:291
+ "opt/include"),
+ llvm::cl::CommaSeparated);
----------------
wwagner19 wrote:
> 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`
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.
================
Comment at: clang-tools-extra/clangd/unittests/PathMappingTests.cpp:41
+ std::vector<std::string> RawPathMappings = {
+ "/C:/home/project|/workarea/project",
+ "/home/project/.includes|/C:/opt/include"};
----------------
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).
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D64305/new/
https://reviews.llvm.org/D64305
More information about the cfe-commits
mailing list