[VFS] Add "expand tilde" argument to getRealPath.

[JDevlieghere]

Summary

Add an optional argument to expand tildes in the path to mirror llvm's
implementation of the corresponding function.

Apparently this change is more controversial than I thought, so happy
to discuss alternatives here.

[JDevlieghere]

In rL346453#296733, @sammccall wrote:

I think this change was landed without review, and it's a complicated interface change, so I'm going to revert it for now.

I also have some specific concerns about the design:

• this doesn't seem to be a per-filesystem concern. e.g. in an overlaid filesystem, whose responsibility is it to resolve ~? It's not clear this belongs in the virtualization layer at all.

I'm not sure this is true. It the overlay specifies a different home directory (this functionality doesn't exist yet) it should take that one, otherwise it should defer to the underlying one.

• implementing this is a substantial burden on VFSes, many of which (by their nature) live out-of-tree. It's not clear what the consequences of ignoring the parameter are, and what callers can expect.
• use of optional-args in virtual methods would be nice to avoid
• the use of boolean params like this in general doesn't scale well

Would a separate method address those 3 concerns? My goal was to keep things simple and mirror llvm's API, which helps with migrating between the two. I don't have a strong opinion on this though, as long as the functionality is available.

Happy to help with review if you do want to move forward with this!

Thanks, I look forward to your input!

[llvm-commits]

Thanks for sending this!
I think fs::real_path isn't a great precedent to be following here, but
that we can put the same functionality behind a slightly different API.

I may well be missing requirements, details, or historical context here.
Hopefully you or someone can correct me if so!

• msg-24308-376.txt162 BDownload

[JDevlieghere]

I created D54448 to add an expand_tilde function to llvm's filesystem as per Sam's suggestion. This still won't work for my reproducer feature in LLDB, but if everyone feels this doesn't belong in the VFS I can simply implement it there.

llvm/trunk/include/llvm/Support/VirtualFileSystem.h

public:
 
   /// Get the working directory of this file system.
   virtual llvm::ErrorOr<std::string> getCurrentWorkingDirectory() const = 0;
 
   /// Gets real path of \p Path e.g. collapse all . and .. patterns, resolve
   /// symlinks. For real file system, this uses `llvm::sys::fs::real_path`.
   /// This returns errc::operation_not_permitted if not implemented by subclass.
   virtual std::error_code getRealPath(const Twine &Path,
-                                      SmallVectorImpl<char> &Output) const;
+                                      SmallVectorImpl<char> &Output,
+                                      bool ExpandTilde = false) const;
 
   /// Check whether a file exists. Provided for convenience.
   bool exists(const Twine &Path);
 
   /// Is the file mounted on a local filesystem?
   virtual std::error_code isLocal(const Twine &Path, bool &Result);
 
   /// Make \a Path an absolute path.
public:
 
   llvm::ErrorOr<Status> status(const Twine &Path) override;
   llvm::ErrorOr<std::unique_ptr<File>>
   openFileForRead(const Twine &Path) override;
   directory_iterator dir_begin(const Twine &Dir, std::error_code &EC) override;
   llvm::ErrorOr<std::string> getCurrentWorkingDirectory() const override;
   std::error_code setCurrentWorkingDirectory(const Twine &Path) override;
   std::error_code isLocal(const Twine &Path, bool &Result) override;
-  std::error_code getRealPath(const Twine &Path,
-                              SmallVectorImpl<char> &Output) const override;
+  std::error_code getRealPath(const Twine &Path, SmallVectorImpl<char> &Output,
+                              bool ExpandTilde = false) const override;
 
   using iterator = FileSystemList::reverse_iterator;
   using const_iterator = FileSystemList::const_reverse_iterator;
 
   /// Get an iterator pointing to the most recently added file system.
   iterator overlays_begin() { return FSList.rbegin(); }
   const_iterator overlays_begin() const { return FSList.rbegin(); }
 
directory_iterator dir_begin(const Twine &Dir, std::error_code &EC) override {
     return FS->dir_begin(Dir, EC);
   }
   llvm::ErrorOr<std::string> getCurrentWorkingDirectory() const override {
     return FS->getCurrentWorkingDirectory();
   }
   std::error_code setCurrentWorkingDirectory(const Twine &Path) override {
     return FS->setCurrentWorkingDirectory(Path);
   }
-  std::error_code getRealPath(const Twine &Path,
-                              SmallVectorImpl<char> &Output) const override {
-    return FS->getRealPath(Path, Output);
+  std::error_code getRealPath(const Twine &Path, SmallVectorImpl<char> &Output,
+                              bool ExpandTilde = false) const override {
+    return FS->getRealPath(Path, Output, ExpandTilde);
   }
 
 protected:
   FileSystem &getUnderlyingFS() { return *FS; }
 
 private:
   IntrusiveRefCntPtr<FileSystem> FS;
 };
llvm::ErrorOr<std::string> getCurrentWorkingDirectory() const override {
     return WorkingDirectory;
   }
   /// Canonicalizes \p Path by combining with the current working
   /// directory and normalizing the path (e.g. remove dots). If the current
   /// working directory is not set, this returns errc::operation_not_permitted.
   ///
   /// This doesn't resolve symlinks as they are not supported in in-memory file
   /// system.
-  std::error_code getRealPath(const Twine &Path,
-                              SmallVectorImpl<char> &Output) const override;
+  std::error_code getRealPath(const Twine &Path, SmallVectorImpl<char> &Output,
+                              bool ExpandTilde = false) const override;
   std::error_code isLocal(const Twine &Path, bool &Result) override;
   std::error_code setCurrentWorkingDirectory(const Twine &Path) override;
 };
 
 /// Get a globally unique ID for a virtual file or directory.
 llvm::sys::fs::UniqueID getNextVirtualUniqueID();
 
 /// Gets a \p FileSystem for a virtual file system described in YAML

llvm/trunk/lib/Support/VirtualFileSystem.cpp

std::error_code FileSystem::makeAbsolute(SmallVectorImpl<char> &Path) const {
   auto WorkingDir = getCurrentWorkingDirectory();
   if (!WorkingDir)
     return WorkingDir.getError();
 
   return llvm::sys::fs::make_absolute(WorkingDir.get(), Path);
 }
 
 std::error_code FileSystem::getRealPath(const Twine &Path,
-                                        SmallVectorImpl<char> &Output) const {
+                                        SmallVectorImpl<char> &Output,
+                                        bool ExpandTilde) const {
   return errc::operation_not_permitted;
 }
 
 std::error_code FileSystem::isLocal(const Twine &Path, bool &Result) {
   return errc::operation_not_permitted;
 }
 
 bool FileSystem::exists(const Twine &Path) {

 public:
   ErrorOr<Status> status(const Twine &Path) override;
   ErrorOr<std::unique_ptr<File>> openFileForRead(const Twine &Path) override;
   directory_iterator dir_begin(const Twine &Dir, std::error_code &EC) override;
 
   llvm::ErrorOr<std::string> getCurrentWorkingDirectory() const override;
   std::error_code setCurrentWorkingDirectory(const Twine &Path) override;
   std::error_code isLocal(const Twine &Path, bool &Result) override;
-  std::error_code getRealPath(const Twine &Path,
-                              SmallVectorImpl<char> &Output) const override;
+  std::error_code getRealPath(const Twine &Path, SmallVectorImpl<char> &Output,
+                              bool ExpandTilde = false) const override;
 
 private:
   mutable std::mutex CWDMutex;
   mutable std::string CWDCache;
 };
 
 } // namespace
 
std::error_code RealFileSystem::setCurrentWorkingDirectory(const Twine &Path) {
   CWDCache.clear();
   return std::error_code();
 }
 
 std::error_code RealFileSystem::isLocal(const Twine &Path, bool &Result) {
   return llvm::sys::fs::is_local(Path, Result);
 }
 
-std::error_code
-RealFileSystem::getRealPath(const Twine &Path,
-                            SmallVectorImpl<char> &Output) const {
+std::error_code RealFileSystem::getRealPath(const Twine &Path,
+                                            SmallVectorImpl<char> &Output,
+                                            bool ExpandTilde) const {
   return llvm::sys::fs::real_path(Path, Output);
 }
 
 IntrusiveRefCntPtr<FileSystem> vfs::getRealFileSystem() {
   static IntrusiveRefCntPtr<FileSystem> FS = new RealFileSystem();
   return FS;
 }
 

 
 std::error_code OverlayFileSystem::isLocal(const Twine &Path, bool &Result) {
   for (auto &FS : FSList)
     if (FS->exists(Path))
       return FS->isLocal(Path, Result);
   return errc::no_such_file_or_directory;
 }
 
-std::error_code
-OverlayFileSystem::getRealPath(const Twine &Path,
-                               SmallVectorImpl<char> &Output) const {
+std::error_code OverlayFileSystem::getRealPath(const Twine &Path,
+                                               SmallVectorImpl<char> &Output,
+                                               bool ExpandTilde) const {
   for (auto &FS : FSList)
     if (FS->exists(Path))
-      return FS->getRealPath(Path, Output);
+      return FS->getRealPath(Path, Output, ExpandTilde);
   return errc::no_such_file_or_directory;
 }
 
 llvm::vfs::detail::DirIterImpl::~DirIterImpl() = default;
 
 namespace {
 
 class OverlayFSDirIterImpl : public llvm::vfs::detail::DirIterImpl {
std::error_code InMemoryFileSystem::setCurrentWorkingDirectory(const Twine &P) {
   if (useNormalizedPaths())
     llvm::sys::path::remove_dots(Path, /*remove_dot_dot=*/true);
 
   if (!Path.empty())
     WorkingDirectory = Path.str();
   return {};
 }
 
-std::error_code
-InMemoryFileSystem::getRealPath(const Twine &Path,
-                                SmallVectorImpl<char> &Output) const {
+std::error_code InMemoryFileSystem::getRealPath(const Twine &Path,
+                                                SmallVectorImpl<char> &Output,
+                                                bool ExpandTilde) const {
   auto CWD = getCurrentWorkingDirectory();
   if (!CWD || CWD->empty())
     return errc::operation_not_permitted;
   Path.toVector(Output);
   if (auto EC = makeAbsolute(Output))
     return EC;
   llvm::sys::path::remove_dots(Output, /*remove_dot_dot=*/true);
   return {};

llvm/trunk/unittests/Support/VirtualFileSystemTest.cpp

public:
   }
   llvm::ErrorOr<std::string> getCurrentWorkingDirectory() const override {
     return std::string();
   }
   std::error_code setCurrentWorkingDirectory(const Twine &Path) override {
     return std::error_code();
   }
   // Map any symlink to "/symlink".
-  std::error_code getRealPath(const Twine &Path,
-                              SmallVectorImpl<char> &Output) const override {
+  std::error_code getRealPath(const Twine &Path, SmallVectorImpl<char> &Output,
+                              bool ExpandTilde) const override {
     auto I = FilesAndDirs.find(Path.str());
     if (I == FilesAndDirs.end())
       return make_error_code(llvm::errc::no_such_file_or_directory);
     if (I->second.isSymlink()) {
       Output.clear();
       Twine("/symlink").toVector(Output);
       return std::error_code();
     }