[clangd] Extract FsPath from file:// uri

[stanionascu]

Summary

rfc8089#appendix-E.2 specifies that paths can begin with a drive letter e.g. as file:///c:/.
In this case just consuming front file:// is not enough and the 3rd slash must be consumed to produce a valid path on windows.

The patch introduce a generic way of converting an uri to a filesystem path and back.

[krasimir]

clangd/ASTManager.cpp

166

I think we should rename the Uri parameter here to File.

178

I think we should rename the Uri parameter here to File.

clangd/Protocol.cpp

61

Hm, seems to me that here the uri should be kept as-is and the clients of Result.uri should convert it to file when appropriate. Otherwise it's confusing.

167

Same thing: I think that the clients should be responsible of converting Result.uri to a filename.

clangd/Protocol.h

32

It's a little confusing: the parse method turns an uri to a file and the unparse method does the opposite. Maybe we should use more descriptive names, like uriToFile and fileToUri. This does not seem to be inconsistent with the rest of the protocol since the protocol only cares about uri-s, and file-s are an implementation detail of clangd I guess.

clangd/clients/clangd-vscode/src/extension.ts

28

By the way, what does this do? I'm not a typescript vs code guru.

[stanionascu]

Refactored URI handling to make it more explicit, also updated the parts which expect a File parameter and not an URI.

[stanionascu]

clangd/Protocol.cpp

61

Implemented the "URI::parse" to store both representations.

clangd/Protocol.h

32

2nd try now, instead of actually storing a string based URI, store the object which both representations (uri and file).

clangd/clients/clangd-vscode/src/extension.ts

28

Extended the FIXME, with the description, e.g. when vscode (on windows) sends the uri to clangd, it's represented as "file:///c%3A/path/tofile" which leads to:

• incorrect detection of the compilation database location.
• cannot find the actual unit in the database.

uri.toString(true) - skips the percent encoding and sends the uri parameter to clangd as "file:///c:/path/tofile"

[krasimir]

Looks good!

[stanionascu]

Thank you for the review!

If there are no other comments/suggestions, would be great if someone would merge it as I'm lacking the permission to do so.

clangd/ASTManager.cpp

 /// Retrieve a copy of the contents of every file in the store, for feeding into
 /// ASTUnit.
 static std::vector<ASTUnit::RemappedFile>
 getRemappedFiles(const DocumentStore &Docs) {
   // FIXME: Use VFS instead. This would allow us to get rid of the chdir below.
   std::vector<ASTUnit::RemappedFile> RemappedFiles;
   for (const auto &P : Docs.getAllDocuments()) {
     StringRef FileName = P.first;
-    FileName.consume_front("file://");
     RemappedFiles.push_back(ASTUnit::RemappedFile(
         FileName,
         llvm::MemoryBuffer::getMemBufferCopy(P.second, FileName).release()));
   }
   return RemappedFiles;
 }
 
 /// Convert from clang diagnostic level to LSP severity.
// Put FixIts into place.
     std::lock_guard<std::mutex> Guard(FixItLock);
     FixIts = std::move(LocalFixIts);
   }
 
   if (!Diagnostics.empty())
     Diagnostics.pop_back(); // Drop trailing comma.
   Output.writeMessage(
       R"({"jsonrpc":"2.0","method":"textDocument/publishDiagnostics","params":{"uri":")" +
-      File + R"(","diagnostics":[)" + Diagnostics + R"(]}})");
+      Uri::unparse(File) + R"(","diagnostics":[)" + Diagnostics + R"(]}})");
 }
 
 ASTManager::~ASTManager() {
   {
     std::lock_guard<std::mutex> Guard(RequestLock);
     // Wake up the clang worker thread, then exit.
     Done = true;
     ClangRequestCV.notify_one();
void ASTManager::onDocumentAdd(StringRef Uri) {
   std::lock_guard<std::mutex> Guard(RequestLock);
   // Currently we discard all pending requests and just enqueue the latest one.
   RequestQueue.clear();
   RequestQueue.push_back(Uri);
   ClangRequestCV.notify_one();
 }
 
 tooling::CompilationDatabase *
 ASTManager::getOrCreateCompilationDatabaseForFile(StringRef Uri) {
   auto &I = CompilationDatabases[Uri];
   if (I)
     return I.get();
 
-  Uri.consume_front("file://");
-
   std::string Error;
   I = tooling::CompilationDatabase::autoDetectFromSource(Uri, Error);
   Output.log("Failed to load compilation database: " + Twine(Error) + "\n");
   return I.get();
 }
 
 std::unique_ptr<clang::ASTUnit>
 ASTManager::createASTUnitForFile(StringRef Uri, const DocumentStore &Docs) {
   tooling::CompilationDatabase *CDB =
       getOrCreateCompilationDatabaseForFile(Uri);
 
-  Uri.consume_front("file://");
   std::vector<tooling::CompileCommand> Commands;
 
   if (CDB) {
     Commands = CDB->getCompileCommands(Uri);
     // chdir. This is thread hostile.
     if (!Commands.empty())
       llvm::sys::fs::set_current_path(Commands.front().Directory);
   }

clangd/Protocol.h

 #include "llvm/ADT/Optional.h"
 #include "llvm/Support/YAMLParser.h"
 #include <string>
 #include <vector>
 
 namespace clang {
 namespace clangd {
 
+struct Uri {
+  static std::string parse(llvm::StringRef uri);
+  static std::string unparse(llvm::StringRef file);
+};
+
 struct TextDocumentIdentifier {
   /// The text document's URI.
   std::string uri;
 
   static llvm::Optional<TextDocumentIdentifier>
   parse(llvm::yaml::MappingNode *Params);
 };
 

clangd/Protocol.cpp

 //
 //===----------------------------------------------------------------------===//
 
 #include "Protocol.h"
 #include "clang/Basic/LLVM.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/Support/Format.h"
 #include "llvm/Support/raw_ostream.h"
+#include "llvm/Support/Path.h"
 using namespace clang::clangd;
 
+std::string Uri::parse(llvm::StringRef uri) {
+  uri.consume_front("file://");
+  // For Windows paths e.g. /X:
+  if (uri.size() > 2 && uri[0] == '/' && uri[2] == ':')
+    uri.consume_front("/");
+  // Make sure that file paths are in native separators
+  std::string Result = llvm::sys::path::convert_to_slash(uri);
+  return Result;
+}
+
+std::string Uri::unparse(llvm::StringRef file) {
+  using namespace llvm::sys;
+  std::string Result = "file://";
+  // For Windows paths e.g. X:
+  if (file.size() > 1 && file[1] == ':')
+    Result += "/";
+  // Make sure that uri paths are with posix separators
+  Result += path::convert_to_slash(file, path::Style::posix);
+  return Result;
+}
+
 llvm::Optional<TextDocumentIdentifier>
 TextDocumentIdentifier::parse(llvm::yaml::MappingNode *Params) {
   TextDocumentIdentifier Result;
   for (auto &NextKeyValue : *Params) {
     auto *KeyString = dyn_cast<llvm::yaml::ScalarNode>(NextKeyValue.getKey());
     if (!KeyString)
       return llvm::None;
 
     llvm::SmallString<10> KeyStorage;
     StringRef KeyValue = KeyString->getValue(KeyStorage);
     auto *Value =
         dyn_cast_or_null<llvm::yaml::ScalarNode>(NextKeyValue.getValue());
     if (!Value)
       return llvm::None;
 
     llvm::SmallString<10> Storage;
     if (KeyValue == "uri") {
-      Result.uri = Value->getValue(Storage);
+      Result.uri = Uri::parse(Value->getValue(Storage));
     } else if (KeyValue == "version") {
       // FIXME: parse version, but only for VersionedTextDocumentIdentifiers.
     } else {
       return llvm::None;
     }
   }
   return Result;
 }
for (auto &NextKeyValue : *Params) {
     StringRef KeyValue = KeyString->getValue(KeyStorage);
     auto *Value =
         dyn_cast_or_null<llvm::yaml::ScalarNode>(NextKeyValue.getValue());
     if (!Value)
       return llvm::None;
 
     llvm::SmallString<10> Storage;
     if (KeyValue == "uri") {
-      Result.uri = Value->getValue(Storage);
+      Result.uri = Uri::parse(Value->getValue(Storage));
     } else if (KeyValue == "languageId") {
       Result.languageId = Value->getValue(Storage);
     } else if (KeyValue == "version") {
       long long Val;
       if (llvm::getAsSignedInteger(Value->getValue(Storage), 0, Val))
         return llvm::None;
       Result.version = Val;
     } else if (KeyValue == "text") {

clangd/clients/clangd-vscode/src/extension.ts

 export function activate(context: vscode.ExtensionContext) {
     const clangdPath = getConfig<string>('path');
     const clangdArgs = getConfig<string[]>('arguments');
 
     const serverOptions: vscodelc.ServerOptions = { command: clangdPath, args: clangdArgs };
 
     const clientOptions: vscodelc.LanguageClientOptions = {
         // Register the server for C/C++ files
-        documentSelector: ['c', 'cc', 'cpp', 'h', 'hh', 'hpp']
+        documentSelector: ['c', 'cc', 'cpp', 'h', 'hh', 'hpp'],
+        uriConverters: {
+            // FIXME: implement percent decoding in clangd
+            code2Protocol: (uri: vscode.Uri) : string => uri.toString(true),
+            protocol2Code: (file: string) : vscode.Uri => vscode.Uri.file(file)
+        }
     };
 
     const clangdClient = new vscodelc.LanguageClient('Clang Language Server', serverOptions, clientOptions);
 
     function applyTextEdits(uri: string, edits: vscodelc.TextEdit[]) {
         let textEditor = vscode.window.activeTextEditor;
 
         if (textEditor && textEditor.document.uri.toString() === uri) {