[clang-tidy] [clangd] Avoid multi-line diagnostic range for else-after-return diagnostic

[nridge]

Summary

Fixes https://bugs.llvm.org/show_bug.cgi?id=47809

[nridge]

I see that you have a more sophisticated fix at D91149. Will leave it up to you if we should just wait for that to land, or land this in the interim.

[njames93]

This small fix is definitely worth while, the more involved patch may not ever land.

clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp

324

There are more places where diag is called. Maybe add this to all callsites of diag that then emit fixes, otherwise this may miss a few cases.

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

488

This needs rebasing, clang tidy interface for checks has changed.

[nridge]

Rebase and address review comment

[njames93]

LGTM

clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp

void ElseAfterReturnCheck::check(const MatchFinder::MatchResult &Result) {
 
   if (checkConditionVarUsageInElse(If) != nullptr) {
     if (!WarnOnConditionVariables)
       return;
     if (IsLastInScope) {
       // If the if statement is the last statement its enclosing statements
       // scope, we can pull the decl out of the if statement.
       DiagnosticBuilder Diag = diag(ElseLoc, WarningMessage)
-                               << ControlFlowInterruptor;
+                               << ControlFlowInterruptor
+                               << SourceRange(ElseLoc);
       if (checkInitDeclUsageInElse(If) != nullptr) {
         Diag << tooling::fixit::createReplacement(
                     SourceRange(If->getIfLoc()),
                     (tooling::fixit::getText(*If->getInit(), *Result.Context) +
                      llvm::StringRef("\n"))
                         .str())
              << tooling::fixit::createRemoval(If->getInit()->getSourceRange());
       }
void ElseAfterReturnCheck::check(const MatchFinder::MatchResult &Result) {
 
   if (checkInitDeclUsageInElse(If) != nullptr) {
     if (!WarnOnConditionVariables)
       return;
     if (IsLastInScope) {
       // If the if statement is the last statement its enclosing statements
       // scope, we can pull the decl out of the if statement.
       DiagnosticBuilder Diag = diag(ElseLoc, WarningMessage)
-                               << ControlFlowInterruptor;
+                               << ControlFlowInterruptor
+                               << SourceRange(ElseLoc);
       Diag << tooling::fixit::createReplacement(
                   SourceRange(If->getIfLoc()),
                   (tooling::fixit::getText(*If->getInit(), *Result.Context) +
                    "\n" +
                    tooling::fixit::getText(If->getIfLoc(), *Result.Context))
                       .str())
            << tooling::fixit::createRemoval(If->getInit()->getSourceRange());
       removeElseAndBrackets(Diag, *Result.Context, Else, ElseLoc);
     } else if (WarnOnUnfixable) {
       // Warn, but don't attempt an autofix.
       diag(ElseLoc, WarningMessage) << ControlFlowInterruptor;
     }
     return;
   }
 
   DiagnosticBuilder Diag = diag(ElseLoc, WarningMessage)
-                           << ControlFlowInterruptor;
+                           << ControlFlowInterruptor << SourceRange(ElseLoc);
   removeElseAndBrackets(Diag, *Result.Context, Else, ElseLoc);
 }
 
 } // namespace readability
 } // namespace tidy
 } // namespace clang

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

int func() {
       return pthread_kill(thread, 0);
     }
   )cpp");
   TestTU TU = TestTU::withCode(Main.code());
   TU.ClangTidyProvider = addTidyChecks("bugprone-bad-signal-to-kill-thread");
   EXPECT_THAT(TU.build().getDiagnostics(), UnorderedElementsAre()); // no-crash
 }
 
+TEST(DiagnosticTest, ElseAfterReturnRange) {
+  Annotations Main(R"cpp(
+    int foo(int cond) {
+    if (cond == 1) {
+      return 42;
+    } [[else]] if (cond == 2) {
+      return 43;
+    }
+    return 44;
+    }
+  )cpp");
+  TestTU TU = TestTU::withCode(Main.code());
+  TU.ClangTidyProvider = addTidyChecks("llvm-else-after-return");
+  EXPECT_THAT(
+      TU.build().getDiagnostics(),
+      ElementsAre(Diag(Main.range(), "do not use 'else' after 'return'")));
+}
+
 TEST(DiagnosticsTest, Preprocessor) {
   // This looks like a preamble, but there's an #else in the middle!
   // Check that:
   //  - the #else doesn't generate diagnostics (we had this bug)
   //  - we get diagnostics from the taken branch
   //  - we get no diagnostics from the not taken branch
   Annotations Test(R"cpp(
     #ifndef FOO