[include-fixer] Add missing namespace qualifiers after inserting a missing header.

[hokein]

Summary

This is an initial version of fixing namespace issues by adding missing
namespace qualifiers to an unidentified symbol.

This version only fixes the first discovered unidentified symbol.
In the long run, include-fixer should fix all unidentified symbols
with a same name at one run.

Currently, it works on command-line tool. The vim integration is not
implemented yet.

[hokein]

Fix a typo.

[djasper]

Sorry, I completely forgot about this. Will try to review today. Is this part about the patch description accurate?

"This version only fixes the first discovered unidentified symbol.
In the long run, include-fixer should fix all unidentified symbols
with a same name at one run.

Currently, it works on command-line tool. The vim integration is not
implemented yet."

Specifically, what needs to be implemented in vim to make this work?

[hokein]

In D21603#468717, @djasper wrote:

Sorry, I completely forgot about this. Will try to review today. Is this part about the patch description accurate?

Yes, the description is accurate.

Specifically, what needs to be implemented in vim to make this work?

We need a way to dump the mapping relationship between symbols and their headers, so that include-fixer can know which namespace prefix should be used when user selects a particular header in vim.

[djasper]

include-fixer/IncludeFixer.cpp

224

How is this change related?

228

Indentation is off.

include-fixer/tool/ClangIncludeFixer.cpp

88

Do we really want this? Should we just always do it? Who is going to use this flag?

If we want it, maybe call it "fix-namespace-qualifiers"? This renaming might also make sense of the patch itself, the test case, ...

unittests/include-fixer/IncludeFixerTest.cpp

231

All the tests in here test with an existing NestedNameSpecifier (i.e. b::bar). Is there a reason to not also have tests with a plain identifier (e.g. bar)?

249

I think we should address this part from the start. Otherwise, we are making the current behavior worse for a significant amount of cases. I suspect that frequently people use types from their own projects (and thus in the same namespace they are already in) and we don't want to add nested name specifiers for those.

[hokein]

Address review comments.

[hokein]

include-fixer/IncludeFixer.cpp

224

We need to check whether the FilePath contains " here because the FilePath in symbol indexer doesn't contains <, but for standard headers, it contains "<".

include-fixer/tool/ClangIncludeFixer.cpp

88

I don't have many ideas about this, but I'd like to add this option at the moment, so that user can enable/disable this feature freely. If we want, we could remove it in the future.

[djasper]

include-fixer/IncludeFixer.cpp

224

Ah, I see. This was just moved. Sorry, missed that.

258

Could you add something about this to the CL description? Or actually, I think this is very separate from fixing the namespace qualifiers. Can we move this part to a different patch?

Or am I missing something so that one cannot go without the other? Fundamentally, this seems to be doing two things:

1. Insert namespace qualifiers if they are missing.
2. Take existing namespace qualifiers to ensure we aren't looking up the wrong symbol.

include-fixer/IncludeFixerContext.h

31

This again seems like something that is unrelated to what the patch is actually meant to do. Can we split this into a separate patch?

include-fixer/find-all-symbols/SymbolInfo.cpp

93

I think "getQualifiedName" is sufficient and then matches what's in NamedDecl.

94

There is an extra "n" in "FullyQuanlifiedName".

include-fixer/tool/ClangIncludeFixer.cpp

88

I'd strongly recommend not doing that. Removing flags that people are using is hard (breaks them). Make this good enough so that it is always the right thing to do.

unittests/include-fixer/IncludeFixerTest.cpp

144

Do you need to set FixNamespaceQualifiers to false here?

[hokein]

Fix review comments.

include-fixer/IncludeFixer.cpp

258

It's not introduced by this patch. Actually it moves from query function caller to the function body. Why we make this change? Because we need to save the scoped qualifiers of the symbol, so that include-fixer can know how to create a correct qualifiers for the symbol.

include-fixer/IncludeFixerContext.h

31

This is also a move from rankByPopularity in SymbolIndexManager.cpp.

unittests/include-fixer/IncludeFixerTest.cpp

144

No need to. Both are OK. Change to true here.

[bkramer]

In the future I'd prefer to have patches like this split up in a part that refactors and a part that contains the actual change. Having that in one patch makes it really hard to review.

include-fixer/IncludeFixer.cpp

73–77

std::string, otherwise this will be a user after free.

include-fixer/IncludeFixerContext.h

76

Explain what a scoped qualifier is in this comment.

89

typo: breif

[hokein]

Address Ben's comments.

[hokein]

In D21603#475011, @bkramer wrote:

In the future I'd prefer to have patches like this split up in a part that refactors and a part that contains the actual change. Having that in one patch makes it really hard to review.

Acknowledged. I'm sorry for it. I didn't foresee that I would change so much code before I implementing this.

So let me summarize the changes of this patch:

1. Instead of returning raw string headers, make SymbolIndexManager::search return Symbolnfos.
2. Move rankByPopularity unique SymbolInfo code from SymbolIndexManager.cpp to IncludeFixerContext.cpp.
3. Extend IncludeFixerContext to track more SymbolInfo information (e.g. SymbolRange, SymbolScopedQualifiers).
4. Some move stuff in Action::query API.
5. Create adding namespace qualifiers replacement for the unidentified symbol.

include-fixer/IncludeFixer.cpp

73–77

Good catch. Done.

[bkramer]

I think this can go in now.

[djasper]

include-fixer/IncludeFixer.cpp

166

Lots of code duplication. Maybe pull out function/lambda?

259

"If that fails, .."

unittests/include-fixer/IncludeFixerTest.cpp

144

I think we should set this to true everywhere or more precisely completely remove this parameter. Now that the flag is gone, we are starting to test implementation details IMO.

[hokein]

• Address Daniel's comments.
• Add tests for nested classes.

[hokein]

unittests/include-fixer/IncludeFixerTest.cpp

144

Yeah, this parameter is not needed. Removed it.

[djasper]

Looks good.

include-fixer/IncludeFixer.cpp

public:
   /// have the fully qualified name ready. Just query that.
   bool MaybeDiagnoseMissingCompleteType(clang::SourceLocation Loc,
                                         clang::QualType T) override {
     // Ignore spurious callbacks from SFINAE contexts.
     if (getCompilerInstance().getSema().isSFINAEContext())
       return false;
 
     clang::ASTContext &context = getCompilerInstance().getASTContext();
-    query(T.getUnqualifiedType().getAsString(context.getPrintingPolicy()), Loc);
+    std::string QueryString =
+        T.getUnqualifiedType().getAsString(context.getPrintingPolicy());
+    DEBUG(llvm::dbgs() << "Query missing complete type '" << QueryString
+                       << "'");
+    query(QueryString, "", tooling::Range());
     return false;
   }
 
   /// Callback for unknown identifiers. Try to piece together as much
   /// qualification as we can get and do a query.
   clang::TypoCorrection CorrectTypo(const DeclarationNameInfo &Typo,
                                     int LookupKind, Scope *S, CXXScopeSpec *SS,
                                     CorrectionCandidateCallback &CCC,
auto ExtendNestedNameSpecifier = [this](CharSourceRange Range) {
       while (isIdentifierBody(*End) || *End == ':')
         ++End;
 
       return std::string(Source.begin(), End);
     };
 
     /// If we have a scope specification, use that to get more precise results.
     std::string QueryString;
+    tooling::Range SymbolRange;
+    const auto &SM = getCompilerInstance().getSourceManager();
     if (SS && SS->getRange().isValid()) {
       auto Range = CharSourceRange::getTokenRange(SS->getRange().getBegin(),
                                                   Typo.getLoc());
 
       QueryString = ExtendNestedNameSpecifier(Range);
+      SymbolRange = tooling::Range(SM.getDecomposedLoc(Range.getBegin()).second,
+                                   QueryString.size());
     } else if (Typo.getName().isIdentifier() && !Typo.getLoc().isMacroID()) {
       auto Range =
           CharSourceRange::getTokenRange(Typo.getBeginLoc(), Typo.getEndLoc());
 
       QueryString = ExtendNestedNameSpecifier(Range);
+      SymbolRange = tooling::Range(SM.getDecomposedLoc(Range.getBegin()).second,
+                                   QueryString.size());
     } else {
       QueryString = Typo.getAsString();
+      SymbolRange = tooling::Range(SM.getDecomposedLoc(Typo.getLoc()).second,
+                                   QueryString.size());
     }
 
-    // Follow C++ Lookup rules. Firstly, lookup the identifier with scoped
-    // namespace contexts. If fails, falls back to identifier.
-    // For example:
-    //
-    // namespace a {
-    // b::foo f;
-    // }
-    //
-    // 1. lookup a::b::foo.
-    // 2. lookup b::foo.
-    if (!query(TypoScopeString + QueryString, Typo.getLoc()))
-      query(QueryString, Typo.getLoc());
+    DEBUG(llvm::dbgs() << "TypoScopeQualifiers: " << TypoScopeString << "\n");
+    query(QueryString, TypoScopeString, SymbolRange);
 
     // 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; }
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;
-    FixerContext.SymbolIdentifier = QuerySymbol;
-    for (const auto &Header : SymbolQueryResults)
-      FixerContext.Headers.push_back(
-          minimizeInclude(Header, SourceManager, HeaderSearch));
-
-    return FixerContext;
+    std::vector<find_all_symbols::SymbolInfo> SymbolCandidates;
+    for (const auto &Symbol : MatchedSymbols) {
+      std::string FilePath = Symbol.getFilePath().str();
+      std::string MinimizedFilePath = minimizeInclude(
+          ((FilePath[0] == '"' || FilePath[0] == '<') ? FilePath
+                                                      : "\"" + FilePath + "\""),
+          SourceManager, HeaderSearch);
+      SymbolCandidates.emplace_back(Symbol.getName(), Symbol.getSymbolKind(),
+                                    MinimizedFilePath, Symbol.getLineNumber(),
+                                    Symbol.getContexts(),
+                                    Symbol.getNumOccurrences());
+    }
+    return IncludeFixerContext(QuerySymbol, SymbolScopedQualifiers,
+                               SymbolCandidates, QuerySymbolRange);
   }
 
 private:
   /// Query the database for a given identifier.
-  bool query(StringRef Query, SourceLocation Loc) {
+  bool query(StringRef Query, StringRef ScopedQualifiers, tooling::Range Range) {
     assert(!Query.empty() && "Empty query!");
 
-    // Skip other identifers once we have discovered an identfier successfully.
-    if (!SymbolQueryResults.empty())
+    // Skip other identifiers once we have discovered an identfier successfully.
+    if (!MatchedSymbols.empty())
       return false;
 
     DEBUG(llvm::dbgs() << "Looking up '" << Query << "' at ");
-    DEBUG(Loc.print(llvm::dbgs(), getCompilerInstance().getSourceManager()));
+    DEBUG(getCompilerInstance()
+              .getSourceManager()
+              .getLocForStartOfFile(
+                  getCompilerInstance().getSourceManager().getMainFileID())
+              .getLocWithOffset(Range.getOffset())
+              .print(llvm::dbgs(), getCompilerInstance().getSourceManager()));
     DEBUG(llvm::dbgs() << " ...");
 
     QuerySymbol = Query.str();
-    SymbolQueryResults = SymbolIndexMgr.search(Query);
-    DEBUG(llvm::dbgs() << SymbolQueryResults.size() << " replies\n");
-    return !SymbolQueryResults.empty();
+    QuerySymbolRange = Range;
+    SymbolScopedQualifiers = ScopedQualifiers;
+
+    // Query the symbol based on C++ name Lookup rules.
+    // Firstly, lookup the identifier with scoped namespace contexts; If fails,
+    // falls back to look up the identifier directly.
+    //
+    // For example:
+    //
+    // namespace a {
+    // b::foo f;
+    // }
+    //
+    // 1. lookup a::b::foo.
+    // 2. lookup b::foo.
+    std::string QueryString = ScopedQualifiers.str() + Query.str();
+    MatchedSymbols = SymbolIndexMgr.search(QueryString);
+    if (MatchedSymbols.empty() && !ScopedQualifiers.empty())
+      MatchedSymbols = SymbolIndexMgr.search(Query);
+    DEBUG(llvm::dbgs() << "Having found " << MatchedSymbols.size()
+                       << " symbols\n");
+    return !MatchedSymbols.empty();
   }
 
   /// The client to use to find cross-references.
   SymbolIndexManager &SymbolIndexMgr;
 
   /// The absolute path to the file being processed.
   std::string Filename;
 
   /// 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;
+  /// The scoped qualifiers of QuerySymbol. It is represented as a sequence of
+  /// names and scope resolution operatiors ::, ending with a scope resolution
+  /// operator (e.g. a::b::). Empty if the symbol is not in a specific scope.
+  std::string SymbolScopedQualifiers;
+
+  /// The replacement range of the first discovered QuerySymbol.
+  tooling::Range QuerySymbolRange;
+
+  /// 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;
 };
 
 } // namespace
 
 IncludeFixerActionFactory::IncludeFixerActionFactory(

include-fixer/IncludeFixerContext.h

 //===-- IncludeFixerContext.h - Include fixer context -----------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_INCLUDE_FIXER_INCLUDEFIXERCONTEXT_H
 #define LLVM_CLANG_TOOLS_EXTRA_INCLUDE_FIXER_INCLUDEFIXERCONTEXT_H
 
+#include "find-all-symbols/SymbolInfo.h"
+#include "clang/Tooling/Core/Replacement.h"
+#include <algorithm>
 #include <string>
 #include <vector>
 
 namespace clang {
 namespace include_fixer {
 
 /// \brief A context for the symbol being queried.
-struct IncludeFixerContext {
+class IncludeFixerContext {
+public:
+  IncludeFixerContext() {}
+  IncludeFixerContext(llvm::StringRef Name, llvm::StringRef ScopeQualifiers,
+                      const std::vector<find_all_symbols::SymbolInfo> Symbols,
+                      tooling::Range Range)
+      : SymbolIdentifier(Name), SymbolScopedQualifiers(ScopeQualifiers),
+        MatchedSymbols(Symbols), SymbolRange(Range) {
+    // Deduplicate headers, so that we don't want to suggest the same header
+    // twice.
+    for (const auto &Symbol : MatchedSymbols)
+      Headers.push_back(Symbol.getFilePath());
+    Headers.erase(std::unique(Headers.begin(), Headers.end(),
+                              [](const std::string &A, const std::string &B) {
+                                return A == B;
+                              }),
+                  Headers.end());
+  }
+
+  /// \brief Create a replacement for adding missing namespace qualifiers to the
+  /// symbol.
+  tooling::Replacement createSymbolReplacement(llvm::StringRef FilePath,
+                                               size_t Idx = 0) {
+    assert(Idx < MatchedSymbols.size());
+    std::string FullyQualifiedSymbolName =
+        MatchedSymbols[Idx].getQualifiedName();
+    auto pos = FullyQualifiedSymbolName.find(SymbolScopedQualifiers);
+    return {FilePath, SymbolRange.getOffset(), SymbolRange.getLength(),
+            FullyQualifiedSymbolName.substr(
+                pos == std::string::npos ? 0 : SymbolScopedQualifiers.size())};
+  }
+
+  /// \brief Get symbol name.
+  llvm::StringRef getSymbolIdentifier() const { return SymbolIdentifier; }
+
+  /// \brief Get replacement range of the symbol.
+  tooling::Range getSymbolRange() const { return SymbolRange; }
+
+  /// \brief Get all matched symbols.
+  const std::vector<find_all_symbols::SymbolInfo> &getMatchedSymbols() const {
+    return MatchedSymbols;
+  }
+
+  /// \brief Get all headers. The headers are sorted in a descending order based
+  /// on the popularity info in SymbolInfo.
+  const std::vector<std::string> &getHeaders() const { return Headers; }
+
+private:
+  friend struct llvm::yaml::MappingTraits<IncludeFixerContext>;
+
   /// \brief The symbol name.
   std::string SymbolIdentifier;
+
+  /// \brief The qualifiers of the scope in which SymbolIdentifier lookup
+  /// occurs. It is represented as a sequence of names and scope resolution
+  /// operatiors ::, ending with a scope resolution operator (e.g. a::b::).
+  /// Empty if SymbolIdentifier is not in a specific scope.
+  std::string SymbolScopedQualifiers;
+
   /// \brief The headers which have SymbolIdentifier definitions.
   std::vector<std::string> Headers;
+
+  /// \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 replacement range of SymbolIdentifier.
+  tooling::Range SymbolRange;
 };
 
 } // namespace include_fixer
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_INCLUDE_FIXER_INCLUDEFIXERCONTEXT_H

include-fixer/SymbolIndexManager.h

 //===-- SymbolIndexManager.h - Managing multiple SymbolIndices --*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_INCLUDE_FIXER_SYMBOLINDEXMANAGER_H
 #define LLVM_CLANG_TOOLS_EXTRA_INCLUDE_FIXER_SYMBOLINDEXMANAGER_H
 
 #include "SymbolIndex.h"
+#include "find-all-symbols/SymbolInfo.h"
 #include "llvm/ADT/StringRef.h"
 
 namespace clang {
 namespace include_fixer {
 
 /// This class provides an interface for finding the header files corresponding
 /// to an indentifier in the source code from multiple symbol databases.
 class SymbolIndexManager {
 public:
   void addSymbolIndex(std::unique_ptr<SymbolIndex> DB) {
     SymbolIndices.push_back(std::move(DB));
   }
 
   /// Search for header files to be included for an identifier.
   /// \param Identifier The identifier being searched for. May or may not be
   ///                   fully qualified.
   /// \returns A list of inclusion candidates, in a format ready for being
   ///          pasted after an #include token.
-  // FIXME: Expose the type name so we can also insert using declarations (or
-  // fix the usage)
   // FIXME: Move mapping from SymbolInfo to headers out of
   // SymbolIndexManager::search and return SymbolInfos instead of header paths.
-  std::vector<std::string> search(llvm::StringRef Identifier) const;
+  std::vector<find_all_symbols::SymbolInfo>
+  search(llvm::StringRef Identifier) const;
 
 private:
   std::vector<std::unique_ptr<SymbolIndex>> SymbolIndices;
 };
 
 } // namespace include_fixer
 } // namespace clang
 
 #endif

include-fixer/SymbolIndexManager.cpp

 
 #define DEBUG_TYPE "include-fixer"
 
 namespace clang {
 namespace include_fixer {
 
 using clang::find_all_symbols::SymbolInfo;
 
-/// Sorts and uniques SymbolInfos based on the popularity info in SymbolInfo.
+/// Sorts SymbolInfos based on the popularity info in SymbolInfo.
 static void rankByPopularity(std::vector<SymbolInfo> &Symbols) {
   // First collect occurrences per header file.
   llvm::DenseMap<llvm::StringRef, unsigned> HeaderPopularity;
   for (const SymbolInfo &Symbol : Symbols) {
     unsigned &Popularity = HeaderPopularity[Symbol.getFilePath()];
     Popularity = std::max(Popularity, Symbol.getNumOccurrences());
   }
 
   // Sort by the gathered popularities. Use file name as a tie breaker so we can
   // deduplicate.
   std::sort(Symbols.begin(), Symbols.end(),
             [&](const SymbolInfo &A, const SymbolInfo &B) {
               auto APop = HeaderPopularity[A.getFilePath()];
               auto BPop = HeaderPopularity[B.getFilePath()];
               if (APop != BPop)
                 return APop > BPop;
               return A.getFilePath() < B.getFilePath();
             });
-
-  // Deduplicate based on the file name. They will have the same popularity and
-  // we don't want to suggest the same header twice.
-  Symbols.erase(std::unique(Symbols.begin(), Symbols.end(),
-                            [](const SymbolInfo &A, const SymbolInfo &B) {
-                              return A.getFilePath() == B.getFilePath();
-                            }),
-                Symbols.end());
 }
 
-std::vector<std::string>
+std::vector<find_all_symbols::SymbolInfo>
 SymbolIndexManager::search(llvm::StringRef Identifier) const {
   // The identifier may be fully qualified, so split it and get all the context
   // names.
   llvm::SmallVector<llvm::StringRef, 8> Names;
   Identifier.split(Names, "::");
 
   bool IsFullyQualified = false;
   if (Identifier.startswith("::")) {
for (const auto &Symbol : Symbols) {
         }
       }
     }
     Names.pop_back();
     TookPrefix = true;
   }
 
   rankByPopularity(MatchedSymbols);
-  std::vector<std::string> Results;
-  for (const auto &Symbol : MatchedSymbols) {
-    // 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 + "\"");
-  }
-
-  return Results;
+  return MatchedSymbols;
 }
 
 } // namespace include_fixer
 } // namespace clang

include-fixer/find-all-symbols/SymbolInfo.h

public:
 
   SymbolInfo(llvm::StringRef Name, SymbolKind Type, llvm::StringRef FilePath,
              int LineNumber, const std::vector<Context> &Contexts,
              unsigned NumOccurrences = 0);
 
   /// \brief Get symbol name.
   llvm::StringRef getName() const { return Name; }
 
+  /// \brief Get the fully-qualified symbol name.
+  std::string getQualifiedName() const;
+
   /// \brief Get symbol type.
   SymbolKind getSymbolKind() const { return Type; }
 
   /// \brief Get a relative file path where symbol comes from.
   llvm::StringRef getFilePath() const { return FilePath; }
 
   /// \brief Get symbol contexts.
   const std::vector<SymbolInfo::Context> &getContexts() const {

include-fixer/find-all-symbols/SymbolInfo.cpp

 }
 
 bool SymbolInfo::operator<(const SymbolInfo &Symbol) const {
   return std::tie(Name, Type, FilePath, LineNumber, Contexts) <
          std::tie(Symbol.Name, Symbol.Type, Symbol.FilePath, Symbol.LineNumber,
                   Symbol.Contexts);
 }
 
+std::string SymbolInfo::getQualifiedName() const {
+  std::string QualifiedName = Name;
+  for (const auto &Context : Contexts) {
+    if (Context.first == ContextType::EnumDecl)
+      continue;
+    QualifiedName = Context.second + "::" + QualifiedName;
+  }
+  return QualifiedName;
+}
+
 bool WriteSymbolInfosToStream(llvm::raw_ostream &OS,
                               const std::set<SymbolInfo> &Symbols) {
   llvm::yaml::Output yout(OS);
   for (auto Symbol : Symbols)
     yout << Symbol;
   return true;
 }
 

include-fixer/tool/ClangIncludeFixer.cpp

 #include "IncludeFixer.h"
 #include "IncludeFixerContext.h"
 #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/Core/Replacement.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;
 
cl::opt<bool> OutputHeaders(
     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. Support YAML/JSON format:\n"
              "  -insert-header=\"{SymbolIdentifier: foo,\n"
              "                   Headers: ['\\\"foo_a.h\\\"']}\""),
     cl::init(""), cl::cat(IncludeFixerCategory));
 
case yaml: {
     break;
   }
   }
   return SymbolIndexMgr;
 }
 
 void writeToJson(llvm::raw_ostream &OS, const IncludeFixerContext& Context) {
   OS << "{\n"
-        "  \"SymbolIdentifier\": \"" << Context.SymbolIdentifier << "\",\n"
+        "  \"SymbolIdentifier\": \"" << Context.getSymbolIdentifier() << "\",\n"
         "  \"Headers\": [ ";
-  for (const auto &Header : Context.Headers) {
+  for (const auto &Header : Context.getHeaders()) {
     OS << " \"" << llvm::yaml::escape(Header) << "\"";
-    if (Header != Context.Headers.back())
+    if (Header != Context.getHeaders().back())
       OS << ", ";
   }
   OS << " ]\n"
         "}\n";
 }
 
 int includeFixerMain(int argc, const char **argv) {
   tooling::CommonOptionsParser options(argc, argv, IncludeFixerCategory);
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) {
+    if (Context.getHeaders().size() != 1) {
       errs() << "Expect exactly one inserted header.\n";
       return 1;
     }
 
     tooling::Replacements Replacements =
         clang::include_fixer::createInsertHeaderReplacements(
-            Code->getBuffer(), FilePath, Context.Headers[0], InsertStyle);
+            Code->getBuffer(), FilePath, Context.getHeaders().front(),
+            InsertStyle);
     std::string ChangedCode =
         tooling::applyAllReplacements(Code->getBuffer(), Replacements);
     llvm::outs() << ChangedCode;
     return 0;
   }
 
   // Set up data source.
   std::unique_ptr<include_fixer::SymbolIndexManager> SymbolIndexMgr =
if (tool.run(&Factory) != 0) {
     return 1;
   }
 
   if (OutputHeaders) {
     writeToJson(llvm::outs(), Context);
     return 0;
   }
 
-  if (Context.Headers.empty())
+  if (Context.getMatchedSymbols().empty())
     return 0;
 
   auto Buffer = llvm::MemoryBuffer::getFile(FilePath);
   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(),
-          InsertStyle);
+          /*Code=*/Buffer.get()->getBuffer(), FilePath,
+          Context.getHeaders().front(), InsertStyle);
 
   if (!Quiet)
-    llvm::errs() << "Added #include" << Context.Headers.front();
+    llvm::errs() << "Added #include" << Context.getHeaders().front();
+
+  // Add missing namespace qualifiers to the unidentified symbol.
+  if (Context.getSymbolRange().getLength() > 0)
+    Replacements.insert(Context.createSymbolReplacement(FilePath, 0));
 
   // 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);
 

unittests/include-fixer/IncludeFixerTest.cpp

static bool runOnCode(tooling::ToolAction *ToolAction, StringRef Code,
   InMemoryFileSystem->addFile("dir/otherdir/qux.h", 0,
                               llvm::MemoryBuffer::getMemBuffer("\n"));
   InMemoryFileSystem->addFile("header.h", 0,
                               llvm::MemoryBuffer::getMemBuffer("bar b;"));
   return Invocation.run();
 }
 
 static std::string runIncludeFixer(
-    StringRef Code,
+    StringRef Code, bool FixNamespaceQualifiers = false,
     const std::vector<std::string> &ExtraArgs = std::vector<std::string>()) {
   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"},
static std::string runIncludeFixer(
   };
   auto SymbolIndexMgr = llvm::make_unique<include_fixer::SymbolIndexManager>();
   SymbolIndexMgr->addSymbolIndex(
       llvm::make_unique<include_fixer::InMemorySymbolIndex>(Symbols));
 
   IncludeFixerContext FixerContext;
   IncludeFixerActionFactory Factory(*SymbolIndexMgr, FixerContext, "llvm");
 
-  runOnCode(&Factory, Code, "input.cc", ExtraArgs);
-  if (FixerContext.Headers.empty())
+  std::string FakeFileName = "input.cc";
+  runOnCode(&Factory, Code, FakeFileName, ExtraArgs);
+  if (FixerContext.getMatchedSymbols().empty())
     return Code;
   tooling::Replacements Replacements =
       clang::include_fixer::createInsertHeaderReplacements(
-          Code, "input.cc", FixerContext.Headers.front());
+          Code, FakeFileName, FixerContext.getHeaders().front());
   clang::RewriterTestContext Context;
-  clang::FileID ID = Context.createInMemoryFile("input.cc", Code);
+  clang::FileID ID = Context.createInMemoryFile(FakeFileName, Code);
+  if (FixNamespaceQualifiers && FixerContext.getSymbolRange().getLength() > 0)
+    Replacements.insert(FixerContext.createSymbolReplacement(FakeFileName, 0));
   clang::tooling::applyAllReplacements(Replacements, Context.Rewrite);
   return Context.getRewrittenText(ID);
 }
 
 TEST(IncludeFixer, Typo) {
   EXPECT_EQ("#include <string>\nstd::string foo;\n",
             runIncludeFixer("std::string foo;\n"));
 
EXPECT_EQ(
       "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\"\na::b::foo bar;\n",
-            runIncludeFixer("a::b::foo bar;\n", IncludePath));
+            runIncludeFixer("a::b::foo bar;\n",
+                            /*FixNamespaceQualifiers=*/true, IncludePath));
 
   IncludePath = {"-isystemdir"};
   EXPECT_EQ("#include <otherdir/qux.h>\na::b::foo bar;\n",
-            runIncludeFixer("a::b::foo bar;\n", IncludePath));
+            runIncludeFixer("a::b::foo bar;\n",
+                            /*FixNamespaceQualifiers=*/true, IncludePath));
 
   IncludePath = {"-iquotedir"};
   EXPECT_EQ("#include \"otherdir/qux.h\"\na::b::foo bar;\n",
-            runIncludeFixer("a::b::foo bar;\n", IncludePath));
+            runIncludeFixer("a::b::foo bar;\n",
+                            /*FixNamespaceQualifiers=*/true, IncludePath));
 
   IncludePath = {"-Idir", "-Idir/otherdir"};
   EXPECT_EQ("#include \"qux.h\"\na::b::foo bar;\n",
-            runIncludeFixer("a::b::foo bar;\n", IncludePath));
+            runIncludeFixer("a::b::foo bar;\n",
+                            /*FixNamespaceQualifiers=*/true, IncludePath));
 }
 
 TEST(IncludeFixer, NestedName) {
   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.
TEST(IncludeFixer, InsertAndSortSingleHeader) {
   EXPECT_EQ(Expected, runIncludeFixer(Code));
 }
 
 TEST(IncludeFixer, DoNotDeleteMatchedSymbol) {
   EXPECT_EQ("#include \"Vector.h\"\na::Vector v;",
             runIncludeFixer("a::Vector v;"));
 }
 
+TEST(IncludeFixer, FixNamespaceQualifiers) {
+  EXPECT_EQ("#include \"bar.h\"\na::b::bar b;\n",
+            runIncludeFixer("b::bar b;\n", /*FixNamespaceQualifiers=*/true));
+  EXPECT_EQ("#include \"bar.h\"\na::b::bar b;\n",
+            runIncludeFixer("a::b::bar b;\n", /*FixNamespaceQualifiers=*/true));
+  EXPECT_EQ("#include \"bar.h\"\na::b::bar b;\n",
+            runIncludeFixer("bar b;\n", /*FixNamespaceQualifiers=*/true));
+  EXPECT_EQ("#include \"bar.h\"\nnamespace a {\nb::bar b;\n}\n",
+            runIncludeFixer("namespace a {\nb::bar b;\n}\n",
+                            /*FixNamespaceQualifiers==*/true));
+  EXPECT_EQ("#include \"bar.h\"\nnamespace a {\nb::bar b;\n}\n",
+            runIncludeFixer("namespace a {\nbar b;\n}\n",
+                            /*FixNamespaceQualifiers==*/true));
+  EXPECT_EQ("#include \"bar.h\"\nnamespace a {\nnamespace b{\nbar b;\n}\n}\n",
+            runIncludeFixer("namespace a {\nnamespace b{\nbar b;\n}\n}\n",
+                            /*FixNamespaceQualifiers==*/true));
+  EXPECT_EQ("c::b::bar b;\n",
+            runIncludeFixer("c::b::bar b;\n", /*FixNamespaceQualifiers=*/true));
+  EXPECT_EQ("#include \"bar.h\"\nnamespace c {\na::b::bar b;\n}\n",
+            runIncludeFixer("namespace c {\nbar b;\n}\n",
+                            /*FixNamespaceQualifiers=*/true));
+
+  EXPECT_EQ(
+      "#include \"color.h\"\nint test = a::b::Green;\n",
+      runIncludeFixer("int test = Green;\n", /*FixNamespaceQualifiers=*/true));
+  EXPECT_EQ("#include \"color.h\"\nnamespace d {\nint test = a::b::Green;\n}\n",
+            runIncludeFixer("namespace d {\nint test = Green;\n}\n",
+                            /*FixNamespaceQualifiers=*/true));
+  EXPECT_EQ("#include \"color.h\"\nnamespace a {\nint test = b::Green;\n}\n",
+            runIncludeFixer("namespace a {\nint test = Green;\n}\n",
+                            /*FixNamespaceQualifiers=*/true));
+
+  // FIXME: Fix-namespace should not fix the global qualified identifier.
+  EXPECT_EQ(
+      "#include \"bar.h\"\na::b::bar b;\n",
+      runIncludeFixer("::a::b::bar b;\n", /*FixNamespaceQualifiers=*/true));
+}
+
 } // namespace
 } // namespace include_fixer
 } // namespace clang