diff --git a/clang-tools-extra/clangd/ClangdLSPServer.h b/clang-tools-extra/clangd/ClangdLSPServer.h --- a/clang-tools-extra/clangd/ClangdLSPServer.h +++ b/clang-tools-extra/clangd/ClangdLSPServer.h @@ -154,6 +154,8 @@ bool SupportsHierarchicalDocumentSymbol = false; /// Whether the client supports showing file status. bool SupportFileStatus = false; + /// Whether the client supports offsets for parameter info labels. + bool SupportsOffsetsInSignatureHelp = false; // Store of the current versions of the open documents. DraftStore DraftMgr; diff --git a/clang-tools-extra/clangd/ClangdLSPServer.cpp b/clang-tools-extra/clangd/ClangdLSPServer.cpp --- a/clang-tools-extra/clangd/ClangdLSPServer.cpp +++ b/clang-tools-extra/clangd/ClangdLSPServer.cpp @@ -358,6 +358,7 @@ SupportsHierarchicalDocumentSymbol = Params.capabilities.HierarchicalDocumentSymbol; SupportFileStatus = Params.initializationOptions.FileStatus; + SupportsOffsetsInSignatureHelp = Params.capabilities.OffsetsInSignatureHelp; llvm::json::Object Result{ {{"capabilities", llvm::json::Object{ @@ -759,7 +760,22 @@ void ClangdLSPServer::onSignatureHelp(const TextDocumentPositionParams &Params, Callback Reply) { Server->signatureHelp(Params.textDocument.uri.file(), Params.position, - std::move(Reply)); + Bind( + [this](decltype(Reply) Reply, + llvm::Expected Signature) { + if (!Signature) + return Reply(Signature.takeError()); + if (SupportsOffsetsInSignatureHelp) + return Reply(std::move(*Signature)); + // Strip out the offsets from signature help for + // clients that only support string labels. + for (auto &Signature : Signature->signatures) { + for (auto &Param : Signature.parameters) + Param.labelOffsets.reset(); + } + return Reply(std::move(*Signature)); + }, + std::move(Reply))); } // Go to definition has a toggle function: if def and decl are distinct, then diff --git a/clang-tools-extra/clangd/CodeComplete.cpp b/clang-tools-extra/clangd/CodeComplete.cpp --- a/clang-tools-extra/clangd/CodeComplete.cpp +++ b/clang-tools-extra/clangd/CodeComplete.cpp @@ -28,6 +28,7 @@ #include "FuzzyMatch.h" #include "Headers.h" #include "Logger.h" +#include "Protocol.h" #include "Quality.h" #include "SourceCode.h" #include "TUScheduler.h" @@ -56,6 +57,7 @@ #include "llvm/ADT/SmallVector.h" #include "llvm/ADT/StringExtras.h" #include "llvm/ADT/StringRef.h" +#include "llvm/Support/Compiler.h" #include "llvm/Support/Debug.h" #include "llvm/Support/Error.h" #include "llvm/Support/Format.h" @@ -146,46 +148,6 @@ llvm_unreachable("Unhandled CodeCompletionResult::ResultKind."); } -/// Get the optional chunk as a string. This function is possibly recursive. -/// -/// The parameter info for each parameter is appended to the Parameters. -std::string getOptionalParameters(const CodeCompletionString &CCS, - std::vector &Parameters, - SignatureQualitySignals &Signal) { - std::string Result; - for (const auto &Chunk : CCS) { - switch (Chunk.Kind) { - case CodeCompletionString::CK_Optional: - assert(Chunk.Optional && - "Expected the optional code completion string to be non-null."); - Result += getOptionalParameters(*Chunk.Optional, Parameters, Signal); - break; - case CodeCompletionString::CK_VerticalSpace: - break; - case CodeCompletionString::CK_Placeholder: - // A string that acts as a placeholder for, e.g., a function call - // argument. - // Intentional fallthrough here. - case CodeCompletionString::CK_CurrentParameter: { - // A piece of text that describes the parameter that corresponds to - // the code-completion location within a function call, message send, - // macro invocation, etc. - Result += Chunk.Text; - ParameterInformation Info; - Info.label = Chunk.Text; - Parameters.push_back(std::move(Info)); - Signal.ContainsActiveParameter = true; - Signal.NumberOfOptionalParameters++; - break; - } - default: - Result += Chunk.Text; - break; - } - } - return Result; -} - // Identifier code completion result. struct RawIdentifier { llvm::StringRef Name; @@ -826,8 +788,7 @@ public: SignatureHelpCollector(const clang::CodeCompleteOptions &CodeCompleteOpts, const SymbolIndex *Index, SignatureHelp &SigHelp) - : CodeCompleteConsumer(CodeCompleteOpts), - SigHelp(SigHelp), + : CodeCompleteConsumer(CodeCompleteOpts), SigHelp(SigHelp), Allocator(std::make_shared()), CCTUInfo(Allocator), Index(Index) {} @@ -940,6 +901,50 @@ CodeCompletionTUInfo &getCodeCompletionTUInfo() override { return CCTUInfo; } private: + void processParameterChunk(llvm::StringRef ChunkText, + SignatureInformation &Signature, + SignatureQualitySignals Signal) const { + // (!) this is O(n), should still be fast compared to building ASTs. + unsigned ParamStartOffset = lspLength(Signature.label); + unsigned ParamEndOffset = ParamStartOffset + lspLength(ChunkText); + // A piece of text that describes the parameter that corresponds to + // the code-completion location within a function call, message send, + // macro invocation, etc. + Signature.label += ChunkText; + ParameterInformation Info; + Info.labelOffsets.emplace(ParamStartOffset, ParamEndOffset); + // FIXME: only set 'labelOffsets' when all clients migrate out of it. + Info.labelString = ChunkText; + + Signature.parameters.push_back(std::move(Info)); + // FIXME: this should only be set on CK_CurrentParameter. + Signal.ContainsActiveParameter = true; + } + + void processOptionalChunk(const CodeCompletionString &CCS, + SignatureInformation &Signature, + SignatureQualitySignals &Signal) const { + for (const auto &Chunk : CCS) { + switch (Chunk.Kind) { + case CodeCompletionString::CK_Optional: + assert(Chunk.Optional && + "Expected the optional code completion string to be non-null."); + processOptionalChunk(*Chunk.Optional, Signature, Signal); + break; + case CodeCompletionString::CK_VerticalSpace: + break; + case CodeCompletionString::CK_CurrentParameter: + case CodeCompletionString::CK_Placeholder: + processParameterChunk(Chunk.Text, Signature, Signal); + Signal.NumberOfOptionalParameters++; + break; + default: + Signature.label += Chunk.Text; + break; + } + } + } + // FIXME(ioeric): consider moving CodeCompletionString logic here to // CompletionString.h. ScoredSignature processOverloadCandidate(const OverloadCandidate &Candidate, @@ -960,28 +965,16 @@ assert(!ReturnType && "Unexpected CK_ResultType"); ReturnType = Chunk.Text; break; + case CodeCompletionString::CK_CurrentParameter: case CodeCompletionString::CK_Placeholder: - // A string that acts as a placeholder for, e.g., a function call - // argument. - // Intentional fallthrough here. - case CodeCompletionString::CK_CurrentParameter: { - // A piece of text that describes the parameter that corresponds to - // the code-completion location within a function call, message send, - // macro invocation, etc. - Signature.label += Chunk.Text; - ParameterInformation Info; - Info.label = Chunk.Text; - Signature.parameters.push_back(std::move(Info)); + processParameterChunk(Chunk.Text, Signature, Signal); Signal.NumberOfParameters++; - Signal.ContainsActiveParameter = true; break; - } case CodeCompletionString::CK_Optional: { // The rest of the parameters are defaulted/optional. assert(Chunk.Optional && "Expected the optional code completion string to be non-null."); - Signature.label += getOptionalParameters(*Chunk.Optional, - Signature.parameters, Signal); + processOptionalChunk(*Chunk.Optional, Signature, Signal); break; } case CodeCompletionString::CK_VerticalSpace: @@ -1033,7 +1026,7 @@ PP.getIdentifierTable().getExternalIdentifierLookup(); if (!PreambleIdentifiers || !PreambleMacros) return; - for (const auto& MacroName : Preamble.MainFileMacros) + for (const auto &MacroName : Preamble.MainFileMacros) if (auto *II = PreambleIdentifiers->get(MacroName)) if (II->isOutOfDate()) PreambleMacros->updateOutOfDateIdentifier(*II); @@ -1209,7 +1202,7 @@ int NSema = 0, NIndex = 0, NSemaAndIndex = 0, NIdent = 0; bool Incomplete = false; // Would more be available with a higher limit? CompletionPrefix HeuristicPrefix; - llvm::Optional Filter; // Initialized once Sema runs. + llvm::Optional Filter; // Initialized once Sema runs. Range ReplacedRange; std::vector QueryScopes; // Initialized once Sema runs. // Initialized once QueryScopes is initialized, if there are scopes. @@ -1703,8 +1696,8 @@ return Result; } -CompletionPrefix -guessCompletionPrefix(llvm::StringRef Content, unsigned Offset) { +CompletionPrefix guessCompletionPrefix(llvm::StringRef Content, + unsigned Offset) { assert(Offset <= Content.size()); StringRef Rest = Content.take_front(Offset); CompletionPrefix Result; diff --git a/clang-tools-extra/clangd/Protocol.h b/clang-tools-extra/clangd/Protocol.h --- a/clang-tools-extra/clangd/Protocol.h +++ b/clang-tools-extra/clangd/Protocol.h @@ -381,6 +381,9 @@ /// Client supports hierarchical document symbols. bool HierarchicalDocumentSymbol = false; + /// Client supports processing label offsets instead of a simple label string. + bool OffsetsInSignatureHelp = false; + /// The supported set of CompletionItemKinds for textDocument/completion. /// textDocument.completion.completionItemKind.valueSet llvm::Optional CompletionItemKinds; @@ -972,8 +975,14 @@ /// A single parameter of a particular signature. struct ParameterInformation { - /// The label of this parameter. Mandatory. - std::string label; + /// The label of this parameter. Ignored when labelOffsets is set. + std::string labelString; + + /// Inclusive start and exclusive end offsets withing the containing signature + /// label. + /// Offsets are computed by lspLength(), which counts UTF-16 code units by + /// default but that can be overriden, see its documentation for details. + llvm::Optional> labelOffsets; /// The documentation of this parameter. Optional. std::string documentation; diff --git a/clang-tools-extra/clangd/Protocol.cpp b/clang-tools-extra/clangd/Protocol.cpp --- a/clang-tools-extra/clangd/Protocol.cpp +++ b/clang-tools-extra/clangd/Protocol.cpp @@ -302,6 +302,14 @@ DocumentSymbol->getBoolean("hierarchicalDocumentSymbolSupport")) R.HierarchicalDocumentSymbol = *HierarchicalSupport; } + if (auto *Help = TextDocument->getObject("signatureHelp")) { + if (auto *Info = Help->getObject("signatureInformation")) { + if (auto *Parameter = Info->getObject("parameterInformation")) { + if (auto OffsetSupport = Parameter->getBoolean("labelOffsetSupport")) + R.OffsetsInSignatureHelp = *OffsetSupport; + } + } + } } if (auto *Workspace = O->getObject("workspace")) { if (auto *Symbol = Workspace->getObject("symbol")) { @@ -791,8 +799,14 @@ } llvm::json::Value toJSON(const ParameterInformation &PI) { - assert(!PI.label.empty() && "parameter information label is required"); - llvm::json::Object Result{{"label", PI.label}}; + assert(PI.labelOffsets.hasValue() || + !PI.labelString.empty() && "parameter information label is required"); + llvm::json::Object Result; + if (PI.labelOffsets) + Result["label"] = + llvm::json::Array({PI.labelOffsets->first, PI.labelOffsets->second}); + else + Result["label"] = PI.labelString; if (!PI.documentation.empty()) Result["documentation"] = PI.documentation; return std::move(Result); diff --git a/clang-tools-extra/clangd/test/signature-help-with-offsets.test b/clang-tools-extra/clangd/test/signature-help-with-offsets.test new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clangd/test/signature-help-with-offsets.test @@ -0,0 +1,50 @@ +# RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s +# Start a session. +{ + "jsonrpc": "2.0", + "id": 0, + "method": "initialize", + "params": { + "processId": 123, + "rootPath": "clangd", + "capabilities": { + "textDocument": { + "signatureHelp": { + "signatureInformation": { + "parameterInformation": { + "labelOffsetSupport": true + } + } + } + } + }, + "trace": "off" + } +} +--- +{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///main.cpp","languageId":"cpp","version":1,"text":"void x(int);\nint main(){\nx("}}} +--- +{"jsonrpc":"2.0","id":1,"method":"textDocument/signatureHelp","params":{"textDocument":{"uri":"test:///main.cpp"},"position":{"line":2,"character":2}}} +# CHECK: "id": 1, +# CHECK-NEXT: "jsonrpc": "2.0", +# CHECK-NEXT: "result": { +# CHECK-NEXT: "activeParameter": 0, +# CHECK-NEXT: "activeSignature": 0, +# CHECK-NEXT: "signatures": [ +# CHECK-NEXT: { +# CHECK-NEXT: "label": "x(int) -> void", +# CHECK-NEXT: "parameters": [ +# CHECK-NEXT: { +# CHECK-NEXT: "label": [ +# CHECK-NEXT: 2, +# CHECK-NEXT: 5 +# CHECK-NEXT: ] +# CHECK-NEXT: } +# CHECK-NEXT: ] +# CHECK-NEXT: } +# CHECK-NEXT: ] +# CHECK-NEXT: } +--- +{"jsonrpc":"2.0","id":100000,"method":"shutdown"} +--- +{"jsonrpc":"2.0","method":"exit"} diff --git a/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp b/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp --- a/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp +++ b/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp @@ -928,19 +928,37 @@ return signatures(Test.code(), Test.point(), std::move(IndexSymbols)); } +struct ExpectedParameter { + std::string Text; + std::pair Offsets; +}; MATCHER_P(ParamsAre, P, "") { if (P.size() != arg.parameters.size()) return false; - for (unsigned I = 0; I < P.size(); ++I) - if (P[I] != arg.parameters[I].label) + for (unsigned I = 0; I < P.size(); ++I) { + if (P[I].Text != arg.parameters[I].labelString || + P[I].Offsets != arg.parameters[I].labelOffsets) return false; + } return true; } MATCHER_P(SigDoc, Doc, "") { return arg.documentation == Doc; } -Matcher Sig(std::string Label, - std::vector Params) { - return AllOf(SigHelpLabeled(Label), ParamsAre(Params)); +/// \p AnnotatedLabel is a signature label with ranges marking parameters, e.g. +/// foo([[int p1]], [[double p2]]) -> void +Matcher Sig(llvm::StringRef AnnotatedLabel) { + llvm::Annotations A(AnnotatedLabel); + std::string Label = A.code(); + std::vector Parameters; + for (auto Range : A.ranges()) { + Parameters.emplace_back(); + + ExpectedParameter &P = Parameters.back(); + P.Text = Label.substr(Range.Begin, Range.End - Range.Begin); + P.Offsets.first = lspLength(llvm::StringRef(Label).substr(0, Range.Begin)); + P.Offsets.second = lspLength(llvm::StringRef(Label).substr(1, Range.End)); + } + return AllOf(SigHelpLabeled(Label), ParamsAre(Parameters)); } TEST(SignatureHelpTest, Overloads) { @@ -953,11 +971,10 @@ int main() { foo(^); } )cpp"); EXPECT_THAT(Results.signatures, - UnorderedElementsAre( - Sig("foo(float x, float y) -> void", {"float x", "float y"}), - Sig("foo(float x, int y) -> void", {"float x", "int y"}), - Sig("foo(int x, float y) -> void", {"int x", "float y"}), - Sig("foo(int x, int y) -> void", {"int x", "int y"}))); + UnorderedElementsAre(Sig("foo([[float x]], [[float y]]) -> void"), + Sig("foo([[float x]], [[int y]]) -> void"), + Sig("foo([[int x]], [[float y]]) -> void"), + Sig("foo([[int x]], [[int y]]) -> void"))); // We always prefer the first signature. EXPECT_EQ(0, Results.activeSignature); EXPECT_EQ(0, Results.activeParameter); @@ -971,9 +988,8 @@ )cpp"); EXPECT_THAT(Results.signatures, UnorderedElementsAre( - Sig("bar(int x, int y = 0) -> void", {"int x", "int y = 0"}), - Sig("bar(float x = 0, int y = 42) -> void", - {"float x = 0", "int y = 42"}))); + Sig("bar([[int x]], [[int y = 0]]) -> void"), + Sig("bar([[float x = 0]], [[int y = 42]]) -> void"))); EXPECT_EQ(0, Results.activeSignature); EXPECT_EQ(0, Results.activeParameter); } @@ -984,8 +1000,7 @@ int main() { baz(baz(1,2,3), ^); } )cpp"); EXPECT_THAT(Results.signatures, - ElementsAre(Sig("baz(int a, int b, int c) -> int", - {"int a", "int b", "int c"}))); + ElementsAre(Sig("baz([[int a]], [[int b]], [[int c]]) -> int"))); EXPECT_EQ(0, Results.activeSignature); EXPECT_EQ(1, Results.activeParameter); } @@ -1722,14 +1737,12 @@ void foo(int x, int y = 0); int main() { foo(^); } )cpp"); - EXPECT_THAT( - Results.signatures, - ElementsAre( - Sig("foo(int x) -> void", {"int x"}), - Sig("foo(int x, int y = 0) -> void", {"int x", "int y = 0"}), - Sig("foo(float x, int y) -> void", {"float x", "int y"}), - Sig("foo(int x, float y) -> void", {"int x", "float y"}), - Sig("foo(float x, float y) -> void", {"float x", "float y"}))); + EXPECT_THAT(Results.signatures, + ElementsAre(Sig("foo([[int x]]) -> void"), + Sig("foo([[int x]], [[int y = 0]]) -> void"), + Sig("foo([[float x]], [[int y]]) -> void"), + Sig("foo([[int x]], [[float y]]) -> void"), + Sig("foo([[float x]], [[float y]]) -> void"))); // We always prefer the first signature. EXPECT_EQ(0, Results.activeSignature); EXPECT_EQ(0, Results.activeParameter); @@ -1746,7 +1759,7 @@ )cpp"; EXPECT_THAT(signatures(Sig0).signatures, - ElementsAre(Sig("foo(T, T, T) -> void", {"T", "T", "T"}))); + ElementsAre(Sig("foo([[T]], [[T]], [[T]]) -> void"))); StringRef Sig1 = R"cpp( template @@ -1757,7 +1770,7 @@ })cpp"; EXPECT_THAT(signatures(Sig1).signatures, - ElementsAre(Sig("foo(T, T, T) -> void", {"T", "T", "T"}))); + ElementsAre(Sig("foo([[T]], [[T]], [[T]]) -> void"))); StringRef Sig2 = R"cpp( template @@ -1769,7 +1782,7 @@ )cpp"; EXPECT_THAT(signatures(Sig2).signatures, - ElementsAre(Sig("foo(T...) -> void", {"T..."}))); + ElementsAre(Sig("foo([[T...]]) -> void"))); // It is debatable whether we should substitute the outer template parameter // ('T') in that case. Currently we don't substitute it in signature help, but @@ -1789,7 +1802,7 @@ )cpp"; EXPECT_THAT(signatures(Sig3).signatures, - ElementsAre(Sig("foo(T, U) -> void", {"T", "U"}))); + ElementsAre(Sig("foo([[T]], [[U]]) -> void"))); } TEST(SignatureHelpTest, IndexDocumentation) { @@ -1810,8 +1823,8 @@ EXPECT_THAT( signatures(Sig0, {Foo0}).signatures, - ElementsAre(AllOf(Sig("foo() -> int", {}), SigDoc("Doc from the index")), - AllOf(Sig("foo(double) -> int", {"double"}), SigDoc("")))); + ElementsAre(AllOf(Sig("foo() -> int"), SigDoc("Doc from the index")), + AllOf(Sig("foo([[double]]) -> int"), SigDoc("")))); StringRef Sig1 = R"cpp( int foo(); @@ -1827,11 +1840,10 @@ EXPECT_THAT( signatures(Sig1, {Foo0, Foo1, Foo2}).signatures, - ElementsAre(AllOf(Sig("foo() -> int", {}), SigDoc("Doc from the index")), - AllOf(Sig("foo(int) -> int", {"int"}), - SigDoc("Overriden doc from sema")), - AllOf(Sig("foo(int, int) -> int", {"int", "int"}), - SigDoc("Doc from sema")))); + ElementsAre( + AllOf(Sig("foo() -> int"), SigDoc("Doc from the index")), + AllOf(Sig("foo([[int]]) -> int"), SigDoc("Overriden doc from sema")), + AllOf(Sig("foo([[int]], [[int]]) -> int"), SigDoc("Doc from sema")))); } TEST(SignatureHelpTest, DynamicIndexDocumentation) { @@ -1862,7 +1874,7 @@ EXPECT_THAT( llvm::cantFail(runSignatureHelp(Server, File, FileContent.point())) .signatures, - ElementsAre(AllOf(Sig("foo() -> int", {}), SigDoc("Member doc")))); + ElementsAre(AllOf(Sig("foo() -> int"), SigDoc("Member doc")))); } TEST(CompletionTest, CompletionFunctionArgsDisabled) { @@ -2140,10 +2152,9 @@ void foo(int x, int y); int main() { foo(1+^); } )cpp"); - EXPECT_THAT( - Results.signatures, - ElementsAre(Sig("foo(int x) -> void", {"int x"}), - Sig("foo(int x, int y) -> void", {"int x", "int y"}))); + EXPECT_THAT(Results.signatures, + ElementsAre(Sig("foo([[int x]]) -> void"), + Sig("foo([[int x]], [[int y]]) -> void"))); EXPECT_EQ(0, Results.activeParameter); } { @@ -2152,10 +2163,9 @@ void foo(int x, int y); int main() { foo(1^); } )cpp"); - EXPECT_THAT( - Results.signatures, - ElementsAre(Sig("foo(int x) -> void", {"int x"}), - Sig("foo(int x, int y) -> void", {"int x", "int y"}))); + EXPECT_THAT(Results.signatures, + ElementsAre(Sig("foo([[int x]]) -> void"), + Sig("foo([[int x]], [[int y]]) -> void"))); EXPECT_EQ(0, Results.activeParameter); } { @@ -2164,10 +2174,9 @@ void foo(int x, int y); int main() { foo(1^0); } )cpp"); - EXPECT_THAT( - Results.signatures, - ElementsAre(Sig("foo(int x) -> void", {"int x"}), - Sig("foo(int x, int y) -> void", {"int x", "int y"}))); + EXPECT_THAT(Results.signatures, + ElementsAre(Sig("foo([[int x]]) -> void"), + Sig("foo([[int x]], [[int y]]) -> void"))); EXPECT_EQ(0, Results.activeParameter); } { @@ -2177,8 +2186,8 @@ int bar(int x, int y); int main() { bar(foo(2, 3^)); } )cpp"); - EXPECT_THAT(Results.signatures, ElementsAre(Sig("foo(int x, int y) -> void", - {"int x", "int y"}))); + EXPECT_THAT(Results.signatures, + ElementsAre(Sig("foo([[int x]], [[int y]]) -> void"))); EXPECT_EQ(1, Results.activeParameter); } } @@ -2195,9 +2204,8 @@ }; )cpp"); EXPECT_THAT(Results.signatures, - UnorderedElementsAre(Sig("A(int)", {"int"}), - Sig("A(A &&)", {"A &&"}), - Sig("A(const A &)", {"const A &"}))); + UnorderedElementsAre(Sig("A([[int]])"), Sig("A([[A &&]])"), + Sig("A([[const A &]])"))); } { const auto Results = signatures(R"cpp( @@ -2214,9 +2222,8 @@ }; )cpp"); EXPECT_THAT(Results.signatures, - UnorderedElementsAre(Sig("A(int)", {"int"}), - Sig("A(A &&)", {"A &&"}), - Sig("A(const A &)", {"const A &"}))); + UnorderedElementsAre(Sig("A([[int]])"), Sig("A([[A &&]])"), + Sig("A([[const A &]])"))); } }