[VFS] Cleanups to VFS interfaces.

[sammccall]

Summary

• add comments clarifying semantics
• Status::copyWithNewName(Status, Name) --> instance method
• Status::copyWithNewName(fs::file_status, Name) --> constructor (it's not a copy)
• File::getName() -> getRealPath(), reflecting its actual behavior/function and stop returning status().getName() in the base class (callers can do this fallback if they want to, it complicates the contracts).

This is mostly NFC, but the behavior of File::getName() affects FileManager's
FileEntry::tryGetRealPathName(), which now fails in more cases:

• non-real file cases
• real-file cases where the underlying vfs::File was opened in a way that doesn't call realpath().

(In these cases we don't know a distinct real name, so in principle it seems OK)

[klimek]

lg

[rupprecht]

Looks like this caused some test failures (http://lab.llvm.org:8011/builders/clang-cmake-aarch64-quick/builds/14441/steps/ninja%20check%201), e.g.:

[ RUN      ] GoToInclude.All
/home/buildslave/buildslave/clang-cmake-aarch64-quick/llvm/tools/clang/tools/extra/unittests/clangd/XRefsTests.cpp:953: Failure
Value of: *Locations
Expected: has 1 element that is equal to 0:0-0:0@file:///clangd-test/foo.h
  Actual: {}

I'm not familiar with this patch, but reverting it locally seems to get tests passing again, so I'd like to revert this change temporarily.

[ilya-biryukov]

For the record: this got reverted in rL337850

cfe/trunk/include/clang/Basic/VirtualFileSystem.h

 
 class MemoryBuffer;
 
 } // namespace llvm
 
 namespace clang {
 namespace vfs {
 
-/// The result of a \p status operation.
+/// File information from a \p File::status() operation.
+/// This is roughly equivalent to a `struct stat` plus a file path.
 class Status {
   std::string Name;
   llvm::sys::fs::UniqueID UID;
   llvm::sys::TimePoint<> MTime;
   uint32_t User;
   uint32_t Group;
   uint64_t Size;
   llvm::sys::fs::file_type Type = llvm::sys::fs::file_type::status_error;
   llvm::sys::fs::perms Perms;
 
 public:
    // FIXME: remove when files support multiple names
   bool IsVFSMapped = false;
 
   Status() = default;
   Status(const llvm::sys::fs::file_status &Status);
   Status(StringRef Name, llvm::sys::fs::UniqueID UID,
          llvm::sys::TimePoint<> MTime, uint32_t User, uint32_t Group,
          uint64_t Size, llvm::sys::fs::file_type Type,
          llvm::sys::fs::perms Perms);
+  Status(const llvm::sys::fs::file_status &FSStatus, StringRef Name);
 
-  /// Get a copy of a Status with a different name.
-  static Status copyWithNewName(const Status &In, StringRef NewName);
-  static Status copyWithNewName(const llvm::sys::fs::file_status &In,
-                                StringRef NewName);
+  /// Get a copy of a this Status with a different name.
+  Status copyWithNewName(StringRef NewName);
 
   /// Returns the name that should be used for this file or directory.
+  /// This is usually the path that the file was opened as, without resolving
+  /// relative paths or symlinks.
   StringRef getName() const { return Name; }
 
   /// @name Status interface from llvm::sys::fs
   /// @{
   llvm::sys::fs::file_type getType() const { return Type; }
   llvm::sys::fs::perms getPermissions() const { return Perms; }
   llvm::sys::TimePoint<> getLastModificationTime() const { return MTime; }
   llvm::sys::fs::UniqueID getUniqueID() const { return UID; }

 class File {
 public:
   /// Destroy the file after closing it (if open).
   /// Sub-classes should generally call close() inside their destructors.  We
   /// cannot do that from the base class, since close is virtual.
   virtual ~File();
 
   /// Get the status of the file.
+  /// This may access the filesystem (e.g. `stat()`), or return a cached value.
   virtual llvm::ErrorOr<Status> status() = 0;
 
-  /// Get the name of the file
-  virtual llvm::ErrorOr<std::string> getName() {
-    if (auto Status = status())
-      return Status->getName().str();
-    else
-      return Status.getError();
-  }
+  /// Get the "real name" of the file, if available.
+  /// This should be absolute, and typically has symlinks resolved.
+  ///
+  /// Only some VFS implementations provide this, and only sometimes.
+  /// FIXME: these maybe-available semantics are not very useful. It would be
+  /// nice if this was more consistent with FileSystem::getRealPath().
+  virtual llvm::Optional<std::string> getRealPath() { return llvm::None; }
 
   /// Get the contents of the file as a \p MemoryBuffer.
   virtual llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>>
   getBuffer(const Twine &Name, int64_t FileSize = -1,
             bool RequiresNullTerminator = true, bool IsVolatile = false) = 0;
 
   /// Closes the file.
   virtual std::error_code close() = 0;

cfe/trunk/lib/Basic/FileManager.cpp

const FileEntry *FileManager::getFile(StringRef Filename, bool openFile,
   UFE.Dir     = DirInfo;
   UFE.UID     = NextFileUID++;
   UFE.UniqueID = Data.UniqueID;
   UFE.IsNamedPipe = Data.IsNamedPipe;
   UFE.InPCH = Data.InPCH;
   UFE.File = std::move(F);
   UFE.IsValid = true;
   if (UFE.File)
-    if (auto RealPathName = UFE.File->getName())
+    if (auto RealPathName = UFE.File->getRealPath())
       UFE.RealPathName = *RealPathName;
   return &UFE;
 }
 
 const FileEntry *
 FileManager::getVirtualFile(StringRef Filename, off_t Size,
                             time_t ModificationTime) {
   ++NumFileLookups;

cfe/trunk/lib/Basic/VirtualFileSystem.cpp

: UID(Status.getUniqueID()), MTime(Status.getLastModificationTime()),
       Type(Status.type()), Perms(Status.permissions())  {}
 
 Status::Status(StringRef Name, UniqueID UID, sys::TimePoint<> MTime,
                uint32_t User, uint32_t Group, uint64_t Size, file_type Type,
                perms Perms)
     : Name(Name), UID(UID), MTime(MTime), User(User), Group(Group), Size(Size),
       Type(Type), Perms(Perms) {}
 
-Status Status::copyWithNewName(const Status &In, StringRef NewName) {
-  return Status(NewName, In.getUniqueID(), In.getLastModificationTime(),
-                In.getUser(), In.getGroup(), In.getSize(), In.getType(),
-                In.getPermissions());
-}
-
-Status Status::copyWithNewName(const file_status &In, StringRef NewName) {
-  return Status(NewName, In.getUniqueID(), In.getLastModificationTime(),
-                In.getUser(), In.getGroup(), In.getSize(), In.type(),
-                In.permissions());
+Status::Status(const file_status &In, StringRef NewName)
+    : Status(NewName, In.getUniqueID(), In.getLastModificationTime(),
+             In.permissions()) {}
+
+Status Status::copyWithNewName(StringRef NewName) {
+  return Status(NewName, getUniqueID(), getLastModificationTime(), getUser(),
+                getGroup(), getSize(), getType(), getPermissions());
 }
 
 bool Status::equivalent(const Status &Other) const {
   assert(isStatusKnown() && Other.isStatusKnown());
   return getUniqueID() == Other.getUniqueID();
 }
 
 bool Status::isDirectory() const {

 /// Wrapper around a raw file descriptor.
 class RealFile : public File {
   friend class RealFileSystem;
 
   int FD;
   Status S;
   std::string RealName;
 
+  // NewRealPathName is used only if non-empty.
   RealFile(int FD, StringRef NewName, StringRef NewRealPathName)
       : FD(FD), S(NewName, {}, {}, {}, {}, {},
                   llvm::sys::fs::file_type::status_error, {}),
         RealName(NewRealPathName.str()) {
     assert(FD >= 0 && "Invalid or inactive file descriptor");
   }
 
 public:
   ~RealFile() override;
 
   ErrorOr<Status> status() override;
-  ErrorOr<std::string> getName() override;
+  Optional<std::string> getRealPath() override;
   ErrorOr<std::unique_ptr<MemoryBuffer>> getBuffer(const Twine &Name,
                                                    int64_t FileSize,
                                                    bool RequiresNullTerminator,
                                                    bool IsVolatile) override;
   std::error_code close() override;
 };
 
 } // namespace
 
 RealFile::~RealFile() { close(); }
 
 ErrorOr<Status> RealFile::status() {
   assert(FD != -1 && "cannot stat closed file");
   if (!S.isStatusKnown()) {
     file_status RealStatus;
     if (std::error_code EC = sys::fs::status(FD, RealStatus))
       return EC;
-    S = Status::copyWithNewName(RealStatus, S.getName());
+    S = Status(RealStatus, S.getName());
   }
   return S;
 }
 
-ErrorOr<std::string> RealFile::getName() {
-  return RealName.empty() ? S.getName().str() : RealName;
+Optional<std::string> RealFile::getRealPath() {
+  if (RealName.empty())
+    return llvm::None;
+  return RealName;
 }
 
 ErrorOr<std::unique_ptr<MemoryBuffer>>
 RealFile::getBuffer(const Twine &Name, int64_t FileSize,
                     bool RequiresNullTerminator, bool IsVolatile) {
   assert(FD != -1 && "cannot get buffer for closed file");
   return MemoryBuffer::getOpenFile(FD, Name, FileSize, RequiresNullTerminator,
                                    IsVolatile);

 };
 
 } // namespace
 
 ErrorOr<Status> RealFileSystem::status(const Twine &Path) {
   sys::fs::file_status RealStatus;
   if (std::error_code EC = sys::fs::status(Path, RealStatus))
     return EC;
-  return Status::copyWithNewName(RealStatus, Path.str());
+  return Status(RealStatus, Path.str());
 }
 
 ErrorOr<std::unique_ptr<File>>
 RealFileSystem::openFileForRead(const Twine &Name) {
   int FD;
   SmallString<256> RealName;
   if (std::error_code EC =
           sys::fs::openFileForRead(Name, FD, sys::fs::OF_None, &RealName))

 class RealFSDirIter : public clang::vfs::detail::DirIterImpl {
   llvm::sys::fs::directory_iterator Iter;
 
 public:
   RealFSDirIter(const Twine &Path, std::error_code &EC) : Iter(Path, EC) {
     if (Iter != llvm::sys::fs::directory_iterator()) {
       llvm::sys::fs::file_status S;
       std::error_code ErrorCode = llvm::sys::fs::status(Iter->path(), S, true);
-      CurrentEntry = Status::copyWithNewName(S, Iter->path());
+      CurrentEntry = Status(S, Iter->path());
       if (!EC)
         EC = ErrorCode;
     }
   }
 
   std::error_code increment() override {
     std::error_code EC;
     Iter.increment(EC);
     if (Iter == llvm::sys::fs::directory_iterator()) {
       CurrentEntry = Status();
     } else {
       llvm::sys::fs::file_status S;
       std::error_code ErrorCode = llvm::sys::fs::status(Iter->path(), S, true);
-      CurrentEntry = Status::copyWithNewName(S, Iter->path());
+      CurrentEntry = Status(S, Iter->path());
       if (!EC)
         EC = ErrorCode;
     }
     return EC;
   }
 };
 
 } // namespace
#endif
   }
   return make_error_code(llvm::errc::no_such_file_or_directory);
 }
 
 static Status getRedirectedFileStatus(const Twine &Path, bool UseExternalNames,
                                       Status ExternalStatus) {
   Status S = ExternalStatus;
   if (!UseExternalNames)
-    S = Status::copyWithNewName(S, Path.str());
+    S = S.copyWithNewName(Path.str());
   S.IsVFSMapped = true;
   return S;
 }
 
 ErrorOr<Status> RedirectingFileSystem::status(const Twine &Path, Entry *E) {
   assert(E != nullptr);
   if (auto *F = dyn_cast<RedirectingFileEntry>(E)) {
     ErrorOr<Status> S = ExternalFS->status(F->getExternalContentsPath());
     assert(!S || S->getName() == F->getExternalContentsPath());
     if (S)
       return getRedirectedFileStatus(Path, F->useExternalName(UseExternalNames),
                                      *S);
     return S;
   } else { // directory
     auto *DE = cast<RedirectingDirectoryEntry>(E);
-    return Status::copyWithNewName(DE->getStatus(), Path.str());
+    return DE->getStatus().copyWithNewName(Path.str());
   }
 }
 
 ErrorOr<Status> RedirectingFileSystem::status(const Twine &Path) {
   ErrorOr<Entry *> Result = lookupPath(Path);
   if (!Result)
     return Result.getError();
   return status(Path, *Result);