[clang][module] Improve -Wincomplete-umbrella

[zixuw]

Summary

Change the warning message for -Wincomplete-umbrella to report the
location of the umbrella header;
Add a fix-it hint to include uncovered headers in the umbrella header.

rdar://57095551

[zixuw]

Abandoning this revision because the upstream for this work is not correctly tracking the llvm.org branch.
The method setGenModuleActionWrapper is available in apple/master but not in llvm.

clang/include/clang/Rewrite/Frontend/FrontendActions.h

 //===-- FrontendActions.h - Useful Frontend Actions -------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_REWRITE_FRONTEND_FRONTENDACTIONS_H
 #define LLVM_CLANG_REWRITE_FRONTEND_FRONTENDACTIONS_H
 
 #include "clang/Frontend/FrontendAction.h"
 #include "llvm/Support/raw_ostream.h"
+#include <memory>
 
 namespace clang {
 class FixItRewriter;
 class FixItOptions;
 
 //===----------------------------------------------------------------------===//
 // AST Consumer Actions
 //===----------------------------------------------------------------------===//
 
 class HTMLPrintAction : public ASTFrontendAction {
 protected:
   std::unique_ptr<ASTConsumer> CreateASTConsumer(CompilerInstance &CI,
                                                  StringRef InFile) override;
 };
 
+class WrappingFixItAction;
+
 class FixItAction : public ASTFrontendAction {
 protected:
   std::unique_ptr<FixItRewriter> Rewriter;
   std::unique_ptr<FixItOptions> FixItOpts;
 
   std::unique_ptr<ASTConsumer> CreateASTConsumer(CompilerInstance &CI,
                                                  StringRef InFile) override;
 
   bool BeginSourceFileAction(CompilerInstance &CI) override;
 
   void EndSourceFileAction() override;
 
   bool hasASTFileSupport() const override { return false; }
 
 public:
   FixItAction();
   ~FixItAction() override;
+  friend WrappingFixItAction;
+};
+
+class WrappingFixItAction : public WrapperFrontendAction {
+  std::unique_ptr<FixItAction> fixItAction;
+
+public:
+  WrappingFixItAction(std::unique_ptr<FrontendAction> WrappedAction)
+      : WrapperFrontendAction(std::move(WrappedAction)),
+        fixItAction(std::make_unique<FixItAction>()) {}
+
+protected:
+  bool BeginSourceFileAction(CompilerInstance &CI) override {
+    // FIXME: Do we abort if the wrapping action returned failure?
+    bool Success = fixItAction->BeginSourceFileAction(CI);
+    Success &= WrapperFrontendAction::BeginSourceFileAction(CI);
+    return Success;
+  }
+
+  void EndSourceFileAction() override {
+    WrapperFrontendAction::EndSourceFileAction();
+    fixItAction->EndSourceFileAction();
+  }
 };
 
 /// Emits changes to temporary files and uses them for the original
 /// frontend action.
 class FixItRecompile : public WrapperFrontendAction {
 public:
   FixItRecompile(std::unique_ptr<FrontendAction> WrappedAction)
     : WrapperFrontendAction(std::move(WrappedAction)) {}

clang/lib/Frontend/Rewrite/FrontendActions.cpp

llvm::sys::fs::createTemporaryFile(llvm::sys::path::filename(Filename),
                                        llvm::sys::path::extension(Filename).drop_front(), fd,
                                        Path);
     return std::string(Path.str());
   }
 };
 } // end anonymous namespace
 
 bool FixItAction::BeginSourceFileAction(CompilerInstance &CI) {
-  const FrontendOptions &FEOpts = getCompilerInstance().getFrontendOpts();
+  const FrontendOptions &FEOpts = CI.getFrontendOpts();
   if (!FEOpts.FixItSuffix.empty()) {
     FixItOpts.reset(new FixItActionSuffixInserter(FEOpts.FixItSuffix,
                                                   FEOpts.FixWhatYouCan));
   } else {
     FixItOpts.reset(new FixItRewriteInPlace);
     FixItOpts->FixWhatYouCan = FEOpts.FixWhatYouCan;
   }
   Rewriter.reset(new FixItRewriter(CI.getDiagnostics(), CI.getSourceManager(),

clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp

 #include "clang/Rewrite/Frontend/FrontendActions.h"
 #include "clang/StaticAnalyzer/Frontend/AnalyzerHelpFlags.h"
 #include "clang/StaticAnalyzer/Frontend/FrontendActions.h"
 #include "llvm/Option/OptTable.h"
 #include "llvm/Option/Option.h"
 #include "llvm/Support/BuryPointer.h"
 #include "llvm/Support/DynamicLibrary.h"
 #include "llvm/Support/ErrorHandling.h"
+#include <memory>
 using namespace clang;
 using namespace llvm::opt;
 
 namespace clang {
 
 static std::unique_ptr<FrontendAction>
 CreateFrontendBaseAction(CompilerInstance &CI) {
   using namespace clang::frontend;
#if CLANG_ENABLE_ARCMT
   }
 #endif
 
   if (!FEOpts.IndexStorePath.empty()) {
     Act = index::createIndexDataRecordingAction(FEOpts, std::move(Act));
     CI.setGenModuleActionWrapper(&index::createIndexDataRecordingAction);
   }
 
+  // Forward the FixItAction as a wrapper when building a module, so that
+  // fix-it hints can be properly applied using the new SourceManager in
+  // the new CompilerInstance.
+  // FIXME: Is this the right way to do it?
+  if (FEOpts.ProgramAction == frontend::FixIt) {
+    CI.setGenModuleActionWrapper(
+        [](const FrontendOptions &FEOpts, std::unique_ptr<FrontendAction> Act)
+            -> std::unique_ptr<FrontendAction> {
+          return std::make_unique<WrappingFixItAction>(std::move(Act));
+        });
+  }
+
   // If there are any AST files to merge, create a frontend action
   // adaptor to perform the merge.
   if (!FEOpts.ASTMergeFiles.empty())
     Act = std::make_unique<ASTMergeAction>(std::move(Act),
                                             FEOpts.ASTMergeFiles);
 
   return Act;
 }

clang/lib/Lex/PPLexerChange.cpp

static void collectAllSubModulesWithUmbrellaHeader(
     const Module &Mod, SmallVectorImpl<const Module *> &SubMods) {
   if (Mod.getUmbrellaHeader())
     SubMods.push_back(&Mod);
   for (auto *M : Mod.submodules())
     collectAllSubModulesWithUmbrellaHeader(*M, SubMods);
 }
 
 void Preprocessor::diagnoseMissingHeaderInUmbrellaDir(const 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))
+  const auto &UmbrellaHeader = Mod.getUmbrellaHeader();
+  assert(UmbrellaHeader.Entry && "Module must use umbrella header");
+  const FileID &File = SourceMgr.translateFile(UmbrellaHeader.Entry);
+  SourceLocation EndLoc = SourceMgr.getLocForEndOfFile(File);
+  if (getDiagnostics().isIgnored(diag::warn_uncovered_module_header, EndLoc))
     return;
 
   ModuleMap &ModMap = getHeaderSearchInfo().getModuleMap();
   const DirectoryEntry *Dir = Mod.getUmbrellaDir().Entry;
   llvm::vfs::FileSystem &FS = FileMgr.getVirtualFileSystem();
   std::error_code EC;
   for (llvm::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->path()))
              .Cases(".h", ".H", ".hh", ".hpp", true)
              .Default(false))
       continue;
 
     if (auto Header = getFileManager().getFile(Entry->path()))
       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;
+          // FIXME: match CRLF style newlines?
+          std::string IncludeStmt =
+              LangOpts.ObjC
+                  ? Twine{"#import \"" + RelativePath + "\"\n"}.str()
+                  : Twine{"#include \"" + RelativePath + "\"\n"}.str();
+          unsigned EndOffset = SourceMgr.getDecomposedLoc(EndLoc).second;
+          // If the umbrella header does not end with a newline,
+          // insert a newline before inserting FixIt code.
+          // FIXME: check CRLF style newlines
+          if (EndOffset > 0) {
+            SourceLocation LastCharLoc =
+                SourceMgr.getComposedLoc(File, EndOffset - 1);
+            if (*SourceMgr.getCharacterData(LastCharLoc) != '\n')
+              IncludeStmt = '\n' + IncludeStmt;
+          }
+          // FIXME: insert the missing include to a better location than EOF.
+          // For example right after an include block, or in lexical order.
+          Diag(EndLoc, diag::warn_uncovered_module_header)
+              << Mod.getFullModuleName() << RelativePath
+              << FixItHint::CreateInsertion(EndLoc, IncludeStmt);
         }
       }
   }
 }
 
 /// 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.

clang/test/Modules/Inputs/incomplete-umbrella/normal-module/A1.h

+// A1.h

clang/test/Modules/Inputs/incomplete-umbrella/normal-module/A2.h

+// A2.h

clang/test/Modules/Inputs/incomplete-umbrella/normal-module/Umbrella.h

+// Umbrella.h
+// CHECK: #import "A1.h"
+// CHECK: #import "A2.h"

clang/test/Modules/Inputs/incomplete-umbrella/normal-module/module.modulemap

+module A {
+  umbrella header "Umbrella.h"
+  export *
+}

clang/test/Modules/incomplete-umbrella-fixit.m

+// RUN: rm -rf %t
+// RUN: cp -R %S/Inputs/incomplete-umbrella/normal-module %t
+// RUN: %clang_cc1 -fmodules -fmodule-map-file=%t/module.modulemap -fmodules-cache-path=%t -Wno-everything -Wincomplete-umbrella -fixit %s && FileCheck %t/Umbrella.h --input-file %t/Umbrella.h --match-full-lines
+// RUN: %clang_cc1 -fmodules -fmodule-map-file=%t/module.modulemap -fmodules-cache-path=%t -Wno-everything -Wincomplete-umbrella -Werror %s
+
+@import A;
+
+int foo() {
+  return 0;
+}

clang/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'
+// CHECK: While building module 'Foo' imported from {{.*[/\]}}incomplete-umbrella.m:4:
+// CHECK-NEXT: In file included from <module-includes>:1:
+// CHECK-NEXT: {{.*Foo[.]framework[/\]Headers[/\]}}FooPublic.h:2:1: warning: umbrella header for module 'Foo' does not include header 'Bar.h'
+// CHECK: While building module 'Foo' imported from {{.*[/\]}}incomplete-umbrella.m:4:
+// CHECK-NEXT: In file included from <module-includes>:2:
+// CHECK-NEXT: {{.*Foo[.]framework[/\]PrivateHeaders[/\]}}Foo.h:2:1: 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;
 }