[clang][modules] Skip submodule & framework re-definitions in module maps

[jansvoboda11]

Summary

Before D150478, there were situations when Clang avoided parsing a module map because it was likely to re-define an already defined module (either by a PCM or by previously-found module map). Since Clang no longer performs that check and does parse the extra module map (due to the FW/FW_Private issue described in D150478), this patch re-implements the same semantics by skipping the duplicate definition of the framework module while parsing the module map.

Depends on D150478.

[benlangmuir]

clang/test/ClangScanDeps/modules-private-framework-submodule.c

43 ↗

(On Diff #521783)

Does this lose any test coverage for the FW2_Private case?

[jansvoboda11]

clang/test/ClangScanDeps/modules-private-framework-submodule.c

43 ↗

(On Diff #521783)

This still tests the same general code path, just a more specific case of it. I can keep this test as is and create new one specifically to test the "explicit submodule" case if that makes more sense to you.

[benlangmuir]

clang/test/ClangScanDeps/modules-private-framework-submodule.c

43 ↗

(On Diff #521783)

Yeah, my preference would be to add a case instead of modifying this one, unless there is somewhere else we're already testing the same thing. The exact behaviour of FW2.Private vs FW2_Private vs FW2Private is all subtlely different

[jansvoboda11]

Skip re-definitions of frameworks too, add new tests

clang/lib/Lex/ModuleMap.cpp

if (!Tok.is(MMToken::LBrace)) {
     return;
   }
   SourceLocation LBraceLoc = consumeToken();
 
   // Determine whether this (sub)module has already been defined.
   Module *ShadowingModule = nullptr;
   if (Module *Existing = Map.lookupModuleQualified(ModuleName, ActiveModule)) {
     // We might see a (re)definition of a module that we already have a
-    // definition for in three cases:
+    // 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
     bool LoadedFromASTFile = Existing->IsFromModuleFile;
     //  - If we previously inferred this module from different module map file.
     bool Inferred = Existing->IsInferred;
+    //  - If we're building a framework that vends a module map, we might've
+    //    previously seen the one in intermediate products and now the system
+    //    one.
+    // FIXME: If we're parsing module map file that looks like this:
+    //          framework module FW { ... }
+    //          module FW.Sub { ... }
+    //        We can't check the framework qualifier, since it's not attached to
+    //        the definition of Sub. Checking that qualifier on \c Existing is
+    //        not correct either, since we might've previously seen:
+    //          module FW { ... }
+    //          module FW.Sub { ... }
+    //        We should enforce consistency of redefinitions so that we can rely
+    //        that \c Existing is part of a framework iff the redefinition of FW
+    //        we have just skipped had it too. Once we do that, stop checking
+    //        the local framework qualifier and only rely on \c Existing.
+    bool PartOfFramework = Framework || Existing->isPartOfFramework();
     //  - If we're building a (preprocessed) module and we've just loaded the
     //    module map file from which it was created.
     bool ParsedAsMainInput =
         Map.LangOpts.getCompilingModule() == LangOptions::CMK_ModuleMap &&
         Map.LangOpts.CurrentModule == ModuleName &&
         SourceMgr.getDecomposedLoc(ModuleNameLoc).first !=
             SourceMgr.getDecomposedLoc(Existing->DefinitionLoc).first;
-    if (!ActiveModule && (LoadedFromASTFile || Inferred || ParsedAsMainInput)) {
+    if (LoadedFromASTFile || Inferred || PartOfFramework || ParsedAsMainInput) {
+      ActiveModule = PreviousActiveModule;
       // Skip the module definition.
       skipUntil(MMToken::RBrace);
       if (Tok.is(MMToken::RBrace))
         consumeToken();
       else {
         Diags.Report(Tok.getLocation(), diag::err_mmap_expected_rbrace);
         Diags.Report(LBraceLoc, diag::note_mmap_lbrace_match);
         HadError = true;

clang/test/Modules/no-check-relocated-fw-private-sub.c

+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+//--- frameworks1/FW1.framework/Modules/module.modulemap
+framework module FW1 { header "FW1.h" }
+//--- frameworks1/FW1.framework/Headers/FW1.h
+#import <FW2/FW2.h>
+
+//--- frameworks2/FW2.framework/Modules/module.modulemap
+framework module FW2 { header "FW2.h" }
+//--- frameworks2/FW2.framework/Modules/module.private.modulemap
+framework module FW2.Private { header "FW2_Private.h" }
+//--- frameworks2/FW2.framework/Headers/FW2.h
+//--- frameworks2/FW2.framework/PrivateHeaders/FW2_Private.h
+
+//--- tu.c
+#import <FW1/FW1.h>         // expected-remark{{importing module 'FW1'}} \
+                            // expected-remark{{importing module 'FW2'}}
+#import <FW2/FW2_Private.h> // expected-remark{{importing module 'FW2'}}
+
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t/cache -fimplicit-module-maps \
+// RUN:   -F %t/frameworks1 -F %t/frameworks2 -fsyntax-only %t/tu.c \
+// RUN:   -fno-modules-check-relocated -Rmodule-import -verify

clang/test/Modules/shadow-framework.m

+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+// This test checks that redefinitions of frameworks are ignored.
+
+//--- include/module.modulemap
+module first { header "first.h" }
+module FW {}
+//--- include/first.h
+
+//--- frameworks/FW.framework/Modules/module.modulemap
+framework module FW { header "FW.h" }
+//--- frameworks/FW.framework/Headers/FW.h
+
+//--- tu.c
+#import "first.h" // expected-remark {{importing module 'first'}}
+#import <FW/FW.h>
+
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t/cache -fimplicit-module-maps \
+// RUN:   -I %t/include -F %t/frameworks -fsyntax-only %t/tu.c -Rmodule-import -verify