From 68c34a083d0c5ae36136b2136d7c986442859100 Mon Sep 17 00:00:00 2001 From: Haojian Wu Date: Wed, 13 Jul 2016 16:43:54 +0000 Subject: [PATCH] [include-fixer] Implement adding missing namespace qualifiers in vim integration. Summary: The patch extends include-fixer's "-output-headers", and "-insert-headers" command line options to make it dump more information (e.g. QualifiedSymbol), so that vim-integration can add missing qualifiers. Reviewers: bkramer Subscribers: cfe-commits Differential Revision: http://reviews.llvm.org/D22299 llvm-svn: 275279 --- .../include-fixer/IncludeFixerContext.cpp | 62 ++++++----- .../include-fixer/IncludeFixerContext.h | 29 +++--- .../include-fixer/tool/ClangIncludeFixer.cpp | 115 ++++++++++++++++----- .../include-fixer/tool/clang-include-fixer.py | 30 ++++-- .../test/include-fixer/commandline_options.cpp | 21 ++-- clang-tools-extra/test/include-fixer/ranking.cpp | 7 +- .../unittests/include-fixer/IncludeFixerTest.cpp | 9 +- 7 files changed, 186 insertions(+), 87 deletions(-) diff --git a/clang-tools-extra/include-fixer/IncludeFixerContext.cpp b/clang-tools-extra/include-fixer/IncludeFixerContext.cpp index 33483b4..02b3a0e 100644 --- a/clang-tools-extra/include-fixer/IncludeFixerContext.cpp +++ b/clang-tools-extra/include-fixer/IncludeFixerContext.cpp @@ -13,32 +13,19 @@ namespace clang { namespace include_fixer { -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()); -} +namespace { -tooling::Replacement -IncludeFixerContext::createSymbolReplacement(llvm::StringRef FilePath, - size_t Idx) { - assert(Idx < MatchedSymbols.size()); +std::string createQualifiedNameForReplacement( + llvm::StringRef RawSymbolName, + llvm::StringRef SymbolScopedQualifiers, + const find_all_symbols::SymbolInfo &MatchedSymbol) { // No need to add missing qualifiers if SymbolIndentifer has a global scope // operator "::". - if (getSymbolIdentifier().startswith("::")) - return tooling::Replacement(); - std::string QualifiedName = MatchedSymbols[Idx].getQualifiedName(); + if (RawSymbolName.startswith("::")) + return RawSymbolName; + + std::string QualifiedName = MatchedSymbol.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 @@ -46,7 +33,7 @@ IncludeFixerContext::createSymbolReplacement(llvm::StringRef FilePath, // // Get stripped qualifiers. llvm::SmallVector SymbolQualifiers; - getSymbolIdentifier().split(SymbolQualifiers, "::"); + RawSymbolName.split(SymbolQualifiers, "::"); std::string StrippedQualifiers; while (!SymbolQualifiers.empty() && !llvm::StringRef(QualifiedName).endswith(SymbolQualifiers.back())) { @@ -56,9 +43,30 @@ IncludeFixerContext::createSymbolReplacement(llvm::StringRef FilePath, // 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())}; + return FullyQualifiedName.substr( + pos == std::string::npos ? 0 : SymbolScopedQualifiers.size()); +} + +} // anonymous namespace + +IncludeFixerContext::IncludeFixerContext( + llvm::StringRef Name, llvm::StringRef ScopeQualifiers, + std::vector Symbols, + tooling::Range Range) + : SymbolIdentifier(Name), SymbolScopedQualifiers(ScopeQualifiers), + MatchedSymbols(std::move(Symbols)), SymbolRange(Range) { + for (const auto &Symbol : MatchedSymbols) { + HeaderInfos.push_back({Symbol.getFilePath().str(), + createQualifiedNameForReplacement( + SymbolIdentifier, ScopeQualifiers, Symbol)}); + } + // Deduplicate header infos. + HeaderInfos.erase(std::unique(HeaderInfos.begin(), HeaderInfos.end(), + [](const HeaderInfo &A, const HeaderInfo &B) { + return A.Header == B.Header && + A.QualifiedName == B.QualifiedName; + }), + HeaderInfos.end()); } } // include_fixer diff --git a/clang-tools-extra/include-fixer/IncludeFixerContext.h b/clang-tools-extra/include-fixer/IncludeFixerContext.h index de51866..f5d3e91 100644 --- a/clang-tools-extra/include-fixer/IncludeFixerContext.h +++ b/clang-tools-extra/include-fixer/IncludeFixerContext.h @@ -26,10 +26,13 @@ public: const std::vector Symbols, tooling::Range Range); - /// \brief Create a replacement for adding missing namespace qualifiers to the - /// symbol. - tooling::Replacement createSymbolReplacement(llvm::StringRef FilePath, - size_t Idx = 0); + struct HeaderInfo { + /// \brief The header where QualifiedName comes from. + std::string Header; + /// \brief A symbol name with completed namespace qualifiers which will + /// replace the original symbol. + std::string QualifiedName; + }; /// \brief Get symbol name. llvm::StringRef getSymbolIdentifier() const { return SymbolIdentifier; } @@ -37,19 +40,13 @@ public: /// \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; } + const std::vector &getHeaderInfos() const { return HeaderInfos; } private: friend struct llvm::yaml::MappingTraits; - /// \brief The symbol name. + /// \brief The raw symbol name being queried in database. This name might miss + /// some namespace qualifiers, and will be replaced by a fully qualified one. std::string SymbolIdentifier; /// \brief The qualifiers of the scope in which SymbolIdentifier lookup @@ -58,15 +55,15 @@ private: /// 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; + + /// \brief The header information. + std::vector HeaderInfos; }; } // namespace include_fixer diff --git a/clang-tools-extra/include-fixer/tool/ClangIncludeFixer.cpp b/clang-tools-extra/include-fixer/tool/ClangIncludeFixer.cpp index 98df220..d32a13c 100644 --- a/clang-tools-extra/include-fixer/tool/ClangIncludeFixer.cpp +++ b/clang-tools-extra/include-fixer/tool/ClangIncludeFixer.cpp @@ -27,13 +27,44 @@ using clang::include_fixer::IncludeFixerContext; LLVM_YAML_IS_DOCUMENT_LIST_VECTOR(IncludeFixerContext) LLVM_YAML_IS_FLOW_SEQUENCE_VECTOR(std::string) +LLVM_YAML_IS_FLOW_SEQUENCE_VECTOR(IncludeFixerContext::HeaderInfo) namespace llvm { namespace yaml { + +template <> struct MappingTraits { + struct NormalizedRange { + NormalizedRange(const IO &) : Offset(0), Length(0) {} + + NormalizedRange(const IO &, const tooling::Range &R) + : Offset(R.getOffset()), Length(R.getLength()) {} + + tooling::Range denormalize(const IO &) { + return tooling::Range(Offset, Length); + } + + unsigned Offset; + unsigned Length; + }; + static void mapping(IO &IO, tooling::Range &Info) { + MappingNormalization Keys(IO, Info); + IO.mapRequired("Offset", Keys->Offset); + IO.mapRequired("Length", Keys->Length); + } +}; + +template <> struct MappingTraits { + static void mapping(IO &io, IncludeFixerContext::HeaderInfo &Info) { + io.mapRequired("Header", Info.Header); + io.mapRequired("QualifiedName", Info.QualifiedName); + } +}; + template <> struct MappingTraits { - static void mapping(IO &io, IncludeFixerContext &Context) { - io.mapRequired("SymbolIdentifier", Context.SymbolIdentifier); - io.mapRequired("Headers", Context.Headers); + static void mapping(IO &IO, IncludeFixerContext &Context) { + IO.mapRequired("SymbolIdentifier", Context.SymbolIdentifier); + IO.mapRequired("HeaderInfos", Context.HeaderInfos); + IO.mapRequired("Range", Context.SymbolRange); } }; } // namespace yaml @@ -81,7 +112,9 @@ cl::opt OutputHeaders( "JSON format to stdout:\n" " {\n" " \"SymbolIdentifier\": \"foo\",\n" - " \"Headers\": [\"\\\"foo_a.h\\\"\"]\n" + " \"Range\": {\"Offset\":0, \"Length\": 3},\n" + " \"HeaderInfos\": [ {\"Header\": \"\\\"foo_a.h\\\"\",\n" + " \"QualifiedName\": \"a::foo\"} ]\n" " }"), cl::init(false), cl::cat(IncludeFixerCategory)); @@ -90,8 +123,11 @@ cl::opt InsertHeader( 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\\\"']}\""), + " -insert-header=\"{\n" + " SymbolIdentifier: foo,\n" + " Range: {Offset: 0, Length: 3},\n" + " HeaderInfos: [ {Headers: \"\\\"foo_a.h\\\"\",\n" + " QualifiedName: \"a::foo\"} ]}\""), cl::init(""), cl::cat(IncludeFixerCategory)); cl::opt @@ -155,15 +191,22 @@ createSymbolIndexManager(StringRef FilePath) { void writeToJson(llvm::raw_ostream &OS, const IncludeFixerContext& Context) { OS << "{\n" - " \"SymbolIdentifier\": \"" << Context.getSymbolIdentifier() << "\",\n" - " \"Headers\": [ "; - for (const auto &Header : Context.getHeaders()) { - OS << " \"" << llvm::yaml::escape(Header) << "\""; - if (Header != Context.getHeaders().back()) - OS << ", "; + " \"SymbolIdentifier\": \"" + << Context.getSymbolIdentifier() << "\",\n"; + OS << " \"Range\": {"; + OS << " \"Offset\":" << Context.getSymbolRange().getOffset() << ","; + OS << " \"Length\":" << Context.getSymbolRange().getLength() << " },\n"; + OS << " \"HeaderInfos\": [\n"; + const auto &HeaderInfos = Context.getHeaderInfos(); + for (const auto &Info : HeaderInfos) { + OS << " {\"Header\": \"" << llvm::yaml::escape(Info.Header) << "\",\n" + << " \"QualifiedName\": \"" << Info.QualifiedName << "\"}"; + if (&Info != &HeaderInfos.back()) + OS << ",\n"; } - OS << " ]\n" - "}\n"; + OS << "\n"; + OS << " ]\n"; + OS << "}\n"; } int includeFixerMain(int argc, const char **argv) { @@ -204,18 +247,44 @@ int includeFixerMain(int argc, const char **argv) { IncludeFixerContext Context; yin >> Context; - if (Context.getHeaders().size() != 1) { - errs() << "Expect exactly one inserted header.\n"; + const auto &HeaderInfos = Context.getHeaderInfos(); + assert(!HeaderInfos.empty()); + // We only accept one unique header. + // Check all elements in HeaderInfos have the same header. + bool IsUniqueHeader = std::equal( + HeaderInfos.begin()+1, HeaderInfos.end(), HeaderInfos.begin(), + [](const IncludeFixerContext::HeaderInfo &LHS, + const IncludeFixerContext::HeaderInfo &RHS) { + return LHS.Header == RHS.Header; + }); + if (!IsUniqueHeader) { + errs() << "Expect exactly one unique header.\n"; return 1; } auto Replacements = clang::include_fixer::createInsertHeaderReplacements( - Code->getBuffer(), FilePath, Context.getHeaders().front(), InsertStyle); + 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 + // qualifiers because we don't know which one is exactly used. + // + // Check whether all elements in HeaderInfos have the same qualified name. + bool IsUniqueQualifiedName = std::equal( + HeaderInfos.begin() + 1, HeaderInfos.end(), HeaderInfos.begin(), + [](const IncludeFixerContext::HeaderInfo &LHS, + 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 ChangedCode = tooling::applyAllReplacements(Code->getBuffer(), *Replacements); if (!ChangedCode) { @@ -248,7 +317,7 @@ int includeFixerMain(int argc, const char **argv) { return 0; } - if (Context.getMatchedSymbols().empty()) + if (Context.getHeaderInfos().empty()) return 0; auto Buffer = llvm::MemoryBuffer::getFile(FilePath); @@ -257,10 +326,9 @@ int includeFixerMain(int argc, const char **argv) { return 1; } - // FIXME: Rank the results and pick the best one instead of the first one. auto Replacements = clang::include_fixer::createInsertHeaderReplacements( /*Code=*/Buffer.get()->getBuffer(), FilePath, - Context.getHeaders().front(), InsertStyle); + Context.getHeaderInfos().front().Header, InsertStyle); if (!Replacements) { errs() << "Failed to create header insertion replacement: " << llvm::toString(Replacements.takeError()) << "\n"; @@ -268,11 +336,12 @@ int includeFixerMain(int argc, const char **argv) { } if (!Quiet) - llvm::errs() << "Added #include" << Context.getHeaders().front(); + llvm::errs() << "Added #include" << Context.getHeaderInfos().front().Header; // Add missing namespace qualifiers to the unidentified symbol. - if (Context.getSymbolRange().getLength() > 0) - Replacements->insert(Context.createSymbolReplacement(FilePath, 0)); + 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); diff --git a/clang-tools-extra/include-fixer/tool/clang-include-fixer.py b/clang-tools-extra/include-fixer/tool/clang-include-fixer.py index 49060ee..6ba8a4c 100644 --- a/clang-tools-extra/include-fixer/tool/clang-include-fixer.py +++ b/clang-tools-extra/include-fixer/tool/clang-include-fixer.py @@ -115,30 +115,44 @@ def main(): include_fixer_context = json.loads(stdout) symbol = include_fixer_context["SymbolIdentifier"] - headers = include_fixer_context["Headers"] + # 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 + # not be suggested twice. + unique_headers = [] + seen = set() + for header_info in header_infos: + header = header_info["Header"] + if header not in seen: + seen.add(header) + unique_headers.append(header) if not symbol: print "The file is fine, no need to add a header.\n" return - if not headers: + if not unique_headers: print "Couldn't find a header for {0}.\n".format(symbol) return - # The first line is the symbol name. # If there is only one suggested header, insert it directly. - if len(headers) == 1 or maximum_suggested_headers == 1: + if len(unique_headers) == 1 or maximum_suggested_headers == 1: InsertHeaderToVimBuffer({"SymbolIdentifier": symbol, - "Headers": [headers[0]]}, text) - print "Added #include {0} for {1}.\n".format(headers[0], symbol) + "Range": include_fixer_context["Range"], + "HeaderInfos": header_infos}, text) + print "Added #include {0} for {1}.\n".format(unique_headers[0], symbol) return try: selected = GetUserSelection("choose a header file for {0}.".format(symbol), - headers, maximum_suggested_headers) + unique_headers, maximum_suggested_headers) + selected_header_infos = [ + header for header in header_infos if header["Header"] == selected] + # Insert a selected header. InsertHeaderToVimBuffer({"SymbolIdentifier": symbol, - "Headers": [selected]}, text) + "Range": include_fixer_context["Range"], + "HeaderInfos": selected_header_infos}, text) print "Added #include {0} for {1}.\n".format(selected, symbol) except Exception as error: print >> sys.stderr, error.message diff --git a/clang-tools-extra/test/include-fixer/commandline_options.cpp b/clang-tools-extra/test/include-fixer/commandline_options.cpp index 8f30f95..0abda21 100644 --- a/clang-tools-extra/test/include-fixer/commandline_options.cpp +++ b/clang-tools-extra/test/include-fixer/commandline_options.cpp @@ -1,11 +1,16 @@ // REQUIRES: shell -// RUN: sed -e 's#//.*$##' %s > %t.cpp -// RUN: clang-include-fixer -db=fixed -input='foo= "foo.h","bar.h"' -output-headers %t.cpp -- | FileCheck %s -check-prefix=CHECK-HEADERS -// RUN: cat %t.cpp | clang-include-fixer -stdin -insert-header='{SymbolIdentifier: foo, Headers: ["\"foo.h\""]}' %t.cpp | FileCheck %s -check-prefix=CHECK +// 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='{SymbolIdentifier: 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='{SymbolIdentifier: 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='{SymbolIdentifier: foo, Range: {Offset: 0, Length: 3}, HeaderInfos: [{Header: "\"foo.h\"", QualifiedName: "a:foo"},{Header: "\"foo.h\"", QualifiedName: "b:foo"}]}' %t.cpp // -// CHECK-HEADERS: "Headers": [ "\"foo.h\"", "\"bar.h\"" ] +// CHECK: "HeaderInfos": [ +// CHECK-NEXT: {"Header": "\"foo.h\"", +// CHECK-NEXT: "QualifiedName": "foo"}, +// CHECK-NEXT: {"Header": "\"bar.h\"", +// CHECK-NEXT: "QualifiedName": "foo"} +// CHECK-NEXT:] // -// CHECK: #include "foo.h" -// CHECK: foo f; - -foo f; +// CHECK-CODE: #include "foo.h" +// CHECK-CODE: foo f; diff --git a/clang-tools-extra/test/include-fixer/ranking.cpp b/clang-tools-extra/test/include-fixer/ranking.cpp index cba1ae1..000f6a5 100644 --- a/clang-tools-extra/test/include-fixer/ranking.cpp +++ b/clang-tools-extra/test/include-fixer/ranking.cpp @@ -1,5 +1,10 @@ // RUN: clang-include-fixer -db=yaml -input=%S/Inputs/fake_yaml_db.yaml -output-headers %s -- | FileCheck %s -// CHECK: "Headers": [ "\"../include/bar.h\"", "\"../include/zbar.h\"" ] +// CHECK: "HeaderInfos": [ +// CHECK-NEXT: {"Header": "\"../include/bar.h\"", +// CHECK-NEXT: "QualifiedName": "b::a::bar"}, +// CHECK-NEXT: {"Header": "\"../include/zbar.h\"", +// CHECK-NEXT: "QualifiedName": "b::a::bar"} +// CHECK-NEXT:] bar b; diff --git a/clang-tools-extra/unittests/include-fixer/IncludeFixerTest.cpp b/clang-tools-extra/unittests/include-fixer/IncludeFixerTest.cpp index 40a2501..9ee7d0a 100644 --- a/clang-tools-extra/unittests/include-fixer/IncludeFixerTest.cpp +++ b/clang-tools-extra/unittests/include-fixer/IncludeFixerTest.cpp @@ -84,18 +84,19 @@ static std::string runIncludeFixer( std::string FakeFileName = "input.cc"; runOnCode(&Factory, Code, FakeFileName, ExtraArgs); - if (FixerContext.getMatchedSymbols().empty()) + if (FixerContext.getHeaderInfos().empty()) return Code; auto Replaces = clang::include_fixer::createInsertHeaderReplacements( - Code, FakeFileName, FixerContext.getHeaders().front()); + Code, FakeFileName, FixerContext.getHeaderInfos().front().Header); EXPECT_TRUE(static_cast(Replaces)) << llvm::toString(Replaces.takeError()) << "\n"; if (!Replaces) return ""; clang::RewriterTestContext Context; clang::FileID ID = Context.createInMemoryFile(FakeFileName, Code); - if (FixerContext.getSymbolRange().getLength() > 0) - Replaces->insert(FixerContext.createSymbolReplacement(FakeFileName, 0)); + Replaces->insert({FakeFileName, FixerContext.getSymbolRange().getOffset(), + FixerContext.getSymbolRange().getLength(), + FixerContext.getHeaderInfos().front().QualifiedName}); clang::tooling::applyAllReplacements(*Replaces, Context.Rewrite); return Context.getRewrittenText(ID); } -- 2.7.4