[clang] [clang][Modules] Diagnosing Module Redefinition Across ModuleMaps (PR #190085)

[Qiongsi Wu via cfe-commits]

================
@@ -1756,9 +1756,24 @@ void ModuleMapLoader::handleModuleDecl(const modulemap::ModuleDecl &MD) {
   if (Module *Existing = Map.lookupModuleQualified(ModuleName, ActiveModule)) {
     // We might see a (re)definition of a module that we already have a
     // definition for in four cases:
-    //  - If we loaded one definition from an AST file and we've just found a
-    //    corresponding definition in a module map file, or
+    //  - If we loaded one definition from an AST file and we've just found the
+    //    corresponding definition in the same module map file, or
     bool LoadedFromASTFile = Existing->IsFromModuleFile;
+    if (LoadedFromASTFile) {
----------------
qiongsiwu wrote:

> previously `LoadedFromASTFile` was short-circuiting the propagation of that check

If I understand this correctly, yes,  as soon as `LoadedFromASTFile` is true, we skip checking the source location. See [here](https://github.com/llvm/llvm-project/pull/190085/changes#diff-d94525ab4a88e7d89af25aefc58749191bd3482aecb157c8d13c7efde65e61e1R1801). 

Checking the source location itself has its own complexity. I discovered that the following statement is not always true

> If the current module decl and the existing module decl come from the same location of the same modulemap file, `SameModuleDecl` is true. 

We can have cases where the current module decl we are processing and `Existing->DefinitionLoc`'s dumps are identical (which I believe means that the module decls are from the same place of the same modulemap), but their raw encodings are different, and `SameModuleDecl` is `false`. The reason for this is that if `Existing->DefinitionLoc` comes from a deserialized AST file, its location is a loaded location, not the location in the current modulemap. I discovered this when I ran `ClangScanDeps/modules-pch.c`.  The check against module map FileRefs works, because the two FileRefs are the same when one of the module is loaded from an AST and the other is not, and we correctly set the value of `LoadedFromASTFile` to true and skips the redefinition check. 

Overall, I think the check here is very convoluted, and the conditions are subtle and brittle. If desirable, I can try combing through all the conditions and perform some global simplifications, rather than just strengthening  `LoadedFromASTFile`. Let me know if this is a direction we'd like to go with in this patch. Otherwise, I can open a separate patch to refactor the checks here. 

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