[clangd] Unify path canonicalizations in the codebase

[kadircet]

Summary

There were a few different places where we canonicalized paths, each
one had its own flavor. This patch tries to unify them all under one place.

[ilya-biryukov]

The change mostly LG, the only important comment from me is about toURI. I believe it would actually have an observable effect in the real world and I'm not sure things won't break after it.
Could we avoid changing it and focus only on tweaking the "canonical" path for the FileEntries in this change? Those look like a no-op in most practical cases.

clangd/SourceCode.h

91

A different name proposal: canonicalizePath? WDYT?

91

Maybe leave it Optional?
Semantic differences make it very hard to verify the code after rewrite is correct, as Expected requires checking for errors.

clangd/index/SymbolCollector.cpp

58

This looks like the only change that might subtly change the behavior of our program. I believe things might break because of this.
Could we keep this review closer to an NFC and avoid introducing this subtle difference here? Happy to take a look at a separate patch with it.

61–64

Why not do this only in the else branch?

62

This comment looks useful. Maybe keep it in the new implementation?

[kadircet]

clangd/SourceCode.h

91

but it rather gets the canonical path for a FileEntry rather than canonicalizing a path ?

clangd/index/SymbolCollector.cpp

58

As discussed offline this is not introducing a behavior change.

61–64

Adding a comment.

[kadircet]

• Address comments

[ilya-biryukov]

clangd/SourceCode.cpp

203

Using : twice is a bit hard to read.
Maybe use something like failed to turn relative path '{0}' to absolute: {1} instead?

205

NIT: remove llvm:: prefix in a cpp file. Same for other occurences.

clangd/SourceCode.h

91

SG, not a big deal anyway. I have a personal bias against methods starting with get if they're not getter xD)

clangd/index/SymbolCollector.cpp

58

Thanks for pointing me to the code I've missed

[kadircet]

• Address comments

clangd/SourceCode.h

 TextEdit replacementToEdit(StringRef Code, const tooling::Replacement &R);
 
 std::vector<TextEdit> replacementsToEdits(StringRef Code,
                                           const tooling::Replacements &Repls);
 
 TextEdit toTextEdit(const FixItHint &FixIt, const SourceManager &M,
                     const LangOptions &L);
 
-/// Get the real/canonical path of \p F.  This means:
+/// Get the canonical path of \p F.  This means:
 ///
 ///   - Absolute path
 ///   - Symlinks resolved
 ///   - No "." or ".." component
 ///   - No duplicate or trailing directory separator
 ///
-/// This function should be used when sending paths to clients, so that paths
-/// are normalized as much as possible.
-llvm::Optional<std::string> getRealPath(const FileEntry *F,
+/// This function should be used when paths needs to be used outside the
+/// component that generate it, so that paths are normalized as much as
+/// possible.
+llvm::Optional<std::string> getCanonicalPath(const FileEntry *F,
                                              const SourceManager &SourceMgr);
 
 bool IsRangeConsecutive(const Range &Left, const Range &Right);
 } // namespace clangd
 } // namespace clang
 #endif

clangd/SourceCode.cpp

 std::vector<TextEdit> replacementsToEdits(StringRef Code,
                                           const tooling::Replacements &Repls) {
   std::vector<TextEdit> Edits;
   for (const auto &R : Repls)
     Edits.push_back(replacementToEdit(Code, R));
   return Edits;
 }
 
-Optional<std::string> getRealPath(const FileEntry *F,
+llvm::Optional<std::string> getCanonicalPath(const FileEntry *F,
                                              const SourceManager &SourceMgr) {
+  if (!F)
+    return llvm::None;
   // Ideally, we get the real path from the FileEntry object.
   SmallString<128> FilePath = F->tryGetRealPathName();
-  if (!FilePath.empty()) {
+  if (!FilePath.empty() && sys::path::is_absolute(FilePath))
     return FilePath.str().str();
-  }
 
   // Otherwise, we try to compute ourselves.
-  vlog("FileEntry for {0} did not contain the real path.", F->getName());
-
-  SmallString<128> Path = F->getName();
+  FilePath = F->getName();
+  vlog("FileEntry for {0} did not contain the real path.", FilePath);
 
-  if (!sys::path::is_absolute(Path)) {
-    if (!SourceMgr.getFileManager().makeAbsolutePath(Path)) {
-      log("Could not turn relative path to absolute: {0}", Path);
-      return None;
+  if (!sys::path::is_absolute(FilePath)) {
+    if (auto EC =
+            SourceMgr.getFileManager().getVirtualFileSystem()->makeAbsolute(
+                FilePath)) {
+      elog("Could not turn relative path to absolute: {0}: {1}", FilePath,
+           EC.message());
+      return llvm::None;
     }
   }
 
+  // Handle the symbolic link path case where the current working directory
+  // (getCurrentWorkingDirectory) is a symlink./ We always want to the real
+  // file path (instead of the symlink path) for the  C++ symbols.
+  //
+  // Consider the following example:
+  //
+  //   src dir: /project/src/foo.h
+  //   current working directory (symlink): /tmp/build -> /project/src/
+  //
+  //  The file path of Symbol is "/project/src/foo.h" instead of
+  //  "/tmp/build/foo.h"
+  if (const DirectoryEntry *Dir = SourceMgr.getFileManager().getDirectory(
+          llvm::sys::path::parent_path(FilePath))) {
     SmallString<128> RealPath;
-  if (SourceMgr.getFileManager().getVirtualFileSystem()->getRealPath(
-          Path, RealPath)) {
-    log("Could not compute real path: {0}", Path);
-    return Path.str().str();
+    StringRef DirName = SourceMgr.getFileManager().getCanonicalName(Dir);
+    llvm::sys::path::append(RealPath, DirName,
+                            llvm::sys::path::filename(FilePath));
+    return RealPath.str().str();
   }
 
-  return RealPath.str().str();
+  return FilePath.str().str();
 }
 
 TextEdit toTextEdit(const FixItHint &FixIt, const SourceManager &M,
                     const LangOptions &L) {
   TextEdit Result;
   Result.range =
       halfOpenToRange(M, Lexer::makeFileCharRange(FixIt.RemoveRange, M, L));
   Result.newText = FixIt.CodeToInsert;

clangd/XRefs.cpp

 }
 
 Optional<Location> makeLocation(ParsedAST &AST, SourceLocation TokLoc,
                                 StringRef TUPath) {
   const SourceManager &SourceMgr = AST.getASTContext().getSourceManager();
   const FileEntry *F = SourceMgr.getFileEntryForID(SourceMgr.getFileID(TokLoc));
   if (!F)
     return None;
-  auto FilePath = getRealPath(F, SourceMgr);
+  auto FilePath = getCanonicalPath(F, SourceMgr);
   if (!FilePath) {
     log("failed to get path!");
     return None;
   }
   Location L;
   L.uri = URIForFile::canonicalize(*FilePath, TUPath);
   L.range = getTokenRange(AST, TokLoc);
   return L;
 }
 
 } // namespace
 
 std::vector<Location> findDefinitions(ParsedAST &AST, Position Pos,
                                       const SymbolIndex *Index) {
   const auto &SM = AST.getASTContext().getSourceManager();
-  auto MainFilePath = getRealPath(SM.getFileEntryForID(SM.getMainFileID()), SM);
+  auto MainFilePath =
+      getCanonicalPath(SM.getFileEntryForID(SM.getMainFileID()), SM);
   if (!MainFilePath) {
     elog("Failed to get a path for the main file, so no references");
     return {};
   }
 
   std::vector<Location> Result;
   // Handle goto definition for #include.
   for (auto &Inc : AST.getIncludeStructure().MainFileIncludes) {
std::vector<Location> findDefinitions(ParsedAST &AST, Position Pos,
   if (Index) {
     LookupRequest QueryRequest;
     // Build request for index query, using SymbolID.
     for (auto It : CandidatesIndex)
       QueryRequest.IDs.insert(It.first);
     std::string TUPath;
     const FileEntry *FE =
         SM.getFileEntryForID(SM.getMainFileID());
-    if (auto Path = getRealPath(FE, SM))
+    if (auto Path = getCanonicalPath(FE, SM))
       TUPath = *Path;
     // Query the index and populate the empty slot.
     Index->lookup(QueryRequest, [&TUPath, &ResultCandidates,
                                  &CandidatesIndex](const Symbol &Sym) {
       auto It = CandidatesIndex.find(Sym.ID);
       assert(It != CandidatesIndex.end());
       auto &Value = ResultCandidates[It->second];
 
Optional<Hover> getHover(ParsedAST &AST, Position Pos) {
 
   return None;
 }
 
 std::vector<Location> findReferences(ParsedAST &AST, Position Pos,
                                      const SymbolIndex *Index) {
   std::vector<Location> Results;
   const SourceManager &SM = AST.getASTContext().getSourceManager();
-  auto MainFilePath = getRealPath(SM.getFileEntryForID(SM.getMainFileID()), SM);
+  auto MainFilePath =
+      getCanonicalPath(SM.getFileEntryForID(SM.getMainFileID()), SM);
   if (!MainFilePath) {
     elog("Failed to get a path for the main file, so no references");
     return Results;
   }
   auto Loc = getBeginningOfIdentifier(AST, Pos, SM.getMainFileID());
   auto Symbols = getSymbolAtPosition(AST, Loc);
 
   std::vector<const Decl *> TargetDecls;

clangd/index/Background.cpp

 // Creates a filter to not collect index results from files with unchanged
 // digests.
 // \p FileDigests contains file digests for the current indexed files, and all
 // changed files will be added to \p FilesToUpdate.
 decltype(SymbolCollector::Options::FileFilter)
 createFileFilter(const llvm::StringMap<FileDigest> &FileDigests,
                  llvm::StringMap<FileDigest> &FilesToUpdate) {
   return [&FileDigests, &FilesToUpdate](const SourceManager &SM, FileID FID) {
-    StringRef Path;
-    if (const auto *F = SM.getFileEntryForID(FID))
-      Path = F->getName();
-    if (Path.empty())
+    const auto *F = SM.getFileEntryForID(FID);
+    if (!F)
       return false; // Skip invalid files.
-    SmallString<128> AbsPath(Path);
-    if (std::error_code EC =
-            SM.getFileManager().getVirtualFileSystem()->makeAbsolute(AbsPath)) {
-      elog("Warning: could not make absolute file: {0}", EC.message());
+    auto AbsPath = getCanonicalPath(F, SM);
+    if (!AbsPath)
       return false; // Skip files without absolute path.
-    }
-    sys::path::remove_dots(AbsPath, /*remove_dot_dot=*/true);
     auto Digest = digestFile(SM, FID);
     if (!Digest)
       return false;
-    auto D = FileDigests.find(AbsPath);
+    auto D = FileDigests.find(*AbsPath);
     if (D != FileDigests.end() && D->second == Digest)
       return false; // Skip files that haven't changed.
 
-    FilesToUpdate[AbsPath] = *Digest;
+    FilesToUpdate[*AbsPath] = *Digest;
     return true;
   };
 }
 
 } // namespace
 
 BackgroundIndex::BackgroundIndex(
     Context BackgroundContext, StringRef ResourceDir,

clangd/index/SymbolCollector.cpp

 // current working directory of the given SourceManager if the Path is not an
 // absolute path. If failed, this resolves relative paths against \p FallbackDir
 // to get an absolute path. Then, this tries creating an URI for the absolute
 // path with schemes specified in \p Opts. This returns an URI with the first
 // working scheme, if there is any; otherwise, this returns None.
 //
 // The Path can be a path relative to the build directory, or retrieved from
 // the SourceManager.
-Optional<std::string> toURI(const SourceManager &SM, StringRef Path,
-                            const SymbolCollector::Options &Opts) {
-  SmallString<128> AbsolutePath(Path);
-  if (std::error_code EC =
-          SM.getFileManager().getVirtualFileSystem()->makeAbsolute(
-              AbsolutePath))
-    log("Warning: could not make absolute file: {0}", EC.message());
-  if (sys::path::is_absolute(AbsolutePath)) {
-    // Handle the symbolic link path case where the current working directory
-    // (getCurrentWorkingDirectory) is a symlink./ We always want to the real
-    // file path (instead of the symlink path) for the  C++ symbols.
-    //
-    // Consider the following example:
-    //
-    //   src dir: /project/src/foo.h
-    //   current working directory (symlink): /tmp/build -> /project/src/
-    //
-    // The file path of Symbol is "/project/src/foo.h" instead of
-    // "/tmp/build/foo.h"
-    if (const DirectoryEntry *Dir = SM.getFileManager().getDirectory(
-            sys::path::parent_path(AbsolutePath.str()))) {
-      StringRef DirName = SM.getFileManager().getCanonicalName(Dir);
-      SmallString<128> AbsoluteFilename;
-      sys::path::append(AbsoluteFilename, DirName,
-                        sys::path::filename(AbsolutePath.str()));
-      AbsolutePath = AbsoluteFilename;
-    }
-  } else if (!Opts.FallbackDir.empty()) {
+std::string toURI(const SourceManager &SM, llvm::StringRef Path,
+  llvm::SmallString<128> AbsolutePath(Path);
+  if (auto CanonPath =
+          getCanonicalPath(SM.getFileManager().getFile(Path), SM)) {
+    AbsolutePath = *CanonPath;
+  }
+  // We don't perform is_absolute check in an else branch because makeAbsolute
+  // might return a relative path on some InMemoryFileSystems.
+  if (!sys::path::is_absolute(AbsolutePath) && !Opts.FallbackDir.empty())
     sys::fs::make_absolute(Opts.FallbackDir, AbsolutePath);
-  }
-
   sys::path::remove_dots(AbsolutePath, /*remove_dot_dot=*/true);
   return URI::create(AbsolutePath).toString();
 }
 
 // All proto generated headers should start with this line.
 static const char *PROTO_HEADER_COMMENT =
     "// Generated by the protocol buffer compiler.  DO NOT EDIT!";
 

 }
 
 // Return the symbol location of the token at \p TokLoc.
 Optional<SymbolLocation> getTokenLocation(SourceLocation TokLoc,
                                           const SourceManager &SM,
                                           const SymbolCollector::Options &Opts,
                                           const clang::LangOptions &LangOpts,
                                           std::string &FileURIStorage) {
-  auto U = toURI(SM, SM.getFilename(TokLoc), Opts);
-  if (!U)
+  auto Path = SM.getFilename(TokLoc);
+  if (Path.empty())
     return None;
-  FileURIStorage = std::move(*U);
+  FileURIStorage = toURI(SM, Path, Opts);
   SymbolLocation Result;
   Result.FileURI = FileURIStorage.c_str();
   auto Range = getTokenRange(TokLoc, SM, LangOpts);
   Result.Start = Range.first;
   Result.End = Range.second;
 
-  return std::move(Result);
+  return Result;
 }
 
 // Checks whether \p ND is a definition of a TagDecl (class/struct/enum/union)
 // in a header file, in which case clangd would prefer to use ND as a canonical
 // declaration.
 // FIXME: handle symbol types that are not TagDecl (e.g. functions), if using
 // the first seen declaration as canonical declaration is not a good enough
 // heuristic.
void SymbolCollector::finish() {
 
   const auto &SM = ASTCtx->getSourceManager();
   DenseMap<FileID, std::string> URICache;
   auto GetURI = [&](FileID FID) -> Optional<std::string> {
     auto Found = URICache.find(FID);
     if (Found == URICache.end()) {
       if (auto *FileEntry = SM.getFileEntryForID(FID)) {
         auto FileURI = toURI(SM, FileEntry->getName(), Opts);
-        if (!FileURI) {
-          log("Failed to create URI for file: {0}\n", FileEntry);
-          FileURI = ""; // reset to empty as we also want to cache this case.
-        }
-        Found = URICache.insert({FID, *FileURI}).first;
+        Found = URICache.insert({FID, FileURI}).first;
       } else {
         // Ignore cases where we can not find a corresponding file entry
         // for the loc, thoses are not interesting, e.g. symbols formed
         // via macro concatenation.
         return None;
       }
     }
     return Found->second;