[clang][lex] NFCI: Use DirectoryEntryRef in HeaderSearch::load*()

[jansvoboda11]

Summary

This patch removes uses of the deprecated DirectoryEntry::getName() from HeaderSearch::load*() functions by using DirectoryEntryRef instead.

Note that we bail out in one case and use the also deprecated FileEntry::getLastRef(). That's to prevent this patch from growing, and is addressed in a follow-up.

[bnbarham]

clang/lib/Lex/HeaderSearch.cpp

341

I'd prefer the following since I had to specifically go and check getDirRef, though I suppose I probably would have done so even with a comment 😅 :

// Only returns None if not a normal directory, which we just checked
DirectoryEntryRef NormalDir = *Dir.getDirRef();
...loadModuleMapFile(NormalDir, ...

1595

Can we just return false in this case, or is it definitely always a logic error?

1643

Shouldn't need the * should it? Also doesn't seem necessary to actually check here since Dir hasn't been set yet. Just Dir = FileMgr.getOptionalDirectoryRef... should be fine.

1647–1649

I assume this is the place you mentioned in the message? (ie. to prevent patch from growing even more)

1653

Can't Dir still be None here? It *shouldn't* be since the directory for OriginalModuleMapFile should exist, but I'd feel more comfortable with either an early return or an assert here.

Same thing below at the switch as well.

[jansvoboda11]

clang/lib/Lex/HeaderSearch.cpp

341

Fair enough.

1595

I agree returning false here would be better, but I wanted to keep this patch as NFC as possible. How about I make that change in a follow-up patch?

1643

Nice catch!

1647–1649

Yes, that's the one.

1653

I think Dir can't be None at this point - adding an assertion here sounds like a good idea.

[jansvoboda11]

Rebase, address review feedback

[bnbarham]

clang/lib/Lex/HeaderSearch.cpp

1595

Sure. It would have been a segfault before, so I assume it must never happen anyway.

[jansvoboda11]

Replace the File->getLastRef().getDir() escape hatch with FileMgr.getOptionalDirectoryRef(File->getDir()->getName()). The former was causing failures in clang/test/VFS/real-path-found-first.m on Windows.

[jansvoboda11]

Re-committed as d9390b6ac387345e7d3348bfc648929bc349d6b1.

clang/include/clang/Lex/HeaderSearch.h

private:
   /// \param Name The name of the module to retrieve.
   ///
   /// \param Dir The framework directory (e.g., ModuleName.framework).
   ///
   /// \param IsSystem Whether the framework directory is part of the system
   /// frameworks.
   ///
   /// \returns The module, if found; otherwise, null.
-  Module *loadFrameworkModule(StringRef Name,
-                              const DirectoryEntry *Dir,
+  Module *loadFrameworkModule(StringRef Name, DirectoryEntryRef Dir,
                               bool IsSystem);
 
   /// Load all of the module maps within the immediate subdirectories
   /// of the given search directory.
   void loadSubdirectoryModuleMaps(DirectoryLookup &SearchDir);
 
   /// Find and suggest a usable module for the given file.
   ///
enum LoadModuleMapResult {
 
     /// There was either no module map file or the module map file was
     /// invalid.
     LMM_InvalidModuleMap
   };
 
   LoadModuleMapResult loadModuleMapFileImpl(const FileEntry *File,
                                             bool IsSystem,
-                                            const DirectoryEntry *Dir,
+                                            DirectoryEntryRef Dir,
                                             FileID ID = FileID(),
                                             unsigned *Offset = nullptr);
 
   /// Try to load the module map file in the given directory.
   ///
   /// \param DirName The name of the directory where we will look for a module
   /// map file.
   /// \param IsSystem Whether this is a system header directory.
   /// \param IsFramework Whether this is a framework directory.
   ///
   /// \returns The result of attempting to load the module map file from the
   /// named directory.
   LoadModuleMapResult loadModuleMapFile(StringRef DirName, bool IsSystem,
                                         bool IsFramework);
 
   /// Try to load the module map file in the given directory.
   ///
   /// \param Dir The directory where we will look for a module map file.
   /// \param IsSystem Whether this is a system header directory.
   /// \param IsFramework Whether this is a framework directory.
   ///
   /// \returns The result of attempting to load the module map file from the
   /// named directory.
-  LoadModuleMapResult loadModuleMapFile(const DirectoryEntry *Dir,
-                                        bool IsSystem, bool IsFramework);
+  LoadModuleMapResult loadModuleMapFile(DirectoryEntryRef Dir, bool IsSystem,
+                                        bool IsFramework);
 };
 
 /// Apply the header search options to get given HeaderSearch object.
 void ApplyHeaderSearchOptions(HeaderSearch &HS,
                               const HeaderSearchOptions &HSOpts,
                               const LangOptions &Lang,
                               const llvm::Triple &triple);
 
 } // namespace clang
 
 #endif // LLVM_CLANG_LEX_HEADERSEARCH_H

clang/lib/Lex/HeaderSearch.cpp

Module *HeaderSearch::lookupModule(StringRef ModuleName, StringRef SearchName,
   for (DirectoryLookup Dir : search_dir_range()) {
     if (Dir.isFramework()) {
       // Search for or infer a module map for a framework. Here we use
       // SearchName rather than ModuleName, to permit finding private modules
       // named FooPrivate in buggy frameworks named Foo.
       SmallString<128> FrameworkDirName;
       FrameworkDirName += Dir.getFrameworkDir()->getName();
       llvm::sys::path::append(FrameworkDirName, SearchName + ".framework");
-      if (auto FrameworkDir = FileMgr.getDirectory(FrameworkDirName)) {
+      if (auto FrameworkDir =
+              FileMgr.getOptionalDirectoryRef(FrameworkDirName)) {
         bool IsSystem = Dir.getDirCharacteristic() != SrcMgr::C_User;
         Module = loadFrameworkModule(ModuleName, *FrameworkDir, IsSystem);
         if (Module)
           break;
       }
     }
 
     // FIXME: Figure out how header maps and module maps will work together.
 
     // Only deal with normal search directories.
     if (!Dir.isNormalDir())
       continue;
 
     bool IsSystem = Dir.isSystemHeaderDirectory();
+    // Only returns None if not a normal directory, which we just checked
+    DirectoryEntryRef NormalDir = *Dir.getDirRef();
     // Search for a module map file in this directory.
-    if (loadModuleMapFile(Dir.getDir(), IsSystem,
+    if (loadModuleMapFile(NormalDir, IsSystem,
                           /*IsFramework*/false) == LMM_NewlyLoaded) {
       // We just loaded a module map file; check whether the module is
       // available now.
       Module = ModMap.findModule(ModuleName);
       if (Module)
         break;
     }
 

 }
 
 /// Given a framework directory, find the top-most framework directory.
 ///
 /// \param FileMgr The file manager to use for directory lookups.
 /// \param DirName The name of the framework directory.
 /// \param SubmodulePath Will be populated with the submodule path from the
 /// returned top-level module to the originally named framework.
-static const DirectoryEntry *
+static Optional<DirectoryEntryRef>
 getTopFrameworkDir(FileManager &FileMgr, StringRef DirName,
                    SmallVectorImpl<std::string> &SubmodulePath) {
   assert(llvm::sys::path::extension(DirName) == ".framework" &&
          "Not a framework directory");
 
   // Note: as an egregious but useful hack we use the real path here, because
   // frameworks moving between top-level frameworks to embedded frameworks tend
   // to be symlinked, and we base the logical structure of modules on the
   // physical layout. In particular, we need to deal with crazy includes like
   //
   //   #include <Foo/Frameworks/Bar.framework/Headers/Wibble.h>
   //
   // where 'Bar' used to be embedded in 'Foo', is now a top-level framework
   // which one should access with, e.g.,
   //
   //   #include <Bar/Wibble.h>
   //
   // Similar issues occur when a top-level framework has moved into an
   // embedded framework.
-  const DirectoryEntry *TopFrameworkDir = nullptr;
-  if (auto TopFrameworkDirOrErr = FileMgr.getDirectory(DirName))
-    TopFrameworkDir = *TopFrameworkDirOrErr;
+  auto TopFrameworkDir = FileMgr.getOptionalDirectoryRef(DirName);
 
   if (TopFrameworkDir)
-    DirName = FileMgr.getCanonicalName(TopFrameworkDir);
+    DirName = FileMgr.getCanonicalName(*TopFrameworkDir);
   do {
     // Get the parent directory name.
     DirName = llvm::sys::path::parent_path(DirName);
     if (DirName.empty())
       break;
 
     // Determine whether this directory exists.
-    auto Dir = FileMgr.getDirectory(DirName);
+    auto Dir = FileMgr.getOptionalDirectoryRef(DirName);
     if (!Dir)
       break;
 
     // If this is a framework directory, then we're a subframework of this
     // framework.
     if (llvm::sys::path::extension(DirName) == ".framework") {
       SubmodulePath.push_back(std::string(llvm::sys::path::stem(DirName)));
       TopFrameworkDir = *Dir;
bool HeaderSearch::hasModuleMap(StringRef FileName,
   StringRef DirName = FileName;
   do {
     // Get the parent directory name.
     DirName = llvm::sys::path::parent_path(DirName);
     if (DirName.empty())
       return false;
 
     // Determine whether this directory exists.
-    auto Dir = FileMgr.getDirectory(DirName);
+    auto Dir = FileMgr.getOptionalDirectoryRef(DirName);
     if (!Dir)
       return false;
 
     // Try to load the module map file in this directory.
     switch (loadModuleMapFile(*Dir, IsSystem,
-                              llvm::sys::path::extension((*Dir)->getName()) ==
+                              llvm::sys::path::extension(Dir->getName()) ==
                                   ".framework")) {
     case LMM_NewlyLoaded:
     case LMM_AlreadyLoaded:
       // Success. All of the directories we stepped through inherit this module
       // map file.
       for (unsigned I = 0, N = FixUpDirectories.size(); I != N; ++I)
         DirectoryHasModuleMap[FixUpDirectories[I]] = true;
       return true;

 
 bool HeaderSearch::findUsableModuleForFrameworkHeader(
     const FileEntry *File, StringRef FrameworkName, Module *RequestingModule,
     ModuleMap::KnownHeader *SuggestedModule, bool IsSystemFramework) {
   // If we're supposed to suggest a module, look for one now.
   if (needModuleLookup(RequestingModule, SuggestedModule)) {
     // Find the top-level framework based on this framework.
     SmallVector<std::string, 4> SubmodulePath;
-    const DirectoryEntry *TopFrameworkDir
-      = ::getTopFrameworkDir(FileMgr, FrameworkName, SubmodulePath);
+    Optional<DirectoryEntryRef> TopFrameworkDir =
+        ::getTopFrameworkDir(FileMgr, FrameworkName, SubmodulePath);
+    assert(TopFrameworkDir && "Could not find the top-most framework dir");
 
     // Determine the name of the top-level framework.
     StringRef ModuleName = llvm::sys::path::stem(TopFrameworkDir->getName());
 
     // Load this framework module. If that succeeds, find the suggested module
     // for this header, if any.
-    loadFrameworkModule(ModuleName, TopFrameworkDir, IsSystemFramework);
+    loadFrameworkModule(ModuleName, *TopFrameworkDir, IsSystemFramework);
 
     // FIXME: This can find a module not part of ModuleName, which is
     // important so that we're consistent about whether this header
     // corresponds to a module. Possibly we should lock down framework modules
     // so that this is not possible.
     return suggestModule(*this, File, RequestingModule, SuggestedModule);
   }
   return true;
static const FileEntry *getPrivateModuleMap(const FileEntry *File,
   return nullptr;
 }
 
 bool HeaderSearch::loadModuleMapFile(const FileEntry *File, bool IsSystem,
                                      FileID ID, unsigned *Offset,
                                      StringRef OriginalModuleMapFile) {
   // Find the directory for the module. For frameworks, that may require going
   // up from the 'Modules' directory.
-  const DirectoryEntry *Dir = nullptr;
+  Optional<DirectoryEntryRef> Dir;
   if (getHeaderSearchOpts().ModuleMapFileHomeIsCwd) {
-    if (auto DirOrErr = FileMgr.getDirectory("."))
-      Dir = *DirOrErr;
+    Dir = FileMgr.getOptionalDirectoryRef(".");
   } else {
     if (!OriginalModuleMapFile.empty()) {
       // We're building a preprocessed module map. Find or invent the directory
       // that it originally occupied.
-      auto DirOrErr = FileMgr.getDirectory(
+      Dir = FileMgr.getOptionalDirectoryRef(
           llvm::sys::path::parent_path(OriginalModuleMapFile));
-      if (DirOrErr) {
-        Dir = *DirOrErr;
-      } else {
-        auto *FakeFile = FileMgr.getVirtualFile(OriginalModuleMapFile, 0, 0);
-        Dir = FakeFile->getDir();
+      if (!Dir) {
+        auto FakeFile = FileMgr.getVirtualFileRef(OriginalModuleMapFile, 0, 0);
+        Dir = FakeFile.getDir();
       }
     } else {
-      Dir = File->getDir();
+      // TODO: Replace with `Dir = File.getDir()` when `File` is switched to
+      // `FileEntryRef`.
+      Dir = FileMgr.getOptionalDirectoryRef(File->getDir()->getName());
     }
 
+    assert(Dir && "parent must exist");
     StringRef DirName(Dir->getName());
     if (llvm::sys::path::filename(DirName) == "Modules") {
       DirName = llvm::sys::path::parent_path(DirName);
       if (DirName.endswith(".framework"))
-        if (auto DirOrErr = FileMgr.getDirectory(DirName))
-          Dir = *DirOrErr;
+        if (auto MaybeDir = FileMgr.getOptionalDirectoryRef(DirName))
+          Dir = *MaybeDir;
       // FIXME: This assert can fail if there's a race between the above check
       // and the removal of the directory.
       assert(Dir && "parent must exist");
     }
   }
 
-  switch (loadModuleMapFileImpl(File, IsSystem, Dir, ID, Offset)) {
+  assert(Dir && "module map home directory must exist");
+  switch (loadModuleMapFileImpl(File, IsSystem, *Dir, ID, Offset)) {
   case LMM_AlreadyLoaded:
   case LMM_NewlyLoaded:
     return false;
   case LMM_NoDirectory:
   case LMM_InvalidModuleMap:
     return true;
   }
   llvm_unreachable("Unknown load module map result");
 }
 
 HeaderSearch::LoadModuleMapResult
 HeaderSearch::loadModuleMapFileImpl(const FileEntry *File, bool IsSystem,
-                                    const DirectoryEntry *Dir, FileID ID,
+                                    DirectoryEntryRef Dir, FileID ID,
                                     unsigned *Offset) {
   assert(File && "expected FileEntry");
 
   // Check whether we've already loaded this module map, and mark it as being
   // loaded in case we recursively try to load it from itself.
   auto AddResult = LoadedModuleMaps.insert(std::make_pair(File, true));
   if (!AddResult.second)
     return AddResult.first->second ? LMM_AlreadyLoaded : LMM_InvalidModuleMap;
if (IsFramework) {
     llvm::sys::path::append(ModuleMapFileName, "Modules",
                             "module.private.modulemap");
     if (auto F = FileMgr.getFile(ModuleMapFileName))
       return *F;
   }
   return nullptr;
 }
 
-Module *HeaderSearch::loadFrameworkModule(StringRef Name,
-                                          const DirectoryEntry *Dir,
+Module *HeaderSearch::loadFrameworkModule(StringRef Name, DirectoryEntryRef Dir,
                                           bool IsSystem) {
   if (Module *Module = ModMap.findModule(Name))
     return Module;
 
   // Try to load a module map file.
   switch (loadModuleMapFile(Dir, IsSystem, /*IsFramework*/true)) {
   case LMM_InvalidModuleMap:
     // Try to infer a module map from the framework directory.
Module *HeaderSearch::loadFrameworkModule(StringRef Name, DirectoryEntryRef Dir,
   }
 
   return ModMap.findModule(Name);
 }
 
 HeaderSearch::LoadModuleMapResult
 HeaderSearch::loadModuleMapFile(StringRef DirName, bool IsSystem,
                                 bool IsFramework) {
-  if (auto Dir = FileMgr.getDirectory(DirName))
+  if (auto Dir = FileMgr.getOptionalDirectoryRef(DirName))
     return loadModuleMapFile(*Dir, IsSystem, IsFramework);
 
   return LMM_NoDirectory;
 }
 
 HeaderSearch::LoadModuleMapResult
-HeaderSearch::loadModuleMapFile(const DirectoryEntry *Dir, bool IsSystem,
+HeaderSearch::loadModuleMapFile(DirectoryEntryRef Dir, bool IsSystem,
                                 bool IsFramework) {
   auto KnownDir = DirectoryHasModuleMap.find(Dir);
   if (KnownDir != DirectoryHasModuleMap.end())
     return KnownDir->second ? LMM_AlreadyLoaded : LMM_InvalidModuleMap;
 
   if (const FileEntry *ModuleMapFile = lookupModuleMapFile(Dir, IsFramework)) {
     LoadModuleMapResult Result =
         loadModuleMapFileImpl(ModuleMapFile, IsSystem, Dir);
for (DirectoryLookup &DL : search_dir_range()) {
         // Search each of the ".framework" directories to load them as modules.
         llvm::vfs::FileSystem &FS = FileMgr.getVirtualFileSystem();
         for (llvm::vfs::directory_iterator Dir = FS.dir_begin(DirNative, EC),
                                            DirEnd;
              Dir != DirEnd && !EC; Dir.increment(EC)) {
           if (llvm::sys::path::extension(Dir->path()) != ".framework")
             continue;
 
-          auto FrameworkDir =
-              FileMgr.getDirectory(Dir->path());
+          auto FrameworkDir = FileMgr.getOptionalDirectoryRef(Dir->path());
           if (!FrameworkDir)
             continue;
 
           // Load this framework module.
           loadFrameworkModule(llvm::sys::path::stem(Dir->path()), *FrameworkDir,
                               IsSystem);
         }
         continue;
       }
 
       // FIXME: Deal with header maps.
       if (DL.isHeaderMap())
         continue;
 
       // Try to load a module map file for the search directory.
-      loadModuleMapFile(DL.getDir(), IsSystem, /*IsFramework*/ false);
+      loadModuleMapFile(*DL.getDirRef(), IsSystem, /*IsFramework*/ false);
 
       // Try to load module map files for immediate subdirectories of this
       // search directory.
       loadSubdirectoryModuleMaps(DL);
     }
   }
 
   // Populate the list of modules.
   llvm::transform(ModMap.modules(), std::back_inserter(Modules),
                   [](const auto &NameAndMod) { return NameAndMod.second; });
 }
 
 void HeaderSearch::loadTopLevelSystemModules() {
   if (!HSOpts->ImplicitModuleMaps)
     return;
 
   // Load module maps for each of the header search directories.
   for (const DirectoryLookup &DL : search_dir_range()) {
     // We only care about normal header directories.
     if (!DL.isNormalDir())
       continue;
 
     // Try to load a module map file for the search directory.
-    loadModuleMapFile(DL.getDir(), DL.isSystemHeaderDirectory(),
+    loadModuleMapFile(*DL.getDirRef(), DL.isSystemHeaderDirectory(),
                       DL.isFramework());
   }
 }
 
 void HeaderSearch::loadSubdirectoryModuleMaps(DirectoryLookup &SearchDir) {
   assert(HSOpts->ImplicitModuleMaps &&
          "Should not be loading subdirectory module maps");