[clang-tidy] Add fix-its to `readability-avoid-return-with-void-value` check #81420

SimplyDanny · 2024-02-11T16:05:32Z

No description provided.

llvmbot · 2024-02-11T16:06:04Z

@llvm/pr-subscribers-clang-tools-extra

Author: Danny Mösch (SimplyDanny)

Changes

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

3 Files Affected:

(modified) clang-tools-extra/clang-tidy/readability/AvoidReturnWithVoidValueCheck.cpp (+21-2)
(modified) clang-tools-extra/docs/ReleaseNotes.rst (+4)
(modified) clang-tools-extra/test/clang-tidy/checkers/readability/avoid-return-with-void-value.cpp (+12-2)

diff --git a/clang-tools-extra/clang-tidy/readability/AvoidReturnWithVoidValueCheck.cpp b/clang-tools-extra/clang-tidy/readability/AvoidReturnWithVoidValueCheck.cpp
index e3400f614fa564..1aa03fe9ba5da7 100644
--- a/clang-tools-extra/clang-tidy/readability/AvoidReturnWithVoidValueCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/AvoidReturnWithVoidValueCheck.cpp
@@ -10,6 +10,7 @@
 #include "clang/AST/Stmt.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Lex/Lexer.h"
 
 using namespace clang::ast_matchers;
 
@@ -42,10 +43,28 @@ 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;
+  const auto ReturnExpr =
+      Lexer::getSourceText(CharSourceRange::getTokenRange(
+                               VoidReturn->getRetValue()->getSourceRange()),
+                           *Result.SourceManager, getLangOpts());
+  SourceLocation SemicolonPos;
+  if (const auto NextToken =
+          Lexer::findNextToken(VoidReturn->getRetValue()->getEndLoc(),
+                               *Result.SourceManager, getLangOpts()))
+    SemicolonPos = NextToken->getEndLoc();
+  auto Replacement = (ReturnExpr + "; return;").str();
+  if (!SurroundingBlock)
+    Replacement = "{" + Replacement + "}";
   diag(VoidReturn->getBeginLoc(), "return statement within a void function "
-                                  "should not have a specified return value");
+                                  "should not have a specified return value")
+      << FixItHint::CreateReplacement(
+             CharSourceRange::getTokenRange(VoidReturn->getBeginLoc(),
+                                            SemicolonPos),
+             Replacement);
 }
 
 void AvoidReturnWithVoidValueCheck::storeOptions(
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index ee68c8f49b3df2..bfe0e4ba2e131d 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -160,6 +160,10 @@ Changes in existing checks
   `AllowStringArrays` option, enabling the exclusion of array types with deduced
   length initialized from string literals.
 
+- Improved :doc:`readability-avoid-return-with-void-value
+  <clang-tidy/checks/readability/avoid-return-with-void-value>` check by adding
+  fix-its.
+
 Removed checks
 ^^^^^^^^^^^^^^
 
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/avoid-return-with-void-value.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/avoid-return-with-void-value.cpp
index f00407c99ce570..0d269ceee82bc9 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/readability/avoid-return-with-void-value.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/avoid-return-with-void-value.cpp
@@ -12,14 +12,18 @@ void f2() {
     return f1();
     // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: return statement within a void function should not have a specified return value [readability-avoid-return-with-void-value]
     // CHECK-MESSAGES-LENIENT: :[[@LINE-2]]:5: warning: return statement within a void function should not have a specified return value [readability-avoid-return-with-void-value]
+    // CHECK-FIXES: f1(); return;
 }
 
 void f3(bool b) {
     if (b) return f1();
     // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: return statement within a void function should not have a specified return value [readability-avoid-return-with-void-value]
+    // CHECK-FIXES: if (b) {f1(); return;}
     return f2();
     // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: return statement within a void function should not have a specified return value [readability-avoid-return-with-void-value]
     // CHECK-MESSAGES-LENIENT: :[[@LINE-2]]:5: warning: return statement within a void function should not have a specified return value [readability-avoid-return-with-void-value]
+    // CHECK-FIXES: f2(); return;
+    // CHECK-FIXES-LENIENT: f2(); return;
 }
 
 template<class T>
@@ -29,6 +33,8 @@ void f5() {
     return f4<void>();
     // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: return statement within a void function should not have a specified return value [readability-avoid-return-with-void-value]
     // CHECK-MESSAGES-LENIENT: :[[@LINE-2]]:5: warning: return statement within a void function should not have a specified return value [readability-avoid-return-with-void-value]
+    // CHECK-FIXES: f4<void>(); return;
+    // CHECK-FIXES-LENIENT: f4<void>(); return;
 }
 
 void f6() { return; }
@@ -41,6 +47,8 @@ void f9() {
     return (void)f7();
     // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: return statement within a void function should not have a specified return value [readability-avoid-return-with-void-value]
     // CHECK-MESSAGES-LENIENT: :[[@LINE-2]]:5: warning: return statement within a void function should not have a specified return value [readability-avoid-return-with-void-value]
+    // CHECK-FIXES: (void)f7(); return;
+    // CHECK-FIXES-LENIENT: (void)f7(); return;
 }
 
 #define RETURN_VOID return (void)1
@@ -50,12 +58,12 @@ void f10() {
     // CHECK-MESSAGES-INCLUDE-MACROS: :[[@LINE-1]]:5: warning: return statement within a void function should not have a specified return value [readability-avoid-return-with-void-value]
 }
 
-template <typename A> 
+template <typename A>
 struct C {
   C(A) {}
 };
 
-template <class T> 
+template <class T>
 C<T> f11() { return {}; }
 
 using VOID = void;
@@ -66,4 +74,6 @@ VOID f13() {
     return f12();
     // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: return statement within a void function should not have a specified return value [readability-avoid-return-with-void-value]
     // CHECK-MESSAGES-LENIENT: :[[@LINE-2]]:5: warning: return statement within a void function should not have a specified return value [readability-avoid-return-with-void-value]
+    // CHECK-FIXES: f12(); return;
+    // CHECK-FIXES-LENIENT: f12(); return;
 }

llvmbot · 2024-02-11T16:06:05Z

@llvm/pr-subscribers-clang-tidy

Author: Danny Mösch (SimplyDanny)

Changes

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

3 Files Affected:

(modified) clang-tools-extra/clang-tidy/readability/AvoidReturnWithVoidValueCheck.cpp (+21-2)
(modified) clang-tools-extra/docs/ReleaseNotes.rst (+4)
(modified) clang-tools-extra/test/clang-tidy/checkers/readability/avoid-return-with-void-value.cpp (+12-2)

diff --git a/clang-tools-extra/clang-tidy/readability/AvoidReturnWithVoidValueCheck.cpp b/clang-tools-extra/clang-tidy/readability/AvoidReturnWithVoidValueCheck.cpp
index e3400f614fa564..1aa03fe9ba5da7 100644
--- a/clang-tools-extra/clang-tidy/readability/AvoidReturnWithVoidValueCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/AvoidReturnWithVoidValueCheck.cpp
@@ -10,6 +10,7 @@
 #include "clang/AST/Stmt.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Lex/Lexer.h"
 
 using namespace clang::ast_matchers;
 
@@ -42,10 +43,28 @@ 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;
+  const auto ReturnExpr =
+      Lexer::getSourceText(CharSourceRange::getTokenRange(
+                               VoidReturn->getRetValue()->getSourceRange()),
+                           *Result.SourceManager, getLangOpts());
+  SourceLocation SemicolonPos;
+  if (const auto NextToken =
+          Lexer::findNextToken(VoidReturn->getRetValue()->getEndLoc(),
+                               *Result.SourceManager, getLangOpts()))
+    SemicolonPos = NextToken->getEndLoc();
+  auto Replacement = (ReturnExpr + "; return;").str();
+  if (!SurroundingBlock)
+    Replacement = "{" + Replacement + "}";
   diag(VoidReturn->getBeginLoc(), "return statement within a void function "
-                                  "should not have a specified return value");
+                                  "should not have a specified return value")
+      << FixItHint::CreateReplacement(
+             CharSourceRange::getTokenRange(VoidReturn->getBeginLoc(),
+                                            SemicolonPos),
+             Replacement);
 }
 
 void AvoidReturnWithVoidValueCheck::storeOptions(
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index ee68c8f49b3df2..bfe0e4ba2e131d 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -160,6 +160,10 @@ Changes in existing checks
   `AllowStringArrays` option, enabling the exclusion of array types with deduced
   length initialized from string literals.
 
+- Improved :doc:`readability-avoid-return-with-void-value
+  <clang-tidy/checks/readability/avoid-return-with-void-value>` check by adding
+  fix-its.
+
 Removed checks
 ^^^^^^^^^^^^^^
 
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/avoid-return-with-void-value.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/avoid-return-with-void-value.cpp
index f00407c99ce570..0d269ceee82bc9 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/readability/avoid-return-with-void-value.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/avoid-return-with-void-value.cpp
@@ -12,14 +12,18 @@ void f2() {
     return f1();
     // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: return statement within a void function should not have a specified return value [readability-avoid-return-with-void-value]
     // CHECK-MESSAGES-LENIENT: :[[@LINE-2]]:5: warning: return statement within a void function should not have a specified return value [readability-avoid-return-with-void-value]
+    // CHECK-FIXES: f1(); return;
 }
 
 void f3(bool b) {
     if (b) return f1();
     // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: return statement within a void function should not have a specified return value [readability-avoid-return-with-void-value]
+    // CHECK-FIXES: if (b) {f1(); return;}
     return f2();
     // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: return statement within a void function should not have a specified return value [readability-avoid-return-with-void-value]
     // CHECK-MESSAGES-LENIENT: :[[@LINE-2]]:5: warning: return statement within a void function should not have a specified return value [readability-avoid-return-with-void-value]
+    // CHECK-FIXES: f2(); return;
+    // CHECK-FIXES-LENIENT: f2(); return;
 }
 
 template<class T>
@@ -29,6 +33,8 @@ void f5() {
     return f4<void>();
     // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: return statement within a void function should not have a specified return value [readability-avoid-return-with-void-value]
     // CHECK-MESSAGES-LENIENT: :[[@LINE-2]]:5: warning: return statement within a void function should not have a specified return value [readability-avoid-return-with-void-value]
+    // CHECK-FIXES: f4<void>(); return;
+    // CHECK-FIXES-LENIENT: f4<void>(); return;
 }
 
 void f6() { return; }
@@ -41,6 +47,8 @@ void f9() {
     return (void)f7();
     // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: return statement within a void function should not have a specified return value [readability-avoid-return-with-void-value]
     // CHECK-MESSAGES-LENIENT: :[[@LINE-2]]:5: warning: return statement within a void function should not have a specified return value [readability-avoid-return-with-void-value]
+    // CHECK-FIXES: (void)f7(); return;
+    // CHECK-FIXES-LENIENT: (void)f7(); return;
 }
 
 #define RETURN_VOID return (void)1
@@ -50,12 +58,12 @@ void f10() {
     // CHECK-MESSAGES-INCLUDE-MACROS: :[[@LINE-1]]:5: warning: return statement within a void function should not have a specified return value [readability-avoid-return-with-void-value]
 }
 
-template <typename A> 
+template <typename A>
 struct C {
   C(A) {}
 };
 
-template <class T> 
+template <class T>
 C<T> f11() { return {}; }
 
 using VOID = void;
@@ -66,4 +74,6 @@ VOID f13() {
     return f12();
     // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: return statement within a void function should not have a specified return value [readability-avoid-return-with-void-value]
     // CHECK-MESSAGES-LENIENT: :[[@LINE-2]]:5: warning: return statement within a void function should not have a specified return value [readability-avoid-return-with-void-value]
+    // CHECK-FIXES: f12(); return;
+    // CHECK-FIXES-LENIENT: f12(); return;
 }

SimplyDanny · 2024-02-11T16:07:23Z

clang-tools-extra/test/clang-tidy/checkers/readability/avoid-return-with-void-value.cpp

 }

 void f3(bool b) {
    if (b) return f1();
    // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: return statement within a void function should not have a specified return value [readability-avoid-return-with-void-value]
+    // CHECK-FIXES: if (b) {f1(); return;}


Is it okay to assume some formatter will run afterwards to make the output readable?

SimplyDanny · 2024-02-11T16:09:55Z

clang-tools-extra/test/clang-tidy/checkers/readability/avoid-return-with-void-value.cpp

@@ -12,14 +12,18 @@ void f2() {
    return f1();
    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: return statement within a void function should not have a specified return value [readability-avoid-return-with-void-value]
    // CHECK-MESSAGES-LENIENT: :[[@LINE-2]]:5: warning: return statement within a void function should not have a specified return value [readability-avoid-return-with-void-value]
+    // CHECK-FIXES: f1(); return;


Is an unnecessary return okay? Otherwise, this would need an analysis that goes beyond the scope of this rule.

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

PiotrZSL

When from functional point of view current code looks fine and could be merged, there are few improvements possible. Add also description to PR request.

Consider also ignoring return; if this is last stmt in function.

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

clang-tools-extra/test/clang-tidy/checkers/readability/avoid-return-with-void-value.cpp

SimplyDanny · 2024-04-06T13:44:16Z

@PiotrZSL: Friendly ping.

PiotrZSL

LGTM.

PiotrZSL · 2024-04-07T07:06:46Z

clang-tools-extra/clang-tidy/utils/BracesAroundStatement.cpp

+  Loc = Loc.getLocWithOffset(1);
+
+  for (;;) {
+    assert(Loc.isValid());


this probably should be an if... instead of assert

mikerice1969 · 2024-04-25T18:55:35Z

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

+  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)


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)

Addressing this in #90173.

llvmbot added clang-tools-extra clang-tidy labels Feb 11, 2024

SimplyDanny commented Feb 11, 2024

View reviewed changes

EugeneZelenko reviewed Feb 11, 2024

View reviewed changes

clang-tools-extra/clang-tidy/readability/AvoidReturnWithVoidValueCheck.cpp Outdated Show resolved Hide resolved

clang-tools-extra/clang-tidy/readability/AvoidReturnWithVoidValueCheck.cpp Outdated Show resolved Hide resolved

SimplyDanny force-pushed the avoid-return-with-void-value-fixits branch 2 times, most recently from 34f2445 to b99fe4b Compare February 18, 2024 21:39

HerrCai0907 requested changes Feb 19, 2024

View reviewed changes

clang-tools-extra/clang-tidy/readability/AvoidReturnWithVoidValueCheck.cpp Outdated Show resolved Hide resolved

EugeneZelenko requested a review from PiotrZSL February 19, 2024 01:51

SimplyDanny force-pushed the avoid-return-with-void-value-fixits branch from b99fe4b to 71a7da3 Compare February 19, 2024 18:08

SimplyDanny requested a review from HerrCai0907 February 19, 2024 18:10

PiotrZSL reviewed Feb 20, 2024

View reviewed changes

SimplyDanny force-pushed the avoid-return-with-void-value-fixits branch from 71a7da3 to 070968b Compare March 30, 2024 18:45

SimplyDanny requested a review from PiotrZSL March 30, 2024 19:08

SimplyDanny added 5 commits April 6, 2024 15:42

[clang-tidy] Add fix-its to avoid-return-with-void-value check

4b7d4a4

Omit blocks for single statements

ebeefa6

Use findNextTerminator

3d36f81

Reuse logic from existing check to insert braces around statements

1571389

Avoid blank return when statement is last in a function body

91db7ba

SimplyDanny force-pushed the avoid-return-with-void-value-fixits branch from 070968b to 91db7ba Compare April 6, 2024 13:43

HerrCai0907 approved these changes Apr 7, 2024

View reviewed changes

PiotrZSL approved these changes Apr 7, 2024

View reviewed changes

SimplyDanny merged commit 7a4e897 into llvm:main Apr 7, 2024

SimplyDanny deleted the avoid-return-with-void-value-fixits branch April 7, 2024 11:28

SimplyDanny changed the title ~~[clang-tidy] Add fix-its to avoid-return-with-void-value check~~ [clang-tidy] Add fix-its to readability-avoid-return-with-void-value check Apr 7, 2024

mikerice1969 reviewed Apr 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[clang-tidy] Add fix-its to `readability-avoid-return-with-void-value` check #81420

[clang-tidy] Add fix-its to `readability-avoid-return-with-void-value` check #81420

Uh oh!

SimplyDanny commented Feb 11, 2024

Uh oh!

llvmbot commented Feb 11, 2024

Uh oh!

llvmbot commented Feb 11, 2024

Uh oh!

SimplyDanny Feb 11, 2024

Uh oh!

SimplyDanny Feb 11, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

PiotrZSL left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SimplyDanny commented Apr 6, 2024

Uh oh!

PiotrZSL left a comment

Uh oh!

PiotrZSL Apr 7, 2024

Uh oh!

mikerice1969 Apr 25, 2024

Uh oh!

SimplyDanny Apr 26, 2024

Uh oh!

Uh oh!

[clang-tidy] Add fix-its to readability-avoid-return-with-void-value check #81420

[clang-tidy] Add fix-its to readability-avoid-return-with-void-value check #81420

Uh oh!

Conversation

SimplyDanny commented Feb 11, 2024

Uh oh!

llvmbot commented Feb 11, 2024

Uh oh!

llvmbot commented Feb 11, 2024

Uh oh!

SimplyDanny Feb 11, 2024

Choose a reason for hiding this comment

Uh oh!

SimplyDanny Feb 11, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

PiotrZSL left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SimplyDanny commented Apr 6, 2024

Uh oh!

PiotrZSL left a comment

Choose a reason for hiding this comment

Uh oh!

PiotrZSL Apr 7, 2024

Choose a reason for hiding this comment

Uh oh!

mikerice1969 Apr 25, 2024

Choose a reason for hiding this comment

Uh oh!

SimplyDanny Apr 26, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

[clang-tidy] Add fix-its to `readability-avoid-return-with-void-value` check #81420

[clang-tidy] Add fix-its to `readability-avoid-return-with-void-value` check #81420