From: Kadir Cetinkaya Date: Wed, 8 Aug 2018 08:59:29 +0000 (+0000) Subject: Added functionality to suggest FixIts for conversion of '->' to '.' and vice versa. X-Git-Tag: llvmorg-8.0.0-rc1~11471 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=2f84d911317b6c0c312ef34fd1ca20a32614ea15;p=platform%2Fupstream%2Fllvm.git Added functionality to suggest FixIts for conversion of '->' to '.' and vice versa. Summary: Added functionality to suggest FixIts for conversion of '->' to '.' and vice versa. Reviewers: ilya-biryukov Reviewed By: ilya-biryukov Subscribers: yvvan, ioeric, jkorous, arphaman, cfe-commits, kadircet Differential Revision: https://reviews.llvm.org/D50193 llvm-svn: 339224 --- diff --git a/clang-tools-extra/clangd/CodeComplete.cpp b/clang-tools-extra/clangd/CodeComplete.cpp index e7e38d816ea2..112a6fe20bb9 100644 --- a/clang-tools-extra/clangd/CodeComplete.cpp +++ b/clang-tools-extra/clangd/CodeComplete.cpp @@ -22,6 +22,7 @@ #include "AST.h" #include "CodeCompletionStrings.h" #include "Compiler.h" +#include "Diagnostics.h" #include "FileDistance.h" #include "FuzzyMatch.h" #include "Headers.h" @@ -280,6 +281,10 @@ struct CodeCompletionBuilder { } Completion.Kind = toCompletionItemKind(C.SemaResult->Kind, C.SemaResult->Declaration); + for (const auto &FixIt : C.SemaResult->FixIts) { + Completion.FixIts.push_back( + toTextEdit(FixIt, ASTCtx.getSourceManager(), ASTCtx.getLangOpts())); + } } if (C.IndexResult) { Completion.Origin |= C.IndexResult->Origin; @@ -906,6 +911,7 @@ clang::CodeCompleteOptions CodeCompleteOptions::getClangCompleteOpts() const { // the index can provide results from the preamble. // Tell Sema not to deserialize the preamble to look for results. Result.LoadExternal = !Index; + Result.IncludeFixIts = IncludeFixIts; return Result; } @@ -1090,8 +1096,8 @@ private: // Groups overloads if desired, to form CompletionCandidate::Bundles. // The bundles are scored and top results are returned, best to worst. std::vector - mergeResults(const std::vector &SemaResults, - const SymbolSlab &IndexResults) { + mergeResults(const std::vector &SemaResults, + const SymbolSlab &IndexResults) { trace::Span Tracer("Merge and score results"); std::vector Bundles; llvm::DenseMap BundleLookup; @@ -1272,13 +1278,18 @@ CompletionItem CodeCompletion::render(const CodeCompleteOptions &Opts) const { LSP.documentation = Documentation; LSP.sortText = sortText(Score.Total, Name); LSP.filterText = Name; + // FIXME(kadircet): Use LSP.textEdit instead of insertText, because it causes + // undesired behaviours. Like completing "this.^" into "this-push_back". LSP.insertText = RequiredQualifier + Name; if (Opts.EnableSnippets) LSP.insertText += SnippetSuffix; LSP.insertTextFormat = Opts.EnableSnippets ? InsertTextFormat::Snippet : InsertTextFormat::PlainText; + LSP.additionalTextEdits.reserve(FixIts.size() + (HeaderInsertion ? 1 : 0)); + for (const auto &FixIt : FixIts) + LSP.additionalTextEdits.push_back(FixIt); if (HeaderInsertion) - LSP.additionalTextEdits = {*HeaderInsertion}; + LSP.additionalTextEdits.push_back(*HeaderInsertion); return LSP; } diff --git a/clang-tools-extra/clangd/CodeComplete.h b/clang-tools-extra/clangd/CodeComplete.h index 884149dfb2b5..c623be34afdc 100644 --- a/clang-tools-extra/clangd/CodeComplete.h +++ b/clang-tools-extra/clangd/CodeComplete.h @@ -77,6 +77,10 @@ struct CodeCompleteOptions { /// FIXME(ioeric): we might want a better way to pass the index around inside /// clangd. const SymbolIndex *Index = nullptr; + + /// Include completions that require small corrections, e.g. change '.' to + /// '->' on member access etc. + bool IncludeFixIts = false; }; // Semi-structured representation of a code-complete suggestion for our C++ API. @@ -115,6 +119,10 @@ struct CodeCompletion { // Present if Header is set and should be inserted to use this item. llvm::Optional HeaderInsertion; + /// Holds information about small corrections that needs to be done. Like + /// converting '->' to '.' on member access. + std::vector FixIts; + // Scores are used to rank completion items. struct Scores { // The score that items are ranked by. diff --git a/clang-tools-extra/clangd/Diagnostics.cpp b/clang-tools-extra/clangd/Diagnostics.cpp index 5a61567f2dc0..3c293dde3436 100644 --- a/clang-tools-extra/clangd/Diagnostics.cpp +++ b/clang-tools-extra/clangd/Diagnostics.cpp @@ -70,15 +70,6 @@ Range diagnosticRange(const clang::Diagnostic &D, const LangOptions &L) { return halfOpenToRange(M, R); } -TextEdit toTextEdit(const FixItHint &FixIt, const SourceManager &M, - const LangOptions &L) { - TextEdit Result; - Result.range = - halfOpenToRange(M, Lexer::makeFileCharRange(FixIt.RemoveRange, M, L)); - Result.newText = FixIt.CodeToInsert; - return Result; -} - bool isInsideMainFile(const SourceLocation Loc, const SourceManager &M) { return Loc.isValid() && M.isInMainFile(Loc); } diff --git a/clang-tools-extra/clangd/Protocol.h b/clang-tools-extra/clangd/Protocol.h index 5056dbef8ee9..b0d810e87fef 100644 --- a/clang-tools-extra/clangd/Protocol.h +++ b/clang-tools-extra/clangd/Protocol.h @@ -171,6 +171,10 @@ struct TextEdit { /// The string to be inserted. For delete operations use an /// empty string. std::string newText; + + bool operator==(const TextEdit &rhs) const { + return newText == rhs.newText && range == rhs.range; + } }; bool fromJSON(const llvm::json::Value &, TextEdit &); llvm::json::Value toJSON(const TextEdit &); diff --git a/clang-tools-extra/clangd/Quality.cpp b/clang-tools-extra/clangd/Quality.cpp index 61195b566606..8cf794b0fe82 100644 --- a/clang-tools-extra/clangd/Quality.cpp +++ b/clang-tools-extra/clangd/Quality.cpp @@ -292,6 +292,8 @@ void SymbolRelevanceSignals::merge(const CodeCompletionResult &SemaCCResult) { // Declarations are scoped, others (like macros) are assumed global. if (SemaCCResult.Declaration) Scope = std::min(Scope, computeScope(SemaCCResult.Declaration)); + + NeedsFixIts = !SemaCCResult.FixIts.empty(); } static std::pair proximityScore(llvm::StringRef SymbolURI, @@ -343,6 +345,10 @@ float SymbolRelevanceSignals::evaluate() const { Score *= 0.5; } + // Penalize for FixIts. + if (NeedsFixIts) + Score *= 0.5; + return Score; } @@ -350,6 +356,7 @@ raw_ostream &operator<<(raw_ostream &OS, const SymbolRelevanceSignals &S) { OS << formatv("=== Symbol relevance: {0}\n", S.evaluate()); OS << formatv("\tName match: {0}\n", S.NameMatch); OS << formatv("\tForbidden: {0}\n", S.Forbidden); + OS << formatv("\tNeedsFixIts: {0}\n", S.NeedsFixIts); OS << formatv("\tIsInstanceMember: {0}\n", S.IsInstanceMember); OS << formatv("\tContext: {0}\n", getCompletionKindString(S.Context)); OS << formatv("\tSymbol URI: {0}\n", S.SymbolURI); diff --git a/clang-tools-extra/clangd/Quality.h b/clang-tools-extra/clangd/Quality.h index 0aed496be6a0..54778b9fa76e 100644 --- a/clang-tools-extra/clangd/Quality.h +++ b/clang-tools-extra/clangd/Quality.h @@ -77,6 +77,8 @@ struct SymbolRelevanceSignals { /// 0-1+ fuzzy-match score for unqualified name. Must be explicitly assigned. float NameMatch = 1; bool Forbidden = false; // Unavailable (e.g const) or inaccessible (private). + /// Whether fixits needs to be applied for that completion or not. + bool NeedsFixIts = false; URIDistance *FileProximityMatch = nullptr; /// This is used to calculate proximity between the index symbol and the diff --git a/clang-tools-extra/clangd/SourceCode.cpp b/clang-tools-extra/clangd/SourceCode.cpp index d302518b061a..88ec2c956830 100644 --- a/clang-tools-extra/clangd/SourceCode.cpp +++ b/clang-tools-extra/clangd/SourceCode.cpp @@ -199,5 +199,14 @@ getAbsoluteFilePath(const FileEntry *F, const SourceManager &SourceMgr) { return FilePath.str().str(); } +TextEdit toTextEdit(const FixItHint &FixIt, const SourceManager &M, + const LangOptions &L) { + TextEdit Result; + Result.range = + halfOpenToRange(M, Lexer::makeFileCharRange(FixIt.RemoveRange, M, L)); + Result.newText = FixIt.CodeToInsert; + return Result; +} + } // namespace clangd } // namespace clang diff --git a/clang-tools-extra/clangd/SourceCode.h b/clang-tools-extra/clangd/SourceCode.h index b67fe6ecfded..cb0c67287816 100644 --- a/clang-tools-extra/clangd/SourceCode.h +++ b/clang-tools-extra/clangd/SourceCode.h @@ -14,6 +14,7 @@ #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_SOURCECODE_H #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_SOURCECODE_H #include "Protocol.h" +#include "clang/Basic/Diagnostic.h" #include "clang/Basic/SourceLocation.h" #include "clang/Tooling/Core/Replacement.h" @@ -64,6 +65,10 @@ std::vector replacementsToEdits(StringRef Code, /// Get the absolute file path of a given file entry. llvm::Optional getAbsoluteFilePath(const FileEntry *F, const SourceManager &SourceMgr); + +TextEdit toTextEdit(const FixItHint &FixIt, const SourceManager &M, + const LangOptions &L); + } // namespace clangd } // namespace clang #endif diff --git a/clang-tools-extra/unittests/clangd/CodeCompleteTests.cpp b/clang-tools-extra/unittests/clangd/CodeCompleteTests.cpp index fefc7b21b9f7..5fd17d0a2c92 100644 --- a/clang-tools-extra/unittests/clangd/CodeCompleteTests.cpp +++ b/clang-tools-extra/unittests/clangd/CodeCompleteTests.cpp @@ -79,6 +79,23 @@ std::unique_ptr memIndex(std::vector Symbols) { return MemIndex::build(std::move(Slab).build()); } +CodeCompleteResult completions(ClangdServer &Server, StringRef TestCode, + Position point, + std::vector IndexSymbols = {}, + clangd::CodeCompleteOptions Opts = {}) { + std::unique_ptr OverrideIndex; + if (!IndexSymbols.empty()) { + assert(!Opts.Index && "both Index and IndexSymbols given!"); + OverrideIndex = memIndex(std::move(IndexSymbols)); + Opts.Index = OverrideIndex.get(); + } + + auto File = testPath("foo.cpp"); + runAddDocument(Server, File, TestCode); + auto CompletionList = cantFail(runCodeComplete(Server, File, point, Opts)); + return CompletionList; +} + CodeCompleteResult completions(ClangdServer &Server, StringRef Text, std::vector IndexSymbols = {}, clangd::CodeCompleteOptions Opts = {}) { @@ -1342,6 +1359,85 @@ TEST(CompletionTest, CodeCompletionContext) { EXPECT_THAT(Results.Context, CodeCompletionContext::CCC_DotMemberAccess); } +TEST(CompletionTest, FixItForArrowToDot) { + MockFSProvider FS; + MockCompilationDatabase CDB; + IgnoreDiagnostics DiagConsumer; + ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest()); + + CodeCompleteOptions Opts; + Opts.IncludeFixIts = true; + Annotations TestCode( + R"cpp( + class Auxilary { + public: + void AuxFunction(); + }; + class ClassWithPtr { + public: + void MemberFunction(); + Auxilary* operator->() const; + Auxilary* Aux; + }; + void f() { + ClassWithPtr x; + x[[->]]^; + } + )cpp"); + auto Results = + completions(Server, TestCode.code(), TestCode.point(), {}, Opts); + EXPECT_EQ(Results.Completions.size(), 3u); + + TextEdit ReplacementEdit; + ReplacementEdit.range = TestCode.range(); + ReplacementEdit.newText = "."; + for (const auto &C : Results.Completions) { + EXPECT_TRUE(C.FixIts.size() == 1u || C.Name == "AuxFunction"); + if (!C.FixIts.empty()) + EXPECT_THAT(C.FixIts, ElementsAre(ReplacementEdit)); + } +} + +TEST(CompletionTest, FixItForDotToArrow) { + MockFSProvider FS; + MockCompilationDatabase CDB; + IgnoreDiagnostics DiagConsumer; + ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest()); + + CodeCompleteOptions Opts; + Opts.IncludeFixIts = true; + Annotations TestCode( + R"cpp( + class Auxilary { + public: + void AuxFunction(); + }; + class ClassWithPtr { + public: + void MemberFunction(); + Auxilary* operator->() const; + Auxilary* Aux; + }; + void f() { + ClassWithPtr x; + x[[.]]^; + } + )cpp"); + auto Results = + completions(Server, TestCode.code(), TestCode.point(), {}, Opts); + EXPECT_EQ(Results.Completions.size(), 3u); + + TextEdit ReplacementEdit; + ReplacementEdit.range = TestCode.range(); + ReplacementEdit.newText = "->"; + for (const auto &C : Results.Completions) { + EXPECT_TRUE(C.FixIts.empty() || C.Name == "AuxFunction"); + if (!C.FixIts.empty()) { + EXPECT_THAT(C.FixIts, ElementsAre(ReplacementEdit)); + } + } +} + } // namespace } // namespace clangd } // namespace clang diff --git a/clang-tools-extra/unittests/clangd/QualityTests.cpp b/clang-tools-extra/unittests/clangd/QualityTests.cpp index 153eef498221..1dcb39117173 100644 --- a/clang-tools-extra/unittests/clangd/QualityTests.cpp +++ b/clang-tools-extra/unittests/clangd/QualityTests.cpp @@ -346,6 +346,28 @@ TEST(QualityTests, ConstructorQuality) { EXPECT_EQ(Q.Category, SymbolQualitySignals::Constructor); } +TEST(QualityTests, ItemWithFixItsRankedDown) { + CodeCompleteOptions Opts; + Opts.IncludeFixIts = true; + + auto Header = TestTU::withHeaderCode(R"cpp( + int x; + )cpp"); + auto AST = Header.build(); + + SymbolRelevanceSignals RelevanceWithFixIt; + RelevanceWithFixIt.merge(CodeCompletionResult(&findDecl(AST, "x"), 0, nullptr, + false, true, {FixItHint{}})); + EXPECT_TRUE(RelevanceWithFixIt.NeedsFixIts); + + SymbolRelevanceSignals RelevanceWithoutFixIt; + RelevanceWithoutFixIt.merge( + CodeCompletionResult(&findDecl(AST, "x"), 0, nullptr, false, true, {})); + EXPECT_FALSE(RelevanceWithoutFixIt.NeedsFixIts); + + EXPECT_LT(RelevanceWithFixIt.evaluate(), RelevanceWithoutFixIt.evaluate()); +} + } // namespace } // namespace clangd } // namespace clang