[clangd] Assert path is absolute when assigning to URIForFile.

[ilya-biryukov]

Summary

The assertion will point directly to misbehaving code, so that
debugging related problems (like the one fixed by r325029) is easier.

[ioeric]

I think another option to prevent the bug in r325029 from happening would be providing a canonical approach for retrieving (absolute) paths from FileManager. We already have code in symbol collector that does this, and we have similar code in XRefs.cpp now. We should probably move them to clangd/Path.h and share the code?

[sammccall]

clangd/Protocol.h

52

I don't like how the API changes here take us further away from the other structs in this file.
Why does this one enforce invariants, when the others don't?

If we do want to make this more "safe", I'd suggest making it look more like a URI string (since that's what its protocol role is), like:

class LSPURI {
  LSPURI(StringRef URI) { assert if bad }
  static LSPURI ForFile(StringRef AbsPath) { assert if bad }
  operator string() { return URI::createFile(File).toString(); }
  StringRef file() { return AbsFile; }
  operator bool() { return !File.empty(); } 
  // move/copy constructor/assignment defaulted

 private:
  string AbsFile;
}

But the approach Eric suggested does seem promising: more consistent with how we handle invariants in protocol structs.

[ilya-biryukov]

• Remove URIForFile::setFile(), rely on copy and move constructors instead.

[ilya-biryukov]

In D43246#1006525, @ioeric wrote:

I think another option to prevent the bug in r325029 from happening would be providing a canonical approach for retrieving (absolute) paths from FileManager. We already have code in symbol collector that does this, and we have similar code in XRefs.cpp now. We should probably move them to clangd/Path.h and share the code?

I agree with you, we should do this.
But this change is not a mitigation for r325029 specifically. URIForFile would still be tremendously easy to misuse in other cases as well.

clangd/Protocol.h

52

I've updated URIForFile to follow the interface you proposed. (Instead of operator string() we have uri(), but everything else is there). Haven't changed the name yet, happy to do it to.

Why does this one enforce invariants, when the others don't?

It seems useful to catch misbehaving code as early as possible. What are the other invariants that could be useful?

But the approach Eric suggested does seem promising: more consistent with how we handle invariants in protocol structs.

I agree with Eric's approach, I wasn't trying to fix that specific bug with this commit.

I'm trying to argue that we should check for non-absolute paths as early as possible and fail with assertions to make writing code easier in the future. Do you agree that would be useful or make we shouldn't do it?

[ioeric]

clangd/Protocol.h

52

I agree that this change would help debugging, so I don't really have objection. I'll leave approval to Sam though, as he had some suggestions.

I wonder if we want to make the assertion more aggressive. Currently, this only helps with assertion on. Would it make sense to also check this in production code and provide useful error message so that it would be easier for us to spot the problem when users report a bug?

57

nit: I like file() that Sam suggested a bit more.

[sammccall]

LG if we want to do this
(please getFile -> file though!)

clangd/Protocol.h

28

can we hold off on adding Path to more places in this patch?
I'd like to see if others find it useful - it mostly seems obscure to me.

52

I'm not going to block this change, but I somewhat prefer the current code without it (and assertions in the places that populate URIForFile instead).

It seems useful to catch misbehaving code as early as possible. What are the other invariants that could be useful?

At a quick scan of protocol.h from the top:

Position.line/character >= 0
Range.start <= Range.end
Location.uri non-empty
TextDocumentItem.uri non empty (and lots of other uris)
FormattingOptions.tabSize >= 0
Diagnostic.message non-empty
ExecuteCommandParams.command has a valid value
...

[ilya-biryukov]

clangd/Protocol.h

28

Done.

I was the one who introduced them and I (obviously) like them. I find the code using them a bit easier to read, they don't add any type-safety, though.
Let's discuss whether others find them useful. If I'm the only one who feels this way, we can remove them.

52

After looking at the code, all instances of URIForFile are created with the assertion enforced, other structs you mention can be created in arbitrary state by fromJSON functions.
I'm not sure if making fromJSON functions more complicated are a good trade-off for enforcing those assertions.

URIForFile is different from other structs, it already has logic to convert file paths to uri (and back indirectly, via fromJSON function). Others merely directly capture the information from the LSP spec.

57

Done.

Note that this technically breaks LLVM Style Guide: "Function names should be verb phrases".
But I think we're breaking it in multiple places already (including here, e.g. uri()).

[ilya-biryukov]

• Rename getFile() to file()
• Removed the Path.h include
• Rebased onto head

clangd/ClangdLSPServer.cpp

reply(json::obj{
             {"definitionProvider", true},
             {"documentHighlightProvider", true},
             {"renameProvider", true},
             {"executeCommandProvider",
              json::obj{
                  {"commands", {ExecuteCommandParams::CLANGD_APPLY_FIX_COMMAND}},
              }},
         }}}});
-  if (Params.rootUri && !Params.rootUri->file.empty())
-    Server.setRootPath(Params.rootUri->file);
+  if (Params.rootUri && *Params.rootUri)
+    Server.setRootPath(Params.rootUri->getFile());
   else if (Params.rootPath && !Params.rootPath->empty())
     Server.setRootPath(*Params.rootPath);
 }
 
 void ClangdLSPServer::onShutdown(ShutdownParams &Params) {
   // Do essentially nothing, just say we're ready to exit.
   ShutdownRequestReceived = true;
   reply(nullptr);
 }
 
 void ClangdLSPServer::onExit(ExitParams &Params) { IsDone = true; }
 
 void ClangdLSPServer::onDocumentDidOpen(DidOpenTextDocumentParams &Params) {
   if (Params.metadata && !Params.metadata->extraFlags.empty())
-    CDB.setExtraFlagsForFile(Params.textDocument.uri.file,
+    CDB.setExtraFlagsForFile(Params.textDocument.uri.getFile(),
                              std::move(Params.metadata->extraFlags));
-  Server.addDocument(Params.textDocument.uri.file, Params.textDocument.text);
+  Server.addDocument(Params.textDocument.uri.getFile(),
+                     Params.textDocument.text);
 }
 
 void ClangdLSPServer::onDocumentDidChange(DidChangeTextDocumentParams &Params) {
   if (Params.contentChanges.size() != 1)
     return replyError(ErrorCode::InvalidParams,
                       "can only apply one change at a time");
   // We only support full syncing right now.
-  Server.addDocument(Params.textDocument.uri.file,
+  Server.addDocument(Params.textDocument.uri.getFile(),
                      Params.contentChanges[0].text);
 }
 
 void ClangdLSPServer::onFileEvent(DidChangeWatchedFilesParams &Params) {
   Server.onFileEvent(Params);
 }
 
 void ClangdLSPServer::onCommand(ExecuteCommandParams &Params) {
if (Params.command == ExecuteCommandParams::CLANGD_APPLY_FIX_COMMAND &&
     // more commands are added, this will be here has a safe guard.
     replyError(
         ErrorCode::InvalidParams,
         llvm::formatv("Unsupported command \"{0}\".", Params.command).str());
   }
 }
 
 void ClangdLSPServer::onRename(RenameParams &Params) {
-  auto File = Params.textDocument.uri.file;
+  auto File = Params.textDocument.uri.getFile();
   auto Code = Server.getDocument(File);
   if (!Code)
     return replyError(ErrorCode::InvalidParams,
                       "onRename called for non-added file");
 
   auto Replacements = Server.rename(File, Params.position, Params.newName);
   if (!Replacements) {
     replyError(ErrorCode::InternalError,
                llvm::toString(Replacements.takeError()));
     return;
   }
 
   std::vector<TextEdit> Edits = replacementsToEdits(*Code, *Replacements);
   WorkspaceEdit WE;
   WE.changes = {{Params.textDocument.uri.uri(), Edits}};
   reply(WE);
 }
 
 void ClangdLSPServer::onDocumentDidClose(DidCloseTextDocumentParams &Params) {
-  Server.removeDocument(Params.textDocument.uri.file);
+  Server.removeDocument(Params.textDocument.uri.getFile());
 }
 
 void ClangdLSPServer::onDocumentOnTypeFormatting(
     DocumentOnTypeFormattingParams &Params) {
-  auto File = Params.textDocument.uri.file;
+  auto File = Params.textDocument.uri.getFile();
   auto Code = Server.getDocument(File);
   if (!Code)
     return replyError(ErrorCode::InvalidParams,
                       "onDocumentOnTypeFormatting called for non-added file");
 
   auto ReplacementsOrError = Server.formatOnType(*Code, File, Params.position);
   if (ReplacementsOrError)
     reply(json::ary(replacementsToEdits(*Code, ReplacementsOrError.get())));
   else
     replyError(ErrorCode::UnknownErrorCode,
                llvm::toString(ReplacementsOrError.takeError()));
 }
 
 void ClangdLSPServer::onDocumentRangeFormatting(
     DocumentRangeFormattingParams &Params) {
-  auto File = Params.textDocument.uri.file;
+  auto File = Params.textDocument.uri.getFile();
   auto Code = Server.getDocument(File);
   if (!Code)
     return replyError(ErrorCode::InvalidParams,
                       "onDocumentRangeFormatting called for non-added file");
 
   auto ReplacementsOrError = Server.formatRange(*Code, File, Params.range);
   if (ReplacementsOrError)
     reply(json::ary(replacementsToEdits(*Code, ReplacementsOrError.get())));
   else
     replyError(ErrorCode::UnknownErrorCode,
                llvm::toString(ReplacementsOrError.takeError()));
 }
 
 void ClangdLSPServer::onDocumentFormatting(DocumentFormattingParams &Params) {
-  auto File = Params.textDocument.uri.file;
+  auto File = Params.textDocument.uri.getFile();
   auto Code = Server.getDocument(File);
   if (!Code)
     return replyError(ErrorCode::InvalidParams,
                       "onDocumentFormatting called for non-added file");
 
   auto ReplacementsOrError = Server.formatFile(*Code, File);
   if (ReplacementsOrError)
     reply(json::ary(replacementsToEdits(*Code, ReplacementsOrError.get())));
   else
     replyError(ErrorCode::UnknownErrorCode,
                llvm::toString(ReplacementsOrError.takeError()));
 }
 
 void ClangdLSPServer::onCodeAction(CodeActionParams &Params) {
   // We provide a code action for each diagnostic at the requested location
   // which has FixIts available.
-  auto Code = Server.getDocument(Params.textDocument.uri.file);
+  auto Code = Server.getDocument(Params.textDocument.uri.getFile());
   if (!Code)
     return replyError(ErrorCode::InvalidParams,
                       "onCodeAction called for non-added file");
 
   json::ary Commands;
   for (Diagnostic &D : Params.context.diagnostics) {
-    auto Edits = getFixIts(Params.textDocument.uri.file, D);
+    auto Edits = getFixIts(Params.textDocument.uri.getFile(), D);
     if (!Edits.empty()) {
       WorkspaceEdit WE;
       WE.changes = {{Params.textDocument.uri.uri(), std::move(Edits)}};
       Commands.push_back(json::obj{
           {"title", llvm::formatv("Apply FixIt {0}", D.message)},
           {"command", ExecuteCommandParams::CLANGD_APPLY_FIX_COMMAND},
           {"arguments", {WE}},
       });
     }
   }
   reply(std::move(Commands));
 }
 
 void ClangdLSPServer::onCompletion(TextDocumentPositionParams &Params) {
-  Server.codeComplete(Params.textDocument.uri.file, Params.position, CCOpts,
+  Server.codeComplete(Params.textDocument.uri.getFile(), Params.position,
+                      CCOpts,
                       [](Tagged<CompletionList> List) { reply(List.Value); });
 }
 
 void ClangdLSPServer::onSignatureHelp(TextDocumentPositionParams &Params) {
   auto SignatureHelp =
-      Server.signatureHelp(Params.textDocument.uri.file, Params.position);
+      Server.signatureHelp(Params.textDocument.uri.getFile(), Params.position);
   if (!SignatureHelp)
     return replyError(ErrorCode::InvalidParams,
                       llvm::toString(SignatureHelp.takeError()));
   reply(SignatureHelp->Value);
 }
 
 void ClangdLSPServer::onGoToDefinition(TextDocumentPositionParams &Params) {
-  auto Items =
-      Server.findDefinitions(Params.textDocument.uri.file, Params.position);
+  auto Items = Server.findDefinitions(Params.textDocument.uri.getFile(),
+                                      Params.position);
   if (!Items)
     return replyError(ErrorCode::InvalidParams,
                       llvm::toString(Items.takeError()));
   reply(json::ary(Items->Value));
 }
 
 void ClangdLSPServer::onSwitchSourceHeader(TextDocumentIdentifier &Params) {
-  llvm::Optional<Path> Result = Server.switchSourceHeader(Params.uri.file);
+  llvm::Optional<Path> Result = Server.switchSourceHeader(Params.uri.getFile());
   reply(Result ? URI::createFile(*Result).toString() : "");
 }
 
 void ClangdLSPServer::onDocumentHighlight(TextDocumentPositionParams &Params) {
-  auto Highlights = Server.findDocumentHighlights(Params.textDocument.uri.file,
-                                                  Params.position);
+  auto Highlights = Server.findDocumentHighlights(
+      Params.textDocument.uri.getFile(), Params.position);
 
   if (!Highlights) {
     replyError(ErrorCode::InternalError,
                llvm::toString(Highlights.takeError()));
     return;
   }
 
   reply(json::ary(Highlights->Value));

clangd/Protocol.h

 // tests, and is not generally machine-readable.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_PROTOCOL_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_PROTOCOL_H
 
 #include "JSONExpr.h"
+#include "Path.h"
 #include "URI.h"
 #include "llvm/ADT/Optional.h"
 #include <string>
 #include <vector>
 
 namespace clang {
 namespace clangd {
 
 enum class ErrorCode {
   // Defined by JSON RPC.
   ParseError = -32700,
   InvalidRequest = -32600,
   MethodNotFound = -32601,
   InvalidParams = -32602,
   InternalError = -32603,
 
   ServerNotInitialized = -32002,
   UnknownErrorCode = -32001,
 
   // Defined by the protocol.
   RequestCancelled = -32800,
 };
 
 struct URIForFile {
-  std::string file;
+  URIForFile() = default;
+  explicit URIForFile(Path AbsPath);
 
-  std::string uri() const { return URI::createFile(file).toString(); }
+  /// Retrieves absolute path to the file.
+  PathRef getFile() const { return File; }
+
+  explicit operator bool() const { return !File.empty(); }
+  std::string uri() const { return URI::createFile(File).toString(); }
 
   friend bool operator==(const URIForFile &LHS, const URIForFile &RHS) {
-    return LHS.file == RHS.file;
+    return LHS.File == RHS.File;
   }
 
   friend bool operator!=(const URIForFile &LHS, const URIForFile &RHS) {
     return !(LHS == RHS);
   }
 
   friend bool operator<(const URIForFile &LHS, const URIForFile &RHS) {
-    return LHS.file < RHS.file;
+    return LHS.File < RHS.File;
   }
+
+private:
+  Path File;
 };
 
 /// Serialize/deserialize \p URIForFile to/from a string URI.
 json::Expr toJSON(const URIForFile &U);
 bool fromJSON(const json::Expr &, URIForFile &);
 
 struct TextDocumentIdentifier {
   /// The text document's URI.

clangd/Protocol.cpp

 //===--- Protocol.cpp - Language Server Protocol Implementation -----------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This file contains the serialization code for the LSP structs.
 //
 //===----------------------------------------------------------------------===//
 
 #include "Protocol.h"
-#include "URI.h"
 #include "Logger.h"
+#include "URI.h"
 #include "clang/Basic/LLVM.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/Support/Format.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/raw_ostream.h"
 
 namespace clang {
 namespace clangd {
 
+URIForFile::URIForFile(Path AbsPath) {
+  assert(llvm::sys::path::is_absolute(AbsPath) && "the path is relative");
+  File = std::move(AbsPath);
+}
+
 bool fromJSON(const json::Expr &E, URIForFile &R) {
   if (auto S = E.asString()) {
     auto U = URI::parse(*S);
     if (!U) {
       log("Failed to parse URI " + *S + ": " + llvm::toString(U.takeError()));
       return false;
     }
     if (U->scheme() != "file" && U->scheme() != "test") {
       log("Clangd only supports 'file' URI scheme for workspace files: " + *S);
       return false;
     }
     auto Path = URI::resolve(*U);
     if (!Path) {
       log(llvm::toString(Path.takeError()));
       return false;
     }
-    R.file = *Path;
+    R = URIForFile(*Path);
     return true;
   }
   return false;
 }
 
-json::Expr toJSON(const URIForFile &U) {
-  return U.uri();
-}
+json::Expr toJSON(const URIForFile &U) { return U.uri(); }
 
 llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const URIForFile &U) {
   return OS << U.uri();
 }
 
 bool fromJSON(const json::Expr &Params, TextDocumentIdentifier &R) {
   json::ObjectMapper O(Params);
   return O && O.map("uri", R.uri);

clangd/XRefs.cpp

if (FilePath.empty())
     FilePath = F->getName();
   if (!llvm::sys::path::is_absolute(FilePath)) {
     if (!SourceMgr.getFileManager().makeAbsolutePath(FilePath)) {
       log("Could not turn relative path to absolute: " + FilePath);
       return llvm::None;
     }
   }
 
-  L.uri.file = FilePath.str();
+  L.uri = URIForFile(FilePath.str());
   L.range = R;
   return L;
 }
 
 } // namespace
 
 std::vector<Location> findDefinitions(ParsedAST &AST, Position Pos) {
   const SourceManager &SourceMgr = AST.getASTContext().getSourceManager();