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

[jansvoboda11]

Summary

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.

[rmaz]

LGTM

clang/include/clang/Basic/FileEntry.h

123–124

If this is always set now, should it be a non optional DirectoryEntryRef?

[jansvoboda11]

Make directory non-optional

clang/include/clang/Basic/FileEntry.h

123–124

Yes, thank you.

clang/include/clang/Basic/FileEntry.h

 //===- clang/Basic/FileEntry.h - File references ----------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 ///
public:
   ///
   /// FIXME: this should be the semantics of getName(). See comment in
   /// FileManager::getFileRef().
   StringRef getNameAsRequested() const { return ME->first(); }
 
   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;
   inline const llvm::sys::fs::UniqueID &getUniqueID() const;
   inline time_t getModificationTime() const;
   inline bool isNamedPipe() const;
   inline void closeFile() const;
 
public:
   struct MapValue {
     /// The pointer at another MapEntry is used when the FileManager should
     /// silently forward from one name to another, which occurs in Redirecting
     /// VFSs that use external names. In that case, the \c FileEntryRef
     /// returned by the \c FileManager will have the external name, and not the
     /// 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.
   bool isSameRef(const FileEntryRef &RHS) const { return ME == RHS.ME; }
 
   /// Allow FileEntryRef to degrade into 'const FileEntry*' to facilitate
   /// incremental adoption.
   ///

clang/lib/Basic/FileManager.cpp

 //===--- FileManager.cpp - File System Probing and Caching ----------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
auto &Redirection =
              .first;
     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);
+    NamedFileEnt->second = FileEntryRef::MapValue(Redirection, DirInfo);
   }
 
   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/test/Modules/vfs-umbrella-same-dir.m

+// 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

clang/unittests/Basic/FileEntryTest.cpp

 //===- unittests/Basic/FileEntryTest.cpp - Test FileEntry/FileEntryRef ----===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #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;
 
 namespace clang {
 
 class FileEntryTestHelper {
   StringMap<llvm::ErrorOr<FileEntryRef::MapValue>> Files;
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) {
+    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);
   }
 };
 
 namespace {
 TEST(FileEntryTest, FileEntryRef) {
   FileEntryTestHelper Refs;
   FileEntryRef R1 = Refs.addFile("1");

clang/unittests/Basic/FileManagerTest.cpp

 //===- unittests/Basic/FileMangerTest.cpp ------------ FileManger tests ---===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
#endif
   EXPECT_EQ("dir/f2.cpp", F2->getName());
   EXPECT_EQ("dir/f2.cpp", F2->getFileEntry().getName());
   EXPECT_EQ("dir/f2.cpp", F2Alias->getName());
   EXPECT_EQ("dir/f2.cpp", F2Alias2->getName());
   EXPECT_EQ(&F2->getFileEntry(), &F2Alias->getFileEntry());
   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) {
   // Inject two real files with the same inode.
   auto statCache = std::make_unique<FakeStatCache>();
   statCache->InjectDirectory("abc", 41);
   statCache->InjectFile("abc/foo.cpp", 42);
   statCache->InjectFile("abc/bar.cpp", 42);