Index: clang-tools-extra/trunk/include-fixer/IncludeFixer.h =================================================================== --- clang-tools-extra/trunk/include-fixer/IncludeFixer.h +++ clang-tools-extra/trunk/include-fixer/IncludeFixer.h @@ -60,19 +60,25 @@ std::string FallbackStyle; }; -/// Create replacements, which are generated by clang-format, for the header -/// insertion. +/// Create replacements, which are generated by clang-format, for the +/// missing header and mising qualifiers insertions. The function uses the +/// first header for insertion. /// /// \param Code The source code. /// \param FilePath The source file path. -/// \param Header The header being inserted. +/// \param Context The context which contains all information for creating +/// include-fixer replacements. /// \param Style clang-format style being used. +/// \param AddQualifiers Whether we should add qualifiers to all instances of +/// an unidentified symbol. /// -/// \return Replacements for inserting and sorting headers on success; -/// otherwise, an llvm::Error carrying llvm::StringError is returned. -llvm::Expected createInsertHeaderReplacements( - StringRef Code, StringRef FilePath, StringRef Header, - const clang::format::FormatStyle &Style = clang::format::getLLVMStyle()); +/// \return Formatted replacements for inserting, sorting headers and adding +/// qualifiers on success; otherwise, an llvm::Error carrying llvm::StringError +/// is returned. +llvm::Expected createIncludeFixerReplacements( + StringRef Code, StringRef FilePath, const IncludeFixerContext &Context, + const format::FormatStyle &Style = format::getLLVMStyle(), + bool AddQualifiers = true); } // namespace include_fixer } // namespace clang 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 @@ -227,17 +227,28 @@ 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 == QuerySymbolInfos.front().ScopedQualifiers && + Query == QuerySymbolInfos.front().RawIdentifier) { + QuerySymbolInfos.push_back({Query.str(), ScopedQualifiers, Range}); + } return false; + } DEBUG(llvm::dbgs() << "Looking up '" << Query << "' at "); DEBUG(getCompilerInstance() @@ -248,7 +259,7 @@ .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; @@ -274,8 +285,8 @@ /// 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 QuerySymbolInfos; /// All symbol candidates which match QuerySymbol. We only include the first /// discovered identifier to avoid getting caught in results from error @@ -332,13 +343,13 @@ return !Compiler.getDiagnostics().hasFatalErrorOccurred(); } -llvm::Expected -createInsertHeaderReplacements(StringRef Code, StringRef FilePath, - StringRef Header, - const clang::format::FormatStyle &Style) { - if (Header.empty()) +llvm::Expected createIncludeFixerReplacements( + StringRef Code, StringRef FilePath, const IncludeFixerContext &Context, + const clang::format::FormatStyle &Style, bool AddQualifiers) { + if (Context.getHeaderInfos().empty()) return tooling::Replacements(); - std::string IncludeName = "#include " + Header.str() + "\n"; + std::string IncludeName = + "#include " + Context.getHeaderInfos().front().Header + "\n"; // Create replacements for the new header. clang::tooling::Replacements Insertions = { tooling::Replacement(FilePath, UINT_MAX, 0, IncludeName)}; @@ -346,6 +357,14 @@ auto CleanReplaces = cleanupAroundReplacements(Code, Insertions, Style); if (!CleanReplaces) return CleanReplaces; + + if (AddQualifiers) { + for (const auto &Info : Context.getQuerySymbolInfos()) { + CleanReplaces->insert({FilePath, Info.Range.getOffset(), + Info.Range.getLength(), + Context.getHeaderInfos().front().QualifiedName}); + } + } return formatReplacements(Code, *CleanReplaces, Style); } 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 @@ -46,31 +46,39 @@ }; IncludeFixerContext() = default; - IncludeFixerContext(const QuerySymbolInfo &QuerySymbol, + IncludeFixerContext(std::vector QuerySymbols, std::vector 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 &getHeaderInfos() const { return HeaderInfos; } + /// \brief Get information of symbols being querid. + const std::vector &getQuerySymbolInfos() const { + return QuerySymbolInfos; + } + private: friend struct llvm::yaml::MappingTraits; + /// \brief All instances of an unidentified symbol being queried. + std::vector 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 MatchedSymbols; /// \brief The header information. std::vector HeaderInfos; - - /// \brief The information of the symbol being queried. - QuerySymbolInfo QuerySymbol; }; } // namespace include_fixer Index: clang-tools-extra/trunk/include-fixer/IncludeFixerContext.cpp =================================================================== --- clang-tools-extra/trunk/include-fixer/IncludeFixerContext.cpp +++ clang-tools-extra/trunk/include-fixer/IncludeFixerContext.cpp @@ -75,14 +75,33 @@ } // anonymous namespace IncludeFixerContext::IncludeFixerContext( - const QuerySymbolInfo &QuerySymbol, + std::vector QuerySymbols, std::vector 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(), 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 @@ -29,6 +29,7 @@ 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 { @@ -70,7 +71,7 @@ template <> struct MappingTraits { static void mapping(IO &IO, IncludeFixerContext &Context) { - IO.mapRequired("QuerySymbolInfo", Context.QuerySymbol); + IO.mapRequired("QuerySymbolInfos", Context.QuerySymbolInfos); IO.mapRequired("HeaderInfos", Context.HeaderInfos); } }; @@ -118,10 +119,10 @@ 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" " }"), @@ -133,10 +134,10 @@ "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)); @@ -202,13 +203,16 @@ 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) { @@ -275,16 +279,7 @@ return 1; } - auto Replacements = clang::include_fixer::createInsertHeaderReplacements( - Code->getBuffer(), FilePath, Context.getHeaderInfos().front().Header, - InsertStyle); - if (!Replacements) { - errs() << "Failed to create header insertion replacement: " - << llvm::toString(Replacements.takeError()) << "\n"; - return 1; - } - - // If a header have multiple symbols, we won't add the missing namespace + // If a header has multiple symbols, we won't add the missing namespace // qualifiers because we don't know which one is exactly used. // // Check whether all elements in HeaderInfos have the same qualified name. @@ -294,10 +289,16 @@ const IncludeFixerContext::HeaderInfo &RHS) { return LHS.QualifiedName == RHS.QualifiedName; }); - if (IsUniqueQualifiedName) - Replacements->insert({FilePath, Context.getSymbolRange().getOffset(), - Context.getSymbolRange().getLength(), - Context.getHeaderInfos().front().QualifiedName}); + + auto Replacements = clang::include_fixer::createIncludeFixerReplacements( + Code->getBuffer(), FilePath, Context, InsertStyle, + /*AddQualifiers=*/IsUniqueQualifiedName); + if (!Replacements) { + errs() << "Failed to create replacements: " + << llvm::toString(Replacements.takeError()) << "\n"; + return 1; + } + auto ChangedCode = tooling::applyAllReplacements(Code->getBuffer(), *Replacements); if (!ChangedCode) { @@ -340,9 +341,8 @@ return 1; } - auto Replacements = clang::include_fixer::createInsertHeaderReplacements( - /*Code=*/Buffer.get()->getBuffer(), FilePath, - Context.getHeaderInfos().front().Header, InsertStyle); + auto Replacements = clang::include_fixer::createIncludeFixerReplacements( + /*Code=*/Buffer.get()->getBuffer(), FilePath, Context, InsertStyle); if (!Replacements) { errs() << "Failed to create header insertion replacement: " << llvm::toString(Replacements.takeError()) << "\n"; @@ -353,11 +353,6 @@ 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(), - Context.getHeaderInfos().front().QualifiedName}); - // Set up a new source manager for applying the resulting replacements. IntrusiveRefCntPtr DiagOpts(new DiagnosticOptions); DiagnosticsEngine Diagnostics(new DiagnosticIDs, &*DiagOpts); Index: clang-tools-extra/trunk/include-fixer/tool/clang-include-fixer.py =================================================================== --- clang-tools-extra/trunk/include-fixer/tool/clang-include-fixer.py +++ clang-tools-extra/trunk/include-fixer/tool/clang-include-fixer.py @@ -127,8 +127,11 @@ 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"] + if not query_symbol_infos: + print "The file is fine, no need to add a header." + return + 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 @@ -141,10 +144,6 @@ 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 @@ -152,7 +151,7 @@ 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 @@ -163,7 +162,7 @@ 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: Index: clang-tools-extra/trunk/test/include-fixer/commandline_options.cpp =================================================================== --- clang-tools-extra/trunk/test/include-fixer/commandline_options.cpp +++ clang-tools-extra/trunk/test/include-fixer/commandline_options.cpp @@ -1,9 +1,9 @@ // 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\"", 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 @@ -89,17 +89,14 @@ runOnCode(&Factory, Code, FakeFileName, ExtraArgs); if (FixerContext.getHeaderInfos().empty()) return Code; - auto Replaces = clang::include_fixer::createInsertHeaderReplacements( - Code, FakeFileName, FixerContext.getHeaderInfos().front().Header); + auto Replaces = clang::include_fixer::createIncludeFixerReplacements( + Code, FakeFileName, FixerContext); EXPECT_TRUE(static_cast(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(), - FixerContext.getHeaderInfos().front().QualifiedName}); clang::tooling::applyAllReplacements(*Replaces, Context.Rewrite); return Context.getRewrittenText(ID); } @@ -211,12 +208,12 @@ std::string Code = "#include \"a.h\"\n" "#include \"foo.h\"\n" "\n" - "namespace a { b::bar b; }"; + "namespace a {\nb::bar b;\n}\n"; std::string Expected = "#include \"a.h\"\n" "#include \"bar.h\"\n" "#include \"foo.h\"\n" "\n" - "namespace a { b::bar b; }"; + "namespace a {\nb::bar b;\n}\n"; EXPECT_EQ(Expected, runIncludeFixer(Code)); } @@ -275,6 +272,69 @@ runIncludeFixer("namespace a {\n::a::b::bar b;\n}\n")); } +TEST(IncludeFixer, FixNamespaceQualifiersForAllInstances) { + const char TestCode[] = R"( +namespace a { +bar b; +int func1() { + bar a; + bar *p = new bar(); + return 0; +} +} // namespace a + +namespace a { +bar func2() { + bar f; + return f; +} +} // namespace a + +// Non-fixed cases: +void f() { + bar b; +} + +namespace a { +namespace c { + bar b; +} // namespace c +} // namespace a +)"; + + const char ExpectedCode[] = R"( +#include "bar.h" +namespace a { +b::bar b; +int func1() { + b::bar a; + b::bar *p = new b::bar(); + return 0; +} +} // namespace a + +namespace a { +b::bar func2() { + b::bar f; + return f; +} +} // namespace a + +// Non-fixed cases: +void f() { + bar b; +} + +namespace a { +namespace c { + bar b; +} // namespace c +} // namespace a +)"; + + EXPECT_EQ(ExpectedCode, runIncludeFixer(TestCode)); +} + } // namespace } // namespace include_fixer } // namespace clang