Index: clang-tools-extra/trunk/include-fixer/IncludeFixer.cpp =================================================================== --- clang-tools-extra/trunk/include-fixer/IncludeFixer.cpp +++ clang-tools-extra/trunk/include-fixer/IncludeFixer.cpp @@ -70,7 +70,11 @@ 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; } @@ -152,32 +156,31 @@ /// If we have a scope specification, use that to get more precise results. std::string QueryString; + tooling::Range SymbolRange; + const auto &SM = getCompilerInstance().getSourceManager(); + auto CreateToolingRange = [&QueryString, &SM](SourceLocation BeginLoc) { + return tooling::Range(SM.getDecomposedLoc(BeginLoc).second, + QueryString.size()); + }; if (SS && SS->getRange().isValid()) { auto Range = CharSourceRange::getTokenRange(SS->getRange().getBegin(), Typo.getLoc()); QueryString = ExtendNestedNameSpecifier(Range); + SymbolRange = CreateToolingRange(Range.getBegin()); } else if (Typo.getName().isIdentifier() && !Typo.getLoc().isMacroID()) { auto Range = CharSourceRange::getTokenRange(Typo.getBeginLoc(), Typo.getEndLoc()); QueryString = ExtendNestedNameSpecifier(Range); + SymbolRange = CreateToolingRange(Range.getBegin()); } else { QueryString = Typo.getAsString(); + SymbolRange = CreateToolingRange(Typo.getLoc()); } - // 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 @@ -215,32 +218,63 @@ 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, 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 that 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. @@ -252,9 +286,18 @@ /// 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 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 MatchedSymbols; /// Whether we should use the smallest possible include path. bool MinimizeIncludePaths = true; Index: clang-tools-extra/trunk/include-fixer/IncludeFixerContext.h =================================================================== --- clang-tools-extra/trunk/include-fixer/IncludeFixerContext.h +++ clang-tools-extra/trunk/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,89 @@ 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 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 QualifiedName = MatchedSymbols[Idx].getQualifiedName(); + // For nested classes, the qualified name constructed from database misses + // some stripped qualifiers, because when we search a symbol in database, + // we strip qualifiers from the end until we find a result. So append the + // missing stripped qualifiers here. + // + // Get stripped qualifiers. + llvm::SmallVector SymbolQualifiers; + getSymbolIdentifier().split(SymbolQualifiers, "::"); + std::string StrippedQualifiers; + while (!SymbolQualifiers.empty() && + !llvm::StringRef(QualifiedName).endswith(SymbolQualifiers.back())) { + StrippedQualifiers= "::" + SymbolQualifiers.back().str(); + SymbolQualifiers.pop_back(); + } + // Append the missing stripped qualifiers. + std::string FullyQualifiedName = QualifiedName + StrippedQualifiers; + auto pos = FullyQualifiedName.find(SymbolScopedQualifiers); + return {FilePath, SymbolRange.getOffset(), SymbolRange.getLength(), + FullyQualifiedName.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 &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 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 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; + + /// \brief The replacement range of SymbolIdentifier. + tooling::Range SymbolRange; }; } // namespace include_fixer Index: clang-tools-extra/trunk/include-fixer/SymbolIndexManager.h =================================================================== --- clang-tools-extra/trunk/include-fixer/SymbolIndexManager.h +++ clang-tools-extra/trunk/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: clang-tools-extra/trunk/include-fixer/SymbolIndexManager.cpp =================================================================== --- clang-tools-extra/trunk/include-fixer/SymbolIndexManager.cpp +++ clang-tools-extra/trunk/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: clang-tools-extra/trunk/include-fixer/find-all-symbols/SymbolInfo.h =================================================================== --- clang-tools-extra/trunk/include-fixer/find-all-symbols/SymbolInfo.h +++ clang-tools-extra/trunk/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 getQualifiedName() const; + /// \brief Get symbol type. SymbolKind getSymbolKind() const { return Type; } Index: clang-tools-extra/trunk/include-fixer/find-all-symbols/SymbolInfo.cpp =================================================================== --- clang-tools-extra/trunk/include-fixer/find-all-symbols/SymbolInfo.cpp +++ clang-tools-extra/trunk/include-fixer/find-all-symbols/SymbolInfo.cpp @@ -90,6 +90,16 @@ 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 &Symbols) { llvm::yaml::Output yout(OS); Index: clang-tools-extra/trunk/include-fixer/tool/ClangIncludeFixer.cpp =================================================================== --- clang-tools-extra/trunk/include-fixer/tool/ClangIncludeFixer.cpp +++ clang-tools-extra/trunk/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" @@ -154,11 +155,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 +204,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 +241,7 @@ return 0; } - if (Context.Headers.empty()) + if (Context.getMatchedSymbols().empty()) return 0; auto Buffer = llvm::MemoryBuffer::getFile(FilePath); @@ -248,14 +250,17 @@ 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 DiagOpts(new DiagnosticOptions); Index: clang-tools-extra/trunk/unittests/include-fixer/IncludeFixerTest.cpp =================================================================== --- clang-tools-extra/trunk/unittests/include-fixer/IncludeFixerTest.cpp +++ clang-tools-extra/trunk/unittests/include-fixer/IncludeFixerTest.cpp @@ -82,14 +82,17 @@ 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 (FixerContext.getSymbolRange().getLength() > 0) + Replacements.insert(FixerContext.createSymbolReplacement(FakeFileName, 0)); clang::tooling::applyAllReplacements(Replacements, Context.Rewrite); return Context.getRewrittenText(ID); } @@ -113,15 +116,9 @@ "#include \"foo.h\"\n#include \nstd::string::size_type foo;\n", runIncludeFixer("#include \"foo.h\"\nstd::string::size_type foo;\n")); - // string without "std::" can also be fixed since fixed db results go through - // SymbolIndexManager, and SymbolIndexManager matches unqualified identifiers - // too. - EXPECT_EQ("#include \nstring foo;\n", + EXPECT_EQ("#include \nstd::string foo;\n", runIncludeFixer("string foo;\n")); - // Fully qualified name. - EXPECT_EQ("#include \n::std::string foo;\n", - runIncludeFixer("::std::string foo;\n")); // Should not match std::string. EXPECT_EQ("::string foo;\n", runIncludeFixer("::string foo;\n")); } @@ -129,7 +126,7 @@ TEST(IncludeFixer, IncompleteType) { EXPECT_EQ( "#include \"foo.h\"\n#include \n" - "namespace std {\nclass string;\n}\nstring foo;\n", + "namespace std {\nclass string;\n}\nstd::string foo;\n", runIncludeFixer("#include \"foo.h\"\n" "namespace std {\nclass string;\n}\nstring foo;\n")); } @@ -186,7 +183,7 @@ runIncludeFixer("namespace A { c::b::bar b; }\n")); // FIXME: The header should not be added here. Remove this after we support // full match. - EXPECT_EQ("#include \"bar.h\"\nnamespace A {\nb::bar b;\n}", + EXPECT_EQ("#include \"bar.h\"\nnamespace A {\na::b::bar b;\n}", runIncludeFixer("namespace A {\nb::bar b;\n}")); } @@ -221,6 +218,44 @@ runIncludeFixer("a::Vector v;")); } +TEST(IncludeFixer, FixNamespaceQualifiers) { + EXPECT_EQ("#include \"bar.h\"\na::b::bar b;\n", + runIncludeFixer("b::bar b;\n")); + EXPECT_EQ("#include \"bar.h\"\na::b::bar b;\n", + runIncludeFixer("a::b::bar b;\n")); + EXPECT_EQ("#include \"bar.h\"\na::b::bar b;\n", + runIncludeFixer("bar b;\n")); + EXPECT_EQ("#include \"bar.h\"\nnamespace a {\nb::bar b;\n}\n", + runIncludeFixer("namespace a {\nb::bar b;\n}\n")); + EXPECT_EQ("#include \"bar.h\"\nnamespace a {\nb::bar b;\n}\n", + runIncludeFixer("namespace a {\nbar b;\n}\n")); + EXPECT_EQ("#include \"bar.h\"\nnamespace a {\nnamespace b{\nbar b;\n}\n}\n", + runIncludeFixer("namespace a {\nnamespace b{\nbar b;\n}\n}\n")); + EXPECT_EQ("c::b::bar b;\n", + runIncludeFixer("c::b::bar b;\n")); + EXPECT_EQ("#include \"bar.h\"\nnamespace c {\na::b::bar b;\n}\n", + runIncludeFixer("namespace c {\nbar b;\n}\n")); + + // Test nested classes. + EXPECT_EQ("#include \"bar.h\"\nnamespace c {\na::b::bar::t b;\n}\n", + runIncludeFixer("namespace c {\nbar::t b;\n}\n")); + EXPECT_EQ("#include \"bar.h\"\nnamespace a {\nb::bar::t b;\n}\n", + runIncludeFixer("namespace a {\nbar::t b;\n}\n")); + + EXPECT_EQ( + "#include \"color.h\"\nint test = a::b::Green;\n", + runIncludeFixer("int test = Green;\n")); + EXPECT_EQ("#include \"color.h\"\nnamespace d {\nint test = a::b::Green;\n}\n", + runIncludeFixer("namespace d {\nint test = Green;\n}\n")); + EXPECT_EQ("#include \"color.h\"\nnamespace a {\nint test = b::Green;\n}\n", + runIncludeFixer("namespace a {\nint test = Green;\n}\n")); + + // 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")); +} + } // namespace } // namespace include_fixer } // namespace clang