[clang-tidy] Add option `RewriteVoidReturnType` to `modernize-use-trailing-return-type`

[Izaron]

Summary

The check doesn't trigger for functions returning void. The new options
is added to allow to have a complete homogeneous code.

Fixes https://github.com/llvm/llvm-project/issues/54383

[Izaron]

clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp

398–401

ctors/dtors are voidType, so we need to add filters for them when removing filter for voidType

[bernhardmgruber]

Hi! I am the original author of this check. I very much welcome your contribution! Thank you for the effort!

I am not a clang tools maintainer though, so you will need someone else to review and approve this change set.

clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp

420–421

I would intuitively check the cheaper condition first to benefit form short-circuit evaluation.

clang-tools-extra/docs/clang-tidy/checks/modernize/use-trailing-return-type.rst

73–76

I picked up the habit to word conditions/options in a positive manner. So I would rather name the option RewriteVoidReturnType instead of IgnoreVoidReturnType and use the inverted Booleans throughout the code. The reason is that my brain parses e.g. if (!RewriteVoidReturnType) easierly than if(!IgnoreVoidReturnType) because of the double negation.

Feel free to debate me here. This is not a strong opinion.

[Izaron]

Change IgnoreVoidReturnType to RewriteVoidReturn type.
Check the cheaper condition first.

A big thank to @bernhardmgruber for nice comments!

[njames93]

The default behaviour of this check should be transform void return types as that's how it has been since the check was first created. Adding an option which defaults to changing this behaviour would be harmful to current users of this check.

clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp

132

There's no reason this should be accessible globally.

clang-tools-extra/docs/clang-tidy/checks/modernize/use-trailing-return-type.rst

73–76

Please change this back to IgnoreVoidReturnType or maybe even IgnoreVoid, A lot of checks use Ignore options to help restrict what the check will report on, so we should follow that convention.
If you want to avoid the double negatives that can be achieved by negative the value when you read and write the option from/to the options map.

RewriteVoidReturnType(!Options.get("IgnoreVoid", false))

clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.h

 };
 
 /// Rewrites function signatures to use a trailing return type.
 ///
 /// For the user-facing documentation see:
 /// http://clang.llvm.org/extra/clang-tidy/checks/modernize/use-trailing-type-return.html
 class UseTrailingReturnTypeCheck : public ClangTidyCheck {
 public:
-  UseTrailingReturnTypeCheck(StringRef Name, ClangTidyContext *Context)
-      : ClangTidyCheck(Name, Context) {}
+  UseTrailingReturnTypeCheck(StringRef Name, ClangTidyContext *Context);
   bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
     return LangOpts.CPlusPlus11;
   }
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
                            Preprocessor *ModuleExpanderPP) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
 
 private:
   Preprocessor *PP = nullptr;
+  const bool RewriteVoidReturnType;
 
   SourceLocation findTrailingReturnTypeSourceLocation(
       const FunctionDecl &F, const FunctionTypeLoc &FTL, const ASTContext &Ctx,
       const SourceManager &SM, const LangOptions &LangOpts);
   llvm::Optional<SmallVector<ClassifiedToken, 8>>
   classifyTokensBeforeFunctionName(const FunctionDecl &F, const ASTContext &Ctx,
                                    const SourceManager &SM,
                                    const LangOptions &LangOpts);

clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp

static SourceLocation expandIfMacroId(SourceLocation Loc,
                                       const SourceManager &SM) {
   if (Loc.isMacroID())
     Loc = expandIfMacroId(SM.getImmediateExpansionRange(Loc).getBegin(), SM);
   assert(!Loc.isMacroID() &&
          "SourceLocation must not be a macro ID after recursive expansion");
   return Loc;
 }
 
+UseTrailingReturnTypeCheck::UseTrailingReturnTypeCheck(
+    StringRef Name, ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      RewriteVoidReturnType(
+          Options.getLocalOrGlobal("RewriteVoidReturnType", false)) {}
+
+void UseTrailingReturnTypeCheck::storeOptions(
+    ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "RewriteVoidReturnType", RewriteVoidReturnType);
+}
+
 SourceLocation UseTrailingReturnTypeCheck::findTrailingReturnTypeSourceLocation(
     const FunctionDecl &F, const FunctionTypeLoc &FTL, const ASTContext &Ctx,
     const SourceManager &SM, const LangOptions &LangOpts) {
   // We start with the location of the closing parenthesis.
   SourceRange ExceptionSpecRange = F.getExceptionSpecSourceRange();
   if (ExceptionSpecRange.isValid())
     return Lexer::getLocForEndOfToken(ExceptionSpecRange.getEnd(), 0, SM,
                                       LangOpts);
if (!llvm::isSpace(Specifier.back()))
       Specifier.push_back(' ');
     Auto.insert(Auto.size() - InitialAutoLength, Specifier);
     ReturnType.erase(TOffsetInRT, TLengthWithWS);
     DeletedChars += TLengthWithWS;
   }
 }
 
 void UseTrailingReturnTypeCheck::registerMatchers(MatchFinder *Finder) {
-  auto F = functionDecl(
-               unless(anyOf(hasTrailingReturn(), returns(voidType()),
-                            cxxConversionDecl(), cxxMethodDecl(isImplicit()))))
+  auto F = functionDecl(unless(anyOf(hasTrailingReturn(), cxxConstructorDecl(),
+                                     cxxDestructorDecl(), cxxConversionDecl(),
+                                     cxxMethodDecl(isImplicit()))))
                .bind("Func");
 
   Finder->addMatcher(F, this);
   Finder->addMatcher(friendDecl(hasDescendant(F)).bind("Friend"), this);
 }
 
 void UseTrailingReturnTypeCheck::registerPPCallbacks(
     const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) {
   this->PP = PP;
 }
 
 void UseTrailingReturnTypeCheck::check(const MatchFinder::MatchResult &Result) {
   assert(PP && "Expected registerPPCallbacks() to have been called before so "
                "preprocessor is available");
 
   const auto *F = Result.Nodes.getNodeAs<FunctionDecl>("Func");
   const auto *Fr = Result.Nodes.getNodeAs<FriendDecl>("Friend");
   assert(F && "Matcher is expected to find only FunctionDecls");
 
+  if (!RewriteVoidReturnType && F->getReturnType()->isVoidType())
+    return;
+
   // Three-way comparison operator<=> is syntactic sugar and generates implicit
   // nodes for all other operators.
   if (F->getLocation().isInvalid() || F->isImplicit())
     return;
 
   // Skip functions which return 'auto' and defaulted operators.
   const auto *AT = F->getDeclaredReturnType()->getAs<AutoType>();
   if (AT != nullptr &&

clang-tools-extra/docs/ReleaseNotes.rst

- Fixed crashes in :doc:`readability-braces-around-statements
   when using a C++23 ``if consteval`` statement.
 
 - Improved :doc:`misc-redundant-expression <clang-tidy/checks/misc/redundant-expression>`
   check.
 
   The check now skips concept definitions since redundant expressions still make sense
   inside them.
 
+- Improved :doc:`modernize-use-trailing-return-type <clang-tidy/checks/modernize/use-trailing-return-type>`
+  check.
+
+  The check now has a new option `RewriteVoidReturnType` which indicates if the
+  check should rewrite function signature for functions with void return type.
+
 Removed checks
 ^^^^^^^^^^^^^^
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 

clang-tools-extra/docs/clang-tidy/checks/modernize/use-trailing-return-type.rst

class CC {
     int S;
     auto m() -> struct S;
   };
   auto CC::m() -> S { return {0}; } // error
 
 This code fails to compile because the S in the context of f refers to the equally named function parameter.
 Similarly, the S in the context of m refers to the equally named class member.
 The check can currently only detect and avoid a clash with a function parameter name.
+
+Options
+-------
+
+.. option:: RewriteVoidReturnType
+
+   When `true`, the check will rewrite function signature for functions
+   with void return type. Default is `false`.

clang-tools-extra/test/clang-tidy/checkers/modernize/use-trailing-return-type-cxx20.cpp

 struct strong_ordering {
   using value_type = signed char;
   static strong_ordering const less;
   static strong_ordering const equal;
   static strong_ordering const equivalent;
   static strong_ordering const greater;
 
   constexpr strong_ordering(value_type v) : val(v) {}
+  constexpr ~strong_ordering() = default;
   template <typename T>
   requires(T{0}) friend constexpr auto
   operator==(strong_ordering v, T u) noexcept -> bool {
     return v.val == u;
   }
   friend constexpr auto operator==(strong_ordering v, strong_ordering w) noexcept -> bool = default;
 
   value_type val{};

clang-tools-extra/test/clang-tidy/checkers/modernize/use-trailing-return-type-void.cpp

+// RUN: %check_clang_tidy %s modernize-use-trailing-return-type %t -- \
+// RUN:   -config="{CheckOptions: [{key: modernize-use-trailing-return-type.RewriteVoidReturnType, value: true}]}"
+
+void c();
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use a trailing return type for this function [modernize-use-trailing-return-type]
+// CHECK-FIXES: {{^}}auto c() -> void;{{$}}
+void c(int arg);
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use a trailing return type for this function [modernize-use-trailing-return-type]
+// CHECK-FIXES: {{^}}auto c(int arg) -> void;{{$}}
+void c(int arg) { return; }
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: use a trailing return type for this function [modernize-use-trailing-return-type]
+// CHECK-FIXES: {{^}}auto c(int arg) -> void { return; }{{$}}
+
+//
+// Samples which do not trigger the check
+//
+
+auto f() -> void;
+auto f(int arg) -> void;
+auto f(int arg) -> void { return; }
+
+class C {
+    C() {}
+    ~C() {}
+};