[clangd] Output inlay hints with `clangd --check`

[upsj]

Summary

With the addition of inlay hints to clangd, it would be useful to output them during verbose clangd --check.
This patch adds an output step for inlay hints and unifies the way --check-lines are passed around

[nridge]

Thanks, this seems useful.

clang-tools-extra/clangd/tool/Check.cpp

195

The comment should probably say "... for the entire AST or the specified range"

201

Might be useful for print the hint kind as well?

[upsj]

Output InlayHintKind and improve comments

[upsj]

forgot to include the old changes in the diff

[upsj]

clang-tools-extra/clangd/tool/Check.cpp

195

done

201

right, is the current solution (adding a public toString) okay?

[nridge]

clang-tools-extra/clangd/tool/Check.cpp

201

Looks reasonable to me.

[kadircet]

can you please upload the patch with full context? see https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

clang-tools-extra/clangd/tool/Check.cpp

196

this function always returns true, let's make it void

201

we usually override the llvm::raw_ostream operator<< instead, can you do that for InlayHint and log Hint directly here?

222

this should be LineRange && !LineRange->contains

296

failing inlay hints should not fail the rest, can you put it after C.testLocationFatures call below instead?

[upsj]

• format

[upsj]

Review updates

• Fix inverted check-lines condition
• Output InlayHintKind via ostream operators

[upsj]

push full patch

[sammccall]

(LGTM too, thanks!)

clang-tools-extra/clangd/Protocol.cpp

1322

nit: static (this doesn't need to be public)
nit: return llvm::StringLiteral (which includes the length)

[upsj]

• use string literal for toString result

[nridge]

Let me know if you'd like me to commit the patch for you.

[upsj]

@nridge Please do, I don't have commit permissions :)

clang-tools-extra/clangd/Protocol.h

struct InlayHint {
   ///
   /// 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 &);
 bool operator==(const InlayHint &, const InlayHint &);
 bool operator<(const InlayHint &, const InlayHint &);
+llvm::raw_ostream &operator<<(llvm::raw_ostream &, InlayHintKind);
 
 struct ReferenceContext {
   /// Include the declaration of the current symbol.
   bool includeDeclaration = false;
 };
 
 struct ReferenceParams : public TextDocumentPositionParams {
   ReferenceContext context;

clang-tools-extra/clangd/Protocol.cpp

 //
 //===----------------------------------------------------------------------===//
 
 #include "Protocol.h"
 #include "URI.h"
 #include "support/Logger.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Index/IndexSymbol.h"
+#include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/JSON.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/raw_ostream.h"
 
 namespace clang {
 namespace clangd {

 }
 
 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) && O.map("range", R.range);
 }
 
-llvm::json::Value toJSON(InlayHintKind K) {
+static llvm::StringLiteral toString(InlayHintKind K) {
   switch (K) {
   case InlayHintKind::ParameterHint:
     return "parameter";
   case InlayHintKind::TypeHint:
     return "type";
   case InlayHintKind::DesignatorHint:
     return "designator";
   }
   llvm_unreachable("Unknown clang.clangd.InlayHintKind");
 }
 
+llvm::json::Value toJSON(InlayHintKind K) { return toString(K); }
+
 llvm::json::Value toJSON(const InlayHint &H) {
   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.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.position, A.range, A.kind, A.label) <
          std::tie(B.position, B.range, B.kind, B.label);
 }
 
+llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, InlayHintKind Kind) {
+  return OS << toString(Kind);
+}
+
 static const char *toString(OffsetEncoding OE) {
   switch (OE) {
   case OffsetEncoding::UTF8:
     return "utf-8";
   case OffsetEncoding::UTF16:
     return "utf-16";
   case OffsetEncoding::UTF32:
     return "utf-32";

clang-tools-extra/clangd/tool/Check.cpp

 //===----------------------------------------------------------------------===//
 
 #include "ClangdLSPServer.h"
 #include "CodeComplete.h"
 #include "CompileCommands.h"
 #include "Config.h"
 #include "GlobalCompilationDatabase.h"
 #include "Hover.h"
+#include "InlayHints.h"
 #include "ParsedAST.h"
 #include "Preamble.h"
+#include "Protocol.h"
 #include "SourceCode.h"
 #include "XRefs.h"
 #include "index/CanonicalIncludes.h"
 #include "index/FileIndex.h"
 #include "refactor/Tweak.h"
 #include "support/ThreadsafeFS.h"
 #include "support/Trace.h"
 #include "clang/AST/ASTContext.h"
bool buildAST() {
 
     if (Opts.BuildDynamicSymbolIndex) {
       log("Indexing AST...");
       Index.updateMain(File, *AST);
     }
     return true;
   }
 
+  // Build Inlay Hints for the entire AST or the specified range
+  void buildInlayHints(llvm::Optional<Range> LineRange) {
+    log("Building inlay hints");
+    auto Hints = inlayHints(*AST, LineRange);
+
+    for (const auto &Hint : Hints) {
+      vlog("  {0} {1} {2}", Hint.kind, Hint.position, Hint.label);
+    }
+  }
+
   // Run AST-based features at each token in the file.
-  void testLocationFeatures(
-      llvm::function_ref<bool(const Position &)> ShouldCheckLine,
+  void testLocationFeatures(llvm::Optional<Range> LineRange,
                             const bool EnableCodeCompletion) {
     trace::Span Trace("testLocationFeatures");
     log("Testing features at each token (may be slow in large files)");
     auto &SM = AST->getSourceManager();
     auto SpelledTokens = AST->getTokens().spelledTokens(SM.getMainFileID());
 
     CodeCompleteOptions CCOpts = Opts.CodeComplete;
     CCOpts.Index = &Index;
 
     for (const auto &Tok : SpelledTokens) {
       unsigned Start = AST->getSourceManager().getFileOffset(Tok.location());
       unsigned End = Start + Tok.length();
       Position Pos = offsetToPosition(Inputs.Contents, Start);
 
-      if (!ShouldCheckLine(Pos))
+      if (LineRange && !LineRange->contains(Pos))
         continue;
 
       trace::Span Trace("Token");
       SPAN_ATTACH(Trace, "pos", Pos);
       SPAN_ATTACH(Trace, "text", Tok.text(AST->getSourceManager()));
 
       // FIXME: dumping the tokens may leak sensitive code into bug reports.
       // Add an option to turn this off, once we decide how options work.
       vlog("  {0} {1}", Pos, Tok.text(AST->getSourceManager()));
for (const auto &Tok : SpelledTokens) {
              CC.Completions.empty() ? "<empty>" : CC.Completions[0].Name);
       }
     }
   }
 };
 
 } // namespace
 
-bool check(llvm::StringRef File,
-           llvm::function_ref<bool(const Position &)> ShouldCheckLine,
+bool check(llvm::StringRef File, llvm::Optional<Range> LineRange,
            const ThreadsafeFS &TFS, const ClangdLSPServer::Options &Opts,
            bool EnableCodeCompletion) {
   llvm::SmallString<0> FakeFile;
   llvm::Optional<std::string> Contents;
   if (File.empty()) {
     llvm::sys::path::system_temp_directory(false, FakeFile);
     llvm::sys::path::append(FakeFile, "test.cc");
     File = FakeFile;
auto ContextProvider = ClangdServer::createConfiguredContextProvider(
       Opts.ConfigProvider, nullptr);
   WithContext Ctx(ContextProvider(
       FakeFile.empty()
           ? File
           : /*Don't turn on local configs for an arbitrary temp path.*/ ""));
   Checker C(File, Opts);
   if (!C.buildCommand(TFS) || !C.buildInvocation(TFS, Contents) ||
       !C.buildAST())
     return false;
-  C.testLocationFeatures(ShouldCheckLine, EnableCodeCompletion);
+  C.buildInlayHints(LineRange);
+  C.testLocationFeatures(LineRange, EnableCodeCompletion);
 
   log("All checks completed, {0} errors", C.ErrCount);
   return C.ErrCount == 0;
 }
 
 } // namespace clangd
 } // namespace clang

clang-tools-extra/clangd/tool/ClangdMain.cpp

 #ifdef __GLIBC__
 #include <malloc.h>
 #endif
 
 namespace clang {
 namespace clangd {
 
 // Implemented in Check.cpp.
-bool check(const llvm::StringRef File,
-           llvm::function_ref<bool(const Position &)> ShouldCheckLine,
+bool check(const llvm::StringRef File, llvm::Optional<Range> LineRange,
            const ThreadsafeFS &TFS, const ClangdLSPServer::Options &Opts,
            bool EnableCodeCompletion);
 
 namespace {
 
 using llvm::cl::cat;
 using llvm::cl::CommaSeparated;
 using llvm::cl::desc;
#endif
   if (CheckFile.getNumOccurrences()) {
     llvm::SmallString<256> Path;
     if (auto Error =
             llvm::sys::fs::real_path(CheckFile, Path, /*expand_tilde=*/true)) {
       elog("Failed to resolve path {0}: {1}", CheckFile, Error.message());
       return 1;
     }
     log("Entering check mode (no LSP server)");
-    uint32_t Begin = 0, End = std::numeric_limits<uint32_t>::max();
+    llvm::Optional<Range> CheckLineRange;
     if (!CheckFileLines.empty()) {
+      uint32_t Begin = 0, End = std::numeric_limits<uint32_t>::max();
       StringRef RangeStr(CheckFileLines);
       bool ParseError = RangeStr.consumeInteger(0, Begin);
       if (RangeStr.empty()) {
         End = Begin;
       } else {
         ParseError |= !RangeStr.consume_front("-");
         ParseError |= RangeStr.consumeInteger(0, End);
       }
-      if (ParseError || !RangeStr.empty()) {
-        elog("Invalid --check-line specified. Use Begin-End format, e.g. 3-17");
+      if (ParseError || !RangeStr.empty() || Begin <= 0 || End < Begin) {
+        elog(
+            "Invalid --check-lines specified. Use Begin-End format, e.g. 3-17");
         return 1;
       }
+      CheckLineRange = Range{Position{static_cast<int>(Begin - 1), 0},
+                             Position{static_cast<int>(End), 0}};
     }
-    auto ShouldCheckLine = [&](const Position &Pos) {
-      uint32_t Line = Pos.line + 1; // Position::line is 0-based.
-      return Line >= Begin && Line <= End;
-    };
     // For now code completion is enabled any time the range is limited via
     // --check-lines. If it turns out to be to slow, we can introduce a
     // dedicated flag for that instead.
-    return check(Path, ShouldCheckLine, TFS, Opts,
+    return check(Path, CheckLineRange, TFS, Opts,
                  /*EnableCodeCompletion=*/!CheckFileLines.empty())
                ? 0
                : static_cast<int>(ErrorResultCode::CheckFailed);
   }
   if (!CheckFileLines.empty()) {
     elog("--check-lines requires --check");
     return 1;
   }