From 8111d3b89ccdc854a88113402f4eed935605d6f4 Mon Sep 17 00:00:00 2001 From: Sam McCall Date: Wed, 13 Dec 2017 08:48:42 +0000 Subject: [PATCH] [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 --- clang-tools-extra/clangd/ClangdLSPServer.cpp | 8 +- clang-tools-extra/clangd/ClangdLSPServer.h | 5 +- clang-tools-extra/clangd/ClangdUnit.cpp | 93 +++++++++++++++---- clang-tools-extra/clangd/ClangdUnit.h | 2 +- .../test/clangd/diagnostics.test | 8 +- .../test/clangd/execute-command.test | 8 +- .../test/clangd/extra-flags.test | 8 +- clang-tools-extra/test/clangd/fixits.test | 14 +-- 8 files changed, 102 insertions(+), 44 deletions(-) diff --git a/clang-tools-extra/clangd/ClangdLSPServer.cpp b/clang-tools-extra/clangd/ClangdLSPServer.cpp index 18f41ebf0f1b..00b51fc426ec 100644 --- 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 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 -ClangdLSPServer::getFixIts(StringRef File, const clangd::Diagnostic &D) { +std::vector ClangdLSPServer::getFixIts(StringRef File, + const clangd::Diagnostic &D) { std::lock_guard 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 a4518f9fef9b..8441fee61a6d 100644 --- 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 - getFixIts(StringRef File, const clangd::Diagnostic &D); + std::vector 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> + typedef std::map> DiagnosticToReplacementMap; /// Caches FixIts per file and diagnostics llvm::StringMap FixItsMap; diff --git a/clang-tools-extra/clangd/ClangdUnit.cpp b/clang-tools-extra/clangd/ClangdUnit.cpp index 313c72edfce8..c699c2f0a409 100644 --- 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 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(M.getSpellingLineNumber(R.getBegin())) - 1, + static_cast(M.getSpellingColumnNumber(R.getBegin())) - 1}, + {static_cast(M.getSpellingLineNumber(R.getEnd())) - 1, + static_cast(M.getSpellingColumnNumber(R.getEnd())) - 1}}; +} - llvm::SmallVector 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 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 &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 &Output; + llvm::Optional LangOpts; }; template bool futureIsReady(std::shared_future const &Future) { diff --git a/clang-tools-extra/clangd/ClangdUnit.h b/clang-tools-extra/clangd/ClangdUnit.h index 3bfd3a1f6ced..61684f6604fe 100644 --- 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 FixIts; + llvm::SmallVector 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 85bbf984bf0d..2332d670a030 100644 --- 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'", # CHECK-NEXT: "range": { # CHECK-NEXT: "end": { -# CHECK-NEXT: "character": 1, +# CHECK-NEXT: "character": 4, # CHECK-NEXT: "line": 0 # CHECK-NEXT: }, # CHECK-NEXT: "start": { -# CHECK-NEXT: "character": 1, +# CHECK-NEXT: "character": 0, # CHECK-NEXT: "line": 0 # CHECK-NEXT: } # CHECK-NEXT: }, @@ -29,11 +29,11 @@ Content-Length: 152 # CHECK-NEXT: "message": "change return type to 'int'", # CHECK-NEXT: "range": { # CHECK-NEXT: "end": { -# CHECK-NEXT: "character": 1, +# CHECK-NEXT: "character": 4, # CHECK-NEXT: "line": 0 # CHECK-NEXT: }, # CHECK-NEXT: "start": { -# CHECK-NEXT: "character": 1, +# CHECK-NEXT: "character": 0, # CHECK-NEXT: "line": 0 # CHECK-NEXT: } # CHECK-NEXT: }, diff --git a/clang-tools-extra/test/clangd/execute-command.test b/clang-tools-extra/test/clangd/execute-command.test index 833690d6b2ff..27ab83e73d4d 100644 --- 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", # CHECK-NEXT: "range": { # CHECK-NEXT: "end": { -# CHECK-NEXT: "character": 35, +# CHECK-NEXT: "character": 37, # CHECK-NEXT: "line": 0 # CHECK-NEXT: }, # CHECK-NEXT: "start": { -# CHECK-NEXT: "character": 35, +# CHECK-NEXT: "character": 32, # CHECK-NEXT: "line": 0 # CHECK-NEXT: } # CHECK-NEXT: }, @@ -33,7 +33,7 @@ Content-Length: 180 # CHECK-NEXT: "line": 0 # CHECK-NEXT: }, # CHECK-NEXT: "start": { -# CHECK-NEXT: "character": 35, +# CHECK-NEXT: "character": 34, # CHECK-NEXT: "line": 0 # CHECK-NEXT: } # CHECK-NEXT: }, @@ -47,7 +47,7 @@ Content-Length: 180 # CHECK-NEXT: "line": 0 # CHECK-NEXT: }, # CHECK-NEXT: "start": { -# CHECK-NEXT: "character": 35, +# CHECK-NEXT: "character": 34, # CHECK-NEXT: "line": 0 # CHECK-NEXT: } # CHECK-NEXT: }, diff --git a/clang-tools-extra/test/clangd/extra-flags.test b/clang-tools-extra/test/clangd/extra-flags.test index 8defbefa6591..b99372054bb6 100644 --- 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 # CHECK-NEXT: }, # CHECK-NEXT: "start": { -# CHECK-NEXT: "character": 28, +# CHECK-NEXT: "character": 27, # CHECK-NEXT: "line": 0 # CHECK-NEXT: } # CHECK-NEXT: }, @@ -33,7 +33,7 @@ Content-Length: 205 # CHECK-NEXT: "line": 0 # CHECK-NEXT: }, # CHECK-NEXT: "start": { -# CHECK-NEXT: "character": 19, +# CHECK-NEXT: "character": 18, # CHECK-NEXT: "line": 0 # CHECK-NEXT: } # CHECK-NEXT: }, @@ -56,7 +56,7 @@ Content-Length: 175 # CHECK-NEXT: "line": 0 # CHECK-NEXT: }, # CHECK-NEXT: "start": { -# CHECK-NEXT: "character": 28, +# CHECK-NEXT: "character": 27, # CHECK-NEXT: "line": 0 # CHECK-NEXT: } # CHECK-NEXT: }, @@ -70,7 +70,7 @@ Content-Length: 175 # CHECK-NEXT: "line": 0 # CHECK-NEXT: }, # CHECK-NEXT: "start": { -# CHECK-NEXT: "character": 19, +# CHECK-NEXT: "character": 18, # CHECK-NEXT: "line": 0 # CHECK-NEXT: } # CHECK-NEXT: }, diff --git a/clang-tools-extra/test/clangd/fixits.test b/clang-tools-extra/test/clangd/fixits.test index 2a16c8c789a0..ea6bd1c33675 100644 --- 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", # CHECK-NEXT: "range": { # CHECK-NEXT: "end": { -# CHECK-NEXT: "character": 35, +# CHECK-NEXT: "character": 37, # CHECK-NEXT: "line": 0 # CHECK-NEXT: }, # CHECK-NEXT: "start": { -# CHECK-NEXT: "character": 35, +# CHECK-NEXT: "character": 32, # CHECK-NEXT: "line": 0 # CHECK-NEXT: } # CHECK-NEXT: }, @@ -33,7 +33,7 @@ Content-Length: 180 # CHECK-NEXT: "line": 0 # CHECK-NEXT: }, # CHECK-NEXT: "start": { -# CHECK-NEXT: "character": 35, +# CHECK-NEXT: "character": 34, # CHECK-NEXT: "line": 0 # CHECK-NEXT: } # CHECK-NEXT: }, @@ -47,7 +47,7 @@ Content-Length: 180 # CHECK-NEXT: "line": 0 # CHECK-NEXT: }, # CHECK-NEXT: "start": { -# CHECK-NEXT: "character": 35, +# CHECK-NEXT: "character": 34, # CHECK-NEXT: "line": 0 # CHECK-NEXT: } # CHECK-NEXT: }, @@ -58,7 +58,7 @@ Content-Length: 180 # CHECK-NEXT: } Content-Length: 746 -{"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"}]}}} +{"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"}]}}} # CHECK: "id": 2, # CHECK-NEXT: "jsonrpc": "2.0", # CHECK-NEXT: "result": [ @@ -128,7 +128,7 @@ Content-Length: 746 # CHECK-NEXT: ] Content-Length: 771 -{"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"}]}}} +{"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"}]}}} # Make sure unused "code" and "source" fields ignored gracefully # CHECK: "id": 3, # CHECK-NEXT: "jsonrpc": "2.0", @@ -246,4 +246,4 @@ Content-Length: 44 {"jsonrpc":"2.0","id":4,"method":"shutdown"} Content-Length: 33 -{"jsonrpc":"2.0":"method":"exit"} +{"jsonrpc":"2.0","method":"exit"} -- 2.34.1