[Clangd] Fixed clangd diagnostics priority

[SureYeaah]

Summary

• Fixed diagnostics where zero width inserted ranges were being used instead of the whole token
• Added unit tests

[SureYeaah]

Remove extra newline

[kadircet]

Mostly LG, just a few re-orderings to make code more readable and get rid of redundant Lexer calls.

clang-tools-extra/clangd/Diagnostics.cpp

108

since we are going to lex again in case of R.isValid(), you can get rid of this lexer call.

109

let's change this condition to rather check for Lexer::getRawToken succeeded and underlying token isNot(tok::comment).
Then you can simply construct the range with CharSourceRange::getTokenRange(Tok.getLocation(), Tok.getEndLoc()).

111

LLVM style uses UpperCammelCase for variable names. so this should be something like, Token Tok;, same for the bool below.

119

the FallbackRange(if set) and CharSourceRange::getCharRange(Loc); are the same.

So you can get rid of FallbackRange and merge the last two branches of this conditional.

[SureYeaah]

Refactored code as pointed out by @kadircet

[kadircet]

LGTM, thanks!

Also please make sure you've clang-formatted the code before you land this.

clang-tools-extra/clangd/Diagnostics.cpp

93–94

FallbackRange is not returned anymore, you can safely delete it.

104

I think you can simply it further by:

// Fallback to zero-width range at diagnostic location.
auto R = CharSourceRange::getCharRange(Loc);
Token Tok;
// For non-comment tokens, use token at the location.
if (!Lexer::getRawToken(Loc, Tok, M, L, true) && Tok.isNot(tok::comment))
  R = CharSourceRange::getTokenRange(Tok.getLocation(), Tok.getEndLoc());
return halfOpenToRange(M, R);

[SureYeaah]

Simplified logic for diagnostics range.

[SureYeaah]

Formatted code

[sammccall]

Code gets shorter, tests get longer, very nice :-)

clang-tools-extra/clangd/Diagnostics.cpp

 #include "Protocol.h"
 #include "SourceCode.h"
 #include "clang/Basic/AllDiagnostics.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/DiagnosticIDs.h"
 #include "clang/Basic/FileManager.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Lex/Lexer.h"
+#include "clang/Lex/Token.h"
 #include "llvm/ADT/ArrayRef.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"

 // LSP needs a single range.
 Range diagnosticRange(const clang::Diagnostic &D, const LangOptions &L) {
   auto &M = D.getSourceManager();
   auto Loc = M.getFileLoc(D.getLocation());
   for (const auto &CR : D.getRanges()) {
     auto R = Lexer::makeFileCharRange(CR, M, L);
     if (locationInRange(Loc, R, M))
       return halfOpenToRange(M, R);
   }
-  llvm::Optional<Range> FallbackRange;
   // 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 (R.getBegin() == R.getEnd() && Loc == R.getBegin())
-      FallbackRange = halfOpenToRange(M, R);
+  }
+  // If the token at the location is not a comment, use the token.
+  // If we can't get the token at the location, fall back to using the location
+  auto R = CharSourceRange::getCharRange(Loc);
+  Token Tok;
+  if(!Lexer::getRawToken(Loc, Tok, M, L, true) && Tok.isNot(tok::comment)) {
+    R = CharSourceRange::getTokenRange(Tok.getLocation(), Tok.getEndLoc());
   }
-  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);
 }
 
 void adjustDiagFromHeader(Diag &D, const clang::Diagnostic &Info,
                           const LangOptions &LangOpts) {
   const SourceLocation &DiagLoc = Info.getLocation();
   const SourceManager &SM = Info.getSourceManager();
   SourceLocation IncludeInMainFile;
   auto GetIncludeLoc = [&SM](SourceLocation SLoc) {
     return SM.getIncludeLoc(SM.getFileID(SLoc));
   };
   for (auto IncludeLocation = GetIncludeLoc(DiagLoc); IncludeLocation.isValid();
        IncludeLocation = GetIncludeLoc(IncludeLocation))
     IncludeInMainFile = IncludeLocation;
   if (IncludeInMainFile.isInvalid())
     return;
 
   // Update diag to point at include inside main file.

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

Position pos(int line, int character) {
   return Res;
 }
 
 TEST(DiagnosticsTest, DiagnosticRanges) {
   // Check we report correct ranges, including various edge-cases.
   Annotations Test(R"cpp(
     namespace test{};
     void $decl[[foo]]();
+    class T{$explicit[[]]$constructor[[T]](int a);};
     int main() {
       $typo[[go\
 o]]();
       foo()$semicolon[[]]//with comments
       $unk[[unknown]]();
       double $type[[bar]] = "foo";
       struct Foo { int x; }; Foo a;
       a.$nomember[[y]];
       test::$nomembernamespace[[test]];
     }
   )cpp");
+  auto TU = TestTU::withCode(Test.code());
+  TU.ClangTidyChecks = "-*,google-explicit-constructor";
   EXPECT_THAT(
-      TestTU::withCode(Test.code()).build().getDiagnostics(),
+      TU.build().getDiagnostics(),
       ElementsAre(
           // This range spans lines.
           AllOf(Diag(Test.range("typo"),
                      "use of undeclared identifier 'goo'; did you mean 'foo'?"),
                 DiagSource(Diag::Clang), DiagName("undeclared_var_use_suggest"),
                 WithFix(
                     Fix(Test.range("typo"), "foo", "change 'go\\…' to 'foo'")),
                 // This is a pretty normal range.
                 WithNote(Diag(Test.range("decl"), "'foo' declared here"))),
           // 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("type"),
                "cannot initialize a variable of type 'double' with an lvalue "
                "of type 'const char [4]'"),
           Diag(Test.range("nomember"), "no member named 'y' in 'Foo'"),
           Diag(Test.range("nomembernamespace"),
-               "no member named 'test' in namespace 'test'")));
+               "no member named 'test' in namespace 'test'"),
+          // We make sure here that the entire token is highlighted
+          AllOf(Diag(Test.range("constructor"),
+                     "single-argument constructors must be marked explicit to "
+                     "avoid unintentional implicit conversions"),
+                WithFix(Fix(Test.range("explicit"), "explicit ",
+                            "insert 'explicit '")))));
 }
 
 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",