[clang] Add FileEntryRef::getNameAsRequested()

[benlangmuir]

Summary

As progress towards having FileManager::getFileRef() return the path as-requested by default, return a FileEntryRef that can use getNameAsRequested() to retrieve this path, with the ultimate goal that this should be the behaviour of getName() and clients should explicitly request the "external" name if they need to (see comment in FileManager::getFileRef). For now, getName() continues to return the external path by looking through the redirects.

For now, the new function is only used in unit tests.

[bnbarham]

Thanks 🙇

clang/unittests/Basic/FileManagerTest.cpp

381

Bit weird that this test was checking for true 🤔. Seems like it isn't used outside of tests except for isEqual in DenseMapInfo though.

clang/include/clang/Basic/FileEntry.h

 namespace clang {
 
 class FileEntry;
 
 /// 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:
-  StringRef getName() const { return ME->first(); }
+  /// The name of this FileEntry. If a VFS uses 'use-external-name', this is
+  /// the redirected name. See getRequestedName().
+  StringRef getName() const { return getBaseMapEntry().first(); }
+
+  /// The name of this FileEntry, as originally requested without applying any
+  /// remappings for VFS 'use-external-name'.
+  ///
+  /// FIXME: this should be the semantics of getName(). See comment in
+  /// FileManager::getFileRef().
+  StringRef getNameAsRequested() const { return ME->first(); }
+
   const FileEntry &getFileEntry() const {
-    return *ME->second->V.get<FileEntry *>();
+    return *getBaseMapEntry().second->V.get<FileEntry *>();
   }
-  DirectoryEntryRef getDir() const { return *ME->second->Dir; }
+  DirectoryEntryRef getDir() const { return *getBaseMapEntry().second->Dir; }
 
   inline off_t getSize() const;
   inline unsigned getUID() const;
   inline const llvm::sys::fs::UniqueID &getUniqueID() const;
   inline time_t getModificationTime() const;
   inline bool isNamedPipe() const;
   inline void closeFile() const;
 
public:
   /// FIXME: Once FileEntryRef is "everywhere" and FileEntry::LastRef and
   /// FileEntry::getName have been deleted, delete this implicit conversion.
   operator const FileEntry *() const { return &getFileEntry(); }
 
   FileEntryRef() = delete;
   explicit FileEntryRef(const MapEntry &ME) : ME(&ME) {
     assert(ME.second && "Expected payload");
     assert(ME.second->V && "Expected non-null");
-    assert(ME.second->V.is<FileEntry *>() && "Expected FileEntry");
   }
 
   /// Expose the underlying MapEntry to simplify packing in a PointerIntPair or
   /// PointerUnion and allow construction in Optional.
   const clang::FileEntryRef::MapEntry &getMapEntry() const { return *ME; }
 
+  /// Retrieve the base MapEntry after redirects.
+  const MapEntry &getBaseMapEntry() const {
+    const MapEntry *ME = this->ME;
+    while (const void *Next = ME->second->V.dyn_cast<const void *>())
+      ME = static_cast<const MapEntry *>(Next);
+    return *ME;
+  }
+
 private:
   friend class FileMgr::MapEntryOptionalStorage<FileEntryRef>;
   struct optional_none_tag {};
 
   // Private constructor for use by OptionalStorage.
   FileEntryRef(optional_none_tag) : ME(nullptr) {}
   bool hasOptionalValue() const { return ME; }
 

clang/lib/Basic/FileManager.cpp

if (!Status.ExposesExternalVFSPath || Status.getName() == Filename) {
     // the VFS (in debug info and dependency '.d' files).
     //
     // FIXME: This is pretty complex and has some very complicated interactions
     // with the rest of clang. It's also inconsistent with how "real"
     // filesystems behave and confuses parts of clang expect to see the
     // name-as-accessed on the \a FileEntryRef.
     //
     // A potential plan to remove this is as follows -
-    //   - Expose the requested filename. One possibility would be to allow
-    //     redirection-FileEntryRefs to be returned, rather than returning
-    //     the pointed-at-FileEntryRef, and customizing `getName()` to look
-    //     through the indirection.
     //   - Update callers such as `HeaderSearch::findUsableModuleForHeader()`
-    //     to explicitly use the requested filename rather than just using
+    //     to explicitly use the `getNameAsRequested()` rather than just using
     //     `getName()`.
     //   - Add a `FileManager::getExternalPath` API for explicitly getting the
     //     remapped external filename when there is one available. Adopt it in
     //     callers like diagnostics/deps reporting instead of calling
     //     `getName()` directly.
     //   - Switch the meaning of `FileEntryRef::getName()` to get the requested
     //     name, not the external name. Once that sticks, revert callers that
     //     want the requested name back to calling `getName()`.
if (!Status.ExposesExternalVFSPath || Status.getName() == Filename) {
     assert(Redirection.second->V.is<FileEntry *>() &&
            "filename redirected to a non-canonical filename?");
     assert(Redirection.second->V.get<FileEntry *>() == UFE &&
            "filename from getStatValue() refers to wrong file");
 
     // Cache the redirection in the previously-inserted entry, still available
     // in the tentative return value.
     NamedFileEnt->second = FileEntryRef::MapValue(Redirection);
-
-    // Fix the tentative return value.
-    NamedFileEnt = &Redirection;
   }
 
   FileEntryRef ReturnedRef(*NamedFileEnt);
   if (ReusingEntry) { // Already have an entry with this inode, return it.
 
     // FIXME: This hack ensures that `getDir()` will use the path that was
     // used to lookup this file, even if we found a file by different path
     // first. This is required in order to find a module's structure when its

clang/unittests/Basic/FileEntryTest.cpp

public:
   FileEntryRef addFileAlias(StringRef Name, FileEntryRef Base) {
     return FileEntryRef(
         *Files
              .insert(
                  {Name, FileEntryRef::MapValue(
                             const_cast<FileEntry &>(Base.getFileEntry()), DR)})
              .first);
   }
+  FileEntryRef addFileRedirect(StringRef Name, FileEntryRef Base) {
+    return FileEntryRef(
+        *Files
+             .insert({Name, FileEntryRef::MapValue(
+                                const_cast<FileEntryRef::MapEntry &>(
+                                    Base.getMapEntry()))})
+             .first);
+  }
 };
 
 namespace {
 TEST(FileEntryTest, FileEntryRef) {
   FileEntryTestHelper Refs;
   FileEntryRef R1 = Refs.addFile("1");
   FileEntryRef R2 = Refs.addFile("2");
   FileEntryRef R1Also = Refs.addFileAlias("1-also", R1);
+  FileEntryRef R1Redirect = Refs.addFileRedirect("1-redirect", R1);
+  FileEntryRef R1Redirect2 = Refs.addFileRedirect("1-redirect2", R1Redirect);
 
   EXPECT_EQ("1", R1.getName());
   EXPECT_EQ("2", R2.getName());
   EXPECT_EQ("1-also", R1Also.getName());
+  EXPECT_EQ("1", R1Redirect.getName());
+  EXPECT_EQ("1", R1Redirect2.getName());
+
+  EXPECT_EQ("1", R1.getNameAsRequested());
+  EXPECT_EQ("1-redirect", R1Redirect.getNameAsRequested());
+  EXPECT_EQ("1-redirect2", R1Redirect2.getNameAsRequested());
 
   EXPECT_NE(&R1.getFileEntry(), &R2.getFileEntry());
   EXPECT_EQ(&R1.getFileEntry(), &R1Also.getFileEntry());
+  EXPECT_EQ(&R1.getFileEntry(), &R1Redirect.getFileEntry());
+  EXPECT_EQ(&R1Redirect.getFileEntry(), &R1Redirect2.getFileEntry());
 
   const FileEntry *CE1 = R1;
   EXPECT_EQ(CE1, &R1.getFileEntry());
 }
 
 TEST(FileEntryTest, OptionalFileEntryRefDegradesToFileEntryPtr) {
   FileEntryTestHelper Refs;
   OptionalFileEntryRefDegradesToFileEntryPtr M0;
TEST(FileEntryTest, OptionalFileEntryRefDegradesToFileEntryPtr) {
   EXPECT_EQ(CE1, &M1->getFileEntry());
 }
 
 TEST(FileEntryTest, equals) {
   FileEntryTestHelper Refs;
   FileEntryRef R1 = Refs.addFile("1");
   FileEntryRef R2 = Refs.addFile("2");
   FileEntryRef R1Also = Refs.addFileAlias("1-also", R1);
+  FileEntryRef R1Redirect = Refs.addFileRedirect("1-redirect", R1);
+  FileEntryRef R1Redirect2 = Refs.addFileRedirect("1-redirect2", R1Redirect);
 
   EXPECT_EQ(R1, &R1.getFileEntry());
   EXPECT_EQ(&R1.getFileEntry(), R1);
   EXPECT_EQ(R1, R1Also);
   EXPECT_NE(R1, &R2.getFileEntry());
   EXPECT_NE(&R2.getFileEntry(), R1);
   EXPECT_NE(R1, R2);
+  EXPECT_EQ(R1, R1Redirect);
+  EXPECT_EQ(R1, R1Redirect2);
 
   OptionalFileEntryRefDegradesToFileEntryPtr M1 = R1;
 
   EXPECT_EQ(M1, &R1.getFileEntry());
   EXPECT_EQ(&R1.getFileEntry(), M1);
   EXPECT_NE(M1, &R2.getFileEntry());
   EXPECT_NE(&R2.getFileEntry(), M1);
 }
 
 TEST(FileEntryTest, isSameRef) {
   FileEntryTestHelper Refs;
   FileEntryRef R1 = Refs.addFile("1");
   FileEntryRef R2 = Refs.addFile("2");
   FileEntryRef R1Also = Refs.addFileAlias("1-also", R1);
+  FileEntryRef R1Redirect = Refs.addFileRedirect("1-redirect", R1);
+  FileEntryRef R1Redirect2 = Refs.addFileRedirect("1-redirect2", R1Redirect);
 
   EXPECT_TRUE(R1.isSameRef(FileEntryRef(R1)));
   EXPECT_TRUE(R1.isSameRef(FileEntryRef(R1.getMapEntry())));
   EXPECT_FALSE(R1.isSameRef(R2));
   EXPECT_FALSE(R1.isSameRef(R1Also));
+  EXPECT_FALSE(R1.isSameRef(R1Redirect));
+  EXPECT_FALSE(R1.isSameRef(R1Redirect2));
+  EXPECT_FALSE(R1Redirect.isSameRef(R1Redirect2));
 }
 
 TEST(FileEntryTest, DenseMapInfo) {
   FileEntryTestHelper Refs;
   FileEntryRef R1 = Refs.addFile("1");
   FileEntryRef R2 = Refs.addFile("2");
   FileEntryRef R1Also = Refs.addFileAlias("1-also", R1);
 

clang/unittests/Basic/FileManagerTest.cpp

TEST_F(FileManagerTest, getFileRefEquality) {
 
   // Check names.
   EXPECT_EQ("dir/f1.cpp", F1->getName());
   EXPECT_EQ("dir/f1.cpp", F1Again->getName());
   EXPECT_EQ("dir/f1-also.cpp", F1Also->getName());
   EXPECT_EQ("dir/f1.cpp", F1Redirect->getName());
   EXPECT_EQ("dir/f2.cpp", F2->getName());
 
+  EXPECT_EQ("dir/f1.cpp", F1->getNameAsRequested());
+  EXPECT_EQ("dir/f1-redirect.cpp", F1Redirect->getNameAsRequested());
+
   // Compare against FileEntry*.
   EXPECT_EQ(&F1->getFileEntry(), *F1);
   EXPECT_EQ(*F1, &F1->getFileEntry());
+  EXPECT_EQ(&F1->getFileEntry(), &F1Redirect->getFileEntry());
   EXPECT_NE(&F2->getFileEntry(), *F1);
   EXPECT_NE(*F1, &F2->getFileEntry());
 
   // Compare using ==.
   EXPECT_EQ(*F1, *F1Also);
   EXPECT_EQ(*F1, *F1Again);
   EXPECT_EQ(*F1, *F1Redirect);
   EXPECT_EQ(*F1Also, *F1Redirect);
   EXPECT_NE(*F2, *F1);
   EXPECT_NE(*F2, *F1Also);
   EXPECT_NE(*F2, *F1Again);
   EXPECT_NE(*F2, *F1Redirect);
 
   // Compare using isSameRef.
   EXPECT_TRUE(F1->isSameRef(*F1Again));
-  EXPECT_TRUE(F1->isSameRef(*F1Redirect));
+  EXPECT_FALSE(F1->isSameRef(*F1Redirect));
   EXPECT_FALSE(F1->isSameRef(*F1Also));
   EXPECT_FALSE(F1->isSameRef(*F2));
 }
 
 // getFile() Should return the same entry as getVirtualFile if the file actually
 // is a virtual file, even if the name is not exactly the same (but is after
 // normalisation done by the file system, like on Windows). This can be checked
 // here by checking the size.