[clangd] Emit ranges for clangd diagnostics, and fix off-by-one positions

Summary:
 - when the diagnostic has an explicit range, we prefer that
 - if the diagnostic has a fixit, its RemoveRange is our next choice
 - otherwise we try to expand the diagnostic location into a whole token.
   (inspired by VSCode, which does this client-side when given an empty range)
 - if all else fails, we return the zero-width range as now.
   (clients react in different ways to this, highlighting a token or a char)
 - this includes the off-by-one fix from D40860, and borrows heavily from it

Reviewers: rwols, hokein

Subscribers: klimek, ilya-biryukov, cfe-commits

Differential Revision: https://reviews.llvm.org/D41118

llvm-svn: 320555

diff --git a/clang-tools-extra/clangd/ClangdLSPServer.cpp b/clang-tools-extra/clangd/ClangdLSPServer.cpp
index 18f41ebf0f1ba3620d676ef7ef0f7caf8fa697b8..00b51fc426ecd1dcea8d28cfe93b6f1c0f6c64eb 100644 (file)
--- a/clang-tools-extra/clangd/ClangdLSPServer.cpp
+++ b/clang-tools-extra/clangd/ClangdLSPServer.cpp
@@ -202,9 +202,7 @@ void ClangdLSPServer::onCodeAction(Ctx C, CodeActionParams &Params) {
   std::string Code = Server.getDocument(Params.textDocument.uri.file);
   json::ary Commands;
   for (Diagnostic &D : Params.context.diagnostics) {
-    std::vector<clang::tooling::Replacement> Fixes =
-        getFixIts(Params.textDocument.uri.file, D);
-    auto Edits = replacementsToEdits(Code, Fixes);
+    auto Edits = getFixIts(Params.textDocument.uri.file, D);
     if (!Edits.empty()) {
       WorkspaceEdit WE;
       WE.changes = {{Params.textDocument.uri.uri, std::move(Edits)}};
@@ -306,8 +304,8 @@ bool ClangdLSPServer::run(std::istream &In) {
   return ShutdownRequestReceived;
 }
 
-std::vector<clang::tooling::Replacement>
-ClangdLSPServer::getFixIts(StringRef File, const clangd::Diagnostic &D) {
+std::vector<TextEdit> ClangdLSPServer::getFixIts(StringRef File,
+                                                 const clangd::Diagnostic &D) {
   std::lock_guard<std::mutex> Lock(FixItsMutex);
   auto DiagToFixItsIter = FixItsMap.find(File);
   if (DiagToFixItsIter == FixItsMap.end())

diff --git a/clang-tools-extra/clangd/ClangdLSPServer.h b/clang-tools-extra/clangd/ClangdLSPServer.h
index a4518f9fef9b6be70592f95df1d260e2bd2cb768..8441fee61a6d33b7cbc0f8ec55f4675629e14b9b 100644 (file)
--- a/clang-tools-extra/clangd/ClangdLSPServer.h
+++ b/clang-tools-extra/clangd/ClangdLSPServer.h
@@ -74,8 +74,7 @@ private:
   void onCommand(Ctx C, ExecuteCommandParams &Params) override;
   void onRename(Ctx C, RenameParams &Parames) override;
 
-  std::vector<clang::tooling::Replacement>
-  getFixIts(StringRef File, const clangd::Diagnostic &D);
+  std::vector<TextEdit> getFixIts(StringRef File, const clangd::Diagnostic &D);
 
   JSONOutput &Out;
   /// Used to indicate that the 'shutdown' request was received from the
@@ -88,7 +87,7 @@ private:
   bool IsDone = false;
 
   std::mutex FixItsMutex;
-  typedef std::map<clangd::Diagnostic, std::vector<clang::tooling::Replacement>>
+  typedef std::map<clangd::Diagnostic, std::vector<TextEdit>>
       DiagnosticToReplacementMap;
   /// Caches FixIts per file and diagnostics
   llvm::StringMap<DiagnosticToReplacementMap> FixItsMap;

diff --git a/clang-tools-extra/clangd/ClangdUnit.cpp b/clang-tools-extra/clangd/ClangdUnit.cpp
index 313c72edfce8b6977025854f2fdc1eacc10e9e5c..c699c2f0a4090ce531cafb58a18b5b43594b19c6 100644 (file)
--- a/clang-tools-extra/clangd/ClangdUnit.cpp
+++ b/clang-tools-extra/clangd/ClangdUnit.cpp
@@ -120,39 +120,100 @@ static int getSeverity(DiagnosticsEngine::Level L) {
   llvm_unreachable("Unknown diagnostic level!");
 }
 
-llvm::Optional<DiagWithFixIts> toClangdDiag(const StoredDiagnostic &D) {
-  auto Location = D.getLocation();
-  if (!Location.isValid() || !Location.getManager().isInMainFile(Location))
-    return llvm::None;
+// Checks whether a location is within a half-open range.
+// Note that clang also uses closed source ranges, which this can't handle!
+bool locationInRange(SourceLocation L, CharSourceRange R,
+                     const SourceManager &M) {
+  assert(R.isCharRange());
+  if (!R.isValid() || M.getFileID(R.getBegin()) != M.getFileID(R.getEnd()) ||
+      M.getFileID(R.getBegin()) != M.getFileID(L))
+    return false;
+  return L != R.getEnd() && M.isPointWithin(L, R.getBegin(), R.getEnd());
+}
 
-  Position P;
-  P.line = Location.getSpellingLineNumber() - 1;
-  P.character = Location.getSpellingColumnNumber();
-  Range R = {P, P};
-  clangd::Diagnostic Diag = {R, getSeverity(D.getLevel()), D.getMessage()};
+// Converts a half-open clang source range to an LSP range.
+// Note that clang also uses closed source ranges, which this can't handle!
+Range toRange(CharSourceRange R, const SourceManager &M) {
+  // Clang is 1-based, LSP uses 0-based indexes.
+  return {{static_cast<int>(M.getSpellingLineNumber(R.getBegin())) - 1,
+           static_cast<int>(M.getSpellingColumnNumber(R.getBegin())) - 1},
+          {static_cast<int>(M.getSpellingLineNumber(R.getEnd())) - 1,
+           static_cast<int>(M.getSpellingColumnNumber(R.getEnd())) - 1}};
+}
 
-  llvm::SmallVector<tooling::Replacement, 1> FixItsForDiagnostic;
-  for (const FixItHint &Fix : D.getFixIts()) {
-    FixItsForDiagnostic.push_back(clang::tooling::Replacement(
-        Location.getManager(), Fix.RemoveRange, Fix.CodeToInsert));
+// Clang diags have a location (shown as ^) and 0 or more ranges (~~~~).
+// LSP needs a single range.
+Range diagnosticRange(const clang::Diagnostic &D, const LangOptions &L) {
+  auto &M = D.getSourceManager();
+  auto Loc = M.getFileLoc(D.getLocation());
+  // Accept the first range that contains the location.
+  for (const auto &CR : D.getRanges()) {
+    auto R = Lexer::makeFileCharRange(CR, M, L);
+    if (locationInRange(Loc, R, M))
+      return toRange(R, M);
+  }
+  // The range may be given as a fixit hint instead.
+  for (const auto &F : D.getFixItHints()) {
+    auto R = Lexer::makeFileCharRange(F.RemoveRange, M, L);
+    if (locationInRange(Loc, R, M))
+      return toRange(R, M);
   }
-  return DiagWithFixIts{Diag, std::move(FixItsForDiagnostic)};
+  // If no suitable range is found, just use the token at the location.
+  auto R = Lexer::makeFileCharRange(CharSourceRange::getTokenRange(Loc), M, L);
+  if (!R.isValid()) // Fall back to location only, let the editor deal with it.
+    R = CharSourceRange::getCharRange(Loc);
+  return toRange(R, M);
+}
+
+TextEdit toTextEdit(const FixItHint &FixIt, const SourceManager &M,
+                    const LangOptions &L) {
+  TextEdit Result;
+  Result.range = toRange(Lexer::makeFileCharRange(FixIt.RemoveRange, M, L), M);
+  Result.newText = FixIt.CodeToInsert;
+  return Result;
+}
+
+llvm::Optional<DiagWithFixIts> toClangdDiag(const clang::Diagnostic &D,
+                                            DiagnosticsEngine::Level Level,
+                                            const LangOptions &LangOpts) {
+  if (!D.hasSourceManager() || !D.getLocation().isValid() ||
+      !D.getSourceManager().isInMainFile(D.getLocation()))
+    return llvm::None;
+
+  DiagWithFixIts Result;
+  Result.Diag.range = diagnosticRange(D, LangOpts);
+  Result.Diag.severity = getSeverity(Level);
+  SmallString<64> Message;
+  D.FormatDiagnostic(Message);
+  Result.Diag.message = Message.str();
+  for (const FixItHint &Fix : D.getFixItHints())
+    Result.FixIts.push_back(toTextEdit(Fix, D.getSourceManager(), LangOpts));
+  return std::move(Result);
 }
 
 class StoreDiagsConsumer : public DiagnosticConsumer {
 public:
   StoreDiagsConsumer(std::vector<DiagWithFixIts> &Output) : Output(Output) {}
 
+  // Track language options in case we need to expand token ranges.
+  void BeginSourceFile(const LangOptions &Opts, const Preprocessor *) override {
+    LangOpts = Opts;
+  }
+
+  void EndSourceFile() override { LangOpts = llvm::None; }
+
   void HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
                         const clang::Diagnostic &Info) override {
     DiagnosticConsumer::HandleDiagnostic(DiagLevel, Info);
 
-    if (auto convertedDiag = toClangdDiag(StoredDiagnostic(DiagLevel, Info)))
-      Output.push_back(std::move(*convertedDiag));
+    if (LangOpts)
+      if (auto D = toClangdDiag(Info, DiagLevel, *LangOpts))
+        Output.push_back(std::move(*D));
   }
 
 private:
   std::vector<DiagWithFixIts> &Output;
+  llvm::Optional<LangOptions> LangOpts;
 };
 
 template <class T> bool futureIsReady(std::shared_future<T> const &Future) {

diff --git a/clang-tools-extra/clangd/ClangdUnit.h b/clang-tools-extra/clangd/ClangdUnit.h
index 3bfd3a1f6ced476bec80f152bd6cb369ec291372..61684f6604fee19962ebbc1b401d956202e5908a 100644 (file)
--- a/clang-tools-extra/clangd/ClangdUnit.h
+++ b/clang-tools-extra/clangd/ClangdUnit.h
@@ -45,7 +45,7 @@ class Logger;
 /// A diagnostic with its FixIts.
 struct DiagWithFixIts {
   clangd::Diagnostic Diag;
-  llvm::SmallVector<tooling::Replacement, 1> FixIts;
+  llvm::SmallVector<TextEdit, 1> FixIts;
 };
 
 // Stores Preamble and associated data.

diff --git a/clang-tools-extra/test/clangd/diagnostics.test b/clang-tools-extra/test/clangd/diagnostics.test
index 85bbf984bf0d8986ff18eec435ead3d814f81ac8..2332d670a030f96f7750228a814fb288f7cddc60 100644 (file)
--- a/clang-tools-extra/test/clangd/diagnostics.test
+++ b/clang-tools-extra/test/clangd/diagnostics.test
@@ -15,11 +15,11 @@ Content-Length: 152
 # CHECK-NEXT:        "message": "return type of 'main' is not 'int'",\r
 # CHECK-NEXT:        "range": {\r
 # CHECK-NEXT:          "end": {\r
-# CHECK-NEXT:            "character": 1,\r
+# CHECK-NEXT:            "character": 4,\r
 # CHECK-NEXT:            "line": 0\r
 # CHECK-NEXT:          },\r
 # CHECK-NEXT:          "start": {\r
-# CHECK-NEXT:            "character": 1,\r
+# CHECK-NEXT:            "character": 0,\r
 # CHECK-NEXT:            "line": 0\r
 # CHECK-NEXT:          }\r
 # CHECK-NEXT:        },\r
@@ -29,11 +29,11 @@ Content-Length: 152
 # CHECK-NEXT:        "message": "change return type to 'int'",\r
 # CHECK-NEXT:        "range": {\r
 # CHECK-NEXT:          "end": {\r
-# CHECK-NEXT:            "character": 1,\r
+# CHECK-NEXT:            "character": 4,\r
 # CHECK-NEXT:            "line": 0\r
 # CHECK-NEXT:          },\r
 # CHECK-NEXT:          "start": {\r
-# CHECK-NEXT:            "character": 1,\r
+# CHECK-NEXT:            "character": 0,\r
 # CHECK-NEXT:            "line": 0\r
 # CHECK-NEXT:          }\r
 # CHECK-NEXT:        },\r

diff --git a/clang-tools-extra/test/clangd/execute-command.test b/clang-tools-extra/test/clangd/execute-command.test
index 833690d6b2ff9a501d5928642663f64c07a58e7d..27ab83e73d4df1b253c9ecd5a1616bca4a288636 100644 (file)
--- a/clang-tools-extra/test/clangd/execute-command.test
+++ b/clang-tools-extra/test/clangd/execute-command.test
@@ -15,11 +15,11 @@ Content-Length: 180
 # CHECK-NEXT:        "message": "using the result of an assignment as a condition without parentheses",\r
 # CHECK-NEXT:        "range": {\r
 # CHECK-NEXT:          "end": {\r
-# CHECK-NEXT:            "character": 35,\r
+# CHECK-NEXT:            "character": 37,\r
 # CHECK-NEXT:            "line": 0\r
 # CHECK-NEXT:          },\r
 # CHECK-NEXT:          "start": {\r
-# CHECK-NEXT:            "character": 35,\r
+# CHECK-NEXT:            "character": 32,\r
 # CHECK-NEXT:            "line": 0\r
 # CHECK-NEXT:          }\r
 # CHECK-NEXT:        },\r
@@ -33,7 +33,7 @@ Content-Length: 180
 # CHECK-NEXT:            "line": 0\r
 # CHECK-NEXT:          },\r
 # CHECK-NEXT:          "start": {\r
-# CHECK-NEXT:            "character": 35,\r
+# CHECK-NEXT:            "character": 34,\r
 # CHECK-NEXT:            "line": 0\r
 # CHECK-NEXT:          }\r
 # CHECK-NEXT:        },\r
@@ -47,7 +47,7 @@ Content-Length: 180
 # CHECK-NEXT:            "line": 0\r
 # CHECK-NEXT:          },\r
 # CHECK-NEXT:          "start": {\r
-# CHECK-NEXT:            "character": 35,\r
+# CHECK-NEXT:            "character": 34,\r
 # CHECK-NEXT:            "line": 0\r
 # CHECK-NEXT:          }\r
 # CHECK-NEXT:        },\r

diff --git a/clang-tools-extra/test/clangd/extra-flags.test b/clang-tools-extra/test/clangd/extra-flags.test
index 8defbefa65914335fe1dd3e062cea2ca741154f1..b99372054bb62a39be760bd18f6d829ddf2f80b9 100644 (file)
--- a/clang-tools-extra/test/clangd/extra-flags.test
+++ b/clang-tools-extra/test/clangd/extra-flags.test
@@ -19,7 +19,7 @@ Content-Length: 205
 # CHECK-NEXT:            "line": 0\r
 # CHECK-NEXT:          },\r
 # CHECK-NEXT:          "start": {\r
-# CHECK-NEXT:            "character": 28,\r
+# CHECK-NEXT:            "character": 27,\r
 # CHECK-NEXT:            "line": 0\r
 # CHECK-NEXT:          }\r
 # CHECK-NEXT:        },\r
@@ -33,7 +33,7 @@ Content-Length: 205
 # CHECK-NEXT:            "line": 0\r
 # CHECK-NEXT:          },\r
 # CHECK-NEXT:          "start": {\r
-# CHECK-NEXT:            "character": 19,\r
+# CHECK-NEXT:            "character": 18,\r
 # CHECK-NEXT:            "line": 0\r
 # CHECK-NEXT:          }\r
 # CHECK-NEXT:        },\r
@@ -56,7 +56,7 @@ Content-Length: 175
 # CHECK-NEXT:            "line": 0\r
 # CHECK-NEXT:          },\r
 # CHECK-NEXT:          "start": {\r
-# CHECK-NEXT:            "character": 28,\r
+# CHECK-NEXT:            "character": 27,\r
 # CHECK-NEXT:            "line": 0\r
 # CHECK-NEXT:          }\r
 # CHECK-NEXT:        },\r
@@ -70,7 +70,7 @@ Content-Length: 175
 # CHECK-NEXT:            "line": 0\r
 # CHECK-NEXT:          },\r
 # CHECK-NEXT:          "start": {\r
-# CHECK-NEXT:            "character": 19,\r
+# CHECK-NEXT:            "character": 18,\r
 # CHECK-NEXT:            "line": 0\r
 # CHECK-NEXT:          }\r
 # CHECK-NEXT:        },\r

diff --git a/clang-tools-extra/test/clangd/fixits.test b/clang-tools-extra/test/clangd/fixits.test
index 2a16c8c789a0a852e39776f1325c445491b0229e..ea6bd1c33675770c1b75441924e3499d499cfc5b 100644 (file)
--- a/clang-tools-extra/test/clangd/fixits.test
+++ b/clang-tools-extra/test/clangd/fixits.test
@@ -15,11 +15,11 @@ Content-Length: 180
 # CHECK-NEXT:        "message": "using the result of an assignment as a condition without parentheses",\r
 # CHECK-NEXT:        "range": {\r
 # CHECK-NEXT:          "end": {\r
-# CHECK-NEXT:            "character": 35,\r
+# CHECK-NEXT:            "character": 37,\r
 # CHECK-NEXT:            "line": 0\r
 # CHECK-NEXT:          },\r
 # CHECK-NEXT:          "start": {\r
-# CHECK-NEXT:            "character": 35,\r
+# CHECK-NEXT:            "character": 32,\r
 # CHECK-NEXT:            "line": 0\r
 # CHECK-NEXT:          }\r
 # CHECK-NEXT:        },\r
@@ -33,7 +33,7 @@ Content-Length: 180
 # CHECK-NEXT:            "line": 0\r
 # CHECK-NEXT:          },\r
 # CHECK-NEXT:          "start": {\r
-# CHECK-NEXT:            "character": 35,\r
+# CHECK-NEXT:            "character": 34,\r
 # CHECK-NEXT:            "line": 0\r
 # CHECK-NEXT:          }\r
 # CHECK-NEXT:        },\r
@@ -47,7 +47,7 @@ Content-Length: 180
 # CHECK-NEXT:            "line": 0\r
 # CHECK-NEXT:          },\r
 # CHECK-NEXT:          "start": {\r
-# CHECK-NEXT:            "character": 35,\r
+# CHECK-NEXT:            "character": 34,\r
 # CHECK-NEXT:            "line": 0\r
 # CHECK-NEXT:          }\r
 # CHECK-NEXT:        },\r
@@ -58,7 +58,7 @@ Content-Length: 180
 # CHECK-NEXT:  }\r
 Content-Length: 746\r
 \r
-{"jsonrpc":"2.0","id":2,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"file:///foo.c"},"range":{"start":{"line":104,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":2,"message":"using the result of an assignment as a condition without parentheses"},{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":3,"message":"place parentheses around the assignment to silence this warning"},{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":3,"message":"use '==' to turn this assignment into an equality comparison"}]}}}\r
+{"jsonrpc":"2.0","id":2,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"file:///foo.c"},"range":{"start":{"line":104,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 37}},"severity":2,"message":"using the result of an assignment as a condition without parentheses"},{"range":{"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}},"severity":3,"message":"place parentheses around the assignment to silence this warning"},{"range":{"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}},"severity":3,"message":"use '==' to turn this assignment into an equality comparison"}]}}}\r
 #      CHECK:  "id": 2,\r
 # CHECK-NEXT:  "jsonrpc": "2.0",\r
 # CHECK-NEXT:  "result": [\r
@@ -128,7 +128,7 @@ Content-Length: 746
 # CHECK-NEXT:  ]\r
 Content-Length: 771\r
 \r
-{"jsonrpc":"2.0","id":3,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"file:///foo.c"},"range":{"start":{"line":104,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":2,"code":"1","source":"foo","message":"using the result of an assignment as a condition without parentheses"},{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":3,"message":"place parentheses around the assignment to silence this warning"},{"range":{"start": {"line": 0, "character": 35}, "end": {"line": 0, "character": 35}},"severity":3,"message":"use '==' to turn this assignment into an equality comparison"}]}}}\r
+{"jsonrpc":"2.0","id":3,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"file:///foo.c"},"range":{"start":{"line":104,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 37}},"severity":2,"code":"1","source":"foo","message":"using the result of an assignment as a condition without parentheses"},{"range":{"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}},"severity":3,"message":"place parentheses around the assignment to silence this warning"},{"range":{"start": {"line": 0, "character": 34}, "end": {"line": 0, "character": 35}},"severity":3,"message":"use '==' to turn this assignment into an equality comparison"}]}}}\r
 # Make sure unused "code" and "source" fields ignored gracefully\r
 #      CHECK:  "id": 3,\r
 # CHECK-NEXT:  "jsonrpc": "2.0",\r
@@ -246,4 +246,4 @@ Content-Length: 44
 {"jsonrpc":"2.0","id":4,"method":"shutdown"}\r
 Content-Length: 33\r
 \r
-{"jsonrpc":"2.0":"method":"exit"}\r
+{"jsonrpc":"2.0","method":"exit"}\r