llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tidy <details> <summary>Changes</summary> Improve diagnostic message to be more straight forward, fix handling of casting to non-void and add new option AllowCastToVoid to control casting to void behavior. Closes #<!-- -->66570 --- Patch is 23.93 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/66573.diff 9 Files Affected: - (modified) clang-tools-extra/clang-tidy/bugprone/UnusedReturnValueCheck.cpp (+25-11) - (modified) clang-tools-extra/clang-tidy/bugprone/UnusedReturnValueCheck.h (+4) - (modified) clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp (+1) - (modified) clang-tools-extra/docs/ReleaseNotes.rst (+6) - (modified) clang-tools-extra/docs/clang-tidy/checks/bugprone/unused-return-value.rst (+4) - (modified) clang-tools-extra/docs/clang-tidy/checks/cert/err33-c.rst (+3) - (modified) clang-tools-extra/test/clang-tidy/checkers/bugprone/unused-return-value-custom.cpp (+11-16) - (modified) clang-tools-extra/test/clang-tidy/checkers/bugprone/unused-return-value.cpp (+54-49) - (modified) clang-tools-extra/test/clang-tidy/checkers/cert/err33-c.c (+6-6) ``````````diff diff --git a/clang-tools-extra/clang-tidy/bugprone/UnusedReturnValueCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UnusedReturnValueCheck.cpp index bdc601c2445f5e6..6bc9f2dd367dcbd 100644 --- a/clang-tools-extra/clang-tidy/bugprone/UnusedReturnValueCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/UnusedReturnValueCheck.cpp @@ -130,26 +130,35 @@ UnusedReturnValueCheck::UnusedReturnValueCheck(llvm::StringRef Name, "::std::error_condition;" "::std::errc;" "::std::expected;" - "::boost::system::error_code"))) {} + "::boost::system::error_code"))), + AllowCastToVoid(Options.get("AllowCastToVoid", false)) {} void UnusedReturnValueCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { Options.store(Opts, "CheckedFunctions", CheckedFunctions); Options.store(Opts, "CheckedReturnTypes", utils::options::serializeStringList(CheckedReturnTypes)); + Options.store(Opts, "AllowCastToVoid", AllowCastToVoid); } void UnusedReturnValueCheck::registerMatchers(MatchFinder *Finder) { auto FunVec = utils::options::parseStringList(CheckedFunctions); - auto MatchedCallExpr = expr(ignoringImplicit(ignoringParenImpCasts( - callExpr(callee(functionDecl( - // Don't match void overloads of checked functions. - unless(returns(voidType())), - anyOf(isInstantiatedFrom(hasAnyName(FunVec)), - returns(hasCanonicalType(hasDeclaration( - namedDecl(matchers::matchesAnyListedName( - CheckedReturnTypes))))))))) - .bind("match")))); + auto MatchedDirectCallExpr = + expr(callExpr(callee(functionDecl( + // Don't match void overloads of checked functions. + unless(returns(voidType())), + anyOf(isInstantiatedFrom(hasAnyName(FunVec)), + returns(hasCanonicalType(hasDeclaration( + namedDecl(matchers::matchesAnyListedName( + CheckedReturnTypes))))))))) + .bind("match")); + + auto CheckCastToVoid = + AllowCastToVoid ? castExpr(unless(hasCastKind(CK_ToVoid))) : castExpr(); + auto MatchedCallExpr = expr( + anyOf(MatchedDirectCallExpr, + explicitCastExpr(unless(cxxFunctionalCastExpr()), CheckCastToVoid, + hasSourceExpression(MatchedDirectCallExpr)))); auto UnusedInCompoundStmt = compoundStmt(forEach(MatchedCallExpr), @@ -178,8 +187,13 @@ void UnusedReturnValueCheck::registerMatchers(MatchFinder *Finder) { void UnusedReturnValueCheck::check(const MatchFinder::MatchResult &Result) { if (const auto *Matched = Result.Nodes.getNodeAs<CallExpr>("match")) { diag(Matched->getBeginLoc(), - "the value returned by this function should be used") + "the value returned by this function should not be disregarded; " + "neglecting it may lead to errors") << Matched->getSourceRange(); + + if (!AllowCastToVoid) + return; + diag(Matched->getBeginLoc(), "cast the expression to void to silence this warning", DiagnosticIDs::Note); diff --git a/clang-tools-extra/clang-tidy/bugprone/UnusedReturnValueCheck.h b/clang-tools-extra/clang-tidy/bugprone/UnusedReturnValueCheck.h index 15881e12ca0c36a..b4356f8379fdc85 100644 --- a/clang-tools-extra/clang-tidy/bugprone/UnusedReturnValueCheck.h +++ b/clang-tools-extra/clang-tidy/bugprone/UnusedReturnValueCheck.h @@ -24,10 +24,14 @@ class UnusedReturnValueCheck : public ClangTidyCheck { void storeOptions(ClangTidyOptions::OptionMap &Opts) override; void registerMatchers(ast_matchers::MatchFinder *Finder) override; void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + std::optional<TraversalKind> getCheckTraversalKind() const override { + return TK_IgnoreUnlessSpelledInSource; + } private: std::string CheckedFunctions; const std::vector<StringRef> CheckedReturnTypes; + const bool AllowCastToVoid; }; } // namespace clang::tidy::bugprone diff --git a/clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp b/clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp index d448d9ba6145481..d334ab8c437d32a 100644 --- a/clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp @@ -328,6 +328,7 @@ class CERTModule : public ClangTidyModule { ClangTidyOptions::OptionMap &Opts = Options.CheckOptions; Opts["cert-dcl16-c.NewSuffixes"] = "L;LL;LU;LLU"; Opts["cert-err33-c.CheckedFunctions"] = CertErr33CCheckedFunctions; + Opts["cert-err33-c.AllowCastToVoid"] = "true"; Opts["cert-oop54-cpp.WarnOnlyIfThisHasSuspiciousField"] = "false"; Opts["cert-str34-c.DiagnoseSignedUnsignedCharComparisons"] = "false"; return Options; diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 6d6f51998a01e57..7432333024f44fe 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -202,6 +202,12 @@ Changes in existing checks <clang-tidy/checks/bugprone/reserved-identifier>` check, so that it does not warn on macros starting with underscore and lowercase letter. +- Improved :doc:`bugprone-unused-return-value + <clang-tidy/checks/bugprone/unused-return-value>` check diagnostic message, + added support for detection of unused results when cast to non-``void`` type. + Casting to ``void`` no longer suppresses issues by default, control this + behavior with the new `AllowCastToVoid` option. + - Improved :doc:`cppcoreguidelines-avoid-non-const-global-variables <clang-tidy/checks/cppcoreguidelines/avoid-non-const-global-variables>` check to ignore ``static`` variables declared within the scope of diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/unused-return-value.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/unused-return-value.rst index 5ca96216a10043e..1e3c8a3268222ed 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/bugprone/unused-return-value.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/unused-return-value.rst @@ -52,5 +52,9 @@ Options By default the following function return types are checked: `::std::error_code`, `::std::error_condition`, `::std::errc`, `::std::expected`, `::boost::system::error_code` +.. option:: AllowCastToVoid + + Controls whether casting return values to ``void`` is permitted. Default: `false`. + :doc:`cert-err33-c <../cert/err33-c>` is an alias of this check that checks a fixed and large set of standard library functions. diff --git a/clang-tools-extra/docs/clang-tidy/checks/cert/err33-c.rst b/clang-tools-extra/docs/clang-tidy/checks/cert/err33-c.rst index f3211e50b7e6cad..c1414ec5e086db1 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/cert/err33-c.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/cert/err33-c.rst @@ -189,6 +189,9 @@ functions are checked: This check is an alias of check :doc:`bugprone-unused-return-value <../bugprone/unused-return-value>` with a fixed set of functions. +Suppressing issues by casting to ``void`` is enabled by default and can be +disabled by setting `AllowCastToVoid` option to ``false``. + The check corresponds to a part of CERT C Coding Standard rule `ERR33-C. Detect and handle standard library errors <https://wiki.sei.cmu.edu/confluence/display/c/ERR33-C.+Detect+and+handle+standard+library+errors>`_. diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unused-return-value-custom.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unused-return-value-custom.cpp index fb5f77031a3276f..d3650b210ab02b5 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unused-return-value-custom.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unused-return-value-custom.cpp @@ -47,40 +47,35 @@ void fun(int); void warning() { fun(); - // CHECK-NOTES: [[@LINE-1]]:3: warning: the value returned by this function should be used - // CHECK-NOTES: [[@LINE-2]]:3: note: cast the expression to void to silence this warning + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should not be disregarded; neglecting it may lead to errors (fun()); - // CHECK-NOTES: [[@LINE-1]]:4: warning: the value {{.*}} should be used - // CHECK-NOTES: [[@LINE-2]]:4: note: cast {{.*}} this warning + // CHECK-MESSAGES: [[@LINE-1]]:4: warning: the value returned by this function should not be disregarded; neglecting it may lead to errors + + (void)fun(); + // CHECK-MESSAGES: [[@LINE-1]]:9: warning: the value returned by this function should not be disregarded; neglecting it may lead to errors ns::Outer::Inner ObjA1; ObjA1.memFun(); - // CHECK-NOTES: [[@LINE-1]]:3: warning: the value {{.*}} should be used - // CHECK-NOTES: [[@LINE-2]]:3: note: cast {{.*}} this warning + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should not be disregarded; neglecting it may lead to errors ns::AliasName::Inner ObjA2; ObjA2.memFun(); - // CHECK-NOTES: [[@LINE-1]]:3: warning: the value {{.*}} should be used - // CHECK-NOTES: [[@LINE-2]]:3: note: cast {{.*}} this warning + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should not be disregarded; neglecting it may lead to errors ns::Derived ObjA3; ObjA3.memFun(); - // CHECK-NOTES: [[@LINE-1]]:3: warning: the value {{.*}} should be used - // CHECK-NOTES: [[@LINE-2]]:3: note: cast {{.*}} this warning + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should not be disregarded; neglecting it may lead to errors ns::Type::staticFun(); - // CHECK-NOTES: [[@LINE-1]]:3: warning: the value {{.*}} should be used - // CHECK-NOTES: [[@LINE-2]]:3: note: cast {{.*}} this warning + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should not be disregarded; neglecting it may lead to errors ns::ClassTemplate<int> ObjA4; ObjA4.memFun(); - // CHECK-NOTES: [[@LINE-1]]:3: warning: the value {{.*}} should be used - // CHECK-NOTES: [[@LINE-2]]:3: note: cast {{.*}} this warning + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should not be disregarded; neglecting it may lead to errors ns::ClassTemplate<int>::staticFun(); - // CHECK-NOTES: [[@LINE-1]]:3: warning: the value {{.*}} should be used - // CHECK-NOTES: [[@LINE-2]]:3: note: cast {{.*}} this warning + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should not be disregarded; neglecting it may lead to errors } void noWarning() { diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unused-return-value.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unused-return-value.cpp index 6da66ca42d90fa8..5c6ce1e4bf1fd21 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unused-return-value.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unused-return-value.cpp @@ -1,4 +1,5 @@ -// RUN: %check_clang_tidy %s bugprone-unused-return-value %t -- -- -fexceptions +// RUN: %check_clang_tidy %s bugprone-unused-return-value %t -- \ +// RUN: --config="{CheckOptions: {bugprone-unused-return-value.AllowCastToVoid: true}}" -- -fexceptions namespace std { @@ -81,121 +82,125 @@ std::error_code errorFunc() { void warning() { std::async(increment, 42); - // CHECK-NOTES: [[@LINE-1]]:3: warning: the value returned by this function should be used - // CHECK-NOTES: [[@LINE-2]]:3: note: cast the expression to void to silence this warning + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should not be disregarded; neglecting it may lead to errors + // CHECK-MESSAGES: [[@LINE-2]]:3: note: cast the expression to void to silence this warning std::async(std::launch::async, increment, 42); - // CHECK-NOTES: [[@LINE-1]]:3: warning: the value {{.*}} should be used - // CHECK-NOTES: [[@LINE-2]]:3: note: cast {{.*}} this warning + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should not be disregarded; neglecting it may lead to errors + // CHECK-MESSAGES: [[@LINE-2]]:3: note: cast the expression to void to silence this warning Foo F; std::launder(&F); - // CHECK-NOTES: [[@LINE-1]]:3: warning: the value {{.*}} should be used - // CHECK-NOTES: [[@LINE-2]]:3: note: cast {{.*}} this warning + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should not be disregarded; neglecting it may lead to errors + // CHECK-MESSAGES: [[@LINE-2]]:3: note: cast the expression to void to silence this warning std::remove(nullptr, nullptr, 1); - // CHECK-NOTES: [[@LINE-1]]:3: warning: the value {{.*}} should be used - // CHECK-NOTES: [[@LINE-2]]:3: note: cast {{.*}} this warning + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should not be disregarded; neglecting it may lead to errors + // CHECK-MESSAGES: [[@LINE-2]]:3: note: cast the expression to void to silence this warning std::remove_if(nullptr, nullptr, nullptr); - // CHECK-NOTES: [[@LINE-1]]:3: warning: the value {{.*}} should be used - // CHECK-NOTES: [[@LINE-2]]:3: note: cast {{.*}} this warning + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should not be disregarded; neglecting it may lead to errors + // CHECK-MESSAGES: [[@LINE-2]]:3: note: cast the expression to void to silence this warning std::unique(nullptr, nullptr); - // CHECK-NOTES: [[@LINE-1]]:3: warning: the value {{.*}} should be used - // CHECK-NOTES: [[@LINE-2]]:3: note: cast {{.*}} this warning + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should not be disregarded; neglecting it may lead to errors + // CHECK-MESSAGES: [[@LINE-2]]:3: note: cast the expression to void to silence this warning std::unique_ptr<Foo> UPtr; UPtr.release(); - // CHECK-NOTES: [[@LINE-1]]:3: warning: the value {{.*}} should be used - // CHECK-NOTES: [[@LINE-2]]:3: note: cast {{.*}} this warning + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should not be disregarded; neglecting it may lead to errors + // CHECK-MESSAGES: [[@LINE-2]]:3: note: cast the expression to void to silence this warning std::string Str; Str.empty(); - // CHECK-NOTES: [[@LINE-1]]:3: warning: the value {{.*}} should be used - // CHECK-NOTES: [[@LINE-2]]:3: note: cast {{.*}} this warning + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should not be disregarded; neglecting it may lead to errors + // CHECK-MESSAGES: [[@LINE-2]]:3: note: cast the expression to void to silence this warning + + (int)Str.empty(); + // CHECK-MESSAGES: [[@LINE-1]]:8: warning: the value returned by this function should not be disregarded; neglecting it may lead to errors + // CHECK-MESSAGES: [[@LINE-2]]:8: note: cast the expression to void to silence this warning std::vector<Foo> Vec; Vec.empty(); - // CHECK-NOTES: [[@LINE-1]]:3: warning: the value {{.*}} should be used - // CHECK-NOTES: [[@LINE-2]]:3: note: cast {{.*}} this warning + // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should not be disregarded; neglecting it may lead to errors + // CHECK-MESSAGES: [[@LINE-2]]:3: note: cast the expression to void to silence this warning // test discarding return values inside different kinds of statements auto Lambda = [] { std::remove(nullptr, nullptr, 1); }; - // CHECK-NOTES: [[@LINE-1]]:22: warning: the value {{.*}} should be used - // CHECK-NOTES: [[@LINE-2]]:22: note: cast {{.*}} this warning + // CHECK-MESSAGES: [[@LINE-1]]:22: warning: the value returned by this function should not be disregarded; neglecting it may lead to errors + // CHECK-MESSAGES: [[@LINE-2]]:22: note: cast the expression to void to silence this warning if (true) std::remove(nullptr, nullptr, 1); - // CHECK-NOTES: [[@LINE-1]]:5: warning: the value {{.*}} should be used - // CHECK-NOTES: [[@LINE-2]]:5: note: cast {{.*}} this warning + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: the value returned by this function should not be disregarded; neglecting it may lead to errors + // CHECK-MESSAGES: [[@LINE-2]]:5: note: cast the expression to void to silence this warning else if (true) std::remove(nullptr, nullptr, 1); - // CHECK-NOTES: [[@LINE-1]]:5: warning: the value {{.*}} should be used - // CHECK-NOTES: [[@LINE-2]]:5: note: cast {{.*}} this warning + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: the value returned by this function should not be disregarded; neglecting it may lead to errors + // CHECK-MESSAGES: [[@LINE-2]]:5: note: cast the expression to void to silence this warning else std::remove(nullptr, nullptr, 1); - // CHECK-NOTES: [[@LINE-1]]:5: warning: the value {{.*}} should be used - // CHECK-NOTES: [[@LINE-2]]:5: note: cast {{.*}} this warning + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: the value returned by this function should not be disregarded; neglecting it may lead to errors + // CHECK-MESSAGES: [[@LINE-2]]:5: note: cast the expression to void to silence this warning while (true) std::remove(nullptr, nullptr, 1); - // CHECK-NOTES: [[@LINE-1]]:5: warning: the value {{.*}} should be used - // CHECK-NOTES: [[@LINE-2]]:5: note: cast {{.*}} this warning + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: the value returned by this function should not be disregarded; neglecting it may lead to errors + // CHECK-MESSAGES: [[@LINE-2]]:5: note: cast the expression to void to silence this warning do std::remove(nullptr, nullptr, 1); - // CHECK-NOTES: [[@LINE-1]]:5: warning: the value {{.*}} should be used - // CHECK-NOTES: [[@LINE-2]]:5: note: cast {{.*}} this warning + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: the value returned by this function should not be disregarded; neglecting it may lead to errors + // CHECK-MESSAGES: [[@LINE-2]]:5: note: cast the expression to void to silence this warning while (true); for (;;) std::remove(nullptr, nullptr, 1); - // CHECK-NOTES: [[@LINE-1]]:5: warning: the value {{.*}} should be used - // CHECK-NOTES: [[@LINE-2]]:5: note: cast {{.*}} this warning + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: the value returned by this function should not be disregarded; neglecting it may lead to errors + // CHECK-MESSAGES: [[@LINE-2]]:5: note: cast the expression to void to silence this warning for (std::remove(nullptr, nullptr, 1);;) - // CHECK-NOTES: [[@LINE-1]]:8: warning: the value {{.*}} should be used - // CHECK-NOTES: [[@LINE-2]]:8: note: cast {{.*}} this warning + // CHECK-MESSAGES: [[@LINE-1]]:8: warning: the value returned by this function should not be disregarded; neglecting it may lead to errors + // CHECK-MESSAGES: [[@LINE-2]]:8: note: cast the expression to void to silence this warning ; for (;; std::remove(nullptr, nullptr, 1)) - // CHECK-NOTES: [[@LINE-1]]:11: warning: the value {{.*}} should be used - // CHECK-NOTES: [[@LINE-2]]:11: note: cast {{.*}} this warning + // CHECK-MESSAGES: [[@LINE-1]]:11: warning: the value returned by this function should not be disregarded; neglecting it may lead to errors + // CHECK-MESSAGES: [[@LINE-2]]:11: note: cast the expression to void to silence this warning ; for (auto C : "foo") std::remove(nullptr, nullptr, 1); - // CHECK-NOTES: [[@LINE-1]]:5: warning: the value {{.*}} should be used - // CHECK-NOTES: [[@LINE-2]]:5: note: cast {{.*}} this warning + // CHECK-MESSAGES: [[@LINE-1]]:5: warning: the value returned by this function should not be disregarded; neglecting it may lead to errors + // CHECK-MESSAGES: [[@LINE-2]]:5: note: cast the expression to void to silence this warning switch (1) { case 1: std::remove(nullptr, nullptr, 1); - // CHECK-NOTES: [[@LINE-1]]:5: warning: the value {{.*}} should be used - // CHECK-NOTES: [[@LINE-2]]:5: note: cast {{.*}} this warning + // CHECK-MES... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/66573 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
