[Modules] Warning for module declarations lacking 'framework' qualifier

When a module declaration for a framework lacks the 'framework'
qualifier, the listed headers aren't found (because there's no
trigger for the special framework style path lookup) and the module
is silently not built. This leads to frameworks not being modularized
by accident, which is pretty bad.

Add a warning and suggest the user to add the 'framework' qualifier
when we can prove that it's the case.

rdar://problem/39193062

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@333718 91177308-0d34-0410-b5e6-96231b3b80d8

include/clang/Basic/DiagnosticGroups.td

@@ -285,6 +285,8 @@ def IncompatiblePointerTypes
     [IncompatiblePointerTypesDiscardsQualifiers,
      IncompatibleFunctionPointerTypes]>;
 def IncompleteUmbrella : DiagGroup<"incomplete-umbrella">;
+def IncompleteFrameworkModuleDeclaration
+  : DiagGroup<"incomplete-framework-module-declaration">;
 def NonModularIncludeInFrameworkModule
   : DiagGroup<"non-modular-include-in-framework-module">;
 def NonModularIncludeInModule : DiagGroup<"non-modular-include-in-module",

include/clang/Basic/DiagnosticLexKinds.td

@@ -694,6 +694,11 @@ def warn_mmap_mismatched_private_module_name : Warning<
   InGroup<PrivateModule>;
 def note_mmap_rename_top_level_private_module : Note<
   "rename '%0' to ensure it can be found by name">;
+def warn_mmap_incomplete_framework_module_declaration : Warning<
+  "skipping '%0' because module declaration of '%1' lacks the 'framework' qualifier">,
+  InGroup<IncompleteFrameworkModuleDeclaration>;
+def note_mmap_add_framework_keyword : Note<
+  "use 'framework module' to declare module '%0'">;
 
 def err_mmap_duplicate_header_attribute : Error<
   "header attribute '%0' specified multiple times">;

include/clang/Lex/ModuleMap.h

@@ -303,8 +303,15 @@ private:
   Module *resolveModuleId(const ModuleId &Id, Module *Mod, bool Complain) const;
 
   /// Add an unresolved header to a module.
+  ///
+  /// \param Mod The module in which we're adding the unresolved header
+  ///        directive.
+  /// \param Header The unresolved header directive.
+  /// \param NeedsFramework If Mod is not a framework but a missing header would
+  ///        be found in case Mod was, set it to true. False otherwise.
   void addUnresolvedHeader(Module *Mod,
-                           Module::UnresolvedHeaderDirective Header);
+                           Module::UnresolvedHeaderDirective Header,
+                           bool &NeedsFramework);
 
   /// Look up the given header directive to find an actual header file.
   ///
@@ -312,14 +319,22 @@ private:
   /// \param Header The header directive to resolve.
   /// \param RelativePathName Filled in with the relative path name from the
   ///        module to the resolved header.
+  /// \param NeedsFramework If M is not a framework but a missing header would
+  ///        be found in case M was, set it to true. False otherwise.
   /// \return The resolved file, if any.
   const FileEntry *findHeader(Module *M,
                               const Module::UnresolvedHeaderDirective &Header,
-                              SmallVectorImpl<char> &RelativePathName);
+                              SmallVectorImpl<char> &RelativePathName,
+                              bool &NeedsFramework);
 
   /// Resolve the given header directive.
-  void resolveHeader(Module *M,
-                     const Module::UnresolvedHeaderDirective &Header);
+  ///
+  /// \param M The module in which we're resolving the header directive.
+  /// \param Header The header directive to resolve.
+  /// \param NeedsFramework If M is not a framework but a missing header would
+  ///        be found in case M was, set it to true. False otherwise.
+  void resolveHeader(Module *M, const Module::UnresolvedHeaderDirective &Header,
+                     bool &NeedsFramework);
 
   /// Attempt to resolve the specified header directive as naming a builtin
   /// header.

lib/Lex/ModuleMap.cpp

@@ -170,10 +170,13 @@ static void appendSubframeworkPaths(Module *Mod,
     llvm::sys::path::append(Path, "Frameworks", Paths[I-1] + ".framework");
 }
 
-const FileEntry *
-ModuleMap::findHeader(Module *M,
-                      const Module::UnresolvedHeaderDirective &Header,
-                      SmallVectorImpl<char> &RelativePathName) {
+const FileEntry *ModuleMap::findHeader(
+    Module *M, const Module::UnresolvedHeaderDirective &Header,
+    SmallVectorImpl<char> &RelativePathName, bool &NeedsFramework) {
+  // Search for the header file within the module's home directory.
+  auto *Directory = M->Directory;
+  SmallString<128> FullPathName(Directory->getName());
+
   auto GetFile = [&](StringRef Filename) -> const FileEntry * {
     auto *File = SourceMgr.getFileManager().getFile(Filename);
     if (!File ||
@@ -183,18 +186,8 @@ ModuleMap::findHeader(Module *M,
     return File;
   };
 
-  if (llvm::sys::path::is_absolute(Header.FileName)) {
-    RelativePathName.clear();
-    RelativePathName.append(Header.FileName.begin(), Header.FileName.end());
-    return GetFile(Header.FileName);
-  }
-
-  // Search for the header file within the module's home directory.
-  auto *Directory = M->Directory;
-  SmallString<128> FullPathName(Directory->getName());
-  unsigned FullPathLength = FullPathName.size();
-
-  if (M->isPartOfFramework()) {
+  auto GetFrameworkFile = [&]() -> const FileEntry * {
+    unsigned FullPathLength = FullPathName.size();
     appendSubframeworkPaths(M, RelativePathName);
     unsigned RelativePathLength = RelativePathName.size();
 
@@ -219,18 +212,46 @@ ModuleMap::findHeader(Module *M,
                             Header.FileName);
     llvm::sys::path::append(FullPathName, RelativePathName);
     return GetFile(FullPathName);
+  };
+
+  if (llvm::sys::path::is_absolute(Header.FileName)) {
+    RelativePathName.clear();
+    RelativePathName.append(Header.FileName.begin(), Header.FileName.end());
+    return GetFile(Header.FileName);
   }
 
+  if (M->isPartOfFramework())
+    return GetFrameworkFile();
+
   // Lookup for normal headers.
   llvm::sys::path::append(RelativePathName, Header.FileName);
   llvm::sys::path::append(FullPathName, RelativePathName);
-  return GetFile(FullPathName);
+  auto *NormalHdrFile = GetFile(FullPathName);
+
+  if (M && !NormalHdrFile && Directory->getName().endswith(".framework")) {
+    // The lack of 'framework' keyword in a module declaration it's a simple
+    // mistake we can diagnose when the header exists within the proper
+    // framework style path.
+    FullPathName.assign(Directory->getName());
+    RelativePathName.clear();
+    if (auto *FrameworkHdr = GetFrameworkFile()) {
+      Diags.Report(Header.FileNameLoc,
+                   diag::warn_mmap_incomplete_framework_module_declaration)
+          << Header.FileName << M->getFullModuleName();
+      NeedsFramework = true;
+    }
+    return nullptr;
+  }
+
+  return NormalHdrFile;
 }
 
 void ModuleMap::resolveHeader(Module *Mod,
-                              const Module::UnresolvedHeaderDirective &Header) {
+                              const Module::UnresolvedHeaderDirective &Header,
+                              bool &NeedsFramework) {
   SmallString<128> RelativePathName;
-  if (const FileEntry *File = findHeader(Mod, Header, RelativePathName)) {
+  if (const FileEntry *File =
+          findHeader(Mod, Header, RelativePathName, NeedsFramework)) {
     if (Header.IsUmbrella) {
       const DirectoryEntry *UmbrellaDir = File->getDir();
       if (Module *UmbrellaMod = UmbrellaDirs[UmbrellaDir])
@@ -1056,7 +1077,8 @@ void ModuleMap::setUmbrellaDir(Module *Mod, const DirectoryEntry *UmbrellaDir,
 }
 
 void ModuleMap::addUnresolvedHeader(Module *Mod,
-                                    Module::UnresolvedHeaderDirective Header) {
+                                    Module::UnresolvedHeaderDirective Header,
+                                    bool &NeedsFramework) {
   // If there is a builtin counterpart to this file, add it now so it can
   // wrap the system header.
   if (resolveAsBuiltinHeader(Mod, Header)) {
@@ -1087,7 +1109,7 @@ void ModuleMap::addUnresolvedHeader(Module *Mod,
 
   // We don't have stat information or can't defer looking this file up.
   // Perform the lookup now.
-  resolveHeader(Mod, Header);
+  resolveHeader(Mod, Header, NeedsFramework);
 }
 
 void ModuleMap::resolveHeaderDirectives(const FileEntry *File) const {
@@ -1107,10 +1129,11 @@ void ModuleMap::resolveHeaderDirectives(const FileEntry *File) const {
 }
 
 void ModuleMap::resolveHeaderDirectives(Module *Mod) const {
+  bool NeedsFramework = false;
   for (auto &Header : Mod->UnresolvedHeaders)
     // This operation is logically const; we're just changing how we represent
     // the header information for this file.
-    const_cast<ModuleMap*>(this)->resolveHeader(Mod, Header);
+    const_cast<ModuleMap*>(this)->resolveHeader(Mod, Header, NeedsFramework);
   Mod->UnresolvedHeaders.clear();
 }
 
@@ -1323,7 +1346,10 @@ namespace clang {
 
     /// The current module map file.
     const FileEntry *ModuleMapFile;
-    
+
+    /// Source location of most recent parsed module declaration
+    SourceLocation CurrModuleDeclLoc;
+
     /// The directory that file names in this module map file should
     /// be resolved relative to.
     const DirectoryEntry *Directory;
@@ -1743,7 +1769,7 @@ void ModuleMapParser::parseModuleDecl() {
     HadError = true;
     return;
   }
-  consumeToken(); // 'module' keyword
+  CurrModuleDeclLoc = consumeToken(); // 'module' keyword
 
   // If we have a wildcard for the module name, this is an inferred submodule.
   // Parse it. 
@@ -2267,7 +2293,13 @@ void ModuleMapParser::parseHeaderDecl(MMToken::TokenKind LeadingToken,
     }
   }
 
-  Map.addUnresolvedHeader(ActiveModule, std::move(Header));
+  bool NeedsFramework = false;
+  Map.addUnresolvedHeader(ActiveModule, std::move(Header), NeedsFramework);
+
+  if (NeedsFramework && ActiveModule)
+    Diags.Report(CurrModuleDeclLoc, diag::note_mmap_add_framework_keyword)
+      << ActiveModule->getFullModuleName()
+      << FixItHint::CreateReplacement(CurrModuleDeclLoc, "framework module");
 }
 
 static int compareModuleHeaders(const Module::Header *A,

test/Modules/Inputs/incomplete-framework-module/Foo.framework/Headers/Foo.h

@@ -0,0 +1 @@
+// Inputs/incomplete-framework-module/Foo.framework/Headers/Foo.h

test/Modules/Inputs/incomplete-framework-module/Foo.framework/Headers/FooB.h

@@ -0,0 +1 @@
+// FooB.h

test/Modules/Inputs/incomplete-framework-module/Foo.framework/Modules/module.modulemap

@@ -0,0 +1,4 @@
+module Foo {
+  umbrella header "Foo.h"
+  header "FooB.h"
+}

test/Modules/incomplete-framework-module.m

@@ -0,0 +1,11 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t \
+// RUN:   -F%S/Inputs/incomplete-framework-module \
+// RUN:   -fsyntax-only %s -verify
+
+#import <Foo/Foo.h>
+
+// expected-warning@Inputs/incomplete-framework-module/Foo.framework/Modules/module.modulemap:2{{skipping 'Foo.h' because module declaration}}
+// expected-warning@Inputs/incomplete-framework-module/Foo.framework/Modules/module.modulemap:3{{skipping 'FooB.h' because module declaration}}
+// expected-note@Inputs/incomplete-framework-module/Foo.framework/Modules/module.modulemap:1{{use 'framework module' to declare module 'Foo'}}
+// expected-note@Inputs/incomplete-framework-module/Foo.framework/Modules/module.modulemap:1{{use 'framework module' to declare module 'Foo'}}