[clang-tidy] Implement CppCoreGuideline CP.53

[ccotter]

Summary

Implement CppCoreGuideline CP.53 to warn when a coroutine accepts
references parameters. Although the guideline mentions that it is safe
to access a reference parameter before suspension points, the guideline
recommends flagging all coroutine parameter references.

[ccotter]

Passing and accessing a reference is safe if the access is done before any suspend point (taking into account whether the initial coroutine suspend actually suspends or not). Although CP.53 recommends warning on any reference parameter regardless of whether the code only accesses the reference before any suspend points, I did implement (not in this changeset - in a separate branch) a version of this check that uses the CFG to determine whether reference parameters are never accessed after a suspend point. If there's interest, I could reimplement this check with this (perhaps as a tool option). That said, I do agree with CP.53 that relying on such access being safe is brittle e.g. if the code is refactored in the future to rearrange suspend points, so I went with this approach for the review.

[Eugene.Zelenko]

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-reference-coroutine-parameters.rst

10

Reference link is usually placed at the end of documentation.

[ccotter]

move reference link

[ccotter]

spelling

[ccotter]

• run clang-format

[ccotter]

• Does not offer fixes

[carlosgalvezp]

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

32

Check for nullptr before dereferencing. Typically it's done like:

if (const auto *Param = Result.Nodes.getNodeAs<ParmVarDecl>("param"))
  diag(Param->getBeginLoc(), ...);

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

32

Nit: this is called "LangOpts" in 157/159 checks, please rename for consistency.

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-reference-coroutine-parameters.rst

21

Add full link to this particular rule:

https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rcoro-reference-parameters

[ccotter]

• Cleanups

[ccotter]

• oops - bad arc diff / bad change

[carlosgalvezp]

LGTM, thanks for the patch! Maybe wait a couple days before landing to give some extra time for other reviewers. You might also want to mark the comments as "Done", so reviewers can get a quick overview of unresolved issues (if any).

[ccotter]

In D140793#4021694, @carlosgalvezp wrote:

LGTM, thanks for the patch! Maybe wait a couple days before landing to give some extra time for other reviewers. You might also want to mark the comments as "Done", so reviewers can get a quick overview of unresolved issues (if any).

Done, and thanks! I don't have permissions to land, so could you help land this for me after a few days have passed (assuming now further feedback)? You can use Chris Cotter <[e-mail]>

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

+//===--- AvoidReferenceCoroutineParametersCheck.h - clang-tidy --*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_AVOIDREFERENCECOROUTINEPARAMETERSCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_AVOIDREFERENCECOROUTINEPARAMETERSCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace cppcoreguidelines {
+
+/// Warns on coroutines that accept reference parameters. Accessing a reference
+/// after a coroutine suspension point is not safe since the reference may no
+/// longer be valid. This implements CppCoreGuideline CP.53.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines/avoid-reference-coroutine-parameters.html
+class AvoidReferenceCoroutineParametersCheck : public ClangTidyCheck {
+public:
+  AvoidReferenceCoroutineParametersCheck(StringRef Name,
+                                         ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  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;
+  }
+};
+
+} // namespace cppcoreguidelines
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_AVOIDREFERENCECOROUTINEPARAMETERSCHECK_H

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

+//===--- AvoidReferenceCoroutineParametersCheck.cpp - clang-tidy ----------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "AvoidReferenceCoroutineParametersCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace 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);
+}
+
+void AvoidReferenceCoroutineParametersCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  if (const auto *Param = Result.Nodes.getNodeAs<ParmVarDecl>("param")) {
+    diag(Param->getBeginLoc(), "coroutine parameters should not be references");
+  }
+}
+
+} // namespace cppcoreguidelines
+} // namespace tidy
+} // namespace clang

clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   FrontendOpenMP
   Support
   )
 
 add_clang_library(clangTidyCppCoreGuidelinesModule
   AvoidConstOrRefDataMembersCheck.cpp
   AvoidDoWhileCheck.cpp
   AvoidGotoCheck.cpp
   AvoidNonConstGlobalVariablesCheck.cpp
+  AvoidReferenceCoroutineParametersCheck.cpp
   CppCoreGuidelinesTidyModule.cpp
   InitVariablesCheck.cpp
   InterfacesGlobalInitCheck.cpp
   MacroUsageCheck.cpp
   NarrowingConversionsCheck.cpp
   NoMallocCheck.cpp
   OwningMemoryCheck.cpp
   PreferMemberInitializerCheck.cpp

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

 #include "../misc/UnconventionalAssignOperatorCheck.h"
 #include "../modernize/AvoidCArraysCheck.h"
 #include "../modernize/UseOverrideCheck.h"
 #include "../readability/MagicNumbersCheck.h"
 #include "AvoidConstOrRefDataMembersCheck.h"
 #include "AvoidDoWhileCheck.h"
 #include "AvoidGotoCheck.h"
 #include "AvoidNonConstGlobalVariablesCheck.h"
+#include "AvoidReferenceCoroutineParametersCheck.h"
 #include "InitVariablesCheck.h"
 #include "InterfacesGlobalInitCheck.h"
 #include "MacroUsageCheck.h"
 #include "NarrowingConversionsCheck.h"
 #include "NoMallocCheck.h"
 #include "OwningMemoryCheck.h"
 #include "PreferMemberInitializerCheck.h"
 #include "ProBoundsArrayToPointerDecayCheck.h"
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<AvoidDoWhileCheck>(
         "cppcoreguidelines-avoid-do-while");
     CheckFactories.registerCheck<AvoidGotoCheck>(
         "cppcoreguidelines-avoid-goto");
     CheckFactories.registerCheck<readability::MagicNumbersCheck>(
         "cppcoreguidelines-avoid-magic-numbers");
     CheckFactories.registerCheck<AvoidNonConstGlobalVariablesCheck>(
         "cppcoreguidelines-avoid-non-const-global-variables");
+    CheckFactories.registerCheck<AvoidReferenceCoroutineParametersCheck>(
+        "cppcoreguidelines-avoid-reference-coroutine-parameters");
     CheckFactories.registerCheck<modernize::UseOverrideCheck>(
         "cppcoreguidelines-explicit-virtual-functions");
     CheckFactories.registerCheck<InitVariablesCheck>(
         "cppcoreguidelines-init-variables");
     CheckFactories.registerCheck<InterfacesGlobalInitCheck>(
         "cppcoreguidelines-interfaces-global-init");
     CheckFactories.registerCheck<MacroUsageCheck>(
         "cppcoreguidelines-macro-usage");

clang-tools-extra/docs/ReleaseNotes.rst

- New :doc:`cppcoreguidelines-avoid-const-or-ref-data-members
 
   Warns when a struct or class uses const or reference (lvalue or rvalue) data members.
 
 - New :doc:`cppcoreguidelines-avoid-do-while
   <clang-tidy/checks/cppcoreguidelines/avoid-do-while>` check.
 
   Warns when using ``do-while`` loops.
 
+- New :doc:`cppcoreguidelines-avoid-reference-coroutine-parameters
+  <clang-tidy/checks/cppcoreguidelines/avoid-reference-coroutine-parameters>` check.
+
+  Warns on coroutines that accept reference parameters.
+
 - New :doc:`misc-use-anonymous-namespace
   <clang-tidy/checks/misc/use-anonymous-namespace>` check.
 
   Warns when using ``static`` function or variables at global scope, and suggests
   moving them into an anonymous namespace.
 
 - New :doc:`bugprone-standalone-empty <clang-tidy/checks/bugprone/standalone-empty>` check.
 

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-reference-coroutine-parameters.rst

+.. title:: clang-tidy - cppcoreguidelines-avoid-reference-coroutine-parameters
+
+cppcoreguidelines-avoid-reference-coroutine-parameters
+======================================================
+
+Warns when a coroutine accepts reference parameters. After a coroutine suspend point,
+references could be dangling and no longer valid. Instead, pass parameters as values.
+
+Examples:
+
+.. code-block:: c++
+
+  std::future<int> someCoroutine(int& val) {
+    co_await ...;
+    // When the coroutine is resumed, 'val' might no longer be valid.
+    if (val) ...
+  }
+
+This check implements
+`CppCoreGuideline CP.53 <https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rcoro-reference-parameters>`_.

clang-tools-extra/docs/clang-tidy/checks/list.rst

.. csv-table::
    `clang-analyzer-valist.Uninitialized <clang-analyzer/valist.Uninitialized.html>`_,
    `clang-analyzer-valist.Unterminated <clang-analyzer/valist.Unterminated.html>`_,
    `concurrency-mt-unsafe <concurrency/mt-unsafe.html>`_,
    `concurrency-thread-canceltype-asynchronous <concurrency/thread-canceltype-asynchronous.html>`_,
    `cppcoreguidelines-avoid-const-or-ref-data-members <cppcoreguidelines/avoid-const-or-ref-data-members.html>`_,
    `cppcoreguidelines-avoid-do-while <cppcoreguidelines/avoid-do-while.html>`_,
    `cppcoreguidelines-avoid-goto <cppcoreguidelines/avoid-goto.html>`_,
    `cppcoreguidelines-avoid-non-const-global-variables <cppcoreguidelines/avoid-non-const-global-variables.html>`_,
+   `cppcoreguidelines-avoid-reference-coroutine-parameters <cppcoreguidelines/avoid-reference-coroutine-parameters.html>`_,
    `cppcoreguidelines-init-variables <cppcoreguidelines/init-variables.html>`_, "Yes"
    `cppcoreguidelines-interfaces-global-init <cppcoreguidelines/interfaces-global-init.html>`_,
    `cppcoreguidelines-macro-usage <cppcoreguidelines/macro-usage.html>`_,
    `cppcoreguidelines-narrowing-conversions <cppcoreguidelines/narrowing-conversions.html>`_,
    `cppcoreguidelines-no-malloc <cppcoreguidelines/no-malloc.html>`_,
    `cppcoreguidelines-owning-memory <cppcoreguidelines/owning-memory.html>`_,
    `cppcoreguidelines-prefer-member-initializer <cppcoreguidelines/prefer-member-initializer.html>`_, "Yes"
    `cppcoreguidelines-pro-bounds-array-to-pointer-decay <cppcoreguidelines/pro-bounds-array-to-pointer-decay.html>`_,

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
+
+// NOLINTBEGIN
+namespace std {
+  template <typename T, typename... Args>
+  struct coroutine_traits {
+    using promise_type = typename T::promise_type;
+  };
+  template <typename T = void>
+  struct coroutine_handle;
+  template <>
+  struct coroutine_handle<void> {
+    coroutine_handle() noexcept;
+    coroutine_handle(decltype(nullptr)) noexcept;
+    static constexpr coroutine_handle from_address(void*);
+  };
+  template <typename T>
+  struct coroutine_handle {
+    coroutine_handle() noexcept;
+    coroutine_handle(decltype(nullptr)) noexcept;
+    static constexpr coroutine_handle from_address(void*);
+    operator coroutine_handle<>() const noexcept;
+  };
+} // namespace std
+
+struct Awaiter {
+  bool await_ready() noexcept;
+  void await_suspend(std::coroutine_handle<>) noexcept;
+  void await_resume() noexcept;
+};
+
+struct Coro {
+  struct promise_type {
+    Awaiter initial_suspend();
+    Awaiter final_suspend() noexcept;
+    void return_void();
+    Coro get_return_object();
+    void unhandled_exception();
+  };
+};
+// NOLINTEND
+
+struct Obj {};
+
+Coro no_args() {
+  co_return;
+}
+
+Coro no_references(int x, int* y, Obj z, const Obj w) {
+  co_return;
+}
+
+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]
+  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]
+  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]
+  co_return;
+}
+
+Coro non_coro_accepts_references(int& x) {
+  if (x);
+  return Coro{};
+}
+
+void defines_a_lambda() {
+  auto NoArgs = [](int x) -> Coro { co_return; };
+
+  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]
+}