Improve diagnostics range reporting.

[kadircet]

Summary

If we have some range information coming from clang diagnostic, promote
that one even if it doesn't contain diagnostic location inside.

[ioeric]

clangd/Diagnostics.cpp

55 ↗

(On Diff #167111)

nit: Maybe FallbackRange?

unittests/clangd/ClangdUnitTests.cpp

82 ↗

(On Diff #167111)

What is this //with comments for? Maybe explain it in the comment if it does something?

[kadircet]

• Address comments.

unittests/clangd/ClangdUnitTests.cpp

82 ↗

(On Diff #167111)

In the previous implementation we were highlighting the "//with comments" as it was the token at that location. But since this is an insertion and we are not going to replace that token it seemed confusing to highlight all the comment.

clang-tools-extra/trunk/clangd/Diagnostics.cpp

 }
 
 // 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.
+  llvm::Optional<Range> FallbackRange;
   for (const auto &CR : D.getRanges()) {
     auto R = Lexer::makeFileCharRange(CR, M, L);
     if (locationInRange(Loc, R, M))
       return halfOpenToRange(M, R);
+    // If there are no ranges that contain the location report the first range.
+    if (!FallbackRange)
+      FallbackRange = halfOpenToRange(M, R);
   }
   // 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 halfOpenToRange(M, R);
+    // If there's a fixit that performs insertion, it has zero-width. Therefore
+    // it can't contain the location of the diag, but it might be possible that
+    // this should be reported as range. For example missing semicolon.
+    if (!FallbackRange && R.getBegin() == R.getEnd())
+      FallbackRange = halfOpenToRange(M, R);
   }
+  if (FallbackRange)
+    return *FallbackRange;
   // 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 halfOpenToRange(M, R);
 }
 
 bool isInsideMainFile(const SourceLocation Loc, const SourceManager &M) {

clang-tools-extra/trunk/unittests/clangd/ClangdUnitTests.cpp

 
 TEST(DiagnosticsTest, DiagnosticRanges) {
   // Check we report correct ranges, including various edge-cases.
   Annotations Test(R"cpp(
     void $decl[[foo]]();
     int main() {
       $typo[[go\
 o]]();
-      foo()$semicolon[[]]
+      foo()$semicolon[[]]//with comments
       $unk[[unknown]]();
+      double bar = $type[["foo"]];
     }
   )cpp");
   EXPECT_THAT(
       TestTU::withCode(Test.code()).build().getDiagnostics(),
       ElementsAre(
           // This range spans lines.
           AllOf(Diag(Test.range("typo"),
                      "use of undeclared identifier 'goo'; did you mean 'foo'?"),
                 WithFix(
                     Fix(Test.range("typo"), "foo", "change 'go\\ o' to 'foo'")),
                 // This is a pretty normal range.
                 WithNote(Diag(Test.range("decl"), "'foo' declared here"))),
-          // This range is zero-width, and at the end of a line.
+          // This range is zero-width and insertion. Therefore make sure we are
+          // not expanding it into other tokens. Since we are not going to
+          // replace those.
           AllOf(Diag(Test.range("semicolon"), "expected ';' after expression"),
                 WithFix(Fix(Test.range("semicolon"), ";", "insert ';'"))),
           // This range isn't provided by clang, we expand to the token.
-          Diag(Test.range("unk"), "use of undeclared identifier 'unknown'")));
+          Diag(Test.range("unk"), "use of undeclared identifier 'unknown'"),
+          Diag(Test.range("type"),
+               "cannot initialize a variable of type 'double' with an lvalue "
+               "of type 'const char [4]'")));
 }
 
 TEST(DiagnosticsTest, FlagsMatter) {
   Annotations Test("[[void]] main() {}");
   auto TU = TestTU::withCode(Test.code());
   EXPECT_THAT(TU.build().getDiagnostics(),
               ElementsAre(AllOf(Diag(Test.range(), "'main' must return 'int'"),
                                 WithFix(Fix(Test.range(), "int",