Author: sammccall Date: Wed Dec 13 00:48:42 2017 New Revision: 320555 URL: http://llvm.org/viewvc/llvm-project?rev=320555&view=rev Log: [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 Modified: clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp clang-tools-extra/trunk/clangd/ClangdLSPServer.h clang-tools-extra/trunk/clangd/ClangdUnit.cpp clang-tools-extra/trunk/clangd/ClangdUnit.h clang-tools-extra/trunk/test/clangd/diagnostics.test clang-tools-extra/trunk/test/clangd/execute-command.test clang-tools-extra/trunk/test/clangd/extra-flags.test clang-tools-extra/trunk/test/clangd/fixits.test Modified: clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp?rev=320555&r1=320554&r2=320555&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp (original) +++ clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp Wed Dec 13 00:48:42 2017 @@ -202,9 +202,7 @@ void ClangdLSPServer::onCodeAction(Ctx C 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 & 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()) Modified: clang-tools-extra/trunk/clangd/ClangdLSPServer.h URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdLSPServer.h?rev=320555&r1=320554&r2=320555&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/ClangdLSPServer.h (original) +++ clang-tools-extra/trunk/clangd/ClangdLSPServer.h Wed Dec 13 00:48:42 2017 @@ -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; Modified: clang-tools-extra/trunk/clangd/ClangdUnit.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdUnit.cpp?rev=320555&r1=320554&r2=320555&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/ClangdUnit.cpp (original) +++ clang-tools-extra/trunk/clangd/ClangdUnit.cpp Wed Dec 13 00:48:42 2017 @@ -120,39 +120,100 @@ static int getSeverity(DiagnosticsEngine 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()); +} + +// 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}}; +} - Position P; - P.line = Location.getSpellingLineNumber() - 1; - P.character = Location.getSpellingColumnNumber(); - Range R = {P, P}; - clangd::Diagnostic Diag = {R, getSeverity(D.getLevel()), D.getMessage()}; - - 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); } - return DiagWithFixIts{Diag, std::move(FixItsForDiagnostic)}; + // 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); + } + // 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) { Modified: clang-tools-extra/trunk/clangd/ClangdUnit.h URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdUnit.h?rev=320555&r1=320554&r2=320555&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/ClangdUnit.h (original) +++ clang-tools-extra/trunk/clangd/ClangdUnit.h Wed Dec 13 00:48:42 2017 @@ -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. Modified: clang-tools-extra/trunk/test/clangd/diagnostics.test URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clangd/diagnostics.test?rev=320555&r1=320554&r2=320555&view=diff ============================================================================== --- clang-tools-extra/trunk/test/clangd/diagnostics.test (original) +++ clang-tools-extra/trunk/test/clangd/diagnostics.test Wed Dec 13 00:48:42 2017 @@ -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: }, Modified: clang-tools-extra/trunk/test/clangd/execute-command.test URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clangd/execute-command.test?rev=320555&r1=320554&r2=320555&view=diff ============================================================================== --- clang-tools-extra/trunk/test/clangd/execute-command.test (original) +++ clang-tools-extra/trunk/test/clangd/execute-command.test Wed Dec 13 00:48:42 2017 @@ -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: }, Modified: clang-tools-extra/trunk/test/clangd/extra-flags.test URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clangd/extra-flags.test?rev=320555&r1=320554&r2=320555&view=diff ============================================================================== --- clang-tools-extra/trunk/test/clangd/extra-flags.test (original) +++ clang-tools-extra/trunk/test/clangd/extra-flags.test Wed Dec 13 00:48:42 2017 @@ -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: }, Modified: clang-tools-extra/trunk/test/clangd/fixits.test URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clangd/fixits.test?rev=320555&r1=320554&r2=320555&view=diff ============================================================================== --- clang-tools-extra/trunk/test/clangd/fixits.test (original) +++ clang-tools-extra/trunk/test/clangd/fixits.test Wed Dec 13 00:48:42 2017 @@ -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"} _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
