[include-fixer] use clang-format cleaner to insert header.

[ioeric]

Summary

clang-format's cleanupAroundReplacements() takes care of header insertions.

[ioeric]

• Nits fixed.

[bkramer]

Look at all this annoying preprocessor code going away! I love it.

[hokein]

include-fixer/IncludeFixer.cpp

29

The forward declaration can be removed too.

include-fixer/IncludeFixer.h

private:
   /// Whether inserted include paths should be optimized.
   bool MinimizeIncludePaths;
 
   /// The fallback format style for formatting after insertion if no
   /// clang-format config file was found.
   std::string FallbackStyle;
 };
 
-/// Create replacements for the header being inserted. The replacements will
-/// insert a header before the first #include in \p Code, and sort all headers
-/// with the given clang-format style.
+/// Create replacements, which are generated by clang-format, for the header
+/// insertion.
 ///
 /// \param Code The source code.
 /// \param FilePath The source file path.
 /// \param Header The header being inserted.
-/// \param FirstIncludeOffset The insertion point for new include directives.
-/// The default value -1U means inserting the header at the first line, and if
-/// there is no #include block, it will create a header block by inserting a
-/// newline.
 /// \param Style clang-format style being used.
 ///
 /// \return Replacements for inserting and sorting headers.
 tooling::Replacements createInsertHeaderReplacements(
     StringRef Code, StringRef FilePath, StringRef Header,
-    unsigned FirstIncludeOffset = -1U,
     const clang::format::FormatStyle &Style = clang::format::getLLVMStyle());
 
 } // namespace include_fixer
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_INCLUDE_FIXER_INCLUDEFIXER_H

include-fixer/IncludeFixer.cpp

 #define DEBUG_TYPE "include-fixer"
 
 using namespace clang;
 
 namespace clang {
 namespace include_fixer {
 namespace {
 
 class Action;
 
-class PreprocessorHooks : public clang::PPCallbacks {
-public:
-  explicit PreprocessorHooks(Action *EnclosingPass)
-      : EnclosingPass(EnclosingPass), TrackedFile(nullptr) {}
-
-  void FileChanged(clang::SourceLocation loc,
-                   clang::PPCallbacks::FileChangeReason reason,
-                   clang::SrcMgr::CharacteristicKind file_type,
-                   clang::FileID prev_fid) override;
-
-  void InclusionDirective(clang::SourceLocation HashLocation,
-                          const clang::Token &IncludeToken,
-                          llvm::StringRef FileName, bool IsAngled,
-                          clang::CharSourceRange FileNameRange,
-                          const clang::FileEntry *IncludeFile,
-                          llvm::StringRef SearchPath,
-                          llvm::StringRef relative_path,
-                          const clang::Module *imported) override;
-
-private:
-  /// The current Action.
-  Action *EnclosingPass;
-
-  /// The current FileEntry.
-  const clang::FileEntry *TrackedFile;
-};
-
 /// Manages the parse, gathers include suggestions.
 class Action : public clang::ASTFrontendAction,
                public clang::ExternalSemaSource {
 public:
   explicit Action(SymbolIndexManager &SymbolIndexMgr, bool MinimizeIncludePaths)
       : SymbolIndexMgr(SymbolIndexMgr),
         MinimizeIncludePaths(MinimizeIncludePaths) {}
 
   std::unique_ptr<clang::ASTConsumer>
   CreateASTConsumer(clang::CompilerInstance &Compiler,
                     StringRef InFile) override {
     Filename = InFile;
-    Compiler.getPreprocessor().addPPCallbacks(
-        llvm::make_unique<PreprocessorHooks>(this));
     return llvm::make_unique<clang::ASTConsumer>();
   }
 
   void ExecuteAction() override {
     clang::CompilerInstance *Compiler = &getCompilerInstance();
     assert(!Compiler->hasSema() && "CI already has Sema");
 
     // Set up our hooks into sema and parse the AST.
clang::TypoCorrection CorrectTypo(const DeclarationNameInfo &Typo,
     // FIXME: We should just return the name we got as input here and prevent
     // clang from trying to correct the typo by itself. That may change the
     // identifier to something that's not wanted by the user.
     return clang::TypoCorrection();
   }
 
   StringRef filename() const { return Filename; }
 
-  /// Called for each include file we discover is in the file.
-  /// \param SourceManager the active SourceManager
-  /// \param canonical_path the canonical path to the include file
-  /// \param uttered_path the path as it appeared in the program
-  /// \param IsAngled whether angle brackets were used
-  /// \param HashLocation the source location of the include's \#
-  /// \param EndLocation the source location following the include
-  void NextInclude(clang::SourceManager *SourceManager,
-                   llvm::StringRef canonical_path, llvm::StringRef uttered_path,
-                   bool IsAngled, clang::SourceLocation HashLocation,
-                   clang::SourceLocation EndLocation) {
-    unsigned Offset = SourceManager->getFileOffset(HashLocation);
-    if (FirstIncludeOffset == -1U)
-      FirstIncludeOffset = Offset;
-  }
-
   /// Get the minimal include for a given path.
   std::string minimizeInclude(StringRef Include,
                               const clang::SourceManager &SourceManager,
                               clang::HeaderSearch &HeaderSearch) {
     if (!MinimizeIncludePaths)
       return Include;
 
     // Get the FileEntry for the include.
public:
   IncludeFixerContext
   getIncludeFixerContext(const clang::SourceManager &SourceManager,
                          clang::HeaderSearch &HeaderSearch) {
     IncludeFixerContext FixerContext;
     if (SymbolQueryResults.empty())
       return FixerContext;
 
     FixerContext.SymbolIdentifer = QuerySymbol;
-    FixerContext.FirstIncludeOffset = FirstIncludeOffset;
     for (const auto &Header : SymbolQueryResults)
       FixerContext.Headers.push_back(
           minimizeInclude(Header, SourceManager, HeaderSearch));
 
     return FixerContext;
   }
 
-  /// Sets the location at the very top of the file.
-  void setFileBegin(clang::SourceLocation Location) { FileBegin = Location; }
-
 private:
   /// Query the database for a given identifier.
   bool query(StringRef Query, SourceLocation Loc) {
     assert(!Query.empty() && "Empty query!");
 
     // Skip other identifers once we have discovered an identfier successfully.
     if (!SymbolQueryResults.empty())
       return false;
private:
   }
 
   /// The client to use to find cross-references.
   SymbolIndexManager &SymbolIndexMgr;
 
   /// The absolute path to the file being processed.
   std::string Filename;
 
-  /// The location of the beginning of the tracked file.
-  clang::SourceLocation FileBegin;
-
-  /// The offset of the last include in the original source file. This will
-  /// be used as the insertion point for new include directives.
-  unsigned FirstIncludeOffset = -1U;
-
   /// The symbol being queried.
   std::string QuerySymbol;
 
   /// The query results of an identifier. We only include the first discovered
   /// identifier to avoid getting caught in results from error recovery.
   std::vector<std::string> SymbolQueryResults;
 
   /// Whether we should use the smallest possible include path.
   bool MinimizeIncludePaths = true;
 };
 
-void PreprocessorHooks::FileChanged(clang::SourceLocation Loc,
-                                    clang::PPCallbacks::FileChangeReason Reason,
-                                    clang::SrcMgr::CharacteristicKind FileType,
-                                    clang::FileID PrevFID) {
-  // Remember where the main file starts.
-  if (Reason == clang::PPCallbacks::EnterFile) {
-    clang::SourceManager *SourceManager =
-        &EnclosingPass->getCompilerInstance().getSourceManager();
-    clang::FileID loc_id = SourceManager->getFileID(Loc);
-    if (const clang::FileEntry *FileEntry =
-            SourceManager->getFileEntryForID(loc_id)) {
-      if (FileEntry->getName() == EnclosingPass->filename()) {
-        EnclosingPass->setFileBegin(Loc);
-        TrackedFile = FileEntry;
-      }
-    }
-  }
-}
-
-void PreprocessorHooks::InclusionDirective(
-    clang::SourceLocation HashLocation, const clang::Token &IncludeToken,
-    llvm::StringRef FileName, bool IsAngled,
-    clang::CharSourceRange FileNameRange, const clang::FileEntry *IncludeFile,
-    llvm::StringRef SearchPath, llvm::StringRef relative_path,
-    const clang::Module *imported) {
-  // Remember include locations so we can insert our new include at the end of
-  // the include block.
-  clang::SourceManager *SourceManager =
-      &EnclosingPass->getCompilerInstance().getSourceManager();
-  auto IDPosition = SourceManager->getDecomposedExpansionLoc(HashLocation);
-  const FileEntry *SourceFile =
-      SourceManager->getFileEntryForID(IDPosition.first);
-  if (!IncludeFile || TrackedFile != SourceFile)
-    return;
-  EnclosingPass->NextInclude(SourceManager, IncludeFile->getName(), FileName,
-                             IsAngled, HashLocation, FileNameRange.getEnd());
-}
-
 } // namespace
 
 IncludeFixerActionFactory::IncludeFixerActionFactory(
     SymbolIndexManager &SymbolIndexMgr, IncludeFixerContext &Context,
     StringRef StyleName, bool MinimizeIncludePaths)
     : SymbolIndexMgr(SymbolIndexMgr), Context(Context),
       MinimizeIncludePaths(MinimizeIncludePaths) {}
 
bool IncludeFixerActionFactory::runInvocation(
   // Technically this should only return true if we're sure that we have a
   // parseable file. We don't know that though. Only inform users of fatal
   // errors.
   return !Compiler.getDiagnostics().hasFatalErrorOccurred();
 }
 
 tooling::Replacements
 createInsertHeaderReplacements(StringRef Code, StringRef FilePath,
-                               StringRef Header, unsigned FirstIncludeOffset,
+                               StringRef Header,
                                const clang::format::FormatStyle &Style) {
   if (Header.empty())
     return tooling::Replacements();
-  // Create replacements for new headers.
-  clang::tooling::Replacements Insertions;
-  if (FirstIncludeOffset == -1U) {
-    // FIXME: skip header guards.
-    FirstIncludeOffset = 0;
-    // If there is no existing #include, then insert an empty line after new
-    // header block.
-    if (Code.front() != '\n')
-      Insertions.insert(
-          clang::tooling::Replacement(FilePath, FirstIncludeOffset, 0, "\n"));
-  }
-  // Keep inserting new headers before the first header.
-  std::string Text = "#include " + Header.str() + "\n";
-  Insertions.insert(
-      clang::tooling::Replacement(FilePath, FirstIncludeOffset, 0, Text));
-  DEBUG({
-    llvm::dbgs() << "Header insertions:\n";
-    for (const auto &R : Insertions)
-      llvm::dbgs() << R.toString() << '\n';
-  });
+  std::string IncludeName = "#include " + Header.str() + "\n";
+  // Create replacements for the new header.
+  clang::tooling::Replacements Insertions = {
+      tooling::Replacement(FilePath, UINT_MAX, 0, IncludeName)};
 
-  return formatReplacements(Code, Insertions, Style);
+  return formatReplacements(
+      Code, cleanupAroundReplacements(Code, Insertions, Style), Style);
 }
 
 } // namespace include_fixer
 } // namespace clang

include-fixer/IncludeFixerContext.h

 namespace include_fixer {
 
 /// \brief A context for the symbol being queried.
 struct IncludeFixerContext {
   /// \brief The symbol name.
   std::string SymbolIdentifer;
   /// \brief The headers which have SymbolIdentifier definitions.
   std::vector<std::string> Headers;
-  /// \brief The insertion point for new include header.
-  unsigned FirstIncludeOffset;
 };
 
 } // namespace include_fixer
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_INCLUDE_FIXER_INCLUDEFIXERCONTEXT_H

include-fixer/tool/ClangIncludeFixer.cpp

int includeFixerMain(int argc, const char **argv) {
   format::FormatStyle InsertStyle = format::getStyle("file", FilePath, Style);
 
   if (!InsertHeader.empty()) {
     if (!STDINMode) {
       errs() << "Should be running in STDIN mode\n";
       return 1;
     }
 
-    // FIXME: Insert the header in right FirstIncludeOffset.
     tooling::Replacements Replacements =
         clang::include_fixer::createInsertHeaderReplacements(
-            Code->getBuffer(), FilePath, InsertHeader,
-            /*FirstIncludeOffset=*/0, InsertStyle);
+            Code->getBuffer(), FilePath, InsertHeader, InsertStyle);
     tooling::Replacements Replaces(Replacements.begin(), Replacements.end());
     std::string ChangedCode =
         tooling::applyAllReplacements(Code->getBuffer(), Replaces);
     llvm::outs() << ChangedCode;
     return 0;
   }
 
   // Set up data source.
if (!Buffer) {
     errs() << "Couldn't open file: " << FilePath;
     return 1;
   }
 
   // FIXME: Rank the results and pick the best one instead of the first one.
   tooling::Replacements Replacements =
       clang::include_fixer::createInsertHeaderReplacements(
           /*Code=*/Buffer.get()->getBuffer(), FilePath, Context.Headers.front(),
-          Context.FirstIncludeOffset, InsertStyle);
+          InsertStyle);
 
   if (!Quiet)
     llvm::errs() << "Added #include" << Context.Headers.front();
 
   // Set up a new source manager for applying the resulting replacements.
   IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts(new DiagnosticOptions);
   DiagnosticsEngine Diagnostics(new DiagnosticIDs, &*DiagOpts);
   TextDiagnosticPrinter DiagnosticPrinter(outs(), &*DiagOpts);

unittests/include-fixer/IncludeFixerTest.cpp

static std::string runIncludeFixer(
   IncludeFixerContext FixerContext;
   IncludeFixerActionFactory Factory(*SymbolIndexMgr, FixerContext, "llvm");
 
   runOnCode(&Factory, Code, "input.cc", ExtraArgs);
   if (FixerContext.Headers.empty())
     return Code;
   tooling::Replacements Replacements =
       clang::include_fixer::createInsertHeaderReplacements(
-          Code, "input.cc", FixerContext.Headers.front(),
-          FixerContext.FirstIncludeOffset);
+          Code, "input.cc", FixerContext.Headers.front());
   clang::RewriterTestContext Context;
   clang::FileID ID = Context.createInMemoryFile("input.cc", Code);
   clang::tooling::applyAllReplacements(Replacements, Context.Rewrite);
   return Context.getRewrittenText(ID);
 }
 
 TEST(IncludeFixer, Typo) {
-  EXPECT_EQ("#include <string>\n\nstd::string foo;\n",
+  EXPECT_EQ("#include <string>\nstd::string foo;\n",
             runIncludeFixer("std::string foo;\n"));
 
+  // FIXME: the current version of include-fixer does not get this test case
+  // right - header should be inserted before definition.
   EXPECT_EQ(
-      "// comment\n#include \"foo.h\"\n#include <string>\nstd::string foo;\n"
-      "#include \"dir/bar.h\"\n",
+      "// comment\n#include \"foo.h\"\nstd::string foo;\n"
+      "#include \"dir/bar.h\"\n#include <string>\n",
       runIncludeFixer("// comment\n#include \"foo.h\"\nstd::string foo;\n"
                       "#include \"dir/bar.h\"\n"));
 
   EXPECT_EQ("#include \"foo.h\"\n#include <string>\nstd::string foo;\n",
             runIncludeFixer("#include \"foo.h\"\nstd::string foo;\n"));
 
   EXPECT_EQ(
       "#include \"foo.h\"\n#include <string>\nstd::string::size_type foo;\n",
       runIncludeFixer("#include \"foo.h\"\nstd::string::size_type foo;\n"));
 
   // string without "std::" can also be fixed since fixed db results go through
   // SymbolIndexManager, and SymbolIndexManager matches unqualified identifiers
   // too.
-  EXPECT_EQ("#include <string>\n\nstring foo;\n",
+  EXPECT_EQ("#include <string>\nstring foo;\n",
             runIncludeFixer("string foo;\n"));
 
   // Fully qualified name.
-  EXPECT_EQ("#include <string>\n\n::std::string foo;\n",
+  EXPECT_EQ("#include <string>\n::std::string foo;\n",
             runIncludeFixer("::std::string foo;\n"));
   // Should not match std::string.
   EXPECT_EQ("::string foo;\n", runIncludeFixer("::string foo;\n"));
 }
 
 TEST(IncludeFixer, IncompleteType) {
   EXPECT_EQ(
       "#include \"foo.h\"\n#include <string>\n"
       "namespace std {\nclass string;\n}\nstring foo;\n",
       runIncludeFixer("#include \"foo.h\"\n"
                       "namespace std {\nclass string;\n}\nstring foo;\n"));
 }
 
 TEST(IncludeFixer, MinimizeInclude) {
   std::vector<std::string> IncludePath = {"-Idir/"};
-  EXPECT_EQ("#include \"otherdir/qux.h\"\n\na::b::foo bar;\n",
+  EXPECT_EQ("#include \"otherdir/qux.h\"\na::b::foo bar;\n",
             runIncludeFixer("a::b::foo bar;\n", IncludePath));
 
   IncludePath = {"-isystemdir"};
-  EXPECT_EQ("#include <otherdir/qux.h>\n\na::b::foo bar;\n",
+  EXPECT_EQ("#include <otherdir/qux.h>\na::b::foo bar;\n",
             runIncludeFixer("a::b::foo bar;\n", IncludePath));
 
   IncludePath = {"-iquotedir"};
-  EXPECT_EQ("#include \"otherdir/qux.h\"\n\na::b::foo bar;\n",
+  EXPECT_EQ("#include \"otherdir/qux.h\"\na::b::foo bar;\n",
             runIncludeFixer("a::b::foo bar;\n", IncludePath));
 
   IncludePath = {"-Idir", "-Idir/otherdir"};
-  EXPECT_EQ("#include \"qux.h\"\n\na::b::foo bar;\n",
+  EXPECT_EQ("#include \"qux.h\"\na::b::foo bar;\n",
             runIncludeFixer("a::b::foo bar;\n", IncludePath));
 }
 
 TEST(IncludeFixer, NestedName) {
-  EXPECT_EQ("#include \"dir/otherdir/qux.h\"\n\n"
+  EXPECT_EQ("#include \"dir/otherdir/qux.h\"\n"
             "int x = a::b::foo(0);\n",
             runIncludeFixer("int x = a::b::foo(0);\n"));
 
   // FIXME: Handle simple macros.
   EXPECT_EQ("#define FOO a::b::foo\nint x = FOO;\n",
             runIncludeFixer("#define FOO a::b::foo\nint x = FOO;\n"));
   EXPECT_EQ("#define FOO(x) a::##x\nint x = FOO(b::foo);\n",
             runIncludeFixer("#define FOO(x) a::##x\nint x = FOO(b::foo);\n"));
 
-  EXPECT_EQ("#include \"dir/otherdir/qux.h\"\n\n"
-            "namespace a {}\nint a = a::b::foo(0);\n",
+  // The empty namespace is cleaned up by clang-format after include-fixer
+  // finishes.
+  EXPECT_EQ("#include \"dir/otherdir/qux.h\"\n"
+            "\nint a = a::b::foo(0);\n",
             runIncludeFixer("namespace a {}\nint a = a::b::foo(0);\n"));
 }
 
 TEST(IncludeFixer, MultipleMissingSymbols) {
-  EXPECT_EQ("#include <string>\n\nstd::string bar;\nstd::sting foo;\n",
+  EXPECT_EQ("#include <string>\nstd::string bar;\nstd::sting foo;\n",
             runIncludeFixer("std::string bar;\nstd::sting foo;\n"));
 }
 
 TEST(IncludeFixer, ScopedNamespaceSymbols) {
-  EXPECT_EQ("#include \"bar.h\"\n\nnamespace a {\nb::bar b;\n}",
+  EXPECT_EQ("#include \"bar.h\"\nnamespace a {\nb::bar b;\n}",
             runIncludeFixer("namespace a {\nb::bar b;\n}"));
-  EXPECT_EQ("#include \"bar.h\"\n\nnamespace A {\na::b::bar b;\n}",
+  EXPECT_EQ("#include \"bar.h\"\nnamespace A {\na::b::bar b;\n}",
             runIncludeFixer("namespace A {\na::b::bar b;\n}"));
-  EXPECT_EQ("#include \"bar.h\"\n\nnamespace a {\nvoid func() { b::bar b; }\n}",
+  EXPECT_EQ("#include \"bar.h\"\nnamespace a {\nvoid func() { b::bar b; }\n}",
             runIncludeFixer("namespace a {\nvoid func() { b::bar b; }\n}"));
   EXPECT_EQ("namespace A { c::b::bar b; }\n",
             runIncludeFixer("namespace A { c::b::bar b; }\n"));
   // FIXME: The header should not be added here. Remove this after we support
   // full match.
-  EXPECT_EQ("#include \"bar.h\"\n\nnamespace A {\nb::bar b;\n}",
+  EXPECT_EQ("#include \"bar.h\"\nnamespace A {\nb::bar b;\n}",
             runIncludeFixer("namespace A {\nb::bar b;\n}"));
 }
 
 TEST(IncludeFixer, EnumConstantSymbols) {
-  EXPECT_EQ("#include \"color.h\"\n\nint test = a::b::Green;\n",
+  EXPECT_EQ("#include \"color.h\"\nint test = a::b::Green;\n",
             runIncludeFixer("int test = a::b::Green;\n"));
 }
 
 // FIXME: add test cases for inserting and sorting multiple headers when
 // include-fixer supports multiple headers insertion.
 TEST(IncludeFixer, InsertAndSortSingleHeader) {
   // Insert one header.
   std::string Code = "#include \"a.h\"\n"