[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

9

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

31

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

31

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

20

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
 //
 //===----------------------------------------------------------------------===//
 

 /// 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)
+  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 &LO) const override {
+  bool isLanguageVersionSupported(const LangOpts &LO) const override {
     return LO.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
 //
 //===----------------------------------------------------------------------===//
 
void AvoidReferenceCoroutineParametersCheck::registerMatchers(
   Finder->addMatcher(parmVarDecl(hasType(type(referenceType())),
                                  hasAncestor(functionDecl(IsCoroMatcher)))
                          .bind("param"),
                      this);
 }
 
 void AvoidReferenceCoroutineParametersCheck::check(
     const MatchFinder::MatchResult &Result) {
-  const auto *Param = Result.Nodes.getNodeAs<ParmVarDecl>("param");
-
+  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/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 <http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines>`_.
+`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>`_, "Yes"
+   `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>`_,