[Modules] Improve diagnostics for incomplete umbrella

[bruno]

Summary

One of the -Wincomplete-umbrella warnings diagnoses when a header is present in the directory but it's not present in the umbrella header. Currently, this warning only happens on top level modules; any submodule using an umbrella header does not get this warning. Fix that by also considering the submodules.

[dexonsmith]

LGTM after a couple of changes inline.

lib/Lex/PPLexerChange.cpp

290–292

I think you can take Mod by reference.

Also: I'm not sure if it can be const, but please check (also, SmallVectorImpl<const Module*>) and use const if possible.

299

Seems better to take Mod by reference here. Also, const if possible.

[dexonsmith]

Oh, and it would be nice to split out Preprocessor::diagnoseMissingHeaderInUmbrellaDir in a separate NFC commit ahead of time.

include/clang/Lex/Preprocessor.h

IdentifierInfo *Ident__exception_info,
                  *Ident___exception_info,
                  *Ident_GetExceptionInfo;
   // __finally
   IdentifierInfo *Ident__abnormal_termination,
                  *Ident___abnormal_termination,
                  *Ident_AbnormalTermination;
 
   const char *getCurLexerEndPos();
+  void diagnoseMissingHeaderInUmbrellaDir(Module *Mod);
 
 public:
   void PoisonSEHIdentifiers(bool Poison = true); // Borland
 
   /// \brief Callback invoked when the lexer reads an identifier and has
   /// filled in the tokens IdentifierInfo member. 
   ///
   /// This callback potentially macro expands it or turns it into a named

lib/Lex/PPLexerChange.cpp

if (EndPos != CurLexer->BufferStart &&
         (EndPos[-1] == '\n' || EndPos[-1] == '\r') &&
         EndPos[-1] != EndPos[0])
       --EndPos;
   }
 
   return EndPos;
 }
 
+static void
+collectAllSubModulesWithUmbrellaHeader(Module *Mod,
+                                       SmallVectorImpl<Module *> &SubMods) {
+  if (Mod->getUmbrellaHeader())
+    SubMods.push_back(Mod);
+  for (auto *M : Mod->submodules())
+    collectAllSubModulesWithUmbrellaHeader(M, SubMods);
+}
+
+void Preprocessor::diagnoseMissingHeaderInUmbrellaDir(Module *Mod) {
+  assert(Mod->getUmbrellaHeader() && "Module must use umbrella header");
+  SourceLocation StartLoc =
+      SourceMgr.getLocForStartOfFile(SourceMgr.getMainFileID());
+  if (getDiagnostics().isIgnored(diag::warn_uncovered_module_header, StartLoc))
+    return;
+
+  ModuleMap &ModMap = getHeaderSearchInfo().getModuleMap();
+  const DirectoryEntry *Dir = Mod->getUmbrellaDir().Entry;
+  vfs::FileSystem &FS = *FileMgr.getVirtualFileSystem();
+  std::error_code EC;
+  for (vfs::recursive_directory_iterator Entry(FS, Dir->getName(), EC), End;
+       Entry != End && !EC; Entry.increment(EC)) {
+    using llvm::StringSwitch;
+
+    // Check whether this entry has an extension typically associated with
+    // headers.
+    if (!StringSwitch<bool>(llvm::sys::path::extension(Entry->getName()))
+             .Cases(".h", ".H", ".hh", ".hpp", true)
+             .Default(false))
+      continue;
+
+    if (const FileEntry *Header = getFileManager().getFile(Entry->getName()))
+      if (!getSourceManager().hasFileInfo(Header)) {
+        if (!ModMap.isHeaderInUnavailableModule(Header)) {
+          // Find the relative path that would access this header.
+          SmallString<128> RelativePath;
+          computeRelativePath(FileMgr, Dir, Header, RelativePath);
+          Diag(StartLoc, diag::warn_uncovered_module_header)
+              << Mod->getFullModuleName() << RelativePath;
+        }
+      }
+  }
+}
 
 /// HandleEndOfFile - This callback is invoked when the lexer hits the end of
 /// the current file.  This either returns the EOF token or pops a level off
 /// the include stack and keeps going.
 bool Preprocessor::HandleEndOfFile(Token &Result, bool isEndOfMacro) {
   assert(!CurTokenLexer &&
          "Ending a file when currently in a macro!");
 
if (TUKind == TU_Complete) {
     // used.
     for (WarnUnusedMacroLocsTy::iterator
            I=WarnUnusedMacroLocs.begin(), E=WarnUnusedMacroLocs.end();
            I!=E; ++I)
       Diag(*I, diag::pp_macro_not_used);
   }
 
   // If we are building a module that has an umbrella header, make sure that
-  // each of the headers within the directory covered by the umbrella header
-  // was actually included by the umbrella header.
+  // each of the headers within the directory, including all submodules, is
+  // covered by the umbrella header was actually included by the umbrella
+  // header.
   if (Module *Mod = getCurrentModule()) {
-    if (Mod->getUmbrellaHeader()) {
-      SourceLocation StartLoc
-        = SourceMgr.getLocForStartOfFile(SourceMgr.getMainFileID());
-
-      if (!getDiagnostics().isIgnored(diag::warn_uncovered_module_header,
-                                      StartLoc)) {
-        ModuleMap &ModMap = getHeaderSearchInfo().getModuleMap();
-        const DirectoryEntry *Dir = Mod->getUmbrellaDir().Entry;
-        vfs::FileSystem &FS = *FileMgr.getVirtualFileSystem();
-        std::error_code EC;
-        for (vfs::recursive_directory_iterator Entry(FS, Dir->getName(), EC), End;
-             Entry != End && !EC; Entry.increment(EC)) {
-          using llvm::StringSwitch;
-          
-          // Check whether this entry has an extension typically associated with
-          // headers.
-          if (!StringSwitch<bool>(llvm::sys::path::extension(Entry->getName()))
-                 .Cases(".h", ".H", ".hh", ".hpp", true)
-                 .Default(false))
-            continue;
-
-          if (const FileEntry *Header =
-                  getFileManager().getFile(Entry->getName()))
-            if (!getSourceManager().hasFileInfo(Header)) {
-              if (!ModMap.isHeaderInUnavailableModule(Header)) {
-                // Find the relative path that would access this header.
-                SmallString<128> RelativePath;
-                computeRelativePath(FileMgr, Dir, Header, RelativePath);              
-                Diag(StartLoc, diag::warn_uncovered_module_header)
-                  << Mod->getFullModuleName() << RelativePath;
-              }
-            }
-        }
-      }
-    }
+    llvm::SmallVector<Module *, 4> AllMods;
+    collectAllSubModulesWithUmbrellaHeader(Mod, AllMods);
+    for (auto *M : AllMods)
+      diagnoseMissingHeaderInUmbrellaDir(M);
   }
 
   return true;
 }
 
 /// HandleEndOfTokenLexer - This callback is invoked when the current TokenLexer
 /// hits the end of its token stream.
 bool Preprocessor::HandleEndOfTokenLexer(Token &Result) {

test/Modules/Inputs/incomplete-umbrella/Foo.framework/Headers/Bar.h

+#define BAR_PUBLIC 1

test/Modules/Inputs/incomplete-umbrella/Foo.framework/Headers/FooPublic.h

+// FooPublic.h

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

+framework module Foo {
+    umbrella header "FooPublic.h"
+    requires objc
+    export *
+}

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

+explicit module Foo.Private {
+    umbrella header "Foo.h"
+    requires objc
+    export *
+}

test/Modules/Inputs/incomplete-umbrella/Foo.framework/PrivateHeaders/Baz.h

+#define BAZ_PRIVATE 1

test/Modules/Inputs/incomplete-umbrella/Foo.framework/PrivateHeaders/Foo.h

+// Foo.h

test/Modules/incomplete-umbrella.m

+// RUN: rm -rf %t
+// RUN: not %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -F%S/Inputs/incomplete-umbrella -fsyntax-only %s 2>&1 | FileCheck %s
+
+#import <Foo/Foo.h>
+#import <Foo/Bar.h>
+#import <Foo/Baz.h>
+@import Foo.Private;
+
+// CHECK: warning: umbrella header for module 'Foo' does not include header 'Bar.h'
+// CHECK: warning: umbrella header for module 'Foo.Private' does not include header 'Baz.h'
+int foo() {
+  int a = BAR_PUBLIC;
+  int b = BAZ_PRIVATE;
+  return 0;
+}