[clangd] Simplify the callside of URI::resolve, NFC.

[hokein]

Summary

• Add a overrloded URI::resolve, which accepts a string URI;
• also fixed some callside that don't check the error;

[hokein]

remove an accident change.

[kadircet]

LGTM, thanks!

clang-tools-extra/clangd/FindSymbols.cpp

48

return make_string_error(... ?

Also the error string regarding CD.FileURI should be contained in Path.takeError maybe just print:
Could not resolve path for symbol '{0}': {1}, Sym.Name, Path.takeError() ?

clang-tools-extra/clangd/URI.cpp

190

why not just Uri.takeError ?

same below for Path.takeError

clang-tools-extra/clangd/URI.h

66

what about:

Same as above, in addition parses \p FileURI using URI::parse to create a URI.

clang-tools-extra/clangd/index/BackgroundIndexLoader.cpp

30

wow, thanks for catching this one.

Failed to resolve URI {0}: {1}

[kadircet]

clang-tools-extra/clangd/index/BackgroundIndexLoader.cpp

30

actually can you just inline this into call site?

[hokein]

address comments.

[hokein]

clang-tools-extra/clangd/FindSymbols.cpp

48

make_string_error is an internal helper in URI.cpp only.

clang-tools-extra/clangd/URI.cpp

190

good point. The code was changed back and forth a few times.

[kadircet]

LGTM, thanks!

clang-tools-extra/clangd/FindSymbols.cpp

48

ah I see, when I searched for make_string_error, there were too many references so I assumed it was a generic helper, but apparently every file has an internal implementation ...

Whatever that can be an adventure for another day...

clang-tools-extra/clangd/CodeComplete.cpp

if (C.IdentifierResult) {
       Completion.Origin |= SymbolOrigin::Identifier;
       Completion.Kind = CompletionItemKind::Text;
       Completion.Name = C.IdentifierResult->Name;
     }
 
     // Turn absolute path into a literal string that can be #included.
     auto Inserted = [&](llvm::StringRef Header)
         -> llvm::Expected<std::pair<std::string, bool>> {
-      auto DeclaringURI =
-          URI::parse(C.IndexResult->CanonicalDeclaration.FileURI);
-      if (!DeclaringURI)
-        return DeclaringURI.takeError();
-      auto ResolvedDeclaring = URI::resolve(*DeclaringURI, FileName);
+      auto ResolvedDeclaring =
+          URI::resolve(C.IndexResult->CanonicalDeclaration.FileURI, FileName);
       if (!ResolvedDeclaring)
         return ResolvedDeclaring.takeError();
       auto ResolvedInserted = toHeaderFile(Header, FileName);
       if (!ResolvedInserted)
         return ResolvedInserted.takeError();
       auto Spelled = Includes.calculateIncludePath(*ResolvedInserted, FileName);
       if (!Spelled)
         return llvm::createStringError(llvm::inconvertibleErrorCode(),

clang-tools-extra/clangd/FindSymbols.cpp

 };
 
 } // namespace
 
 llvm::Expected<Location> symbolToLocation(const Symbol &Sym,
                                           llvm::StringRef HintPath) {
   // Prefer the definition over e.g. a function declaration in a header
   auto &CD = Sym.Definition ? Sym.Definition : Sym.CanonicalDeclaration;
-  auto Uri = URI::parse(CD.FileURI);
-  if (!Uri) {
-    return llvm::make_error<llvm::StringError>(
-        formatv("Could not parse URI '{0}' for symbol '{1}'.", CD.FileURI,
-                Sym.Name),
-        llvm::inconvertibleErrorCode());
-  }
-  auto Path = URI::resolve(*Uri, HintPath);
+  auto Path = URI::resolve(CD.FileURI, HintPath);
   if (!Path) {
     return llvm::make_error<llvm::StringError>(
-        formatv("Could not resolve path for URI '{0}' for symbol '{1}'.",
-                Uri->toString(), Sym.Name),
+        formatv("Could not resolve path for URI '{0}' for symbol '{1}': {2}",
+                CD.FileURI, Sym.Name, llvm::toString(Path.takeError())),
         llvm::inconvertibleErrorCode());
   }
   Location L;
   // Use HintPath as TUPath since there is no TU associated with this
   // request.
   L.uri = URIForFile::canonicalize(*Path, HintPath);
   Position Start, End;
   Start.line = CD.Start.line();

clang-tools-extra/clangd/IncludeFixer.cpp

if (!Matched.IncludeHeaders.empty() && Matched.Definition &&
       Fixes = fixesForSymbols(Syms);
   }
   return Fixes;
 }
 
 std::vector<Fix> IncludeFixer::fixesForSymbols(const SymbolSlab &Syms) const {
   auto Inserted = [&](const Symbol &Sym, llvm::StringRef Header)
       -> llvm::Expected<std::pair<std::string, bool>> {
-    auto DeclaringURI = URI::parse(Sym.CanonicalDeclaration.FileURI);
-    if (!DeclaringURI)
-      return DeclaringURI.takeError();
-    auto ResolvedDeclaring = URI::resolve(*DeclaringURI, File);
+    auto ResolvedDeclaring =
+        URI::resolve(Sym.CanonicalDeclaration.FileURI, File);
     if (!ResolvedDeclaring)
       return ResolvedDeclaring.takeError();
     auto ResolvedInserted = toHeaderFile(Header, File);
     if (!ResolvedInserted)
       return ResolvedInserted.takeError();
     auto Spelled = Inserter->calculateIncludePath(*ResolvedInserted, File);
     if (!Spelled)
       return llvm::createStringError(llvm::inconvertibleErrorCode(),

clang-tools-extra/clangd/URI.h

public:
   /// Resolves the absolute path of \p U. If there is no matching scheme, or the
   /// URI is invalid in the scheme, this returns an error.
   ///
   /// \p HintPath A related path, such as the current file or working directory,
   /// which can help disambiguate when the same file exists in many workspaces.
   static llvm::Expected<std::string> resolve(const URI &U,
                                              llvm::StringRef HintPath = "");
 
+  /// Resolves the absolute path of a URI string.
+  static llvm::Expected<std::string> resolve(llvm::StringRef FileURI,
+                                             llvm::StringRef HintPath = "");
+
   /// Resolves \p AbsPath into a canonical path of its URI, by converting
   /// \p AbsPath to URI and resolving the URI to get th canonical path.
   /// This ensures that paths with the same URI are resolved into consistent
   /// file path.
   static llvm::Expected<std::string> resolvePath(llvm::StringRef AbsPath,
                                                  llvm::StringRef HintPath = "");
 
   /// Gets the preferred spelling of this file for #include, if there is one,

clang-tools-extra/clangd/URI.cpp

if (Uri.consume_front("//")) {
     Pos = Uri.find('/');
     U.Authority = percentDecode(Uri.substr(0, Pos));
     Uri = Uri.substr(Pos);
   }
   U.Body = percentDecode(Uri);
   return U;
 }
 
+llvm::Expected<std::string> URI::resolve(llvm::StringRef FileURI,
+                                         llvm::StringRef HintPath) {
+  auto Uri = URI::parse(FileURI);
+  if (!Uri)
+    return make_string_error(llvm::toString(Uri.takeError()));
+  auto Path = URI::resolve(*Uri, HintPath);
+  if (!Path)
+    return make_string_error(llvm::toString(Path.takeError()));
+  return *Path;
+}
+
 llvm::Expected<URI> URI::create(llvm::StringRef AbsolutePath,
                                 llvm::StringRef Scheme) {
   if (!llvm::sys::path::is_absolute(AbsolutePath))
     return make_string_error("Not a valid absolute path: " + AbsolutePath);
   auto S = findSchemeByName(Scheme);
   if (!S)
     return S.takeError();
   return S->get()->uriFromAbsolutePath(AbsolutePath);

clang-tools-extra/clangd/index/Background.cpp

 // Resolves URI to file paths with cache.
 class URIToFileCache {
 public:
   URIToFileCache(llvm::StringRef HintPath) : HintPath(HintPath) {}
 
   llvm::StringRef resolve(llvm::StringRef FileURI) {
     auto I = URIToPathCache.try_emplace(FileURI);
     if (I.second) {
-      auto U = URI::parse(FileURI);
-      if (!U) {
-        elog("Failed to parse URI {0}: {1}", FileURI, U.takeError());
-        assert(false && "Failed to parse URI");
-        return "";
-      }
-      auto Path = URI::resolve(*U, HintPath);
+      auto Path = URI::resolve(FileURI, HintPath);
       if (!Path) {
         elog("Failed to resolve URI {0}: {1}", FileURI, Path.takeError());
         assert(false && "Failed to resolve URI");
         return "";
       }
       I.first->second = *Path;
     }
     return I.first->second;

clang-tools-extra/clangd/index/BackgroundIndexLoader.cpp

 #include <utility>
 #include <vector>
 
 namespace clang {
 namespace clangd {
 namespace {
 
 llvm::Optional<Path> uriToAbsolutePath(llvm::StringRef URI, PathRef HintPath) {
-  auto U = URI::parse(URI);
-  if (!U)
-    return llvm::None;
-  auto AbsolutePath = URI::resolve(*U, HintPath);
-  if (!AbsolutePath)
+  auto AbsolutePath = URI::resolve(URI, HintPath);
+  if (!AbsolutePath) {
+    elog("Failed to resolve URI {0}:", URI, AbsolutePath.takeError());
     return llvm::None;
+  }
   return *AbsolutePath;
 }
 
 /// A helper class to cache BackgroundIndexStorage operations and keep the
 /// inverse dependency mapping.
 class BackgroundIndexLoader {
 public:
   BackgroundIndexLoader(BackgroundIndexStorage::Factory &IndexStorageFactory)