Index: include-fixer/IncludeFixer.cpp =================================================================== --- include-fixer/IncludeFixer.cpp +++ include-fixer/IncludeFixer.cpp @@ -70,7 +70,11 @@ return false; clang::ASTContext &context = getCompilerInstance().getASTContext(); - query(T.getUnqualifiedType().getAsString(context.getPrintingPolicy()), Loc); + llvm::StringRef QueryString = + T.getUnqualifiedType().getAsString(context.getPrintingPolicy()); + DEBUG(llvm::dbgs() << "Query missing complete type '" << QueryString + << "'"); + query(QueryString, tooling::Range()); return false; } @@ -152,18 +156,26 @@ /// 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 @@ -176,8 +188,8 @@ // // 1. lookup a::b::foo. // 2. lookup b::foo. - if (!query(TypoScopeString + QueryString, Typo.getLoc())) - query(QueryString, Typo.getLoc()); + if (!query(TypoScopeString + QueryString, SymbolRange)) + query(QueryString, 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 @@ -215,32 +227,45 @@ 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 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, SymbolCandidates, QuerySymbolRange); } private: /// Query the database for a given identifier. - bool query(StringRef Query, SourceLocation Loc) { + bool query(StringRef Query, 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; + MatchedSymbols = SymbolIndexMgr.search(Query); + DEBUG(llvm::dbgs() << "Having found " << MatchedSymbols.size() + << " symbols\n"); + return !MatchedSymbols.empty(); } /// The client to use to find cross-references. @@ -252,9 +277,13 @@ /// 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 SymbolQueryResults; + /// The repacement 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 MatchedSymbols; /// Whether we should use the smallest possible include path. bool MinimizeIncludePaths = true; Index: include-fixer/IncludeFixerContext.h =================================================================== --- include-fixer/IncludeFixerContext.h +++ include-fixer/IncludeFixerContext.h @@ -10,6 +10,9 @@ #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 #include #include @@ -17,11 +20,54 @@ namespace include_fixer { /// \brief A context for the symbol being queried. -struct IncludeFixerContext { +class IncludeFixerContext { +public: + IncludeFixerContext() {} + IncludeFixerContext(llvm::StringRef Name, + const std::vector Symbols, + tooling::Range Range) + : SymbolIdentifier(Name), 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 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 &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 &getHeaders() const { return Headers; } + +private: + friend struct llvm::yaml::MappingTraits; + /// \brief The symbol name. std::string SymbolIdentifier; + /// \brief The headers which have SymbolIdentifier definitions. std::vector 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 MatchedSymbols; + + /// \breif The replacement range of SymbolIdentifier. + tooling::Range SymbolRange; }; } // namespace include_fixer Index: include-fixer/SymbolIndexManager.h =================================================================== --- include-fixer/SymbolIndexManager.h +++ include-fixer/SymbolIndexManager.h @@ -11,6 +11,7 @@ #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 { @@ -29,11 +30,10 @@ /// 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 search(llvm::StringRef Identifier) const; + std::vector + search(llvm::StringRef Identifier) const; private: std::vector> SymbolIndices; Index: include-fixer/SymbolIndexManager.cpp =================================================================== --- include-fixer/SymbolIndexManager.cpp +++ include-fixer/SymbolIndexManager.cpp @@ -20,7 +20,7 @@ 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 &Symbols) { // First collect occurrences per header file. llvm::DenseMap HeaderPopularity; @@ -39,17 +39,9 @@ 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::vector SymbolIndexManager::search(llvm::StringRef Identifier) const { // The identifier may be fully qualified, so split it and get all the context // names. @@ -127,20 +119,7 @@ } rankByPopularity(MatchedSymbols); - std::vector 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 Index: include-fixer/find-all-symbols/SymbolInfo.h =================================================================== --- include-fixer/find-all-symbols/SymbolInfo.h +++ include-fixer/find-all-symbols/SymbolInfo.h @@ -57,6 +57,9 @@ /// \brief Get symbol name. llvm::StringRef getName() const { return Name; } + /// \brief Get the fully-qualified symbol name. + std::string getFullyQualifiedName() const; + /// \brief Get symbol type. SymbolKind getSymbolKind() const { return Type; } Index: include-fixer/find-all-symbols/SymbolInfo.cpp =================================================================== --- include-fixer/find-all-symbols/SymbolInfo.cpp +++ include-fixer/find-all-symbols/SymbolInfo.cpp @@ -90,6 +90,16 @@ Symbol.Contexts); } +std::string SymbolInfo::getFullyQualifiedName() const { + std::string FullyQuanlifiedName = Name; + for (const auto &Context : Contexts) { + if (Context.first == ContextType::EnumDecl) + continue; + FullyQuanlifiedName = Context.second + "::" + FullyQuanlifiedName; + } + return FullyQuanlifiedName; +} + bool WriteSymbolInfosToStream(llvm::raw_ostream &OS, const std::set &Symbols) { llvm::yaml::Output yout(OS); Index: include-fixer/tool/ClangIncludeFixer.cpp =================================================================== --- include-fixer/tool/ClangIncludeFixer.cpp +++ include-fixer/tool/ClangIncludeFixer.cpp @@ -16,6 +16,7 @@ #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" @@ -84,6 +85,11 @@ " }"), cl::init(false), cl::cat(IncludeFixerCategory)); +cl::opt FixNamespace("fix-namespace", + cl::desc("Add missing namespace prefix to the " + "unidentified symbol automatically."), + cl::init(false), cl::cat(IncludeFixerCategory)); + cl::opt InsertHeader( "insert-header", cl::desc("Insert a specific header. This should run with STDIN mode.\n" @@ -154,11 +160,11 @@ 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" @@ -203,14 +209,15 @@ 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; @@ -239,7 +246,7 @@ return 0; } - if (Context.Headers.empty()) + if (Context.getMatchedSymbols().empty()) return 0; auto Buffer = llvm::MemoryBuffer::getFile(FilePath); @@ -248,14 +255,23 @@ 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(); + + if (FixNamespace) { + const auto& Range = Context.getSymbolRange(); + if (Range.getLength() > 0) { + std::string Name = + Context.getMatchedSymbols().front().getFullyQualifiedName(); + Replacements.emplace(FilePath, Range.getOffset(), Range.getLength(), + Name); + } + } // Set up a new source manager for applying the resulting replacements. IntrusiveRefCntPtr DiagOpts(new DiagnosticOptions); Index: unittests/include-fixer/IncludeFixerTest.cpp =================================================================== --- unittests/include-fixer/IncludeFixerTest.cpp +++ unittests/include-fixer/IncludeFixerTest.cpp @@ -50,7 +50,7 @@ } static std::string runIncludeFixer( - StringRef Code, + StringRef Code, bool FixNamespace = false, const std::vector &ExtraArgs = std::vector()) { std::vector Symbols = { SymbolInfo("string", SymbolInfo::SymbolKind::Class, "", 1, @@ -82,14 +82,21 @@ 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 (FixNamespace && FixerContext.getSymbolRange().getLength() > 0) { + Replacements.emplace( + FakeFileName, FixerContext.getSymbolRange().getOffset(), + FixerContext.getSymbolRange().getLength(), + FixerContext.getMatchedSymbols().front().getFullyQualifiedName()); + } clang::tooling::applyAllReplacements(Replacements, Context.Rewrite); return Context.getRewrittenText(ID); } @@ -136,20 +143,24 @@ TEST(IncludeFixer, MinimizeInclude) { std::vector IncludePath = {"-Idir/"}; - EXPECT_EQ("#include \"otherdir/qux.h\"\na::b::foo bar;\n", - runIncludeFixer("a::b::foo bar;\n", IncludePath)); + EXPECT_EQ( + "#include \"otherdir/qux.h\"\na::b::foo bar;\n", + runIncludeFixer("a::b::foo bar;\n", /*FixNamespace=*/false, IncludePath)); IncludePath = {"-isystemdir"}; - EXPECT_EQ("#include \na::b::foo bar;\n", - runIncludeFixer("a::b::foo bar;\n", IncludePath)); + EXPECT_EQ( + "#include \na::b::foo bar;\n", + runIncludeFixer("a::b::foo bar;\n", /*FixNamespace=*/false, IncludePath)); IncludePath = {"-iquotedir"}; - EXPECT_EQ("#include \"otherdir/qux.h\"\na::b::foo bar;\n", - runIncludeFixer("a::b::foo bar;\n", IncludePath)); + EXPECT_EQ( + "#include \"otherdir/qux.h\"\na::b::foo bar;\n", + runIncludeFixer("a::b::foo bar;\n", /*FixNamespace=*/false, IncludePath)); IncludePath = {"-Idir", "-Idir/otherdir"}; - EXPECT_EQ("#include \"qux.h\"\na::b::foo bar;\n", - runIncludeFixer("a::b::foo bar;\n", IncludePath)); + EXPECT_EQ( + "#include \"qux.h\"\na::b::foo bar;\n", + runIncludeFixer("a::b::foo bar;\n", /*FixNamespace=*/false, IncludePath)); } TEST(IncludeFixer, NestedName) { @@ -221,6 +232,31 @@ runIncludeFixer("a::Vector v;")); } +TEST(IncludeFixer, FixNamespace) { + EXPECT_EQ("#include \"bar.h\"\na::b::bar b;\n", + runIncludeFixer("b::bar b;\n", /*FixNamespace=*/true)); + + EXPECT_EQ("#include \"bar.h\"\na::b::bar b;\n", + runIncludeFixer("a::b::bar b;\n", /*FixNamespace=*/true)); + + EXPECT_EQ( + "c::b::bar b;\n", + runIncludeFixer("c::b::bar b;\n", /*FixNamespace=*/true)); + + EXPECT_EQ("#include \"color.h\"\nint test = a::b::Green;\n", + runIncludeFixer("int test = Green;\n", /*FixNamespace=*/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", /*FixNamespace=*/true)); + + // FIXME: Fix-namespace should not add the missing namespace prefix to the + // unidentified symbol which is already in that namespace. + EXPECT_EQ( + "#include \"bar.h\"\nnamespace a {\na::b::bar b;\n}\n", + runIncludeFixer("namespace a {\nb::bar b;\n}\n", /*FixNamespace==*/true)); +} + } // namespace } // namespace include_fixer } // namespace clang