[clang-tidy] Fix formatting of else after return check

[njames93]

Summary

Simplifies the code in the fix-it generation.
Addresses https://bugs.llvm.org/show_bug.cgi?id=47809 (hopefully)
By not changing the whole else block it also means other checks with fix-its can be applied in the else block.

Depends on D91103

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

if (isa<DeclStmt>(Node))
     return true;
   if (const auto *Compound = dyn_cast<CompoundStmt>(Node))
     return llvm::any_of(Compound->body(), [](const Stmt *SubNode) {
       return isa<DeclStmt>(SubNode);
     });
   return false;
 }
 
-static void removeElseAndBrackets(DiagnosticBuilder &Diag, ASTContext &Context,
+void removeElseAndBrackets(DiagnosticBuilder &Diag, const SourceManager &SM,
                            const Stmt *Else, SourceLocation ElseLoc) {
-  auto Remap = [&](SourceLocation Loc) {
-    return Context.getSourceManager().getExpansionLoc(Loc);
-  };
-  auto TokLen = [&](SourceLocation Loc) {
-    return Lexer::MeasureTokenLength(Loc, Context.getSourceManager(),
-                                     Context.getLangOpts());
-  };
+  auto Remap = [&](SourceLocation Loc) { return SM.getExpansionLoc(Loc); };
 
   if (const auto *CS = dyn_cast<CompoundStmt>(Else)) {
-    Diag << tooling::fixit::createRemoval(ElseLoc);
-    SourceLocation LBrace = CS->getLBracLoc();
-    SourceLocation RBrace = CS->getRBracLoc();
-    SourceLocation RangeStart =
-        Remap(LBrace).getLocWithOffset(TokLen(LBrace) + 1);
-    SourceLocation RangeEnd = Remap(RBrace).getLocWithOffset(-1);
-
-    llvm::StringRef Repl = Lexer::getSourceText(
-        CharSourceRange::getTokenRange(RangeStart, RangeEnd),
-        Context.getSourceManager(), Context.getLangOpts());
-    Diag << tooling::fixit::createReplacement(CS->getSourceRange(), Repl);
+    Diag << tooling::fixit::createRemoval(ElseLoc)
+         << tooling::fixit::createRemoval(Remap(CS->getLBracLoc()))
+         << tooling::fixit::createRemoval(Remap(CS->getRBracLoc()))
+         << FixItHint::CreateReformatFixIt(CS->getSourceRange());
   } else {
-    SourceLocation ElseExpandedLoc = Remap(ElseLoc);
-    SourceLocation EndLoc = Remap(Else->getEndLoc());
-
-    llvm::StringRef Repl = Lexer::getSourceText(
-        CharSourceRange::getTokenRange(
-            ElseExpandedLoc.getLocWithOffset(TokLen(ElseLoc) + 1), EndLoc),
-        Context.getSourceManager(), Context.getLangOpts());
-    Diag << tooling::fixit::createReplacement(
-        SourceRange(ElseExpandedLoc, EndLoc), Repl);
+    Diag << tooling::fixit::createRemoval(Remap(ElseLoc))
+         << FixItHint::CreateReformatFixIt(Else->getSourceRange());
   }
 }
 
 ElseAfterReturnCheck::ElseAfterReturnCheck(StringRef Name,
                                            ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       WarnOnUnfixable(Options.get(WarnOnUnfixableStr, true)),
       WarnOnConditionVariables(Options.get(WarnOnConditionVariablesStr, true)) {
if (IsLastInScope) {
           (tooling::fixit::getText(*VDeclStmt, *Result.Context) +
            llvm::StringRef(";\n") +
            tooling::fixit::getText(If->getIfLoc(), *Result.Context))
               .str();
       Diag << tooling::fixit::createReplacement(SourceRange(If->getIfLoc()),
                                                 Repl)
            << tooling::fixit::createReplacement(VDeclStmt->getSourceRange(),
                                                 VDecl->getName());
-      removeElseAndBrackets(Diag, *Result.Context, Else, ElseLoc);
+      removeElseAndBrackets(Diag, Result.Context->getSourceManager(), Else,
+                            ElseLoc);
     } else if (WarnOnUnfixable) {
       // Warn, but don't attempt an autofix.
       diag(ElseLoc, WarningMessage) << ControlFlowInterruptor;
     }
     return;
   }
 
   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;
       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);
+      removeElseAndBrackets(Diag, Result.Context->getSourceManager(), Else,
+                            ElseLoc);
     } else if (WarnOnUnfixable) {
       // Warn, but don't attempt an autofix.
       diag(ElseLoc, WarningMessage) << ControlFlowInterruptor;
     }
     return;
   }
 
   DiagnosticBuilder Diag = diag(ElseLoc, WarningMessage)
                            << ControlFlowInterruptor;
-  removeElseAndBrackets(Diag, *Result.Context, Else, ElseLoc);
+  removeElseAndBrackets(Diag, Result.Context->getSourceManager(), Else,
+                        ElseLoc);
 }
 
 } // namespace readability
 } // namespace tidy
 } // namespace clang

clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return.cpp

-// RUN: %check_clang_tidy %s readability-else-after-return %t -- -- -fexceptions -std=c++17
+// RUN: %check_clang_tidy %s readability-else-after-return -format-style=LLVM %t -- -- -fexceptions -std=c++17
 
 namespace std {
 struct string {
   string(const char *);
   ~string();
 };
 } // namespace std
 

 }
 
 int g();
 int h(int);
 
 int declInConditionUsedInElse() {
   if (int X = g()) { // comment-11
     // CHECK-FIXES: {{^}}  int X = g();
-    // CHECK-FIXES-NEXT: {{^}}if (X) { // comment-11
+    // CHECK-FIXES-NEXT: {{^}}  if (X) { // comment-11
     return X;
   } else { // comment-11
            // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return'
            // CHECK-FIXES: {{^}}  } // comment-11
     return h(X);
   }
 }
 int declInConditionUnusedInElse() {
   if (int X = g()) {
     return h(X);
   } else { // comment-12
            // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return'
            // CHECK-FIXES: {{^}}  } // comment-12
     return 0;
   }
 }
 
 int varInitAndCondition() {
   if (int X = g(); X != 0) { // comment-13
     // CHECK-FIXES: {{^}}  int X = g();
-    // CHECK-FIXES-NEXT: {{^}}if ( X != 0) { // comment-13
+    // CHECK-FIXES-NEXT: {{^}}  if (X != 0) { // comment-13
     return X;
   } else { // comment-13
            // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return'
            // CHECK-FIXES: {{^}}  } // comment-13
     return h(X);
   }
 }
 
if (int X = g(); X != 0) {
     h(Y);
   }
   return Y;
 }
 
 int varInitAndCondVarUsedInElse() {
   if (int X = g(); int Y = g()) { // comment-17
     // CHECK-FIXES:      {{^}}  int X = g();
-    // CHECK-FIXES-NEXT: {{^}}int Y = g();
-    // CHECK-FIXES-NEXT: {{^}}if ( Y) { // comment-17
+    // CHECK-FIXES-NEXT: {{^}}  int Y = g();
+    // CHECK-FIXES-NEXT: {{^}}  if (Y) { // comment-17
     return X ? X : Y;
   } else { // comment-17
            // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return'
            // CHECK-FIXES: {{^}}  } // comment-17
     return X ? X : h(Y);
   }
 }
 
 int lifeTimeExtensionTests(int a) {
   if (a > 0) {
     return a;
   } else {
       // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return'
     int b = 0;
     h(b);
   }
   if (int b = a; (b & 1) == 0) {
     return a;
   } else {
     // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return'
     b++;
   }
   if (int b = a; b > 1) { // comment-18
     // CHECK-FIXES:      {{^}}  int b = a;
-    // CHECK-FIXES-NEXT: {{^}}if ( b > 1) { // comment-18
+    // CHECK-FIXES-NEXT: {{^}}  if (b > 1) { // comment-18
     return a;
   } else { // comment-18
            // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return'
            // CHECK-FIXES: {{^}}  } // comment-18
     return b;
   }
 }
 
 void test_B44745() {
   // This is the actual minimum test case for the crash in bug 44745. We aren't
   // too worried about the warning or fix here, more we don't want a crash.
   // CHECK-MESSAGES: :[[@LINE+3]]:5: warning: do not use 'else' after 'return' [readability-else-after-return]
   if (auto X = false) {
     return;
   } else {
     for (;;) {
     }
   }
   return;
 }
+
+int complicatedFormat() {
+  // CHECK-MESSAGES: :[[@LINE+6]]:5: warning: do not use 'else' after 'return' [readability-else-after-return]
+  // CHECK-MESSAGES: :[[@LINE+8]]:7: warning: do not use 'else' after 'return' [readability-else-after-return]
+  // CHECK-MESSAGES: :[[@LINE+10]]:9: warning: do not use 'else' after 'return' [readability-else-after-return]
+  // CHECK-MESSAGES: :[[@LINE+12]]:11: warning: do not use 'else' after 'return' [readability-else-after-return]
+  if (true) {
+    return 1;
+  } else {
+    if (true) {
+      return 2;
+    } else {
+      if (true) {
+        return 3;
+      } else {
+        if (true) {
+          return 4;
+        } else {
+          return 5;
+        }
+      }
+    }
+  }
+  // CHECK-FIXES:      {{^  }}if (true) {
+  // CHECK-FIXES-NEXT: {{^  }}  return 1;
+  // CHECK-FIXES-NEXT: {{^  }}}
+  // CHECK-FIXES-NEXT: {{^  }}if (true) {
+  // CHECK-FIXES-NEXT: {{^  }}  return 2;
+  // CHECK-FIXES-NEXT: {{^  }}}
+  // CHECK-FIXES-NEXT: {{^  }}if (true) {
+  // CHECK-FIXES-NEXT: {{^  }}  return 3;
+  // CHECK-FIXES-NEXT: {{^  }}}
+  // CHECK-FIXES-NEXT: {{^  }}if (true) {
+  // CHECK-FIXES-NEXT: {{^  }}  return 4;
+  // CHECK-FIXES-NEXT: {{^  }}}
+  // CHECK-FIXES-NEXT: {{^  }}return 5;
+}