FileManager: Use llvm::Expected in new getFileRef API

[dexonsmith]

Summary

FileManager::getFileRef is a modern API which we expect to convert to
over time. We should modernize the error handling as well, using
llvm::Expected instead of llvm::ErrorOr, to help clients that care
about errors to ensure nothing is missed.

However, not all clients care. I've also added another path for those
that don't:

• FileEntryRef is now copy- and move-assignable (using a pointer instead of a reference).
• FileManager::getOptionalFileRef returns an llvm::Optional instead of llvm::Expected.
• Added an llvm::expectedToOptional utility in case this is useful elsewhere.

[dexonsmith]

@arphaman, is there a reason you think ErrorOr is more appropriate long-term here?

[arphaman]

clang/lib/Lex/HeaderSearch.cpp

1081

It should be fine to update the return type, but I believe that Expected will cause an assertion as the error is unhandled in cases like this one. Can you verify that all errors are consumed somehow?

[dexonsmith]

Updated patch after running check-clang and learning more about Expected. I've added a parallel API using Optional<FileEntryRef> for clients that don't want to do anything with the error.

@lhames, is it reasonable to add llvm::expectedToOptional?

[dexonsmith]

clang/lib/Lex/HeaderSearch.cpp

1081

Yup, when the tests finished running I saw that I'd misunderstood how Expected::operator bool works.

For the users that don't care about the errors, consuming them is harmful boilerplate. I've switched to a split API that either returns Expected or Optional. This way clients that do care get the assertions if they mishandle it, and the clients that don't annotate that at the call site. WDYT?

[dexonsmith]

Also update FileEntryRef to be copy- and move-assignable so that Optional<FileEntryRef> can be assigned to.

[dexonsmith]

Fix the change to CompilerInstance.cpp.

[lhames]

In D66705#1644166, @dexonsmith wrote:

Updated patch after running check-clang and learning more about Expected. I've added a parallel API using Optional<FileEntryRef> for clients that don't want to do anything with the error.

@lhames, is it reasonable to add llvm::expectedToOptional?

Absolutely: We've already got an errorToBool, so expectedToOptional makes sense.

[arphaman]

LGTM

[dexonsmith]

r369943.

clang/include/clang/Basic/FileManager.h

public:
   // relying on RealPathName being empty.
   bool isOpenForTests() const { return File != nullptr; }
 };
 
 /// A reference to a \c FileEntry that includes the name of the file as it was
 /// accessed by the FileManager's client.
 class FileEntryRef {
 public:
+  FileEntryRef() = delete;
   FileEntryRef(StringRef Name, const FileEntry &Entry)
-      : Name(Name), Entry(Entry) {}
+      : Name(Name), Entry(&Entry) {}
 
   const StringRef getName() const { return Name; }
 
-  const FileEntry &getFileEntry() const { return Entry; }
+  const FileEntry &getFileEntry() const { return *Entry; }
 
-  off_t getSize() const { return Entry.getSize(); }
+  off_t getSize() const { return Entry->getSize(); }
 
-  unsigned getUID() const { return Entry.getUID(); }
+  unsigned getUID() const { return Entry->getUID(); }
 
   const llvm::sys::fs::UniqueID &getUniqueID() const {
-    return Entry.getUniqueID();
+    return Entry->getUniqueID();
   }
 
-  time_t getModificationTime() const { return Entry.getModificationTime(); }
+  time_t getModificationTime() const { return Entry->getModificationTime(); }
 
 private:
   StringRef Name;
-  const FileEntry &Entry;
+  const FileEntry *Entry;
 };
 
 /// Implements support for file system lookup, file system caching,
 /// and directory search management.
 ///
 /// This also handles more advanced properties, such as uniquing files based
 /// on "inode", so that a file with two names (e.g. symlinked) will be treated
 /// as a single file.
public:
   ///
   /// This returns a \c std::error_code if there was an error loading the file,
   /// or a \c FileEntryRef otherwise.
   ///
   /// \param OpenFile if true and the file exists, it will be opened.
   ///
   /// \param CacheFailure If true and the file does not exist, we'll cache
   /// the failure to find this file.
-  llvm::ErrorOr<FileEntryRef> getFileRef(StringRef Filename,
+  llvm::Expected<FileEntryRef> getFileRef(StringRef Filename,
                                           bool OpenFile = false,
                                           bool CacheFailure = true);
 
+  /// Get a FileEntryRef if it exists, without doing anything on error.
+  llvm::Optional<FileEntryRef> getOptionalFileRef(StringRef Filename,
+                                                  bool OpenFile = false,
+                                                  bool CacheFailure = true) {
+    return llvm::expectedToOptional(
+        getFileRef(Filename, OpenFile, CacheFailure));
+  }
+
   /// Returns the current file system options
   FileSystemOptions &getFileSystemOpts() { return FileSystemOpts; }
   const FileSystemOptions &getFileSystemOpts() const { return FileSystemOpts; }
 
   llvm::vfs::FileSystem &getVirtualFileSystem() const { return *FS; }
 
   void setVirtualFileSystem(IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS) {
     this->FS = std::move(FS);

clang/lib/Basic/FileManager.cpp

#endif
   return &UDE;
 }
 
 llvm::ErrorOr<const FileEntry *>
 FileManager::getFile(StringRef Filename, bool openFile, bool CacheFailure) {
   auto Result = getFileRef(Filename, openFile, CacheFailure);
   if (Result)
     return &Result->getFileEntry();
-  return Result.getError();
+  return llvm::errorToErrorCode(Result.takeError());
 }
 
-llvm::ErrorOr<FileEntryRef>
+llvm::Expected<FileEntryRef>
 FileManager::getFileRef(StringRef Filename, bool openFile, bool CacheFailure) {
   ++NumFileLookups;
 
   // See if there is already an entry in the map.
   auto SeenFileInsertResult =
       SeenFileEntries.insert({Filename, std::errc::no_such_file_or_directory});
   if (!SeenFileInsertResult.second) {
     if (!SeenFileInsertResult.first->second)
-      return SeenFileInsertResult.first->second.getError();
+      return llvm::errorCodeToError(
+          SeenFileInsertResult.first->second.getError());
     // Construct and return and FileEntryRef, unless it's a redirect to another
     // filename.
     SeenFileEntryOrRedirect Value = *SeenFileInsertResult.first->second;
     FileEntry *FE;
     if (LLVM_LIKELY(FE = Value.dyn_cast<FileEntry *>()))
       return FileEntryRef(SeenFileInsertResult.first->first(), *FE);
     return getFileRef(*Value.get<const StringRef *>(), openFile, CacheFailure);
   }
FileManager::getFileRef(StringRef Filename, bool openFile, bool CacheFailure) {
   // without a 'sys' subdir will get a cached failure result.
   auto DirInfoOrErr = getDirectoryFromFile(*this, Filename, CacheFailure);
   if (!DirInfoOrErr) { // Directory doesn't exist, file can't exist.
     if (CacheFailure)
       NamedFileEnt.second = DirInfoOrErr.getError();
     else
       SeenFileEntries.erase(Filename);
 
-    return DirInfoOrErr.getError();
+    return llvm::errorCodeToError(DirInfoOrErr.getError());
   }
   const DirectoryEntry *DirInfo = *DirInfoOrErr;
 
   // FIXME: Use the directory info to prune this, before doing the stat syscall.
   // FIXME: This will reduce the # syscalls.
 
   // Check to see if the file exists.
   std::unique_ptr<llvm::vfs::File> F;
   llvm::vfs::Status Status;
   auto statError = getStatValue(InterndFileName, Status, true,
                                 openFile ? &F : nullptr);
   if (statError) {
     // There's no real file at the given path.
     if (CacheFailure)
       NamedFileEnt.second = statError;
     else
       SeenFileEntries.erase(Filename);
 
-    return statError;
+    return llvm::errorCodeToError(statError);
   }
 
   assert((openFile || !F) && "undesired open file");
 
   // It exists.  See if we have already opened a file with the same inode.
   // This occurs when one dir is symlinked to another, for example.
   FileEntry &UFE = UniqueRealFiles[Status.getUniqueID()];
 

clang/lib/Frontend/CompilerInstance.cpp

bool CompilerInstance::InitializeSourceManager(
   }
 
   StringRef InputFile = Input.getFile();
 
   // Figure out where to get and map in the main file.
   if (InputFile != "-") {
     auto FileOrErr = FileMgr.getFileRef(InputFile, /*OpenFile=*/true);
     if (!FileOrErr) {
+      // FIXME: include the error in the diagnostic.
+      consumeError(FileOrErr.takeError());
       Diags.Report(diag::err_fe_error_reading) << InputFile;
       return false;
     }
     FileEntryRef File = *FileOrErr;
 
     // The natural SourceManager infrastructure can't currently handle named
     // pipes, but we would at least like to accept them for the main
     // file. Detect them here, read them with the volatile flag so FileMgr will

clang/lib/Lex/HeaderMap.cpp

 Optional<FileEntryRef> HeaderMap::LookupFile(StringRef Filename,
                                              FileManager &FM) const {
 
   SmallString<1024> Path;
   StringRef Dest = HeaderMapImpl::lookupFilename(Filename, Path);
   if (Dest.empty())
     return None;
 
-  if (auto File = FM.getFileRef(Dest))
-    return *File;
-  return None;
+  return FM.getOptionalFileRef(Dest);
 }
 
 StringRef HeaderMapImpl::lookupFilename(StringRef Filename,
                                         SmallVectorImpl<char> &DestPath) const {
   const HMapHeader &Hdr = getHeader();
   unsigned NumBuckets = getEndianAdjustedWord(Hdr.NumBuckets);
 
   // Don't probe infinitely.  This should be checked before constructing.

clang/lib/Lex/HeaderSearch.cpp

Optional<FileEntryRef> HeaderSearch::getFileAndSuggestModule(
     bool IsSystemHeaderDir, Module *RequestingModule,
     ModuleMap::KnownHeader *SuggestedModule) {
   // If we have a module map that might map this header, load it and
   // check whether we'll have a suggestion for a module.
   auto File = getFileMgr().getFileRef(FileName, /*OpenFile=*/true);
   if (!File) {
     // For rare, surprising errors (e.g. "out of file handles"), diag the EC
     // message.
-    std::error_code EC = File.getError();
+    std::error_code EC = llvm::errorToErrorCode(File.takeError());
     if (EC != llvm::errc::no_such_file_or_directory &&
         EC != llvm::errc::invalid_argument &&
         EC != llvm::errc::is_a_directory && EC != llvm::errc::not_a_directory) {
       Diags.Report(IncludeLoc, diag::err_cannot_open_file)
           << FileName << EC.message();
     }
     return None;
   }
if (llvm::sys::path::is_relative(Dest)) {
     MappedName.append(Dest.begin(), Dest.end());
     Filename = StringRef(MappedName.begin(), MappedName.size());
     HasBeenMapped = true;
     Optional<FileEntryRef> Result = HM->LookupFile(Filename, HS.getFileMgr());
     if (Result) {
       FixupSearchPath();
       return *Result;
     }
-  } else if (auto Res = HS.getFileMgr().getFileRef(Dest)) {
+  } else if (auto Res = HS.getFileMgr().getOptionalFileRef(Dest)) {
     FixupSearchPath();
     return *Res;
   }
 
   return None;
 }
 
 /// Given a framework directory, find the top-most framework directory.
Optional<FileEntryRef> DirectoryLookup::DoFrameworkLookup(
   if (SearchPath) {
     SearchPath->clear();
     // Without trailing '/'.
     SearchPath->append(FrameworkName.begin(), FrameworkName.end()-1);
   }
 
   FrameworkName.append(Filename.begin()+SlashPos+1, Filename.end());
 
-  llvm::ErrorOr<FileEntryRef> File =
-      FileMgr.getFileRef(FrameworkName, /*OpenFile=*/!SuggestedModule);
-
+  auto File =
+      FileMgr.getOptionalFileRef(FrameworkName, /*OpenFile=*/!SuggestedModule);
   if (!File) {
     // Check "/System/Library/Frameworks/Cocoa.framework/PrivateHeaders/file.h"
     const char *Private = "Private";
     FrameworkName.insert(FrameworkName.begin()+OrigSize, Private,
                          Private+strlen(Private));
     if (SearchPath)
       SearchPath->insert(SearchPath->begin()+OrigSize, Private,
                          Private+strlen(Private));
 
-    File = FileMgr.getFileRef(FrameworkName, /*OpenFile=*/!SuggestedModule);
+    File = FileMgr.getOptionalFileRef(FrameworkName,
+                                      /*OpenFile=*/!SuggestedModule);
   }
 
   // If we found the header and are allowed to suggest a module, do so now.
   if (File && needModuleLookup(RequestingModule, SuggestedModule)) {
     // Find the framework in which this header occurs.
     StringRef FrameworkPath = File->getFileEntry().getDir()->getName();
     bool FoundFramework = false;
     do {
Optional<FileEntryRef> HeaderSearch::LookupSubframeworkHeader(
   HeadersFilename += "Headers/";
   if (SearchPath) {
     SearchPath->clear();
     // Without trailing '/'.
     SearchPath->append(HeadersFilename.begin(), HeadersFilename.end()-1);
   }
 
   HeadersFilename.append(Filename.begin()+SlashPos+1, Filename.end());
-  llvm::ErrorOr<FileEntryRef> File =
-      FileMgr.getFileRef(HeadersFilename, /*OpenFile=*/true);
-
+  auto File = FileMgr.getOptionalFileRef(HeadersFilename, /*OpenFile=*/true);
   if (!File) {
     // Check ".../Frameworks/HIToolbox.framework/PrivateHeaders/HIToolbox.h"
     HeadersFilename = FrameworkName;
     HeadersFilename += "PrivateHeaders/";
     if (SearchPath) {
       SearchPath->clear();
       // Without trailing '/'.
       SearchPath->append(HeadersFilename.begin(), HeadersFilename.end()-1);
     }
 
     HeadersFilename.append(Filename.begin()+SlashPos+1, Filename.end());
-    File = FileMgr.getFileRef(HeadersFilename, /*OpenFile=*/true);
+    File = FileMgr.getOptionalFileRef(HeadersFilename, /*OpenFile=*/true);
 
     if (!File)
       return None;
   }
 
   // This file is a system header or C++ unfriendly if the old file is.
   //
   // Note that the temporary 'DirInfo' is required here, as either call to

llvm/include/llvm/Support/Error.h

 ///
 /// Uses of this method are potentially indicative of design problems: If it's
 /// legitimate to do nothing while processing an "error", the error-producer
 /// might be more clearly refactored to return an Optional<T>.
 inline void consumeError(Error Err) {
   handleAllErrors(std::move(Err), [](const ErrorInfoBase &) {});
 }
 
+/// Convert an Expected to an Optional without doing anything. This method
+/// should be used only where an error can be considered a reasonable and
+/// expected return value.
+///
+/// Uses of this method are potentially indicative of problems: perhaps the
+/// error should be propagated further, or the error-producer should just
+/// return an Optional in the first place.
+template <typename T> Optional<T> expectedToOptional(Expected<T> &&E) {
+  if (E)
+    return std::move(*E);
+  consumeError(E.takeError());
+  return None;
+}
+
 /// Helper for converting an Error to a bool.
 ///
 /// This method returns true if Err is in an error state, or false if it is
 /// in a success state.  Puts Err in a checked state in both cases (unlike
 /// Error::operator bool(), which only does this for success states).
 inline bool errorToBool(Error Err) {
   bool IsError = static_cast<bool>(Err);
   if (IsError)