[clang-tools-extra] [clang-tidy] Add fix-its to `readability-avoid-return-with-void-value` check (PR #81420)

[Mike Rice via cfe-commits]

Danny =?utf-8?q?Mösch?= <danny.moesch at icloud.com>,
Danny =?utf-8?q?Mösch?= <danny.moesch at icloud.com>,
Danny =?utf-8?q?Mösch?= <danny.moesch at icloud.com>,
Danny =?utf-8?q?Mösch?= <danny.moesch at icloud.com>
Message-ID:
In-Reply-To: <llvm.org/llvm/llvm-project/pull/81420 at github.com>


================
@@ -42,10 +44,30 @@ void AvoidReturnWithVoidValueCheck::check(
   const auto *VoidReturn = Result.Nodes.getNodeAs<ReturnStmt>("void_return");
   if (IgnoreMacros && VoidReturn->getBeginLoc().isMacroID())
     return;
-  if (!StrictMode && !Result.Nodes.getNodeAs<CompoundStmt>("compound_parent"))
+  const auto *SurroundingBlock =
+      Result.Nodes.getNodeAs<CompoundStmt>("compound_parent");
+  if (!StrictMode && !SurroundingBlock)
     return;
-  diag(VoidReturn->getBeginLoc(), "return statement within a void function "
-                                  "should not have a specified return value");
+  DiagnosticBuilder Diag = diag(VoidReturn->getBeginLoc(),
+                                "return statement within a void function "
+                                "should not have a specified return value");
+  const SourceLocation SemicolonPos = utils::lexer::findNextTerminator(
+      VoidReturn->getEndLoc(), *Result.SourceManager, getLangOpts());
+  if (SemicolonPos.isInvalid())
+    return;
+  if (!SurroundingBlock) {
+    const auto BraceInsertionHints = utils::getBraceInsertionsHints(
+        VoidReturn, getLangOpts(), *Result.SourceManager,
+        VoidReturn->getBeginLoc());
+    if (BraceInsertionHints)
+      Diag << BraceInsertionHints.openingBraceFixIt()
+           << BraceInsertionHints.closingBraceFixIt();
+  }
+  Diag << FixItHint::CreateRemoval(VoidReturn->getReturnLoc());
+  if (!Result.Nodes.getNodeAs<FunctionDecl>("function_parent") ||
+      SurroundingBlock->body_back() != VoidReturn)
----------------
mikerice1969 wrote:

Hi @SimplyDanny, we are getting a static verifier hit on this code now since there is a nullptr check of SurroundingBlock followed by a use of SurroundingBlock without a check. 

It seems when there is a function parent SurroundingBlock won't be a nullptr since it will be set to the function's CompoundStmt, but that's not completely obvious to the reader of the code.

You might consider adding an assert to make it clear for anyone modifying in the future. Maybe:

```
const auto *FunctionParent =                                       
    Result.Nodes.getNodeAs<FunctionDecl>("function_parent");       
assert((!FunctionParent || SurroundingBlock) &&                    
       "missing surrounding block when function parent");          
if (!FunctionParent || SurroundingBlock->body_back() != VoidReturn)
```

https://github.com/llvm/llvm-project/pull/81420