[include-fixer] Add mising qualifiers to all instances of an unidentified symbol.

[ioeric]

include-fixer/tool/ClangIncludeFixer.cpp

365

Now that we insert namespace qualifiers, we might want to do a formatReplacements before applying them.

[bkramer]

include-fixer/IncludeFixer.cpp

246

Drop .str() when comparing StringRef with std::string.

include-fixer/IncludeFixerContext.h

73

This comment is rather useless as-is, maybe explain what it means to have multiple elements in the vector.

[hokein]

Address review comments.

include-fixer/tool/ClangIncludeFixer.cpp

365

Good point! So that we won't break the current code style. The line probably exceeds 80 characters after adding qualifiers.

[hokein]

update error message.

[bkramer]

lgtm

include-fixer/IncludeFixer.cpp

for (const auto &Symbol : MatchedSymbols) {
           ((FilePath[0] == '"' || FilePath[0] == '<') ? FilePath
                                                       : "\"" + FilePath + "\""),
           SourceManager, HeaderSearch);
       SymbolCandidates.emplace_back(Symbol.getName(), Symbol.getSymbolKind(),
                                     MinimizedFilePath, Symbol.getLineNumber(),
                                     Symbol.getContexts(),
                                     Symbol.getNumOccurrences());
     }
-    return IncludeFixerContext(QuerySymbolInfo, SymbolCandidates);
+    return IncludeFixerContext(QuerySymbolInfos, SymbolCandidates);
   }
 
 private:
   /// Query the database for a given identifier.
-  bool query(StringRef Query, StringRef ScopedQualifiers, tooling::Range Range) {
+  bool query(StringRef Query, StringRef ScopedQualifiers,
+             tooling::Range Range) {
     assert(!Query.empty() && "Empty query!");
 
-    // Skip other identifiers once we have discovered an identfier successfully.
-    if (!MatchedSymbols.empty())
+    // Save all instances of an unidentified symbol.
+    //
+    // We use conservative behavior for detecting the same unidentified symbol
+    // here. The symbols which have the same ScopedQualifier and RawIdentifier
+    // are considered equal. So that include-fixer avoids false positives, and
+    // always adds missing qualifiers to correct symbols.
+    if (!QuerySymbolInfos.empty()) {
+      if (ScopedQualifiers.str() == QuerySymbolInfos.front().ScopedQualifiers &&
+          Query.str() == QuerySymbolInfos.front().RawIdentifier) {
+        QuerySymbolInfos.push_back({Query.str(), ScopedQualifiers, Range});
+      }
       return false;
+    }
 
     DEBUG(llvm::dbgs() << "Looking up '" << Query << "' at ");
     DEBUG(getCompilerInstance()
               .getSourceManager()
               .getLocForStartOfFile(
                   getCompilerInstance().getSourceManager().getMainFileID())
               .getLocWithOffset(Range.getOffset())
               .print(llvm::dbgs(), getCompilerInstance().getSourceManager()));
     DEBUG(llvm::dbgs() << " ...");
 
-    QuerySymbolInfo = {Query.str(), ScopedQualifiers, Range};
+    QuerySymbolInfos.push_back({Query.str(), ScopedQualifiers, Range});
 
     // Query the symbol based on C++ name Lookup rules.
     // Firstly, lookup the identifier with scoped namespace contexts;
     // If that fails, falls back to look up the identifier directly.
     //
     // For example:
     //
     // namespace a {
bool query(StringRef Query, StringRef ScopedQualifiers,
     DEBUG(llvm::dbgs() << "Having found " << MatchedSymbols.size()
                        << " symbols\n");
     return !MatchedSymbols.empty();
   }
 
   /// The client to use to find cross-references.
   SymbolIndexManager &SymbolIndexMgr;
 
-  /// The symbol information.
-  IncludeFixerContext::QuerySymbolInfo QuerySymbolInfo;
+  /// The information of the symbols being queried.
+  std::vector<IncludeFixerContext::QuerySymbolInfo> QuerySymbolInfos;
 
   /// All symbol candidates which match QuerySymbol. We only include the first
   /// discovered identifier to avoid getting caught in results from error
   /// recovery.
   std::vector<find_all_symbols::SymbolInfo> MatchedSymbols;
 
   /// Whether we should use the smallest possible include path.
   bool MinimizeIncludePaths = true;

include-fixer/IncludeFixerContext.h

struct QuerySymbolInfo {
     /// Empty if SymbolIdentifier is not in a specific scope.
     std::string ScopedQualifiers;
 
     /// \brief The replacement range of RawIdentifier.
     tooling::Range Range;
   };
 
   IncludeFixerContext() = default;
-  IncludeFixerContext(const QuerySymbolInfo &QuerySymbol,
+  IncludeFixerContext(std::vector<QuerySymbolInfo> QuerySymbols,
                       std::vector<find_all_symbols::SymbolInfo> Symbols);
 
   /// \brief Get symbol name.
   llvm::StringRef getSymbolIdentifier() const {
-    return QuerySymbol.RawIdentifier;
+   return QuerySymbolInfos.front().RawIdentifier;
   }
 
   /// \brief Get replacement range of the symbol.
-  tooling::Range getSymbolRange() const { return QuerySymbol.Range; }
+  tooling::Range getSymbolRange() const {
+    return QuerySymbolInfos.front().Range;
+  }
 
+  /// \brief Get header information.
   const std::vector<HeaderInfo> &getHeaderInfos() const { return HeaderInfos; }
 
+  /// \brief Get information of symbols being querid.
+  const std::vector<QuerySymbolInfo> &getQuerySymbolInfos() const {
+    return QuerySymbolInfos;
+  }
+
 private:
   friend struct llvm::yaml::MappingTraits<IncludeFixerContext>;
 
+  /// \brief The information of the symbol being queried.
+  std::vector<QuerySymbolInfo> QuerySymbolInfos;
+
   /// \brief The symbol candidates which match SymbolIdentifier. The symbols are
   /// sorted in a descending order based on the popularity info in SymbolInfo.
   std::vector<find_all_symbols::SymbolInfo> MatchedSymbols;
 
   /// \brief The header information.
   std::vector<HeaderInfo> HeaderInfos;
-
-  /// \brief The information of the symbol being queried.
-  QuerySymbolInfo QuerySymbol;
 };
 
 } // namespace include_fixer
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_INCLUDE_FIXER_INCLUDEFIXERCONTEXT_H

include-fixer/IncludeFixerContext.cpp

for (; FullySymbolQualifiersIter != FullySymbolQualifiers.end();
     Result += *FullySymbolQualifiersIter;
   }
   return Result;
 }
 
 } // anonymous namespace
 
 IncludeFixerContext::IncludeFixerContext(
-    const QuerySymbolInfo &QuerySymbol,
+    std::vector<QuerySymbolInfo> QuerySymbols,
     std::vector<find_all_symbols::SymbolInfo> Symbols)
-    : MatchedSymbols(std::move(Symbols)), QuerySymbol(QuerySymbol) {
+    : QuerySymbolInfos(std::move(QuerySymbols)),
+      MatchedSymbols(std::move(Symbols)) {
+  // Remove replicated QuerySymbolInfos with the same range.
+  //
+  // QuerySymbolInfos may contain replicated elements. Because CorrectTypo
+  // callback doesn't always work as we expected. In somecases, it will be
+  // triggered at the same position or unidentified symbol multiple times.
+  std::sort(QuerySymbolInfos.begin(), QuerySymbolInfos.end(),
+            [&](const QuerySymbolInfo &A, const QuerySymbolInfo &B) {
+              if (A.Range.getOffset() != B.Range.getOffset())
+                return A.Range.getOffset() < B.Range.getOffset();
+              return A.Range.getLength() == B.Range.getLength();
+            });
+  QuerySymbolInfos.erase(
+      std::unique(QuerySymbolInfos.begin(), QuerySymbolInfos.end(),
+                  [](const QuerySymbolInfo &A, const QuerySymbolInfo &B) {
+                    return A.Range == B.Range;
+                  }),
+      QuerySymbolInfos.end());
   for (const auto &Symbol : MatchedSymbols) {
     HeaderInfos.push_back(
         {Symbol.getFilePath().str(),
          createQualifiedNameForReplacement(
-             QuerySymbol.RawIdentifier, QuerySymbol.ScopedQualifiers, Symbol)});
+             QuerySymbolInfos.front().RawIdentifier,
+             QuerySymbolInfos.front().ScopedQualifiers, Symbol)});
   }
   // Deduplicate header infos.
   HeaderInfos.erase(std::unique(HeaderInfos.begin(), HeaderInfos.end(),
                                 [](const HeaderInfo &A, const HeaderInfo &B) {
                                   return A.Header == B.Header &&
                                          A.QualifiedName == B.QualifiedName;
                                 }),
                     HeaderInfos.end());
 }
 
 } // include_fixer
 } // clang

include-fixer/tool/ClangIncludeFixer.cpp

 
 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)
 LLVM_YAML_IS_FLOW_SEQUENCE_VECTOR(IncludeFixerContext::HeaderInfo)
+LLVM_YAML_IS_FLOW_SEQUENCE_VECTOR(IncludeFixerContext::QuerySymbolInfo)
 
 namespace llvm {
 namespace yaml {
 
 template <> struct MappingTraits<tooling::Range> {
   struct NormalizedRange {
     NormalizedRange(const IO &) : Offset(0), Length(0) {}
 
template <> struct MappingTraits<IncludeFixerContext::QuerySymbolInfo> {
   static void mapping(IO &io, IncludeFixerContext::QuerySymbolInfo &Info) {
     io.mapRequired("RawIdentifier", Info.RawIdentifier);
     io.mapRequired("Range", Info.Range);
   }
 };
 
 template <> struct MappingTraits<IncludeFixerContext> {
   static void mapping(IO &IO, IncludeFixerContext &Context) {
-    IO.mapRequired("QuerySymbolInfo", Context.QuerySymbol);
+    IO.mapRequired("QuerySymbolInfos", Context.QuerySymbolInfos);
     IO.mapRequired("HeaderInfos", Context.HeaderInfos);
   }
 };
 } // namespace yaml
 } // namespace llvm
 
 namespace {
 cl::OptionCategory IncludeFixerCategory("Tool options");
STDINMode("stdin",
                        "used for editor integration."),
               cl::init(false), cl::cat(IncludeFixerCategory));
 
 cl::opt<bool> OutputHeaders(
     "output-headers",
     cl::desc("Print the symbol being queried and all its relevant headers in\n"
              "JSON format to stdout:\n"
              "  {\n"
-             "    \"QuerySymbolInfo\": {\n"
-             "       \"RawIdentifier\": \"foo\",\n"
-             "       \"Range\": {\"Offset\": 0, \"Length\": 3}\n"
-             "    },\n"
+             "    \"QuerySymbolInfos\": [\n"
+             "       {\"RawIdentifier\": \"foo\",\n"
+             "        \"Range\": {\"Offset\": 0, \"Length\": 3}}\n"
+             "    ],\n"
              "    \"HeaderInfos\": [ {\"Header\": \"\\\"foo_a.h\\\"\",\n"
              "                      \"QualifiedName\": \"a::foo\"} ]\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. Support YAML/JSON format:\n"
              "  -insert-header=\"{\n"
-             "     QuerySymbolInfo: {\n"
-             "       RawIdentifier: foo,\n"
-             "       Range: {Offset: 0, Length: 3}\n"
-             "     },\n"
+             "     QuerySymbolInfos: [\n"
+             "       {RawIdentifier: foo,\n"
+             "        Range: {Offset: 0, Length: 3}}\n"
+             "     ],\n"
              "     HeaderInfos: [ {Headers: \"\\\"foo_a.h\\\"\",\n"
              "                     QualifiedName: \"a::foo\"} ]}\""),
     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."),
case yaml: {
     break;
   }
   }
   return SymbolIndexMgr;
 }
 
 void writeToJson(llvm::raw_ostream &OS, const IncludeFixerContext& Context) {
   OS << "{\n"
-        "  \"QuerySymbolInfo\": {\n"
-        "     \"RawIdentifier\": \""
-     << Context.getSymbolIdentifier() << "\",\n";
-  OS << "     \"Range\": {";
-  OS << " \"Offset\":" << Context.getSymbolRange().getOffset() << ",";
-  OS << " \"Length\":" << Context.getSymbolRange().getLength() << " }\n";
-  OS << "  },\n";
+        "  \"QuerySymbolInfos\": [\n";
+  for (const auto &Info : Context.getQuerySymbolInfos()) {
+    OS << "     {\"RawIdentifier\": \"" << Info.RawIdentifier << "\",\n";
+    OS << "      \"Range\":{";
+    OS << "\"Offset\":" << Info.Range.getOffset() << ",";
+    OS << "\"Length\":" << Info.Range.getLength() << "}}";
+    if (&Info != &Context.getQuerySymbolInfos().back())
+      OS << ",\n";
+  }
+  OS << "\n  ],\n";
   OS << "  \"HeaderInfos\": [\n";
   const auto &HeaderInfos = Context.getHeaderInfos();
   for (const auto &Info : HeaderInfos) {
     OS << "     {\"Header\": \"" << llvm::yaml::escape(Info.Header) << "\",\n"
        << "      \"QualifiedName\": \"" << Info.QualifiedName << "\"}";
     if (&Info != &HeaderInfos.back())
       OS << ",\n";
   }
if (!InsertHeader.empty()) {
     //
     // Check whether all elements in HeaderInfos have the same qualified name.
     bool IsUniqueQualifiedName = std::equal(
         HeaderInfos.begin() + 1, HeaderInfos.end(), HeaderInfos.begin(),
         [](const IncludeFixerContext::HeaderInfo &LHS,
            const IncludeFixerContext::HeaderInfo &RHS) {
           return LHS.QualifiedName == RHS.QualifiedName;
         });
-    if (IsUniqueQualifiedName)
-      Replacements->insert({FilePath, Context.getSymbolRange().getOffset(),
-                            Context.getSymbolRange().getLength(),
+    if (IsUniqueQualifiedName) {
+      for (const auto &Info : Context.getQuerySymbolInfos()) {
+        Replacements->insert({FilePath, Info.Range.getOffset(),
+                              Info.Range.getLength(),
                               Context.getHeaderInfos().front().QualifiedName});
+      }
+    }
     auto ChangedCode =
         tooling::applyAllReplacements(Code->getBuffer(), *Replacements);
     if (!ChangedCode) {
       llvm::errs() << llvm::toString(ChangedCode.takeError()) << "\n";
       return 1;
     }
     llvm::outs() << *ChangedCode;
     return 0;
if (!Replacements) {
     return 1;
   }
 
   if (!Quiet)
     errs() << "Added #include " << Context.getHeaderInfos().front().Header
            << '\n';
 
   // Add missing namespace qualifiers to the unidentified symbol.
-  Replacements->insert({FilePath, Context.getSymbolRange().getOffset(),
-                        Context.getSymbolRange().getLength(),
+  for (const auto &Info : Context.getQuerySymbolInfos()) {
+    Replacements->insert({FilePath, Info.Range.getOffset(),
+                          Info.Range.getLength(),
                           Context.getHeaderInfos().front().QualifiedName});
+  }
 
   // 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);
   SourceManager SM(Diagnostics, tool.getFiles());
   Diagnostics.setClient(&DiagnosticPrinter, false);
 

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

def main():
   command = [binary, "-stdin", "-output-headers", "-db=" + args.db,
              "-input=" + args.input, vim.current.buffer.name]
   stdout, stderr = execute(command, text)
   if stderr:
     print >> sys.stderr, "Error while running clang-include-fixer: " + stderr
     return
 
   include_fixer_context = json.loads(stdout)
-  query_symbol_info = include_fixer_context["QuerySymbolInfo"]
-  symbol = query_symbol_info["RawIdentifier"]
+  query_symbol_infos = include_fixer_context["QuerySymbolInfos"]
+  symbol = query_symbol_infos[0]["RawIdentifier"]
   # The header_infos is already sorted by include-fixer.
   header_infos = include_fixer_context["HeaderInfos"]
   # Deduplicate headers while keeping the order, so that the same header would
   # not be suggested twice.
   unique_headers = []
   seen = set()
   for header_info in header_infos:
     header = header_info["Header"]
     if header not in seen:
       seen.add(header)
       unique_headers.append(header)
 
   if not symbol:
     print "The file is fine, no need to add a header."
     return
 
   if not unique_headers:
     print "Couldn't find a header for {0}.".format(symbol)
     return
 
   try:
     # If there is only one suggested header, insert it directly.
     if len(unique_headers) == 1 or maximum_suggested_headers == 1:
-      InsertHeaderToVimBuffer({"QuerySymbolInfo": query_symbol_info,
+      InsertHeaderToVimBuffer({"QuerySymbolInfos": query_symbol_infos,
                                "HeaderInfos": header_infos}, text)
       print "Added #include {0} for {1}.".format(unique_headers[0], symbol)
       return
 
     selected = GetUserSelection("choose a header file for {0}.".format(symbol),
                                 unique_headers, maximum_suggested_headers)
     selected_header_infos = [
       header for header in header_infos if header["Header"] == selected]
 
     # Insert a selected header.
-    InsertHeaderToVimBuffer({"QuerySymbolInfo": query_symbol_info,
+    InsertHeaderToVimBuffer({"QuerySymbolInfos": query_symbol_infos,
                              "HeaderInfos": selected_header_infos}, text)
     print "Added #include {0} for {1}.".format(selected, symbol)
   except Exception as error:
     print >> sys.stderr, error.message
   return
 
 
 if __name__ == '__main__':
   main()

test/include-fixer/commandline_options.cpp

 // REQUIRES: shell
 // RUN: echo "foo f;" > %t.cpp
 // RUN: clang-include-fixer -db=fixed -input='foo= "foo.h","bar.h"' -output-headers %t.cpp -- | FileCheck %s
-// RUN: cat %t.cpp | clang-include-fixer -stdin -insert-header='{QuerySymbolInfo: {RawIdentifier: foo, Range: {Offset: 0, Length: 3}}, HeaderInfos: [{Header: "\"foo.h\"", QualifiedName: "foo"}]}' %t.cpp | FileCheck %s -check-prefix=CHECK-CODE
-// RUN: cat %t.cpp | not clang-include-fixer -stdin -insert-header='{QuerySymbolInfo: {RawIdentifier: foo, Range: {Offset: 0, Length: 3}}, HeaderInfos: [{Header: "\"foo.h\"", QualifiedName: "foo"},{Header: "\"foo2.h\"", QualifiedName: "foo"}]}' %t.cpp
-// RUN: cat %t.cpp | clang-include-fixer -stdin -insert-header='{QuerySymbolInfo: {RawIdentifier: foo, Range: {Offset: 0, Length: 3}}, HeaderInfos: [{Header: "\"foo.h\"", QualifiedName: "a:foo"},{Header: "\"foo.h\"", QualifiedName: "b:foo"}]}' %t.cpp
+// RUN: cat %t.cpp | clang-include-fixer -stdin -insert-header='{QuerySymbolInfos: [{RawIdentifier: foo, Range: {Offset: 0, Length: 3}}], HeaderInfos: [{Header: "\"foo.h\"", QualifiedName: "foo"}]}' %t.cpp | FileCheck %s -check-prefix=CHECK-CODE
+// RUN: cat %t.cpp | not clang-include-fixer -stdin -insert-header='{QuerySymbolInfos: [{RawIdentifier: foo, Range: {Offset: 0, Length: 3}}], HeaderInfos: [{Header: "\"foo.h\"", QualifiedName: "foo"},{Header: "\"foo2.h\"", QualifiedName: "foo"}]}' %t.cpp
+// RUN: cat %t.cpp | clang-include-fixer -stdin -insert-header='{QuerySymbolInfos: [{RawIdentifier: foo, Range: {Offset: 0, Length: 3}}], HeaderInfos: [{Header: "\"foo.h\"", QualifiedName: "a:foo"},{Header: "\"foo.h\"", QualifiedName: "b:foo"}]}' %t.cpp
 //
 // CHECK:     "HeaderInfos": [
 // CHECK-NEXT:  {"Header": "\"foo.h\"",
 // CHECK-NEXT:   "QualifiedName": "foo"},
 // CHECK-NEXT:  {"Header": "\"bar.h\"",
 // CHECK-NEXT:   "QualifiedName": "foo"}
 // CHECK-NEXT:]
 //
 // CHECK-CODE: #include "foo.h"
 // CHECK-CODE: foo f;

unittests/include-fixer/IncludeFixerTest.cpp

static std::string runIncludeFixer(
   auto Replaces = clang::include_fixer::createInsertHeaderReplacements(
       Code, FakeFileName, FixerContext.getHeaderInfos().front().Header);
   EXPECT_TRUE(static_cast<bool>(Replaces))
       << llvm::toString(Replaces.takeError()) << "\n";
   if (!Replaces)
     return "";
   clang::RewriterTestContext Context;
   clang::FileID ID = Context.createInMemoryFile(FakeFileName, Code);
-  Replaces->insert({FakeFileName, FixerContext.getSymbolRange().getOffset(),
-                    FixerContext.getSymbolRange().getLength(),
+  for (const auto &Info : FixerContext.getQuerySymbolInfos()) {
+    Replaces->insert({FakeFileName, Info.Range.getOffset(),
+                      Info.Range.getLength(),
                       FixerContext.getHeaderInfos().front().QualifiedName});
+  }
   clang::tooling::applyAllReplacements(*Replaces, Context.Rewrite);
   return Context.getRewrittenText(ID);
 }
 
 TEST(IncludeFixer, Typo) {
   EXPECT_EQ("#include <string>\nstd::string foo;\n",
             runIncludeFixer("std::string foo;\n"));
 
TEST(IncludeFixer, FixNamespaceQualifiers) {
 
   // Test global scope operator.
   EXPECT_EQ("#include \"bar.h\"\n::a::b::bar b;\n",
             runIncludeFixer("::a::b::bar b;\n"));
   EXPECT_EQ("#include \"bar.h\"\nnamespace a {\n::a::b::bar b;\n}\n",
             runIncludeFixer("namespace a {\n::a::b::bar b;\n}\n"));
 }
 
+TEST(IncludeFixer, FixNamespaceQualifiersForAllInstances) {
+  const char TestCode[] = R"(
+namespace a {
+bar b; // Fix
+int func1() {
+  bar a; // Fix
+  bar *p = new bar(); // Fix
+  return 0;
+}
+} // namespace a
+
+namespace a {
+bar func2() { // Fix
+  bar f; // Fix
+  return f;
+}
+} // namespace a
+
+void f() {
+  bar b; // No-fix: Not in the same scope.
+}
+
+namespace a {
+namespace c {
+  bar b; // No-fix: Not in the same scope.
+} // namespace c
+} // namespace a
+)";
+
+  const char ExpectedCode[] = R"(
+#include "bar.h"
+namespace a {
+b::bar b; // Fix
+int func1() {
+  b::bar a; // Fix
+  b::bar *p = new b::bar(); // Fix
+  return 0;
+}
+} // namespace a
+
+namespace a {
+b::bar func2() { // Fix
+  b::bar f; // Fix
+  return f;
+}
+} // namespace a
+
+void f() {
+  bar b; // No-fix: Not in the same scope.
+}
+
+namespace a {
+namespace c {
+  bar b; // No-fix: Not in the same scope.
+} // namespace c
+} // namespace a
+)";
+
+  EXPECT_EQ(ExpectedCode, runIncludeFixer(TestCode));
+}
+
 } // namespace
 } // namespace include_fixer
 } // namespace clang