[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

1319 ↗

(On Diff #424839)

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/tool/Check.cpp

 #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"

     return true;
   }
 
+  // Build Inlay Hints for the entire AST
+  bool buildInlayHints(llvm::Optional<Range> LineRange) {
+    log("Building inlay hints");
+    auto Hints = inlayHints(*AST, LineRange);
+
+    for (const auto &Hint : Hints) {
+      vlog("  {0} {1}", Hint.position, Hint.label);
+    }
+    return true;
+  }
+
   // Run AST-based features at each token in the file.
-  void testLocationFeatures(
-      llvm::function_ref<bool(const Position &)> ShouldCheckLine,
-      const bool EnableCodeCompletion) {
+  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();

       unsigned End = Start + Tok.length();
       Position Pos = offsetToPosition(Inputs.Contents, Start);
 
-      if (!ShouldCheckLine(Pos))
+      if (LineRange && LineRange->contains(Pos))
         continue;
 
       trace::Span Trace("Token");

 
 } // 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;

           : /*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())
+      !C.buildAST() || !C.buildInlayHints(LineRange))
     return false;
-  C.testLocationFeatures(ShouldCheckLine, EnableCodeCompletion);
+  C.testLocationFeatures(LineRange, EnableCodeCompletion);
 
   log("All checks completed, {0} errors", C.ErrCount);
   return C.ErrCount == 0;

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

 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);
 

       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()) {

         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);