[clang-tools-extra] 7c19fdd - [clangd] Polish clangd/inlayHints and expose them by default.
Sam McCall via cfe-commits
cfe-commits at lists.llvm.org
Fri Jan 7 06:12:49 PST 2022
Author: Sam McCall
Date: 2022-01-07T15:12:43+01:00
New Revision: 7c19fdd59939249c23384f0900d49aab4a5f0695
URL: https://github.com/llvm/llvm-project/commit/7c19fdd59939249c23384f0900d49aab4a5f0695
DIFF: https://github.com/llvm/llvm-project/commit/7c19fdd59939249c23384f0900d49aab4a5f0695.diff
LOG: [clangd] Polish clangd/inlayHints and expose them by default.
This means it's a "real feature" in clangd 14, albeit one that requires special
client support.
- remove "preview" from the flag description
- expose the `clangdInlayHints` capability by default
- provide `position` as well as `range`
- support `InlayHintsParams.range` to restrict the range retrieved
- inlay hint list is in document order (sorted by position)
Still to come: control feature via config rather than flag.
Fixes https://github.com/clangd/clangd/issues/313
Protocol doc is in https://github.com/llvm/clangd-www/pull/56/files
Differential Revision: https://reviews.llvm.org/D116699
Added:
clang-tools-extra/clangd/test/inlayHints.test
Modified:
clang-tools-extra/clangd/ClangdLSPServer.cpp
clang-tools-extra/clangd/ClangdLSPServer.h
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/ClangdServer.h
clang-tools-extra/clangd/InlayHints.cpp
clang-tools-extra/clangd/InlayHints.h
clang-tools-extra/clangd/Protocol.cpp
clang-tools-extra/clangd/Protocol.h
clang-tools-extra/clangd/test/initialize-params.test
clang-tools-extra/clangd/tool/ClangdMain.cpp
clang-tools-extra/clangd/unittests/InlayHintTests.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/ClangdLSPServer.cpp b/clang-tools-extra/clangd/ClangdLSPServer.cpp
index edde19f96202..76e3806f421e 100644
--- a/clang-tools-extra/clangd/ClangdLSPServer.cpp
+++ b/clang-tools-extra/clangd/ClangdLSPServer.cpp
@@ -608,6 +608,7 @@ void ClangdLSPServer::onInitialize(const InitializeParams &Params,
if (Opts.FoldingRanges)
ServerCaps["foldingRangeProvider"] = true;
+ // FIXME: once inlayHints can be disabled in config, always advertise.
if (Opts.InlayHints)
ServerCaps["clangdInlayHintsProvider"] = true;
@@ -1210,7 +1211,8 @@ void ClangdLSPServer::onCallHierarchyIncomingCalls(
void ClangdLSPServer::onInlayHints(const InlayHintsParams &Params,
Callback<std::vector<InlayHint>> Reply) {
- Server->inlayHints(Params.textDocument.uri.file(), std::move(Reply));
+ Server->inlayHints(Params.textDocument.uri.file(), Params.range,
+ std::move(Reply));
}
void ClangdLSPServer::applyConfiguration(
diff --git a/clang-tools-extra/clangd/ClangdLSPServer.h b/clang-tools-extra/clangd/ClangdLSPServer.h
index bd7c1a05b0e2..0a5af762ad32 100644
--- a/clang-tools-extra/clangd/ClangdLSPServer.h
+++ b/clang-tools-extra/clangd/ClangdLSPServer.h
@@ -63,8 +63,8 @@ class ClangdLSPServer : private ClangdServer::Callbacks,
return !T.hidden(); // only enable non-hidden tweaks.
};
- /// Enable preview of InlayHints feature.
- bool InlayHints = false;
+ /// Enable InlayHints feature.
+ bool InlayHints = true;
/// Limit the number of references returned (0 means no limit).
size_t ReferencesLimit = 0;
diff --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp
index e106c31ce0df..69f37662c4ab 100644
--- a/clang-tools-extra/clangd/ClangdServer.cpp
+++ b/clang-tools-extra/clangd/ClangdServer.cpp
@@ -759,12 +759,13 @@ void ClangdServer::incomingCalls(
});
}
-void ClangdServer::inlayHints(PathRef File,
+void ClangdServer::inlayHints(PathRef File, llvm::Optional<Range> RestrictRange,
Callback<std::vector<InlayHint>> CB) {
- auto Action = [CB = std::move(CB)](Expected<InputsAndAST> InpAST) mutable {
+ auto Action = [RestrictRange(std::move(RestrictRange)),
+ CB = std::move(CB)](Expected<InputsAndAST> InpAST) mutable {
if (!InpAST)
return CB(InpAST.takeError());
- CB(clangd::inlayHints(InpAST->AST));
+ CB(clangd::inlayHints(InpAST->AST, std::move(RestrictRange)));
};
WorkScheduler->runWithAST("InlayHints", File, std::move(Action));
}
diff --git a/clang-tools-extra/clangd/ClangdServer.h b/clang-tools-extra/clangd/ClangdServer.h
index 0589f4fc4214..14be307e6fda 100644
--- a/clang-tools-extra/clangd/ClangdServer.h
+++ b/clang-tools-extra/clangd/ClangdServer.h
@@ -264,7 +264,8 @@ class ClangdServer {
Callback<std::vector<CallHierarchyIncomingCall>>);
/// Resolve inlay hints for a given document.
- void inlayHints(PathRef File, Callback<std::vector<InlayHint>>);
+ void inlayHints(PathRef File, llvm::Optional<Range> RestrictRange,
+ Callback<std::vector<InlayHint>>);
/// Retrieve the top symbols from the workspace matching a query.
void workspaceSymbols(StringRef Query, int Limit,
diff --git a/clang-tools-extra/clangd/InlayHints.cpp b/clang-tools-extra/clangd/InlayHints.cpp
index 6b9c8b1eee54..60d20a38ecd2 100644
--- a/clang-tools-extra/clangd/InlayHints.cpp
+++ b/clang-tools-extra/clangd/InlayHints.cpp
@@ -8,20 +8,24 @@
#include "InlayHints.h"
#include "HeuristicResolver.h"
#include "ParsedAST.h"
-#include "support/Logger.h"
#include "clang/AST/DeclarationName.h"
#include "clang/AST/ExprCXX.h"
#include "clang/AST/RecursiveASTVisitor.h"
#include "clang/Basic/SourceManager.h"
-#include "llvm/Support/raw_ostream.h"
namespace clang {
namespace clangd {
+namespace {
+
+// For now, inlay hints are always anchored at the left or right of their range.
+enum class HintSide { Left, Right };
class InlayHintVisitor : public RecursiveASTVisitor<InlayHintVisitor> {
public:
- InlayHintVisitor(std::vector<InlayHint> &Results, ParsedAST &AST)
+ InlayHintVisitor(std::vector<InlayHint> &Results, ParsedAST &AST,
+ llvm::Optional<Range> RestrictRange)
: Results(Results), AST(AST.getASTContext()),
+ RestrictRange(std::move(RestrictRange)),
MainFileID(AST.getSourceManager().getMainFileID()),
Resolver(AST.getHeuristicResolver()),
TypeHintPolicy(this->AST.getPrintingPolicy()),
@@ -88,7 +92,7 @@ class InlayHintVisitor : public RecursiveASTVisitor<InlayHintVisitor> {
QualType Deduced = AT->getDeducedType();
if (!Deduced.isNull()) {
addTypeHint(D->getFunctionTypeLoc().getRParenLoc(), D->getReturnType(),
- "-> ");
+ /*Prefix=*/"-> ");
}
}
@@ -100,7 +104,7 @@ class InlayHintVisitor : public RecursiveASTVisitor<InlayHintVisitor> {
// but show hints for the individual bindings.
if (auto *DD = dyn_cast<DecompositionDecl>(D)) {
for (auto *Binding : DD->bindings()) {
- addTypeHint(Binding->getLocation(), Binding->getType(), ": ",
+ addTypeHint(Binding->getLocation(), Binding->getType(), /*Prefix=*/": ",
StructuredBindingPolicy);
}
return true;
@@ -113,7 +117,7 @@ class InlayHintVisitor : public RecursiveASTVisitor<InlayHintVisitor> {
// (e.g. for `const auto& x = 42`, print `const int&`).
// Alternatively, we could place the hint on the `auto`
// (and then just print the type deduced for the `auto`).
- addTypeHint(D->getLocation(), D->getType(), ": ");
+ addTypeHint(D->getLocation(), D->getType(), /*Prefix=*/": ");
}
}
return true;
@@ -160,8 +164,9 @@ class InlayHintVisitor : public RecursiveASTVisitor<InlayHintVisitor> {
if (!shouldHint(Args[I], Name))
continue;
- addInlayHint(Args[I]->getSourceRange(), InlayHintKind::ParameterHint,
- Name.str() + ": ");
+ addInlayHint(Args[I]->getSourceRange(), HintSide::Left,
+ InlayHintKind::ParameterHint, /*Prefix=*/"", Name,
+ /*Suffix=*/": ");
}
}
@@ -313,20 +318,28 @@ class InlayHintVisitor : public RecursiveASTVisitor<InlayHintVisitor> {
return Result;
}
- void addInlayHint(SourceRange R, InlayHintKind Kind, llvm::StringRef Label) {
+ // We pass HintSide rather than SourceLocation because we want to ensure
+ // it is in the same file as the common file range.
+ void addInlayHint(SourceRange R, HintSide Side, InlayHintKind Kind,
+ llvm::StringRef Prefix, llvm::StringRef Label,
+ llvm::StringRef Suffix) {
auto FileRange =
toHalfOpenFileRange(AST.getSourceManager(), AST.getLangOpts(), R);
if (!FileRange)
return;
+ Range LSPRange{
+ sourceLocToPosition(AST.getSourceManager(), FileRange->getBegin()),
+ sourceLocToPosition(AST.getSourceManager(), FileRange->getEnd())};
+ Position LSPPos = Side == HintSide::Left ? LSPRange.start : LSPRange.end;
+ if (RestrictRange &&
+ (LSPPos < RestrictRange->start || !(LSPPos < RestrictRange->end)))
+ return;
// The hint may be in a file other than the main file (for example, a header
// file that was included after the preamble), do not show in that case.
if (!AST.getSourceManager().isWrittenInMainFile(FileRange->getBegin()))
return;
- Results.push_back(InlayHint{
- Range{
- sourceLocToPosition(AST.getSourceManager(), FileRange->getBegin()),
- sourceLocToPosition(AST.getSourceManager(), FileRange->getEnd())},
- Kind, Label.str()});
+ Results.push_back(
+ InlayHint{LSPPos, LSPRange, Kind, (Prefix + Label + Suffix).str()});
}
void addTypeHint(SourceRange R, QualType T, llvm::StringRef Prefix) {
@@ -341,11 +354,13 @@ class InlayHintVisitor : public RecursiveASTVisitor<InlayHintVisitor> {
std::string TypeName = T.getAsString(Policy);
if (TypeName.length() < TypeNameLimit)
- addInlayHint(R, InlayHintKind::TypeHint, std::string(Prefix) + TypeName);
+ addInlayHint(R, HintSide::Right, InlayHintKind::TypeHint, Prefix,
+ TypeName, /*Suffix=*/"");
}
std::vector<InlayHint> &Results;
ASTContext &AST;
+ llvm::Optional<Range> RestrictRange;
FileID MainFileID;
StringRef MainFileBuf;
const HeuristicResolver *Resolver;
@@ -361,9 +376,12 @@ class InlayHintVisitor : public RecursiveASTVisitor<InlayHintVisitor> {
static const size_t TypeNameLimit = 32;
};
-std::vector<InlayHint> inlayHints(ParsedAST &AST) {
+} // namespace
+
+std::vector<InlayHint> inlayHints(ParsedAST &AST,
+ llvm::Optional<Range> RestrictRange) {
std::vector<InlayHint> Results;
- InlayHintVisitor Visitor(Results, AST);
+ InlayHintVisitor Visitor(Results, AST, std::move(RestrictRange));
Visitor.TraverseAST(AST.getASTContext());
// De-duplicate hints. Duplicates can sometimes occur due to e.g. explicit
diff --git a/clang-tools-extra/clangd/InlayHints.h b/clang-tools-extra/clangd/InlayHints.h
index 4157a45bd437..a3c16788435d 100644
--- a/clang-tools-extra/clangd/InlayHints.h
+++ b/clang-tools-extra/clangd/InlayHints.h
@@ -22,8 +22,10 @@ namespace clang {
namespace clangd {
class ParsedAST;
-// Compute and return all inlay hints for a file.
-std::vector<InlayHint> inlayHints(ParsedAST &AST);
+/// Compute and return inlay hints for a file.
+/// If RestrictRange is set, return only hints whose location is in that range.
+std::vector<InlayHint> inlayHints(ParsedAST &AST,
+ llvm::Optional<Range> RestrictRange);
} // namespace clangd
} // namespace clang
diff --git a/clang-tools-extra/clangd/Protocol.cpp b/clang-tools-extra/clangd/Protocol.cpp
index 6c3b1444852b..42f452f74f97 100644
--- a/clang-tools-extra/clangd/Protocol.cpp
+++ b/clang-tools-extra/clangd/Protocol.cpp
@@ -1317,7 +1317,7 @@ llvm::json::Value toJSON(const CallHierarchyOutgoingCall &C) {
bool fromJSON(const llvm::json::Value &Params, InlayHintsParams &R,
llvm::json::Path P) {
llvm::json::ObjectMapper O(Params, P);
- return O && O.map("textDocument", R.textDocument);
+ return O && O.map("textDocument", R.textDocument) && O.map("range", R.range);
}
llvm::json::Value toJSON(InlayHintKind K) {
@@ -1331,16 +1331,18 @@ llvm::json::Value toJSON(InlayHintKind K) {
}
llvm::json::Value toJSON(const InlayHint &H) {
- return llvm::json::Object{
- {"range", H.range}, {"kind", H.kind}, {"label", H.label}};
+ return llvm::json::Object{{"position", H.position},
+ {"range", H.range},
+ {"kind", H.kind},
+ {"label", H.label}};
}
bool operator==(const InlayHint &A, const InlayHint &B) {
- return std::tie(A.kind, A.range, A.label) ==
- std::tie(B.kind, B.range, B.label);
+ return std::tie(A.position, A.range, A.kind, A.label) ==
+ std::tie(B.position, B.range, B.kind, B.label);
}
bool operator<(const InlayHint &A, const InlayHint &B) {
- return std::tie(A.kind, A.range, A.label) <
- std::tie(B.kind, B.range, B.label);
+ return std::tie(A.position, A.range, A.kind, A.label) <
+ std::tie(B.position, B.range, B.kind, B.label);
}
static const char *toString(OffsetEncoding OE) {
diff --git a/clang-tools-extra/clangd/Protocol.h b/clang-tools-extra/clangd/Protocol.h
index 819fa51821e0..d7ca580dceff 100644
--- a/clang-tools-extra/clangd/Protocol.h
+++ b/clang-tools-extra/clangd/Protocol.h
@@ -1512,10 +1512,15 @@ struct CallHierarchyOutgoingCall {
};
llvm::json::Value toJSON(const CallHierarchyOutgoingCall &);
-/// The parameter of a `textDocument/inlayHints` request.
+/// The parameter of a `clangd/inlayHints` request.
+///
+/// This is a clangd extension.
struct InlayHintsParams {
/// The text document for which inlay hints are requested.
TextDocumentIdentifier textDocument;
+
+ /// If set, requests inlay hints for only part of the document.
+ llvm::Optional<Range> range;
};
bool fromJSON(const llvm::json::Value &, InlayHintsParams &, llvm::json::Path);
@@ -1542,19 +1547,27 @@ enum class InlayHintKind {
llvm::json::Value toJSON(InlayHintKind);
/// An annotation to be displayed inline next to a range of source code.
+///
+/// This is a clangd extension.
struct InlayHint {
+ /// The position between two characters where the hint should be displayed.
+ ///
+ /// For example, n parameter hint may be positioned before an argument.
+ Position position;
+
/// The range of source code to which the hint applies.
- /// We provide the entire range, rather than just the endpoint
- /// relevant to `position` (e.g. the start of the range for
- /// InlayHintPosition::Before), to give clients the flexibility
- /// to make choices like only displaying the hint while the cursor
- /// is over the range, rather than displaying it all the time.
+ ///
+ /// For example, a parameter hint may have the argument as its range.
+ /// The range allows clients more flexibility of when/how to display the hint.
Range range;
- /// The type of hint.
+ /// The type of hint, such as a parameter hint.
InlayHintKind kind;
- /// The label that is displayed in the editor.
+ /// The label that is displayed in the editor, such as a parameter name.
+ ///
+ /// The label may contain punctuation and/or whitespace to allow it to read
+ /// naturally when placed inline with the code.
std::string label;
};
llvm::json::Value toJSON(const InlayHint &);
diff --git a/clang-tools-extra/clangd/test/initialize-params.test b/clang-tools-extra/clangd/test/initialize-params.test
index 2affc8b2466d..e5701876684e 100644
--- a/clang-tools-extra/clangd/test/initialize-params.test
+++ b/clang-tools-extra/clangd/test/initialize-params.test
@@ -7,6 +7,7 @@
# CHECK-NEXT: "capabilities": {
# CHECK-NEXT: "astProvider": true,
# CHECK-NEXT: "callHierarchyProvider": true,
+# CHECK-NEXT: "clangdInlayHintsProvider": true,
# CHECK-NEXT: "codeActionProvider": true,
# CHECK-NEXT: "compilationDatabase": {
# CHECK-NEXT: "automaticReload": true
diff --git a/clang-tools-extra/clangd/test/inlayHints.test b/clang-tools-extra/clangd/test/inlayHints.test
new file mode 100644
index 000000000000..48b1d83dca25
--- /dev/null
+++ b/clang-tools-extra/clangd/test/inlayHints.test
@@ -0,0 +1,45 @@
+# RUN: clangd -lit-test < %s | FileCheck %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{
+ "uri":"test:///main.cpp",
+ "languageId":"cpp",
+ "version":1,
+ "text":"int foo(int bar);\nint x = foo(42);\nint y = foo(42);"
+}}}
+---
+{"jsonrpc":"2.0","id":1,"method":"clangd/inlayHints","params":{
+ "textDocument":{"uri":"test:///main.cpp"},
+ "range":{
+ "start": {"line":1,"character":0},
+ "end": {"line":2,"character":0}
+ }
+}}
+# CHECK: "id": 1,
+# CHECK-NEXT: "jsonrpc": "2.0",
+# CHECK-NEXT: "result": [
+# CHECK-NEXT: {
+# CHECK-NEXT: "kind": "parameter",
+# CHECK-NEXT: "label": "bar: ",
+# CHECK-NEXT: "position": {
+# CHECK-NEXT: "character": 12,
+# CHECK-NEXT: "line": 1
+# CHECK-NEXT: },
+# CHECK-NEXT: "range": {
+# CHECK-NEXT: "end": {
+# CHECK-NEXT: "character": 14,
+# CHECK-NEXT: "line": 1
+# CHECK-NEXT: },
+# CHECK-NEXT: "start": {
+# CHECK-NEXT: "character": 12,
+# CHECK-NEXT: "line": 1
+# CHECK-NEXT: }
+# CHECK-NEXT: }
+# CHECK-NEXT: }
+# CHECK-NEXT: ]
+# CHECK-NEXT:}
+---
+{"jsonrpc":"2.0","id":100,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}
+
diff --git a/clang-tools-extra/clangd/tool/ClangdMain.cpp b/clang-tools-extra/clangd/tool/ClangdMain.cpp
index ba38aeed87a1..605114233780 100644
--- a/clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ b/clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -327,8 +327,14 @@ opt<bool> FoldingRanges{
Hidden,
};
-opt<bool> InlayHints{"inlay-hints", cat(Features),
- desc("Enable preview of InlayHints feature"), init(false)};
+opt<bool> InlayHints{
+ "inlay-hints",
+ cat(Features),
+ desc("Enable InlayHints feature"),
+ init(ClangdLSPServer::Options().InlayHints),
+ // FIXME: allow inlayHints to be disabled in Config and remove this option.
+ Hidden,
+};
opt<unsigned> WorkerThreadsCount{
"j",
diff --git a/clang-tools-extra/clangd/unittests/InlayHintTests.cpp b/clang-tools-extra/clangd/unittests/InlayHintTests.cpp
index b6c6dd17dbe3..e8880981c1a4 100644
--- a/clang-tools-extra/clangd/unittests/InlayHintTests.cpp
+++ b/clang-tools-extra/clangd/unittests/InlayHintTests.cpp
@@ -23,20 +23,23 @@ std::ostream &operator<<(std::ostream &Stream, const InlayHint &Hint) {
namespace {
-using ::testing::UnorderedElementsAre;
+using ::testing::ElementsAre;
std::vector<InlayHint> hintsOfKind(ParsedAST &AST, InlayHintKind Kind) {
std::vector<InlayHint> Result;
- for (auto &Hint : inlayHints(AST)) {
+ for (auto &Hint : inlayHints(AST, /*RestrictRange=*/llvm::None)) {
if (Hint.kind == Kind)
Result.push_back(Hint);
}
return Result;
}
+enum HintSide { Left, Right };
+
struct ExpectedHint {
std::string Label;
std::string RangeName;
+ HintSide Side = Left;
friend std::ostream &operator<<(std::ostream &Stream,
const ExpectedHint &Hint) {
@@ -46,9 +49,13 @@ struct ExpectedHint {
MATCHER_P2(HintMatcher, Expected, Code, "") {
return arg.label == Expected.Label &&
- arg.range == Code.range(Expected.RangeName);
+ arg.range == Code.range(Expected.RangeName) &&
+ arg.position ==
+ ((Expected.Side == Left) ? arg.range.start : arg.range.end);
}
+MATCHER_P(labelIs, Label, "") { return arg.label == Label; }
+
template <typename... ExpectedHints>
void assertHints(InlayHintKind Kind, llvm::StringRef AnnotatedSource,
ExpectedHints... Expected) {
@@ -58,18 +65,23 @@ void assertHints(InlayHintKind Kind, llvm::StringRef AnnotatedSource,
auto AST = TU.build();
EXPECT_THAT(hintsOfKind(AST, Kind),
- UnorderedElementsAre(HintMatcher(Expected, Source)...));
+ ElementsAre(HintMatcher(Expected, Source)...));
}
+// Hack to allow expression-statements operating on parameter packs in C++14.
+template <typename... T> void ignore(T &&...) {}
+
template <typename... ExpectedHints>
void assertParameterHints(llvm::StringRef AnnotatedSource,
ExpectedHints... Expected) {
+ ignore(Expected.Side = Left...);
assertHints(InlayHintKind::ParameterHint, AnnotatedSource, Expected...);
}
template <typename... ExpectedHints>
void assertTypeHints(llvm::StringRef AnnotatedSource,
ExpectedHints... Expected) {
+ ignore(Expected.Side = Right...);
assertHints(InlayHintKind::TypeHint, AnnotatedSource, Expected...);
}
@@ -631,6 +643,18 @@ TEST(TypeHints, Deduplication) {
ExpectedHint{": int", "var"});
}
+TEST(InlayHints, RestrictRange) {
+ Annotations Code(R"cpp(
+ auto a = false;
+ [[auto b = 1;
+ auto c = '2';]]
+ auto d = 3.f;
+ )cpp");
+ auto AST = TestTU::withCode(Code.code()).build();
+ EXPECT_THAT(inlayHints(AST, Code.range()),
+ ElementsAre(labelIs(": int"), labelIs(": char")));
+}
+
// FIXME: Low-hanging fruit where we could omit a type hint:
// - auto x = TypeName(...);
// - auto x = (TypeName) (...);
More information about the cfe-commits
mailing list