[clang] [clang-tools-extra] [clang][modules] Stop uniquing implicit modules via `FileEntry` (PR #185765)

[Jan Svoboda via cfe-commits]

================
@@ -123,91 +143,88 @@ ModuleManager::addModule(StringRef FileName, ModuleKind Type,
     // contents, but we can't check that.)
     ExpectedModTime = 0;
   }
-  // Note: ExpectedSize and ExpectedModTime will be 0 for MK_ImplicitModule
-  // when using an ASTFileSignature.
-  if (lookupModuleFile(FileName, ExpectedSize, ExpectedModTime, Entry)) {
-    ErrorStr = IgnoreModTime ? "module file has a different size than expected"
-                             : "module file has a different size or "
-                               "modification time than expected";
-    return OutOfDate;
-  }
 
-  if (!Entry) {
+  std::optional<ModuleFileKey> FileKey = FileName.makeKey(FileMgr);
+  if (!FileKey) {
     ErrorStr = "module file not found";
     return Missing;
   }
 
-  // The ModuleManager's use of FileEntry nodes as the keys for its map of
-  // loaded modules is less than ideal. Uniqueness for FileEntry nodes is
-  // maintained by FileManager, which in turn uses inode numbers on hosts
-  // that support that. When coupled with the module cache's proclivity for
-  // turning over and deleting stale PCMs, this means entries for different
-  // module files can wind up reusing the same underlying inode. When this
-  // happens, subsequent accesses to the Modules map will disagree on the
-  // ModuleFile associated with a given file. In general, it is not sufficient
-  // to resolve this conundrum with a type like FileEntryRef that stores the
-  // name of the FileEntry node on first access because of path canonicalization
-  // issues. However, the paths constructed for implicit module builds are
-  // fully under Clang's control. We *can*, therefore, rely on their structure
-  // being consistent across operating systems and across subsequent accesses
-  // to the Modules map.
-  auto implicitModuleNamesMatch = [](ModuleKind Kind, const ModuleFile *MF,
-                                     FileEntryRef Entry) -> bool {
-    if (Kind != MK_ImplicitModule)
-      return true;
-    return Entry.getName() == MF->FileName;
-  };
-
-  // Check whether we already loaded this module, before
-  if (ModuleFile *ModuleEntry = Modules.lookup(*Entry)) {
-    if (implicitModuleNamesMatch(Type, ModuleEntry, *Entry)) {
-      // Check the stored signature.
-      if (checkSignature(ModuleEntry->Signature, ExpectedSignature, ErrorStr))
-        return OutOfDate;
-
-      Module = ModuleEntry;
-      updateModuleImports(*ModuleEntry, ImportedBy, ImportLoc);
-      return AlreadyLoaded;
+  ModuleFile *ModuleEntry = lookup(*FileKey);
+  if (ModuleEntry)
+    ModuleEntry->Keys.insert(*FileKey);
+
+  // TODO: Remove this.
----------------
jansvoboda11 wrote:

At the time I thought there are multiple places that may look for the same file with inconsistent explicit/implicit path, so this seemed necessary to temporarily support both. But now that I look back, I might've been observing the fact that some tests used `%t` for both an include path and the module cache, so the `DirectoryEntry` got cached as non-existing. This made `ModuleManager` lookups fail with the implicit path, and turning it explicit and just looking up the full path in `FileManager` would succeed. I've removed this in [86ee991](https://github.com/llvm/llvm-project/pull/185765/commits/86ee9919673be5a650f94c13a3f33882b6899b35) and also removed the storage for multiple keys on `ModuleFile` in [a109526](https://github.com/llvm/llvm-project/pull/185765/commits/a109526c24ebff990aaeb9153a74da5a7ab0fc4d), since that was there only to support this hack.

LMK if you're happy with how it looks now.

https://github.com/llvm/llvm-project/pull/185765