[Modules] Improve .Private fix-its to handle 'explicit' and 'framework'

When in the context of suggestion the fix-it from .Private to _Private
for private modules, trim off the 'explicit' and add 'framework' when
appropriate.

rdar://problem/41030554

llvm-svn: 334859

clang/lib/Lex/ModuleMap.cpp

@@ -1403,6 +1403,13 @@ namespace clang {
     void parseConflict();
     void parseInferredModuleDecl(bool Framework, bool Explicit);
 
+    /// Private modules are canonicalized as Foo_Private. Clang provides extra
+    /// module map search logic to find the appropriate private module when PCH
+    /// is used with implicit module maps. Warn when private modules are written
+    /// in other ways (FooPrivate and Foo.Private), providing notes and fixits.
+    void diagnosePrivateModules(SourceLocation ExplicitLoc,
+                                SourceLocation FrameworkLoc);
+
     using Attributes = ModuleMap::Attributes;
 
     bool parseOptionalAttributes(Attributes &Attrs);
@@ -1672,11 +1679,8 @@ namespace {
 /// module map search logic to find the appropriate private module when PCH
 /// is used with implicit module maps. Warn when private modules are written
 /// in other ways (FooPrivate and Foo.Private), providing notes and fixits.
-static void diagnosePrivateModules(const ModuleMap &Map,
-                                   DiagnosticsEngine &Diags,
-                                   const Module *ActiveModule,
-                                   SourceLocation InlineParent) {
-
+void ModuleMapParser::diagnosePrivateModules(SourceLocation ExplicitLoc,
+                                             SourceLocation FrameworkLoc) {
   auto GenNoteAndFixIt = [&](StringRef BadName, StringRef Canonical,
                              const Module *M, SourceRange ReplLoc) {
     auto D = Diags.Report(ActiveModule->DefinitionLoc,
@@ -1693,6 +1697,7 @@ static void diagnosePrivateModules(const ModuleMap &Map,
     SmallString<128> FullName(ActiveModule->getFullModuleName());
     if (!FullName.startswith(M->Name) && !FullName.endswith("Private"))
       continue;
+    SmallString<128> FixedPrivModDecl;
     SmallString<128> Canonical(M->Name);
     Canonical.append("_Private");
 
@@ -1702,8 +1707,20 @@ static void diagnosePrivateModules(const ModuleMap &Map,
       Diags.Report(ActiveModule->DefinitionLoc,
                    diag::warn_mmap_mismatched_private_submodule)
           << FullName;
-      GenNoteAndFixIt(FullName, Canonical, M,
-                      SourceRange(InlineParent, ActiveModule->DefinitionLoc));
+
+      SourceLocation FixItInitBegin = CurrModuleDeclLoc;
+      if (FrameworkLoc.isValid())
+        FixItInitBegin = FrameworkLoc;
+      if (ExplicitLoc.isValid())
+        FixItInitBegin = ExplicitLoc;
+
+      if (FrameworkLoc.isValid() || ActiveModule->Parent->IsFramework)
+        FixedPrivModDecl.append("framework ");
+      FixedPrivModDecl.append("module ");
+      FixedPrivModDecl.append(Canonical);
+
+      GenNoteAndFixIt(FullName, FixedPrivModDecl, M,
+                      SourceRange(FixItInitBegin, ActiveModule->DefinitionLoc));
       continue;
     }
 
@@ -1747,6 +1764,7 @@ void ModuleMapParser::parseModuleDecl() {
 
   // Parse 'explicit' or 'framework' keyword, if present.
   SourceLocation ExplicitLoc;
+  SourceLocation FrameworkLoc;
   bool Explicit = false;
   bool Framework = false;
 
@@ -1758,7 +1776,7 @@ void ModuleMapParser::parseModuleDecl() {
 
   // Parse 'framework' keyword, if present.
   if (Tok.is(MMToken::FrameworkKeyword)) {
-    consumeToken();
+    FrameworkLoc = consumeToken();
     Framework = true;
   } 
   
@@ -1800,7 +1818,6 @@ void ModuleMapParser::parseModuleDecl() {
   }
   
   Module *PreviousActiveModule = ActiveModule;  
-  SourceLocation LastInlineParentLoc = SourceLocation();
   if (Id.size() > 1) {
     // This module map defines a submodule. Go find the module of which it
     // is a submodule.
@@ -1811,7 +1828,6 @@ void ModuleMapParser::parseModuleDecl() {
         if (I == 0)
           TopLevelModule = Next;
         ActiveModule = Next;
-        LastInlineParentLoc = Id[I].second;
         continue;
       }
       
@@ -1934,7 +1950,7 @@ void ModuleMapParser::parseModuleDecl() {
       !Diags.isIgnored(diag::warn_mmap_mismatched_private_module_name,
                        StartLoc) &&
       ActiveModule->ModuleMapIsPrivate)
-    diagnosePrivateModules(Map, Diags, ActiveModule, LastInlineParentLoc);
+    diagnosePrivateModules(ExplicitLoc, FrameworkLoc);
 
   bool Done = false;
   do {

clang/test/Modules/Inputs/implicit-private-with-submodule/A.framework/Modules/module.modulemap

@@ -2,3 +2,9 @@ framework module A {
   header "a.h"
   export *
 }
+
+framework module B {
+}
+
+framework module C {
+}

clang/test/Modules/Inputs/implicit-private-with-submodule/A.framework/Modules/module.private.modulemap

@@ -2,3 +2,9 @@ framework module A.Private {
   header "aprivate.h"
   export *
 }
+
+explicit module B.Private {
+}
+
+explicit framework module C.Private {
+}

clang/test/Modules/implicit-private-with-submodule.m

@@ -13,7 +13,16 @@
 
 // expected-warning@Inputs/implicit-private-with-submodule/A.framework/Modules/module.private.modulemap:1{{private submodule 'A.Private' in private module map, expected top-level module}}
 // expected-note@Inputs/implicit-private-with-submodule/A.framework/Modules/module.private.modulemap:1{{rename 'A.Private' to ensure it can be found by name}}
-// CHECK: fix-it:"{{.*}}module.private.modulemap":{1:18-1:27}:"A_Private"
+
+// expected-warning@Inputs/implicit-private-with-submodule/A.framework/Modules/module.private.modulemap:6{{private submodule 'B.Private' in private module map, expected top-level module}}
+// expected-note@Inputs/implicit-private-with-submodule/A.framework/Modules/module.private.modulemap:6{{rename 'B.Private' to ensure it can be found by name}}
+
+// expected-warning@Inputs/implicit-private-with-submodule/A.framework/Modules/module.private.modulemap:9{{private submodule 'C.Private' in private module map, expected top-level module}}
+// expected-note@Inputs/implicit-private-with-submodule/A.framework/Modules/module.private.modulemap:9{{rename 'C.Private' to ensure it can be found by name}}
+
+// CHECK: fix-it:"{{.*}}module.private.modulemap":{1:1-1:27}:"framework module A_Private"
+// CHECK: fix-it:"{{.*}}module.private.modulemap":{6:1-6:26}:"framework module B_Private"
+// CHECK: fix-it:"{{.*}}module.private.modulemap":{9:1-9:36}:"framework module C_Private"
 
 #ifndef HEADER
 #define HEADER