[clangd] Fix conversion from Windows UNC paths to file URI format.

[ilya-golovenko]

Summary

The fix improves handling of Windows UNC paths to align with Appendix E. Nonstandard Syntax Variations of RFC 8089.

Before this fix it was difficult to use Windows UNC paths in compile_commands.json database as such paths were converted to file URIs using 'file:////auth/share/file.cpp' notation instead of recommended 'file://auth/share/file.cpp'.

As an example, VS.Code cannot understand file URIs with 4 starting slashes, thus such features as go-to-definition, jump-to-file, hover tooltip, etc. stop working. This also applicable to files which reside on Windows network-mapped drives because clangd internally resolves file paths to real paths in some cases and such paths get resolved to UNC paths.

[sammccall]

Thanks for doing this! And sorry about the shaky windows support...

(There are potentially other lurking issues due to filenames being used as keys internally, particularly case-insensitivity issues...)

clang-tools-extra/clangd/URI.cpp

29

the UNC paths are also basically a windows thing, can we have hasWindowsDriveLetter and isWindowsNetworkPath (or isWindowsUNCPath)?

34

conventionally LLVM tools accept both slashes on windows and / on other platforms - i.e. we should probably treat //foo/bar as a valid network path on windows unless there's a good reason not to.

So Path.size() > 2 && llvm::sys::path::is_separator(Path[0]) && llvm::sys::path::is_separator(Path[1]), I guess

51–56

this raises the question: what if both conditions are true?

file://abc/c:/foo

\\abc\c: seems much more plausible than the current \\abcc:, so I think this should be else if.

52

These comments could be extended a little and be even more useful:

Windows UNC paths e.g. \\server\share <=> file://server/share

68

FWIW, I think we were previously parsing drive letters even on unix, and will no longer do so.
I don't think this is a real problem though.

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

149

Could we add a test that the old four-slash version can still be resolved to a path?

It probably worked by accident - we probably weren't thinking about UNC paths at all. But someone still might depend on it now. I don't think this patch breaks it, just want to check.

[ilya-golovenko]

Address code review comments.

[ilya-golovenko]

clang-tools-extra/clangd/URI.cpp

29

This kind of network paths are also supported on Unix/Linux system, e.g. //hostname/path/file.txt and isNetworkPath will handle those as well. For example, samba and samba client support such paths. RFC 8089 calls them "non-local files" with unspecified type of protocol to access the file, i.e. it is not necessary a UNC path. Does it make sense to continue supporting Linux/Unix version of network path?

34

I tried to prevent treating paths like /\path or \/path as network paths. Maybe the following variant will work:

bool isNetworkPath(llvm::StringRef Path) {
 return Path.size() > 2 && Path[0] == Path[1] && llvm::sys::path::is_separator(Path[0]);
}

[ilya-golovenko]

In D84172#2162284, @sammccall wrote:

Thanks for doing this! And sorry about the shaky windows support...

(There are potentially other lurking issues due to filenames being used as keys internally, particularly case-insensitivity issues...)

I'm glad to be useful. And thank you much for the time you spend reviewing the code. I really appreciate it!

[sammccall]

Thanks! Do you have commit access, or should I land this for you?

clang-tools-extra/clangd/URI.cpp

29

Ah, I wasn't aware. This seems fine to me.

[ilya-golovenko]

In D84172#2162736, @sammccall wrote:

Thanks! Do you have commit access, or should I land this for you?

No, I don't have commit access. Could you please commit on my behalf?

Ilya Golovenko <[e-mail]>

And thanks again for the review.

[ilya-golovenko]

@sammccall sorry for bothering you, could you please land this for me?

Ilya Golovenko <[e-mail]>

[ilya-golovenko]

@kbobyrev Kirill, thank you for landing this for me! Unfortunately @walrus is my not-used-anymore account...
By the way, do you know is it possible to delete my unused account @walrus to avoid such confusions in future?

[kbobyrev]

In D84172#2166448, @ilya-golovenko wrote:

@kbobyrev Kirill, thank you for landing this for me! Unfortunately @walrus is my not-used-anymore account...
By the way, do you know is it possible to delete my unused account @walrus to avoid such confusions in future?

Not that I'm aware of :( I have way too many accounts myself, I'd be interested in that, too. I think not having your email associated with it anymore would be good.

Hopefully, reviews move to GitHub at some point and this will not be necessary.

[walrus]

This comment was removed by walrus.

[ilya-golovenko]

In D84172#2166496, @kbobyrev wrote:

In D84172#2166448, @ilya-golovenko wrote:

@kbobyrev Kirill, thank you for landing this for me! Unfortunately @walrus is my not-used-anymore account...
By the way, do you know is it possible to delete my unused account @walrus to avoid such confusions in future?

Not that I'm aware of :( I have way too many accounts myself, I'd be interested in that, too. I think not having your email associated with it anymore would be good.

Hopefully, reviews move to GitHub at some point and this will not be necessary.

I asked because according to policies all my commits should be attributed to Huawei (as this work is done here) and should be committed using my work email: [e-mail], not with personal [e-mail]

[kbobyrev]

In D84172#2166503, @ilya-golovenko wrote:

In D84172#2166496, @kbobyrev wrote:

In D84172#2166448, @ilya-golovenko wrote:

@kbobyrev Kirill, thank you for landing this for me! Unfortunately @walrus is my not-used-anymore account...
By the way, do you know is it possible to delete my unused account @walrus to avoid such confusions in future?

Not that I'm aware of :( I have way too many accounts myself, I'd be interested in that, too. I think not having your email associated with it anymore would be good.

Hopefully, reviews move to GitHub at some point and this will not be necessary.

I asked because according to policies all my commits should be attributed to Huawei (as this work is done here) and should be committed using my work email: [e-mail], not with personal [e-mail]

Uh, I see, sorry for that. I'm afraid we can not rewrite the commit history to change the email for the one already submitted, but I think the commits are attributed to the primary email for your account, so simply changing it should do the trick.

clang-tools-extra/clangd/URI.cpp

 namespace clangd {
 namespace {
 
 inline llvm::Error make_string_error(const llvm::Twine &Message) {
   return llvm::make_error<llvm::StringError>(Message,
                                              llvm::inconvertibleErrorCode());
 }
 
+bool isWindowsPath(llvm::StringRef Path) {
+  return Path.size() > 1 && llvm::isAlpha(Path[0]) && Path[1] == ':';
+}
+
+bool isNetworkPath(llvm::StringRef Path) {
+  return Path.size() > 2 && Path[0] == Path[1] &&
+         llvm::sys::path::is_separator(Path[0]);
+}
+
 /// This manages file paths in the file system. All paths in the scheme
 /// are absolute (with leading '/').
 /// Note that this scheme is hardcoded into the library and not registered in
 /// registry.
 class FileSystemScheme : public URIScheme {
 public:
   llvm::Expected<std::string>
-  getAbsolutePath(llvm::StringRef /*Authority*/, llvm::StringRef Body,
+  getAbsolutePath(llvm::StringRef Authority, llvm::StringRef Body,
                   llvm::StringRef /*HintPath*/) const override {
     if (!Body.startswith("/"))
       return make_string_error("File scheme: expect body to be an absolute "
                                "path starting with '/': " +
                                Body);
-    // For Windows paths e.g. /X:
-    if (Body.size() > 2 && Body[0] == '/' && Body[2] == ':')
+    llvm::SmallString<128> Path;
+    if (!Authority.empty()) {
+      // Windows UNC paths e.g. file://server/share => \\server\share
+      ("//" + Authority).toVector(Path);
+    } else if (isWindowsPath(Body.substr(1))) {
+      // Windows paths e.g. file:///X:/path => X:\path
       Body.consume_front("/");
-    llvm::SmallVector<char, 16> Path(Body.begin(), Body.end());
+    }
+    Path.append(Body);
     llvm::sys::path::native(Path);
-    return std::string(Path.begin(), Path.end());
+    return std::string(Path);
   }
 
   llvm::Expected<URI>
   uriFromAbsolutePath(llvm::StringRef AbsolutePath) const override {
     std::string Body;
-    // For Windows paths e.g. X:
-    if (AbsolutePath.size() > 1 && AbsolutePath[1] == ':')
+    llvm::StringRef Authority;
+    llvm::StringRef Root = llvm::sys::path::root_name(AbsolutePath);
+    if (isNetworkPath(Root)) {
+      // Windows UNC paths e.g. \\server\share => file://server/share
+      Authority = Root.drop_front(2);
+      AbsolutePath.consume_front(Root);
+    } else if (isWindowsPath(Root)) {
+      // Windows paths e.g. X:\path => file:///X:/path
       Body = "/";
+    }
     Body += llvm::sys::path::convert_to_slash(AbsolutePath);
-    return URI("file", /*Authority=*/"", Body);
+    return URI("file", Authority, Body);
   }
 };
 
 llvm::Expected<std::unique_ptr<URIScheme>>
 findSchemeByName(llvm::StringRef Scheme) {
   if (Scheme == "file")
     return std::make_unique<FileSystemScheme>();
 

 
 /// Encodes a string according to percent-encoding.
 /// - Unreserved characters are not escaped.
 /// - Reserved characters always escaped with exceptions like '/'.
 /// - All other characters are escaped.
 void percentEncode(llvm::StringRef Content, std::string &Out) {
   std::string Result;
   for (unsigned char C : Content)
-    if (shouldEscape(C))
-    {
+    if (shouldEscape(C)) {
       Out.push_back('%');
       Out.push_back(llvm::hexdigit(C / 16));
       Out.push_back(llvm::hexdigit(C % 16));
-    } else
-    { Out.push_back(C); }
+    } else {
+      Out.push_back(C);
+    }
 }
 
 /// Decodes a string according to percent-encoding.
 std::string percentDecode(llvm::StringRef Content) {
   std::string Result;
   for (auto I = Content.begin(), E = Content.end(); I != E; ++I) {
     if (*I != '%') {
       Result += *I;

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

 #ifdef _WIN32
   EXPECT_THAT(createOrDie("c:\\x\\y\\z"), "file:///c:/x/y/z");
 #else
   EXPECT_THAT(createOrDie("/x/y/z"), "file:///x/y/z");
   EXPECT_THAT(createOrDie("/(x)/y/\\ z"), "file:///%28x%29/y/%5C%20z");
 #endif
 }
 
+TEST(URITest, CreateUNC) {
+#ifdef _WIN32
+  EXPECT_THAT(createOrDie("\\\\test.org\\x\\y\\z"), "file://test.org/x/y/z");
+  EXPECT_THAT(createOrDie("\\\\10.0.0.1\\x\\y\\z"), "file://10.0.0.1/x/y/z");
+#else
+  EXPECT_THAT(createOrDie("//test.org/x/y/z"), "file://test.org/x/y/z");
+  EXPECT_THAT(createOrDie("//10.0.0.1/x/y/z"), "file://10.0.0.1/x/y/z");
+#endif
+}
+
 TEST(URITest, FailedCreate) {
   EXPECT_ERROR(URI::create("/x/y/z", "no"));
   // Path has to be absolute.
   EXPECT_ERROR(URI::create("x/y/z", "file"));
 }
 
 TEST(URITest, Parse) {
   EXPECT_THAT(parseOrDie("file://auth/x/y/z"),

 }
 
 TEST(URITest, Resolve) {
 #ifdef _WIN32
   EXPECT_THAT(resolveOrDie(parseOrDie("file:///c%3a/x/y/z")), "c:\\x\\y\\z");
   EXPECT_THAT(resolveOrDie(parseOrDie("file:///c:/x/y/z")), "c:\\x\\y\\z");
 #else
   EXPECT_EQ(resolveOrDie(parseOrDie("file:/a/b/c")), "/a/b/c");
-  EXPECT_EQ(resolveOrDie(parseOrDie("file://auth/a/b/c")), "/a/b/c");
+  EXPECT_EQ(resolveOrDie(parseOrDie("file://auth/a/b/c")), "//auth/a/b/c");
   EXPECT_THAT(resolveOrDie(parseOrDie("file://au%3dth/%28x%29/y/%20z")),
-              "/(x)/y/ z");
+              "//au=th/(x)/y/ z");
   EXPECT_THAT(resolveOrDie(parseOrDie("file:///c:/x/y/z")), "c:/x/y/z");
 #endif
   EXPECT_EQ(resolveOrDie(parseOrDie("unittest:///a"), testPath("x")),
             testPath("a"));
 }
 
+TEST(URITest, ResolveUNC) {
+#ifdef _WIN32
+  EXPECT_THAT(resolveOrDie(parseOrDie("file://example.com/x/y/z")),
+              "\\\\example.com\\x\\y\\z");
+  EXPECT_THAT(resolveOrDie(parseOrDie("file://127.0.0.1/x/y/z")),
+              "\\\\127.0.0.1\\x\\y\\z");
+  // Ensure non-traditional file URI still resolves to correct UNC path.
+  EXPECT_THAT(resolveOrDie(parseOrDie("file:////127.0.0.1/x/y/z")),
+              "\\\\127.0.0.1\\x\\y\\z");
+#else
+  EXPECT_THAT(resolveOrDie(parseOrDie("file://example.com/x/y/z")),
+              "//example.com/x/y/z");
+  EXPECT_THAT(resolveOrDie(parseOrDie("file://127.0.0.1/x/y/z")),
+              "//127.0.0.1/x/y/z");
+#endif
+}
+
 std::string resolvePathOrDie(llvm::StringRef AbsPath,
                              llvm::StringRef HintPath = "") {
   auto Path = URI::resolvePath(AbsPath, HintPath);
   if (!Path)
     llvm_unreachable(toString(Path.takeError()).c_str());
   return *Path;
 }