[include-fixer] only deduplicate symbols after matching symbols with the undefined identifier.

[ioeric]

Summary

we should only deduplicate symbols after matching symbols with the
undefined identifier. This patch tries to fix the situation where matching
symbols are deleted when it has the same header as a non-matching symbol, which
can prevent finding the correct header even if there is a matched symbol.

[bkramer]

Thanks!

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

if (Identifier.startswith("::")) {
     IsFullyQualified = true;
   }
 
   // As long as we don't find a result keep stripping name parts from the end.
   // This is to support nested classes which aren't recorded in the database.
   // Eventually we will either hit a class (namespaces aren't in the database
   // either) and can report that result.
   bool TookPrefix = false;
-  std::vector<std::string> Results;
-  while (Results.empty() && !Names.empty()) {
+  std::vector<clang::find_all_symbols::SymbolInfo> MatchedSymbols;
+  while (MatchedSymbols.empty() && !Names.empty()) {
     std::vector<clang::find_all_symbols::SymbolInfo> Symbols;
     for (const auto &DB : SymbolIndices) {
       auto Res = DB->search(Names.back().str());
       Symbols.insert(Symbols.end(), Res.begin(), Res.end());
     }
 
     DEBUG(llvm::dbgs() << "Searching " << Names.back() << "... got "
                        << Symbols.size() << " results...\n");
 
-    rankByPopularity(Symbols);
-
     for (const auto &Symbol : Symbols) {
       // Match the identifier name without qualifier.
       if (Symbol.getName() == Names.back()) {
         bool IsMatched = true;
         auto SymbolContext = Symbol.getContexts().begin();
         auto IdentiferContext = Names.rbegin() + 1; // Skip identifier name.
         // Match the remaining context names.
         while (IdentiferContext != Names.rend() &&
for (const auto &Symbol : Symbols) {
           if (TookPrefix &&
               (Symbol.getSymbolKind() == SymbolInfo::SymbolKind::Function ||
                Symbol.getSymbolKind() == SymbolInfo::SymbolKind::Variable ||
                Symbol.getSymbolKind() ==
                    SymbolInfo::SymbolKind::EnumConstantDecl ||
                Symbol.getSymbolKind() == SymbolInfo::SymbolKind::Macro))
             continue;
 
-          // FIXME: file path should never be in the form of <...> or "...", but
-          // the unit test with fixed database use <...> file path, which might
-          // need to be changed.
-          // FIXME: if the file path is a system header name, we want to use
-          // angle brackets.
-          std::string FilePath = Symbol.getFilePath().str();
-          Results.push_back((FilePath[0] == '"' || FilePath[0] == '<')
-                                ? FilePath
-                                : "\"" + FilePath + "\"");
-        }
-      }
-    }
-    Names.pop_back();
-    TookPrefix = true;
-  }
+          MatchedSymbols.push_back(Symbol);
+        }
+      }
+    }
+    Names.pop_back();
+    TookPrefix = true;
+  }
+
+  rankByPopularity(MatchedSymbols);
+  std::vector<std::string> Results;
+  for (const auto &Symbol : MatchedSymbols) {
 
   return Results;
 }
 
 } // namespace include_fixer
 } // namespace clang

clang-tools-extra/trunk/unittests/include-fixer/IncludeFixerTest.cpp

static std::string runIncludeFixer(
   std::vector<SymbolInfo> Symbols = {
       SymbolInfo("string", SymbolInfo::SymbolKind::Class, "<string>", 1,
                  {{SymbolInfo::ContextType::Namespace, "std"}}),
       SymbolInfo("sting", SymbolInfo::SymbolKind::Class, "\"sting\"", 1,
                  {{SymbolInfo::ContextType::Namespace, "std"}}),
       SymbolInfo("foo", SymbolInfo::SymbolKind::Class, "\"dir/otherdir/qux.h\"",
                  1, {{SymbolInfo::ContextType::Namespace, "b"},
                      {SymbolInfo::ContextType::Namespace, "a"}}),
-      SymbolInfo("bar", SymbolInfo::SymbolKind::Class, "\"bar.h\"",
-                 1, {{SymbolInfo::ContextType::Namespace, "b"},
-                     {SymbolInfo::ContextType::Namespace, "a"}}),
-      SymbolInfo("Green", SymbolInfo::SymbolKind::Class, "\"color.h\"",
-                 1, {{SymbolInfo::ContextType::EnumDecl, "Color"},
-                     {SymbolInfo::ContextType::Namespace, "b"},
-                     {SymbolInfo::ContextType::Namespace, "a"}}),
+      SymbolInfo("bar", SymbolInfo::SymbolKind::Class, "\"bar.h\"", 1,
+                 {{SymbolInfo::ContextType::Namespace, "b"},
+      SymbolInfo("Green", SymbolInfo::SymbolKind::Class, "\"color.h\"", 1,
+                 {{SymbolInfo::ContextType::EnumDecl, "Color"},
+      SymbolInfo("Vector", SymbolInfo::SymbolKind::Class, "\"Vector.h\"", 1,
+                 {{SymbolInfo::ContextType::Namespace, "__a"},
+                  {SymbolInfo::ContextType::Namespace, "a"}},
+                 /*num_occurrences=*/2),
+      SymbolInfo("Vector", SymbolInfo::SymbolKind::Class, "\"Vector.h\"", 2,
+                 {{SymbolInfo::ContextType::Namespace, "a"}},
+                 /*num_occurrences=*/1),
   };
   auto SymbolIndexMgr = llvm::make_unique<include_fixer::SymbolIndexManager>();
   SymbolIndexMgr->addSymbolIndex(
       llvm::make_unique<include_fixer::InMemorySymbolIndex>(Symbols));
 
   IncludeFixerContext FixerContext;
   IncludeFixerActionFactory Factory(*SymbolIndexMgr, FixerContext, "llvm");
 
TEST(IncludeFixer, InsertAndSortSingleHeader) {
   std::string Expected = "#include \"a.h\"\n"
                          "#include \"bar.h\"\n"
                          "#include \"foo.h\"\n"
                          "\n"
                          "namespace a { b::bar b; }";
   EXPECT_EQ(Expected, runIncludeFixer(Code));
 }
 
+TEST(IncludeFixer, DoNotDeleteMatchedSymbol) {
+  EXPECT_EQ("#include \"Vector.h\"\na::Vector v;",
+            runIncludeFixer("a::Vector v;"));
+}
+
 } // namespace
 } // namespace include_fixer
 } // namespace clang