[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

94

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

110

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 "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"

     if (R.getBegin() == R.getEnd() && Loc == R.getBegin())
       FallbackRange = halfOpenToRange(M, R);
   }
-  if (FallbackRange)
-    return *FallbackRange;
-  // If no suitable range is found, just use the token at the location.
+  // If the token at location is not a comment, use the token.
+  // Otherwise use zero width insertion range
   auto R = Lexer::makeFileCharRange(CharSourceRange::getTokenRange(Loc), M, L);
-  if (!R.isValid()) // Fall back to location only, let the editor deal with it.
+  if (R.isValid()) {
+    // check if token at location is a priority i.e. not a comment
+    Token token;
+    bool isTokenPriority = false;
+    if(!Lexer::getRawToken(Loc, token, M, L, true))
+      isTokenPriority = token.getKind() != tok::comment;
+    if (!isTokenPriority && FallbackRange)
+      return *FallbackRange;
+  }
+  else if (FallbackRange)
+    return *FallbackRange;
+  else // Fall back to location only, let the editor deal with it.
     R = CharSourceRange::getCharRange(Loc);
   return halfOpenToRange(M, R);
 }

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

   Annotations Test(R"cpp(
     namespace test{};
     void $decl[[foo]]();
+    class T{$explicit[[]]$constructor[[T]](int a);};
     int main() {
       $typo[[go\
 o]]();

       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"),

                "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) {