[clang] Make FileEntryRef::getDir() return the as-requested DirectoryEntryRef

For redirected file entries, `FileEntryRef::getDir()` returns the parent directory entry of the target file entry. This differs from `FileEntry::getDir()` that always returns the parent directory that was last used to look up that file.

After switching from `FileEntry` to `FileEntryRef` for umbrella headers in D142113, this discrepancy became observable and caused Clang to emit incorrect diagnostics.

This patch changes Clang so that it always associates `FileEntryRef` with the parent directory that was used to look it up. This brings its behavior closer to `FileEntry`, but without the hacky mutation.

This also ensures that `llvm::sys::path::parent_path(FileRef->getNameAsRequested()) == FileRef->getDir()->getName()`. Previously, `FileRef->getDir()` would fall underneath the redirecting VFS into the world of on-disk paths.

Reviewed By: benlangmuir, rmaz

Differential Revision: https://reviews.llvm.org/D151398

Author: jansvoboda11

Diff for: clang/include/clang/Basic/FileEntry.h

@@ -70,7 +70,7 @@ class FileEntryRef {
   const FileEntry &getFileEntry() const {
     return *getBaseMapEntry().second->V.get<FileEntry *>();
   }
-  DirectoryEntryRef getDir() const { return *getBaseMapEntry().second->Dir; }
+  DirectoryEntryRef getDir() const { return ME->second->Dir; }
 
   inline off_t getSize() const;
   inline unsigned getUID() const;
@@ -120,12 +120,12 @@ class FileEntryRef {
     /// name that was used to lookup the file.
     llvm::PointerUnion<FileEntry *, const MapEntry *> V;
 
-    /// Directory the file was found in. Set if and only if V is a FileEntry.
-    OptionalDirectoryEntryRef Dir;
+    /// Directory the file was found in.
+    DirectoryEntryRef Dir;
 
     MapValue() = delete;
     MapValue(FileEntry &FE, DirectoryEntryRef Dir) : V(&FE), Dir(Dir) {}
-    MapValue(MapEntry &ME) : V(&ME) {}
+    MapValue(MapEntry &ME, DirectoryEntryRef Dir) : V(&ME), Dir(Dir) {}
   };
 
   /// Check if RHS referenced the file in exactly the same way.

Diff for: clang/lib/Basic/FileManager.cpp

@@ -319,7 +319,7 @@ FileManager::getFileRef(StringRef Filename, bool openFile, bool CacheFailure) {
 
     // Cache the redirection in the previously-inserted entry, still available
     // in the tentative return value.
-    NamedFileEnt->second = FileEntryRef::MapValue(Redirection);
+    NamedFileEnt->second = FileEntryRef::MapValue(Redirection, DirInfo);
   }
 
   FileEntryRef ReturnedRef(*NamedFileEnt);

Diff for: clang/test/Modules/vfs-umbrella-same-dir.m

@@ -0,0 +1,53 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+//--- sources/FW/Private.h
+#include <FW/PrivateUnmapped.h>
+//--- sources/FW/PrivateUnmapped.h
+#include <FW/Public.h>
+//--- sources/FW/Public.h
+#include <FW/PublicPresent.h>
+//--- frameworks/FW.framework/Headers/PublicPresent.h
+// empty
+//--- frameworks/FW.framework/Modules/module.modulemap
+framework module FW { umbrella header "Public.h" }
+//--- frameworks/FW.framework/Modules/module.private.modulemap
+framework module FW_Private { umbrella header "Private.h" }
+//--- vfs.json.in
+{
+  "case-sensitive": "false",
+  "version": 0,
+  "roots": [
+    {
+      "contents": [
+        {
+          "external-contents": "DIR/sources/FW/Public.h",
+          "name": "Public.h",
+          "type": "file"
+        }
+      ],
+      "name": "DIR/frameworks/FW.framework/Headers",
+      "type": "directory"
+    },
+    {
+      "contents": [
+        {
+          "external-contents": "DIR/sources/FW/Private.h",
+          "name": "Private.h",
+          "type": "file"
+        }
+      ],
+      "name": "DIR/frameworks/FW.framework/PrivateHeaders",
+      "type": "directory"
+    }
+  ]
+}
+
+//--- tu.m
+#import <FW/Private.h>
+// expected-no-diagnostics
+
+// RUN: sed -e "s|DIR|%/t|g" %t/vfs.json.in > %t/vfs.json
+
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache \
+// RUN:            -ivfsoverlay %t/vfs.json -I %t/sources -F %t/frameworks -fsyntax-only %t/tu.m -verify

Diff for: clang/unittests/Basic/FileEntryTest.cpp

@@ -9,6 +9,7 @@
 #include "clang/Basic/FileEntry.h"
 #include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/StringMap.h"
+#include "llvm/Support/Path.h"
 #include "gtest/gtest.h"
 
 using namespace llvm;
@@ -51,11 +52,14 @@ class FileEntryTestHelper {
              .first);
   }
   FileEntryRef addFileRedirect(StringRef Name, FileEntryRef Base) {
+    auto Dir = addDirectory(llvm::sys::path::parent_path(Name));
+
     return FileEntryRef(
         *Files
              .insert({Name, FileEntryRef::MapValue(
                                 const_cast<FileEntryRef::MapEntry &>(
-                                    Base.getMapEntry()))})
+                                    Base.getMapEntry()),
+                                Dir)})
              .first);
   }
 };

Diff for: clang/unittests/Basic/FileManagerTest.cpp

@@ -310,6 +310,26 @@ TEST_F(FileManagerTest, getFileRefReturnsCorrectNameForDifferentStatPath) {
   EXPECT_EQ(&F2->getFileEntry(), &F2Alias2->getFileEntry());
 }
 
+TEST_F(FileManagerTest, getFileRefReturnsCorrectDirNameForDifferentStatPath) {
+  // Inject files with the same inode into distinct directories (name & inode).
+  auto StatCache = std::make_unique<FakeStatCache>();
+  StatCache->InjectDirectory("dir1", 40);
+  StatCache->InjectDirectory("dir2", 41);
+  StatCache->InjectFile("dir1/f.cpp", 42);
+  StatCache->InjectFile("dir2/f.cpp", 42, "dir1/f.cpp");
+
+  manager.setStatCache(std::move(StatCache));
+  auto Dir1F = manager.getFileRef("dir1/f.cpp");
+  auto Dir2F = manager.getFileRef("dir2/f.cpp");
+
+  ASSERT_FALSE(!Dir1F);
+  ASSERT_FALSE(!Dir2F);
+  EXPECT_EQ("dir1", Dir1F->getDir().getName());
+  EXPECT_EQ("dir2", Dir2F->getDir().getName());
+  EXPECT_EQ("dir1/f.cpp", Dir1F->getNameAsRequested());
+  EXPECT_EQ("dir2/f.cpp", Dir2F->getNameAsRequested());
+}
+
 // getFile() returns the same FileEntry for virtual files that have
 // corresponding real files that are aliases.
 TEST_F(FileManagerTest, getFileReturnsSameFileEntryForAliasedVirtualFiles) {