[include-fixer] Use YAML format in -output-headers and -insert-header mode.

[hokein]

Summary

And some improvements:

• Show better error messages on unfound symbols.
• Fix a typo.

[hokein]

Remove unused code.

[bkramer]

include-fixer/tool/ClangIncludeFixer.cpp

80 ↗

(On Diff #59100)

drop 'the'

include-fixer/tool/clang-include-fixer.py

22 ↗

(On Diff #59100)

We cannot rely on python-yaml being available, I don't want users to install python modules just to run include-fixer. Can we restrict ourselves to the JSON subset of YAML here?

[hokein]

Get rid of yaml dependency, using json module.

[hokein]

Show error message when clang-include-fixer died with a fatal error.

[bkramer]

include-fixer/tool/ClangIncludeFixer.cpp

160 ↗

(On Diff #59184)

Can you just llvm::yaml::escape the string?

[hokein]

Use llvm::yaml::escape to escape double quote.

[bkramer]

include-fixer/tool/ClangIncludeFixer.cpp

160 ↗

(On Diff #59185)

I think we should do the escaping always and not just when the first character is a quote.

[hokein]

Always escape headers.

clang-tools-extra/trunk/include-fixer/IncludeFixer.cpp

std::string minimizeInclude(StringRef Include,
     return IsSystem ? '<' + Suggestion + '>' : '"' + Suggestion + '"';
   }
 
   /// Get the include fixer context for the queried symbol.
   IncludeFixerContext
   getIncludeFixerContext(const clang::SourceManager &SourceManager,
                          clang::HeaderSearch &HeaderSearch) {
     IncludeFixerContext FixerContext;
-    if (SymbolQueryResults.empty())
-      return FixerContext;
-
-    FixerContext.SymbolIdentifer = QuerySymbol;
+    FixerContext.SymbolIdentifier = QuerySymbol;
     for (const auto &Header : SymbolQueryResults)
       FixerContext.Headers.push_back(
           minimizeInclude(Header, SourceManager, HeaderSearch));
 
     return FixerContext;
   }
 
 private:

clang-tools-extra/trunk/include-fixer/IncludeFixerContext.h

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

clang-tools-extra/trunk/include-fixer/tool/ClangIncludeFixer.cpp

 #include "SymbolIndexManager.h"
 #include "YamlSymbolIndex.h"
 #include "clang/Format/Format.h"
 #include "clang/Frontend/TextDiagnosticPrinter.h"
 #include "clang/Rewrite/Core/Rewriter.h"
 #include "clang/Tooling/CommonOptionsParser.h"
 #include "clang/Tooling/Tooling.h"
 #include "llvm/Support/CommandLine.h"
+#include "llvm/Support/YAMLTraits.h"
 
 using namespace clang;
 using namespace llvm;
+using clang::include_fixer::IncludeFixerContext;
+
+LLVM_YAML_IS_DOCUMENT_LIST_VECTOR(IncludeFixerContext)
+LLVM_YAML_IS_FLOW_SEQUENCE_VECTOR(std::string)
+
+namespace llvm {
+namespace yaml {
+template <> struct MappingTraits<IncludeFixerContext> {
+  static void mapping(IO &io, IncludeFixerContext &Context) {
+    io.mapRequired("SymbolIdentifier", Context.SymbolIdentifier);
+    io.mapRequired("Headers", Context.Headers);
+  }
+};
+} // namespace yaml
+} // namespace llvm
 
 namespace {
 cl::OptionCategory IncludeFixerCategory("Tool options");
 
 enum DatabaseFormatTy {
   fixed, ///< Hard-coded mapping.
   yaml,  ///< Yaml database created by find-all-symbols.
 };
STDINMode("stdin",
                        "virtual file system) with input from <stdin> and run\n"
                        "the tool on the new content with the compilation\n"
                        "options of the source file. This mode is currently\n"
                        "used for editor integration."),
               cl::init(false), cl::cat(IncludeFixerCategory));
 
 cl::opt<bool> OutputHeaders(
     "output-headers",
-    cl::desc("Output the symbol being quired and all its relevant headers.\n"
-             "The first line is the symbol name; The other lines\n"
-             "are the headers: \n"
-             "   b::foo\n"
-             "   path/to/foo_a.h\n"
-             "   path/to/foo_b.h\n"),
+    cl::desc("Print the symbol being queried and all its relevant headers in\n"
+             "JSON format to stdout:\n"
+             "  {\n"
+             "    \"SymbolIdentifier\": \"foo\",\n"
+             "    \"Headers\": [\"\\\"foo_a.h\\\"\"]\n"
+             "  }"),
     cl::init(false), cl::cat(IncludeFixerCategory));
 
 cl::opt<std::string> InsertHeader(
     "insert-header",
     cl::desc("Insert a specific header. This should run with STDIN mode.\n"
              "The result is written to stdout. It is currently used for\n"
-             "editor integration."),
+             "editor integration. Support YAML/JSON format:\n"
+             "  -insert-header=\"{SymbolIdentifier: foo,\n"
+             "                   Headers: ['\\\"foo_a.h\\\"']}\""),
     cl::init(""), cl::cat(IncludeFixerCategory));
 
 cl::opt<std::string>
     Style("style",
           cl::desc("Fallback style for reformatting after inserting new "
                    "headers if there is no clang-format config file found."),
           cl::init("llvm"), cl::cat(IncludeFixerCategory));
 
case yaml: {
 
     SymbolIndexMgr->addSymbolIndex(std::move(*DB));
     break;
   }
   }
   return SymbolIndexMgr;
 }
 
+void writeToJson(llvm::raw_ostream &OS, const IncludeFixerContext& Context) {
+  OS << "{\n"
+        "  \"SymbolIdentifier\": \"" << Context.SymbolIdentifier << "\",\n"
+        "  \"Headers\": [ ";
+  for (const auto &Header : Context.Headers) {
+    OS << " \"" << llvm::yaml::escape(Header) << "\"";
+    if (Header != Context.Headers.back())
+      OS << ", ";
+  }
+  OS << " ]\n"
+        "}\n";
+}
+
 int includeFixerMain(int argc, const char **argv) {
   tooling::CommonOptionsParser options(argc, argv, IncludeFixerCategory);
   tooling::ClangTool tool(options.getCompilations(),
                           options.getSourcePathList());
 
   // In STDINMode, we override the file content with the <stdin> input.
   // Since `tool.mapVirtualFile` takes `StringRef`, we define `Code` outside of
   // the if-block so that `Code` is not released after the if-block.
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;
     }
 
+    llvm::yaml::Input yin(InsertHeader);
+    IncludeFixerContext Context;
+    yin >> Context;
+
+    if (Context.Headers.size() != 1) {
+      errs() << "Expect exactly one inserted header.\n";
+      return 1;
+    }
+
     tooling::Replacements Replacements =
         clang::include_fixer::createInsertHeaderReplacements(
-            Code->getBuffer(), FilePath, InsertHeader, InsertStyle);
+            Code->getBuffer(), FilePath, Context.Headers[0], 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.
int includeFixerMain(int argc, const char **argv) {
 
   if (tool.run(&Factory) != 0) {
     llvm::errs()
         << "Clang died with a fatal error! (incorrect include paths?)\n";
     return 1;
   }
 
   if (OutputHeaders) {
-    // FIXME: Output IncludeFixerContext as YAML.
-    llvm::outs() << Context.SymbolIdentifer << "\n";
-    for (const auto &Header : Context.Headers)
-      llvm::outs() << Header << "\n";
+    writeToJson(llvm::outs(), Context);
     return 0;
   }
 
   if (Context.Headers.empty())
     return 0;
 
   auto Buffer = llvm::MemoryBuffer::getFile(FilePath);
   if (!Buffer) {

clang-tools-extra/trunk/include-fixer/tool/clang-include-fixer.py

 #
 # It operates on the current, potentially unsaved buffer and does not create
 # or save any files. To revert a fix, just undo.
 
 import argparse
 import difflib
 import subprocess
 import vim
+import json
 
 # set g:clang_include_fixer_path to the path to clang-include-fixer if it is not
 # on the path.
 # Change this to the full path if clang-include-fixer is not on the path.
 binary = 'clang-include-fixer'
 if vim.eval('exists("g:clang_include_fixer_path")') == "1":
   binary = vim.eval('g:clang_include_fixer_path')
 

 def execute(command, text):
   p = subprocess.Popen(command,
                        stdout=subprocess.PIPE, stderr=subprocess.PIPE,
                        stdin=subprocess.PIPE)
   return p.communicate(input=text)
 
 
 def InsertHeaderToVimBuffer(header, text):
-  command = [binary, "-stdin", "-insert-header="+header,
+  command = [binary, "-stdin", "-insert-header="+json.dumps(header),
              vim.current.buffer.name]
   stdout, stderr = execute(command, text)
   if stdout:
     lines = stdout.splitlines()
     sequence = difflib.SequenceMatcher(None, vim.current.buffer, lines)
     for op in reversed(sequence.get_opcodes()):
       if op[0] is not 'equal':
         vim.current.buffer[op[1]:op[2]] = lines[op[3]:op[4]]
def main():
   # Get the current text.
   buf = vim.current.buffer
   text = '\n'.join(buf)
 
   # Run command to get all headers.
   command = [binary, "-stdin", "-output-headers", "-db="+args.db,
              "-input="+args.input, vim.current.buffer.name]
   stdout, stderr = execute(command, text)
-  lines = stdout.splitlines()
-  if len(lines) < 2:
-    print "No header is included.\n"
+  if stderr:
+    print >> sys.stderr, "Error while running clang-include-fixer: " + stderr
+    return
+
+  include_fixer_context = json.loads(stdout)
+  symbol = include_fixer_context["SymbolIdentifier"]
+  headers = include_fixer_context["Headers"]
+
+  if not symbol:
+    print "The file is fine, no need to add a header.\n"
+    return;
+
+  if not headers:
+    print "Couldn't find a header for {0}.\n".format(symbol)
     return
 
   # The first line is the symbol name.
-  symbol = lines[0]
   # If there is only one suggested header, insert it directly.
-  if len(lines) == 2 or maximum_suggested_headers == 1:
-    InsertHeaderToVimBuffer(lines[1], text)
-    print "Added #include {0} for {1}.\n".format(lines[1], symbol)
+  if len(headers) == 1 or maximum_suggested_headers == 1:
+    InsertHeaderToVimBuffer({"SymbolIdentifier": symbol,
+                             "Headers":[headers[0]]}, text)
+    print "Added #include {0} for {1}.\n".format(headers[0], symbol)
     return
 
   choices_message = ""
   index = 1;
-  for header in lines[1:1+maximum_suggested_headers]:
+  for header in headers[0:maximum_suggested_headers]:
     choices_message += "&{0} {1}\n".format(index, header)
     index += 1
 
   select = ShowDialog("choose a header file for {0}.".format(symbol),
                       choices_message)
   # Insert a selected header.
-  InsertHeaderToVimBuffer(lines[select], text)
-  print "Added #include {0} for {1}.\n".format(lines[select], symbol)
+  InsertHeaderToVimBuffer({"SymbolIdentifier": symbol,
+                           "Headers":[headers[select-1]]}, text)
+  print "Added #include {0} for {1}.\n".format(headers[select-1], symbol)
   return;
 
 
 if __name__ == '__main__':
   main()

clang-tools-extra/trunk/test/include-fixer/commandline_options.cpp

 // REQUIRES: shell
 // RUN: sed -e 's#//.*$##' %s > %t.cpp
 // RUN: clang-include-fixer -db=fixed -input='foo= "foo.h","bar.h"' -output-headers %t.cpp -- | FileCheck %s -check-prefix=CHECK-HEADERS
-// RUN: cat %t.cpp | clang-include-fixer -stdin -insert-header='"foo.h"' %t.cpp | FileCheck %s -check-prefix=CHECK
+// RUN: cat %t.cpp | clang-include-fixer -stdin -insert-header='{SymbolIdentifier: foo, Headers: ["\"foo.h\""]}' %t.cpp | FileCheck %s -check-prefix=CHECK
 //
-// CHECK-HEADERS: "foo.h"
-// CHECK-HEADERS: "bar.h"
+// CHECK-HEADERS: "Headers": [ "\"foo.h\"", "\"bar.h\"" ]
 //
 // CHECK: #include "foo.h"
 // CHECK: foo f;
 
 foo f;

clang-tools-extra/trunk/test/include-fixer/ranking.cpp

-// RUN: clang-include-fixer -db=yaml -input=%S/Inputs/fake_yaml_db.yaml -output-headers %s -- | FileCheck %s -implicit-check-not=.h
+// RUN: clang-include-fixer -db=yaml -input=%S/Inputs/fake_yaml_db.yaml -output-headers %s -- | FileCheck %s
 
-// CHECK: "../include/bar.h"
-// CHECK-NEXT: "../include/zbar.h"
+// CHECK: "Headers": [ "\"../include/bar.h\"", "\"../include/zbar.h\"" ]
 
 bar b;