llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tidy Author: Justin Ottesen (justinottesen) <details> <summary>Changes</summary> Coroutines whose return type matches one of the AllowedReturnTypes regular expressions are no longer flagged for reference parameters. Some coroutine task types make reference parameters safe by construction: a task that is non-copyable, non-movable, and awaitable only as a prvalue must be awaited in the full expression that creates it, so every argument temporary outlives the coroutine's execution. For such codebases the check is noise on the safe pattern, and per-site NOLINT does not scale because the exemption is a property of the type, not of call sites. This follows the existing pattern of opt-in exemption options in this module (e.g. owning-memory's LegacyResourceProducers, special-member-functions' Allow* options). The default is an empty list, which preserves existing behavior. --- Full diff: https://github.com/llvm/llvm-project/pull/208970.diff 5 Files Affected: - (modified) clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidReferenceCoroutineParametersCheck.cpp (+17-1) - (modified) clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidReferenceCoroutineParametersCheck.h (+7-2) - (modified) clang-tools-extra/docs/ReleaseNotes.rst (+6) - (modified) clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-reference-coroutine-parameters.rst (+22) - (modified) clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-reference-coroutine-parameters.cpp (+45-8) ``````````diff diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidReferenceCoroutineParametersCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidReferenceCoroutineParametersCheck.cpp index 7ef1e2bc6178d..3ba185200806d 100644 --- a/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidReferenceCoroutineParametersCheck.cpp +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidReferenceCoroutineParametersCheck.cpp @@ -7,16 +7,32 @@ //===----------------------------------------------------------------------===// #include "AvoidReferenceCoroutineParametersCheck.h" +#include "../utils/Matchers.h" +#include "../utils/OptionsUtils.h" #include "clang/ASTMatchers/ASTMatchFinder.h" using namespace clang::ast_matchers; namespace clang::tidy::cppcoreguidelines { +AvoidReferenceCoroutineParametersCheck::AvoidReferenceCoroutineParametersCheck( + StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + AllowedReturnTypes(utils::options::parseStringList( + Options.get("AllowedReturnTypes", ""))) {} + +void AvoidReferenceCoroutineParametersCheck::storeOptions( + ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "AllowedReturnTypes", + utils::options::serializeStringList(AllowedReturnTypes)); +} + void AvoidReferenceCoroutineParametersCheck::registerMatchers( MatchFinder *Finder) { Finder->addMatcher( - functionDecl(unless(parameterCountIs(0)), hasBody(coroutineBodyStmt())) + functionDecl(unless(parameterCountIs(0)), hasBody(coroutineBodyStmt()), + unless(returns( + matchers::matchesAnyListedTypeName(AllowedReturnTypes)))) .bind("fnt"), this); } diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidReferenceCoroutineParametersCheck.h b/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidReferenceCoroutineParametersCheck.h index 78ed547787b9a..b7699ce69dd20 100644 --- a/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidReferenceCoroutineParametersCheck.h +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidReferenceCoroutineParametersCheck.h @@ -22,13 +22,18 @@ namespace clang::tidy::cppcoreguidelines { class AvoidReferenceCoroutineParametersCheck : public ClangTidyCheck { public: AvoidReferenceCoroutineParametersCheck(StringRef Name, - ClangTidyContext *Context) - : ClangTidyCheck(Name, Context) {} + ClangTidyContext *Context); + void storeOptions(ClangTidyOptions::OptionMap &Opts) override; void registerMatchers(ast_matchers::MatchFinder *Finder) override; void check(const ast_matchers::MatchFinder::MatchResult &Result) override; bool isLanguageVersionSupported(const LangOptions &LangOpts) const override { return LangOpts.CPlusPlus20; } + +private: + // Regular expressions matched against the coroutine's return type. Coroutines + // whose return type matches are exempt from the check. + const std::vector<StringRef> AllowedReturnTypes; }; } // namespace clang::tidy::cppcoreguidelines diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index f43b90f5a2386..7f2d148c022a2 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -504,6 +504,12 @@ Changes in existing checks check by adding the `IgnoreMacros` option. When enabled, non-const global variables defined in macros are ignored. +- Improved :doc:`cppcoreguidelines-avoid-reference-coroutine-parameters + <clang-tidy/checks/cppcoreguidelines/avoid-reference-coroutine-parameters>` + check by adding the `AllowedReturnTypes` option. Coroutines whose return type + matches one of the listed regular expressions are not flagged, allowing task + types that make reference parameters safe by construction to be exempted. + - Improved :doc:`cppcoreguidelines-init-variables <clang-tidy/checks/cppcoreguidelines/init-variables>` check by ensuring that member pointers are correctly flagged as uninitialized. diff --git a/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-reference-coroutine-parameters.rst b/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-reference-coroutine-parameters.rst index 887bdc66d938a..665f979e8f221 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-reference-coroutine-parameters.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-reference-coroutine-parameters.rst @@ -20,3 +20,25 @@ Examples: This check implements `CP.53 <https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#rcoro-reference-parameters>`_ from the C++ Core Guidelines. + +Options +------- + +.. option:: AllowedReturnTypes + + A semicolon-separated list of regular expressions matched against the + coroutine's return type. Coroutines whose return type matches one of the + expressions are not flagged, even if they accept reference parameters. This + is useful for task types that make reference parameters safe by + construction, such as non-copyable, non-movable coroutine types that can + only be awaited within the full expression that created them, so every + argument temporary outlives the coroutine. + + Matching is performed against the canonical, fully-qualified return type, so + type aliases are resolved before matching. For example, given + ``using MyTask = ns::Co<int>;``, a coroutine returning ``MyTask`` is matched + by ``ns::Co<int>`` (or a regular expression such as ``ns::Co<.*>``), but not + by ``MyTask``. + + The default value is an empty string, which preserves the behavior of + flagging every coroutine with reference parameters. diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-reference-coroutine-parameters.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-reference-coroutine-parameters.cpp index 2ad93e22fc884..f5db716bfbe15 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-reference-coroutine-parameters.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-reference-coroutine-parameters.cpp @@ -1,4 +1,5 @@ -// RUN: %check_clang_tidy -std=c++20-or-later %s cppcoreguidelines-avoid-reference-coroutine-parameters %t +// RUN: %check_clang_tidy -check-suffixes=DEFAULT -std=c++20-or-later %s cppcoreguidelines-avoid-reference-coroutine-parameters %t +// RUN: %check_clang_tidy -check-suffixes=ALLOWED -std=c++20-or-later %s cppcoreguidelines-avoid-reference-coroutine-parameters %t -- -config="{CheckOptions: {cppcoreguidelines-avoid-reference-coroutine-parameters.AllowedReturnTypes: 'RefSafeCoro'}}" // NOLINTBEGIN namespace std { @@ -38,6 +39,18 @@ struct Coro { void unhandled_exception(); }; }; + +// A coroutine task type whose reference parameters are safe by construction, +// used to exercise the 'AllowedReturnTypes' option. +struct RefSafeCoro { + struct promise_type { + Awaiter initial_suspend(); + Awaiter final_suspend() noexcept; + void return_void(); + RefSafeCoro get_return_object(); + void unhandled_exception(); + }; +}; // NOLINTEND struct Obj {}; @@ -51,18 +64,22 @@ Coro no_references(int x, int* y, Obj z, const Obj w) { } Coro accepts_references(int& x, const int &y) { - // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: coroutine parameters should not be references [cppcoreguidelines-avoid-reference-coroutine-parameters] - // CHECK-MESSAGES: :[[@LINE-2]]:33: warning: coroutine parameters should not be references [cppcoreguidelines-avoid-reference-coroutine-parameters] + // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:25: warning: coroutine parameters should not be references [cppcoreguidelines-avoid-reference-coroutine-parameters] + // CHECK-MESSAGES-ALLOWED: :[[@LINE-2]]:25: warning: coroutine parameters should not be references [cppcoreguidelines-avoid-reference-coroutine-parameters] + // CHECK-MESSAGES-DEFAULT: :[[@LINE-3]]:33: warning: coroutine parameters should not be references [cppcoreguidelines-avoid-reference-coroutine-parameters] + // CHECK-MESSAGES-ALLOWED: :[[@LINE-4]]:33: warning: coroutine parameters should not be references [cppcoreguidelines-avoid-reference-coroutine-parameters] co_return; } Coro accepts_references_and_non_references(int& x, int y) { - // CHECK-MESSAGES: :[[@LINE-1]]:44: warning: coroutine parameters should not be references [cppcoreguidelines-avoid-reference-coroutine-parameters] + // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:44: warning: coroutine parameters should not be references [cppcoreguidelines-avoid-reference-coroutine-parameters] + // CHECK-MESSAGES-ALLOWED: :[[@LINE-2]]:44: warning: coroutine parameters should not be references [cppcoreguidelines-avoid-reference-coroutine-parameters] co_return; } Coro accepts_references_to_objects(Obj& x) { - // CHECK-MESSAGES: :[[@LINE-1]]:36: warning: coroutine parameters should not be references [cppcoreguidelines-avoid-reference-coroutine-parameters] + // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:36: warning: coroutine parameters should not be references [cppcoreguidelines-avoid-reference-coroutine-parameters] + // CHECK-MESSAGES-ALLOWED: :[[@LINE-2]]:36: warning: coroutine parameters should not be references [cppcoreguidelines-avoid-reference-coroutine-parameters] co_return; } @@ -77,10 +94,12 @@ void defines_a_lambda() { auto NoReferences = [](int x) -> Coro { co_return; }; auto WithReferences = [](int& x) -> Coro { co_return; }; - // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: coroutine parameters should not be references [cppcoreguidelines-avoid-reference-coroutine-parameters] + // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:28: warning: coroutine parameters should not be references [cppcoreguidelines-avoid-reference-coroutine-parameters] + // CHECK-MESSAGES-ALLOWED: :[[@LINE-2]]:28: warning: coroutine parameters should not be references [cppcoreguidelines-avoid-reference-coroutine-parameters] auto WithReferences2 = [](int&) -> Coro { co_return; }; - // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: coroutine parameters should not be references [cppcoreguidelines-avoid-reference-coroutine-parameters] + // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:29: warning: coroutine parameters should not be references [cppcoreguidelines-avoid-reference-coroutine-parameters] + // CHECK-MESSAGES-ALLOWED: :[[@LINE-2]]:29: warning: coroutine parameters should not be references [cppcoreguidelines-avoid-reference-coroutine-parameters] } void coroInFunctionWithReference(int&) { @@ -94,6 +113,24 @@ Coro lambdaWithReferenceInCoro() { using MyIntegerRef = int&; Coro coroWithReferenceBehindTypedef(MyIntegerRef ref) { -// CHECK-MESSAGES: :[[@LINE-1]]:37: warning: coroutine parameters should not be references [cppcoreguidelines-avoid-reference-coroutine-parameters] +// CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:37: warning: coroutine parameters should not be references [cppcoreguidelines-avoid-reference-coroutine-parameters] +// CHECK-MESSAGES-ALLOWED: :[[@LINE-2]]:37: warning: coroutine parameters should not be references [cppcoreguidelines-avoid-reference-coroutine-parameters] + co_return; +} + +// The return type matches 'AllowedReturnTypes' in the second run, so the +// reference parameter is exempt there, but is still flagged by default. This +// confirms the option is type-specific and does not blanket-allow references: +// the 'Coro' coroutines above are still flagged in the same run. +RefSafeCoro allowedReturnType(int& x) { + // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:31: warning: coroutine parameters should not be references [cppcoreguidelines-avoid-reference-coroutine-parameters] + co_return; +} + +// The return type is matched canonically, so a type alias to an allowed type is +// also exempt in the second run. +using MyTask = RefSafeCoro; +MyTask allowedReturnTypeAlias(int& x) { + // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:31: warning: coroutine parameters should not be references [cppcoreguidelines-avoid-reference-coroutine-parameters] co_return; } `````````` </details> https://github.com/llvm/llvm-project/pull/208970 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
