[clang-tools-extra] r320555 - [clangd] Emit ranges for clangd diagnostics, and fix off-by-one positions
Sam McCall via cfe-commits
cfe-commits at lists.llvm.org
Wed Dec 13 00:48:42 PST 2017
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"}
More information about the cfe-commits
mailing list