[clang-tidy] Improve cppcoreguidelines-avoid-reference-coroutine-parameters check

[PiotrZSL]

Summary

Ignore false positives related to matching parameters of non
coroutine functions and increase issue detection for cases
involving type aliases with references.

Fixes: #64915

[ccotter]

Thanks!

clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidReferenceCoroutineParametersCheck.cpp

20

Can we do this by matching if any parameter is of reference type, rather than relying on the logic living in check (I wrote this originally, although perhaps this would be an improvement). Happy to take this up as a followup...

[PiotrZSL]

clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidReferenceCoroutineParametersCheck.cpp

20

Matching functionDecl first is cheaper...
In theory using hasParent instead of hasAncestor should be sufficient, but I never trusted that relation.

Something like this should do a trick also:

functionDecl(unless(parameterCountIs(0)), hasBody(coroutineBodyStmt()), forEach(parmVarDecl(hasType(qualType(hasCanonicalType(	references(qualType()))))).bind("param"))

should also work, but I didn't test it.
Current code will do a same thing, not using matchers should even make it faster, and check method is called only for coroutines.

Personally I just prefer matcher to match nodes that we got less.
This also enable other thing, like providing 1 warning per coroutine and additional notes for every parameter with parameter name in it.

[PiotrZSL]

I'm pushing this in current state, if you want to change something after that, fell free to do it.

clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidReferenceCoroutineParametersCheck.cpp

 #include "clang/ASTMatchers/ASTMatchFinder.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang::tidy::cppcoreguidelines {
 
 void AvoidReferenceCoroutineParametersCheck::registerMatchers(
     MatchFinder *Finder) {
-  auto IsCoroMatcher =
-      hasDescendant(expr(anyOf(coyieldExpr(), coreturnStmt(), coawaitExpr())));
-  Finder->addMatcher(parmVarDecl(hasType(type(referenceType())),
-                                 hasAncestor(functionDecl(IsCoroMatcher)))
-                         .bind("param"),
-                     this);
+  Finder->addMatcher(
+      functionDecl(unless(parameterCountIs(0)), hasBody(coroutineBodyStmt()))
+          .bind("fnt"),
 }
 
 void AvoidReferenceCoroutineParametersCheck::check(
     const MatchFinder::MatchResult &Result) {
-  if (const auto *Param = Result.Nodes.getNodeAs<ParmVarDecl>("param")) {
+  const auto *Function = Result.Nodes.getNodeAs<FunctionDecl>("fnt");
+  for (const ParmVarDecl *Param : Function->parameters()) {
+    if (!Param->getType().getCanonicalType()->isReferenceType())
+      continue;
+
     diag(Param->getBeginLoc(), "coroutine parameters should not be references");
   }
 }
 
 } // namespace clang::tidy::cppcoreguidelines

clang-tools-extra/docs/ReleaseNotes.rst

- Improved :doc:`bugprone-reserved-identifier
   <clang-tidy/checks/bugprone/reserved-identifier>` check, so that it does not
   warn on macros starting with underscore and lowercase letter.
 
 - 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
   ``class``/``struct``.
 
+- Improved :doc:`cppcoreguidelines-avoid-reference-coroutine-parameters
+  <clang-tidy/checks/cppcoreguidelines/avoid-reference-coroutine-parameters>`
+  check to ignore false positives related to matching parameters of non
+  coroutine functions and increase issue detection for cases involving type
+  aliases with references.
+
 - Improved :doc:`cppcoreguidelines-prefer-member-initializer
   <clang-tidy/checks/cppcoreguidelines/prefer-member-initializer>` check to
   ignore delegate constructors.
 
 - Improved :doc:`cppcoreguidelines-pro-bounds-array-to-pointer-decay
   <clang-tidy/checks/cppcoreguidelines/pro-bounds-array-to-pointer-decay>` check
   to ignore predefined expression (e.g., ``__func__``, ...).
 

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-reference-coroutine-parameters.cpp

-// RUN: %check_clang_tidy -std=c++20 %s cppcoreguidelines-avoid-reference-coroutine-parameters %t
+// RUN: %check_clang_tidy -std=c++20 %s cppcoreguidelines-avoid-reference-coroutine-parameters %t --
 
 // NOLINTBEGIN
 namespace std {
   template <typename T, typename... Args>
   struct coroutine_traits {
     using promise_type = typename T::promise_type;
   };
   template <typename T = void>
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]
 
   auto WithReferences2 = [](int&) -> Coro { co_return; };
   // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: coroutine parameters should not be references [cppcoreguidelines-avoid-reference-coroutine-parameters]
 }
+
+void coroInFunctionWithReference(int&) {
+  auto SampleCoro = [](int x) -> Coro { co_return; };
+}
+
+Coro lambdaWithReferenceInCoro() {
+  auto SampleLambda = [](int& x) {};
+  co_return;
+}
+
+using MyIntegerRef = int&;
+Coro coroWithReferenceBehindTypedef(MyIntegerRef ref) {
+// CHECK-MESSAGES: :[[@LINE-1]]:37: warning: coroutine parameters should not be references [cppcoreguidelines-avoid-reference-coroutine-parameters]
+  co_return;
+}