[clang] Use FileError in FileManager::getFileRef, getDirectoryRef

[benlangmuir]

Most callers are unchanged, since they either ignore the specific error or have their own formatting of the error that includes both the path and the errorToErrorCode-unwrapped value. However, for clients that just forward the error it's helpful to ensure we do not lose track of the filename that the error is associated with, so use FileError.

Incidentally remove two uses of errorToErrorCode that were being used instead of consumeError; in both cases getOptionalFileRef was more appropriate.

[llvmorg-github-actions]

@llvm/pr-subscribers-clang

Author: Ben Langmuir (benlangmuir)

Changes

Most callers are unchanged, since they either ignore the specific error or have their own formatting of the error that includes both the path and the errorToErrorCode-unwrapped value. However, for clients that just forward the error it's helpful to ensure we do not lose track of the filename that the error is associated with, so use FileError.

Incidentally remove two uses of errorToErrorCode that were being used instead of consumeError; in both cases getOptionalFileRef was more appropriate.

---

Full diff: https://github.com/llvm/llvm-project/pull/199126.diff

3 Files Affected:

• (modified) clang/lib/Basic/FileManager.cpp (+13-10)
• (modified) clang/lib/Lex/HeaderSearch.cpp (+2-6)
• (modified) clang/unittests/Basic/FileManagerTest.cpp (+69)

diff --git a/clang/lib/Basic/FileManager.cpp b/clang/lib/Basic/FileManager.cpp
index a126d14087963..477f34a509daf 100644
--- a/clang/lib/Basic/FileManager.cpp
+++ b/clang/lib/Basic/FileManager.cpp
@@ -21,6 +21,7 @@
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/Statistic.h"
 #include "llvm/Config/llvm-config.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/IOSandbox.h"
 #include "llvm/Support/MemoryBuffer.h"
@@ -88,11 +89,12 @@ static llvm::Expected<DirectoryEntryRef>
 getDirectoryFromFile(FileManager &FileMgr, StringRef Filename,
                      bool CacheFailure) {
   if (Filename.empty())
-    return llvm::errorCodeToError(
-        make_error_code(std::errc::no_such_file_or_directory));
+    return llvm::createFileError(
+        Filename, make_error_code(std::errc::no_such_file_or_directory));
 
   if (llvm::sys::path::is_separator(Filename[Filename.size() - 1]))
-    return llvm::errorCodeToError(make_error_code(std::errc::is_a_directory));
+    return llvm::createFileError(Filename,
+                                 make_error_code(std::errc::is_a_directory));
 
   StringRef DirName = llvm::sys::path::parent_path(Filename);
   // Use the current directory if file has no path component.
@@ -176,7 +178,8 @@ FileManager::getDirectoryRef(StringRef DirName, bool CacheFailure) {
   if (!SeenDirInsertResult.second) {
     if (SeenDirInsertResult.first->second)
       return DirectoryEntryRef(*SeenDirInsertResult.first);
-    return llvm::errorCodeToError(SeenDirInsertResult.first->second.getError());
+    return llvm::createFileError(DirName,
+                                 SeenDirInsertResult.first->second.getError());
   }
 
   // We've not seen this before. Fill it in.
@@ -198,7 +201,7 @@ FileManager::getDirectoryRef(StringRef DirName, bool CacheFailure) {
       NamedDirEnt.second = statError;
     else
       SeenDirEntries.erase(DirName);
-    return llvm::errorCodeToError(statError);
+    return llvm::createFileError(DirName, statError);
   }
 
   // It exists.
@@ -219,8 +222,8 @@ llvm::Expected<FileEntryRef> FileManager::getFileRef(StringRef Filename,
       SeenFileEntries.insert({Filename, std::errc::no_such_file_or_directory});
   if (!SeenFileInsertResult.second) {
     if (!SeenFileInsertResult.first->second)
-      return llvm::errorCodeToError(
-          SeenFileInsertResult.first->second.getError());
+      return llvm::createFileError(
+          Filename, SeenFileInsertResult.first->second.getError());
     return FileEntryRef(*SeenFileInsertResult.first);
   }
 
@@ -246,7 +249,7 @@ llvm::Expected<FileEntryRef> FileManager::getFileRef(StringRef Filename,
     else
       SeenFileEntries.erase(Filename);
 
-    return llvm::errorCodeToError(Err);
+    return llvm::createFileError(Filename, Err);
   }
   DirectoryEntryRef DirInfo = *DirInfoOrErr;
 
@@ -265,7 +268,7 @@ llvm::Expected<FileEntryRef> FileManager::getFileRef(StringRef Filename,
     else
       SeenFileEntries.erase(Filename);
 
-    return llvm::errorCodeToError(statError);
+    return llvm::createFileError(Filename, statError);
   }
 
   assert((openFile || !F) && "undesired open file");
@@ -367,7 +370,7 @@ llvm::Expected<FileEntryRef> FileManager::getSTDIN() {
   }();
 
   if (!ContentOrError)
-    return llvm::errorCodeToError(ContentOrError.getError());
+    return llvm::createFileError("-", ContentOrError.getError());
 
   auto Content = std::move(*ContentOrError);
   STDIN = getVirtualFileRef(Content->getBufferIdentifier(),
diff --git a/clang/lib/Lex/HeaderSearch.cpp b/clang/lib/Lex/HeaderSearch.cpp
index 5cc2c04a68077..782b789f9260b 100644
--- a/clang/lib/Lex/HeaderSearch.cpp
+++ b/clang/lib/Lex/HeaderSearch.cpp
@@ -907,15 +907,13 @@ void HeaderSearch::diagnoseHeaderShadowing(
       const auto &IncluderAndDir = Includers[i];
       SmallString<1024> TmpDir = IncluderAndDir.second.getName();
       llvm::sys::path::append(TmpDir, Filename);
-      if (auto File = getFileMgr().getFileRef(TmpDir, false, false)) {
+      if (auto File = getFileMgr().getOptionalFileRef(TmpDir, false, false)) {
         if (&File->getFileEntry() == *FE)
           continue;
         Diags.Report(IncludeLoc, diag::warn_header_shadowing)
             << Filename << (*FE).getDir().getName()
             << IncluderAndDir.second.getName();
         return;
-      } else {
-        llvm::errorToErrorCode(File.takeError());
       }
     }
   }
@@ -934,14 +932,12 @@ void HeaderSearch::diagnoseHeaderShadowing(
       continue;
     SmallString<1024> TmpPath = It->getName();
     llvm::sys::path::append(TmpPath, Filename);
-    if (auto File = getFileMgr().getFileRef(TmpPath, false, false)) {
+    if (auto File = getFileMgr().getOptionalFileRef(TmpPath, false, false)) {
       if (&File->getFileEntry() == *FE)
         continue;
       Diags.Report(IncludeLoc, diag::warn_header_shadowing)
           << Filename << (*FE).getDir().getName() << It->getName();
       return;
-    } else {
-      llvm::errorToErrorCode(File.takeError());
     }
   }
 }
diff --git a/clang/unittests/Basic/FileManagerTest.cpp b/clang/unittests/Basic/FileManagerTest.cpp
index e54d3f8f9e416..c3f306d023fb0 100644
--- a/clang/unittests/Basic/FileManagerTest.cpp
+++ b/clang/unittests/Basic/FileManagerTest.cpp
@@ -228,6 +228,75 @@ TEST_F(FileManagerTest, getFileReturnsErrorForNonexistentFile) {
             std::make_error_code(std::errc::not_a_directory));
 }
 
+TEST_F(FileManagerTest, getFileRefErrorIncludesFilename) {
+  auto FS = llvm::makeIntrusiveRefCnt<llvm::vfs::InMemoryFileSystem>();
+  auto EmptyBuffer = llvm::MemoryBuffer::getMemBuffer("");
+  ASSERT_TRUE(
+      FS->addFileNoOwn("/MyDirectory/file", 0, EmptyBuffer->getMemBufferRef()));
+  FileSystemOptions Opts;
+  FileManager Mgr(Opts, FS);
+
+  // Build the expected message for a given filename and error code, since the
+  // system-provided message text (and capitalization) for std::errc values
+  // varies by platform.
+  auto ExpectedMsg = [](StringRef Name, std::errc EC) {
+    return ("'" + Name + "': " + std::make_error_code(EC).message()).str();
+  };
+
+  // Nonexistent file.
+  auto Missing = Mgr.getFileRef("/xyz.txt");
+  ASSERT_FALSE(Missing);
+  EXPECT_EQ(ExpectedMsg("/xyz.txt", std::errc::no_such_file_or_directory),
+            llvm::toString(Missing.takeError()));
+
+  // Cached failure
+  auto MissingAgain = Mgr.getFileRef("/xyz.txt");
+  ASSERT_FALSE(MissingAgain);
+  EXPECT_EQ(std::make_error_code(std::errc::no_such_file_or_directory),
+            llvm::errorToErrorCode(MissingAgain.takeError()));
+
+  // Reading a directory as a file.
+  auto DirAsFile = Mgr.getFileRef("/MyDirectory");
+  ASSERT_FALSE(DirAsFile);
+  EXPECT_EQ(ExpectedMsg("/MyDirectory", std::errc::is_a_directory),
+            llvm::toString(DirAsFile.takeError()));
+
+  auto Trailing = Mgr.getFileRef("/some/dir/");
+  ASSERT_FALSE(Trailing);
+  EXPECT_EQ(ExpectedMsg("/some/dir/", std::errc::is_a_directory),
+            llvm::toString(Trailing.takeError()));
+}
+
+TEST_F(FileManagerTest, getDirectoryRefErrorIncludesFilename) {
+  auto FS = llvm::makeIntrusiveRefCnt<llvm::vfs::InMemoryFileSystem>();
+  auto EmptyBuffer = llvm::MemoryBuffer::getMemBuffer("");
+  ASSERT_TRUE(FS->addFileNoOwn("/foo.cpp", 0, EmptyBuffer->getMemBufferRef()));
+  FileSystemOptions Opts;
+  FileManager Mgr(Opts, FS);
+
+  auto ExpectedMsg = [](StringRef Name, std::errc EC) {
+    return ("'" + Name + "': " + std::make_error_code(EC).message()).str();
+  };
+
+  // Nonexistent directory.
+  auto Missing = Mgr.getDirectoryRef("/no_such_dir");
+  ASSERT_FALSE(Missing);
+  EXPECT_EQ(ExpectedMsg("/no_such_dir", std::errc::no_such_file_or_directory),
+            llvm::toString(Missing.takeError()));
+
+  // Cached failure
+  auto MissingAgain = Mgr.getDirectoryRef("/no_such_dir");
+  ASSERT_FALSE(MissingAgain);
+  EXPECT_EQ(std::make_error_code(std::errc::no_such_file_or_directory),
+            llvm::errorToErrorCode(MissingAgain.takeError()));
+
+  // Reading a file as a directory.
+  auto FileAsDir = Mgr.getDirectoryRef("/foo.cpp");
+  ASSERT_FALSE(FileAsDir);
+  EXPECT_EQ(ExpectedMsg("/foo.cpp", std::errc::not_a_directory),
+            llvm::toString(FileAsDir.takeError()));
+}
+
 // The following tests apply to Unix-like system only.
 
 #ifndef _WIN32