kadircet created this revision.
kadircet added a reviewer: ilya-biryukov.
Herald added subscribers: cfe-commits, usaxena95, arphaman, jkorous, MaskRay.
Herald added a project: clang.
LSP requires diagnostics to lay inside main file. In clangd we keep
diagnostics in three different cases:
- already in main file
- adjusted to a header included in main file
- has a note covering some range in main file
In the last case, we were not adjusting the diagnostics range to be in main
file, therefore these diagnostics ended up pointing some arbitrary locations.
This patch fixes that issue by adjusting the range of diagnostics to be the
first note inside main file when converting to LSP.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D72458
Files:
clang-tools-extra/clangd/Diagnostics.cpp
clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
Index: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -1014,6 +1014,29 @@
EXPECT_THAT(TU.build().getDiagnostics(), UnorderedElementsAre());
}
+TEST(ToLSPDiag, RangeIsInMain) {
+ ClangdDiagnosticOptions Opts;
+ clangd::Diag D;
+ D.Range = {pos(1, 2), pos(3, 4)};
+ D.Notes.emplace_back();
+ Note &N = D.Notes.back();
+ N.Range = {pos(2, 3), pos(3, 4)};
+
+ D.InsideMainFile = true;
+ N.InsideMainFile = false;
+ toLSPDiags(D, {}, Opts,
+ [&](clangd::Diagnostic LSPDiag, ArrayRef<clangd::Fix>) {
+ EXPECT_EQ(LSPDiag.range, D.Range);
+ });
+
+ D.InsideMainFile = false;
+ N.InsideMainFile = true;
+ toLSPDiags(D, {}, Opts,
+ [&](clangd::Diagnostic LSPDiag, ArrayRef<clangd::Fix>) {
+ EXPECT_EQ(LSPDiag.range, N.Range);
+ });
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/Diagnostics.cpp
===================================================================
--- clang-tools-extra/clangd/Diagnostics.cpp
+++ clang-tools-extra/clangd/Diagnostics.cpp
@@ -22,6 +22,7 @@
#include "clang/Lex/Token.h"
#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/DenseSet.h"
+#include "llvm/ADT/Optional.h"
#include "llvm/ADT/SmallString.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/ADT/Twine.h"
@@ -328,14 +329,27 @@
void toLSPDiags(
const Diag &D, const URIForFile &File, const ClangdDiagnosticOptions &Opts,
llvm::function_ref<void(clangd::Diagnostic, llvm::ArrayRef<Fix>)> OutFn) {
- auto FillBasicFields = [](const DiagBase &D) -> clangd::Diagnostic {
- clangd::Diagnostic Res;
- Res.range = D.Range;
- Res.severity = getSeverity(D.Severity);
- return Res;
- };
+ clangd::Diagnostic Main;
+ Main.severity = getSeverity(D.Severity);
+
+ // Main diagnostic should always refer to a range inside main file. If a
+ // diagnostic made it so for, it means either itself or one of its notes is
+ // inside main file.
+ if (D.InsideMainFile) {
+ Main.range = D.Range;
+ } else {
+ llvm::Optional<Note> NoteInsideMainFile;
+ for (auto &N : D.Notes) {
+ if (!N.InsideMainFile)
+ continue;
+ NoteInsideMainFile = N;
+ break;
+ }
+ assert(NoteInsideMainFile &&
+ "neither the main diagnostic nor notes are inside main file");
+ Main.range = NoteInsideMainFile->Range;
+ }
- clangd::Diagnostic Main = FillBasicFields(D);
Main.code = D.Name;
switch (D.Source) {
case Diag::Clang:
@@ -379,7 +393,9 @@
for (auto &Note : D.Notes) {
if (!Note.InsideMainFile)
continue;
- clangd::Diagnostic Res = FillBasicFields(Note);
+ clangd::Diagnostic Res;
+ Res.severity = getSeverity(Note.Severity);
+ Res.range = Note.Range;
Res.message = noteMessage(D, Note, Opts);
OutFn(std::move(Res), llvm::ArrayRef<Fix>());
}
Index: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -1014,6 +1014,29 @@
EXPECT_THAT(TU.build().getDiagnostics(), UnorderedElementsAre());
}
+TEST(ToLSPDiag, RangeIsInMain) {
+ ClangdDiagnosticOptions Opts;
+ clangd::Diag D;
+ D.Range = {pos(1, 2), pos(3, 4)};
+ D.Notes.emplace_back();
+ Note &N = D.Notes.back();
+ N.Range = {pos(2, 3), pos(3, 4)};
+
+ D.InsideMainFile = true;
+ N.InsideMainFile = false;
+ toLSPDiags(D, {}, Opts,
+ [&](clangd::Diagnostic LSPDiag, ArrayRef<clangd::Fix>) {
+ EXPECT_EQ(LSPDiag.range, D.Range);
+ });
+
+ D.InsideMainFile = false;
+ N.InsideMainFile = true;
+ toLSPDiags(D, {}, Opts,
+ [&](clangd::Diagnostic LSPDiag, ArrayRef<clangd::Fix>) {
+ EXPECT_EQ(LSPDiag.range, N.Range);
+ });
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/Diagnostics.cpp
===================================================================
--- clang-tools-extra/clangd/Diagnostics.cpp
+++ clang-tools-extra/clangd/Diagnostics.cpp
@@ -22,6 +22,7 @@
#include "clang/Lex/Token.h"
#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/DenseSet.h"
+#include "llvm/ADT/Optional.h"
#include "llvm/ADT/SmallString.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/ADT/Twine.h"
@@ -328,14 +329,27 @@
void toLSPDiags(
const Diag &D, const URIForFile &File, const ClangdDiagnosticOptions &Opts,
llvm::function_ref<void(clangd::Diagnostic, llvm::ArrayRef<Fix>)> OutFn) {
- auto FillBasicFields = [](const DiagBase &D) -> clangd::Diagnostic {
- clangd::Diagnostic Res;
- Res.range = D.Range;
- Res.severity = getSeverity(D.Severity);
- return Res;
- };
+ clangd::Diagnostic Main;
+ Main.severity = getSeverity(D.Severity);
+
+ // Main diagnostic should always refer to a range inside main file. If a
+ // diagnostic made it so for, it means either itself or one of its notes is
+ // inside main file.
+ if (D.InsideMainFile) {
+ Main.range = D.Range;
+ } else {
+ llvm::Optional<Note> NoteInsideMainFile;
+ for (auto &N : D.Notes) {
+ if (!N.InsideMainFile)
+ continue;
+ NoteInsideMainFile = N;
+ break;
+ }
+ assert(NoteInsideMainFile &&
+ "neither the main diagnostic nor notes are inside main file");
+ Main.range = NoteInsideMainFile->Range;
+ }
- clangd::Diagnostic Main = FillBasicFields(D);
Main.code = D.Name;
switch (D.Source) {
case Diag::Clang:
@@ -379,7 +393,9 @@
for (auto &Note : D.Notes) {
if (!Note.InsideMainFile)
continue;
- clangd::Diagnostic Res = FillBasicFields(Note);
+ clangd::Diagnostic Res;
+ Res.severity = getSeverity(Note.Severity);
+ Res.range = Note.Range;
Res.message = noteMessage(D, Note, Opts);
OutFn(std::move(Res), llvm::ArrayRef<Fix>());
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
