From 189aa5b7a4584677ad628ecc2c369db61d4d2515 Mon Sep 17 00:00:00 2001
From: Kadir Cetinkaya <kadircet@google.com>
Date: Thu, 9 Jan 2020 16:54:22 +0100
Subject: [PATCH] [clangd] Adjust diagnostic range to be inside main file

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.

Reviewers: ilya-biryukov

Subscribers: MaskRay, jkorous, arphaman, usaxena95, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D72458
---
 clang-tools-extra/clangd/Diagnostics.cpp           | 28 +++++++++++++++-------
 .../clangd/unittests/DiagnosticsTests.cpp          | 23 ++++++++++++++++++
 2 files changed, 43 insertions(+), 8 deletions(-)

diff --git a/clang-tools-extra/clangd/Diagnostics.cpp b/clang-tools-extra/clangd/Diagnostics.cpp
index e78df03..ad8f6c8 100644
--- a/clang-tools-extra/clangd/Diagnostics.cpp
+++ b/clang-tools-extra/clangd/Diagnostics.cpp
@@ -22,6 +22,8 @@
 #include "clang/Lex/Token.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/DenseSet.h"
+#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/Twine.h"
@@ -328,14 +330,22 @@ CodeAction toCodeAction(const Fix &F, const URIForFile &File) {
 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 {
+    auto It =
+        llvm::find_if(D.Notes, [](const Note &N) { return N.InsideMainFile; });
+    assert(It != D.Notes.end() &&
+           "neither the main diagnostic nor notes are inside main file");
+    Main.range = It->Range;
+  }
 
-  clangd::Diagnostic Main = FillBasicFields(D);
   Main.code = D.Name;
   switch (D.Source) {
   case Diag::Clang:
@@ -379,7 +389,9 @@ void toLSPDiags(
     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>());
     }
diff --git a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
index 0941af2..ef73519 100644
--- a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -1014,6 +1014,29 @@ TEST(IgnoreDiags, FromNonWrittenInclude) {
   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
-- 
2.7.4