[clangd] Adjust diagnostic range to be inside main file

[kadircet]

Summary

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.

[merge_guards_bot]

Unit tests: pass. 61663 tests passed, 0 failed and 779 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[ilya-biryukov]

LGTM

clang-tools-extra/clangd/Diagnostics.cpp

342

NIT: arguably simpler with the standard algorithm

auto It = llvm::find_if(D.Notes, [](const Node&N) { return N.InsideMainFile; });
assert(It != D.Notes.end());

Up to you.

[kadircet]

clang-tools-extra/clangd/Diagnostics.cpp

342

agreed, thanks!

[kadircet]

• Use find_if instead of raw loop

[merge_guards_bot]

Unit tests: pass. 61663 tests passed, 0 failed and 779 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

clang-tools-extra/clangd/Diagnostics.cpp

 #include "clang/Basic/DiagnosticIDs.h"
 #include "clang/Basic/FileManager.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Lex/Lexer.h"
 #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"
 #include "llvm/Support/Capacity.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/ScopedPrinter.h"
 #include "llvm/Support/Signals.h"
 #include "llvm/Support/raw_ostream.h"
CodeAction toCodeAction(const Fix &F, const URIForFile &File) {
   Action.edit->changes.emplace();
   (*Action.edit->changes)[File.uri()] = {F.Edits.begin(), F.Edits.end()};
   return Action;
 }
 
 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:
     Main.source = "clang";
     break;
   case Diag::ClangTidy:
     Main.source = "clang-tidy";
     break;
void toLSPDiags(
   OutFn(std::move(Main), D.Fixes);
 
   // If we didn't emit the notes as relatedLocations, emit separate diagnostics
   // so the user can find the locations easily.
   if (!Opts.EmitRelatedLocations)
     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>());
     }
 }
 
 int getSeverity(DiagnosticsEngine::Level L) {
   switch (L) {
   case DiagnosticsEngine::Remark:

clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp

TEST(IgnoreDiags, FromNonWrittenInclude) {
   TestTU TU;
   TU.ExtraArgs.push_back("--include=a.h");
   TU.AdditionalFiles = {{"a.h", "void main();"}};
   // The diagnostic "main must return int" is from the header, we don't attempt
   // to render it in the main file as there is no written location there.
   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