[clang-tidy][performance-unnecessary-value-param] Avoid in coroutines

[dmpolukhin]

Summary:
Replacing by-value parameters with passing by-reference is not safe for coroutines because the caller may be executed in parallel with the callee, which increases the chances of resulting in dangling references and hard-to-find crashes. See for the reference cppcoreguidelines-avoid-reference-coroutine-parameters.

Test Plan: check-clang-tools

[llvmbot]

@llvm/pr-subscribers-clang-tools-extra

Author: Dmitry Polukhin (dmpolukhin)

Changes

Summary:
Replacing by-value parameters with passing by-reference is not safe for coroutines because the caller may be executed in parallel with the callee, which increases the chances of resulting in dangling references and hard-to-find crashes.

Test Plan: check-clang-tools

---

Full diff: https://github.com/llvm/llvm-project/pull/140912.diff

2 Files Affected:

• (modified) clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp (+1)
• (added) clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param-coroutine.cpp (+81)

diff --git a/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp b/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
index a877f9a7ee912..52d4c70a83f65 100644
--- a/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
+++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
@@ -65,6 +65,7 @@ void UnnecessaryValueParamCheck::registerMatchers(MatchFinder *Finder) {
           TK_AsIs,
           functionDecl(hasBody(stmt()), isDefinition(), unless(isImplicit()),
                        unless(cxxMethodDecl(anyOf(isOverride(), isFinal()))),
+                       unless(hasBody(coroutineBodyStmt())),
                        has(typeLoc(forEach(ExpensiveValueParamDecl))),
                        decl().bind("functionDecl"))),
       this);
diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param-coroutine.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param-coroutine.cpp
new file mode 100644
index 0000000000000..f2fb477143578
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param-coroutine.cpp
@@ -0,0 +1,81 @@
+// RUN: %check_clang_tidy -std=c++20-or-later %s performance-unnecessary-value-param %t -- -fix-errors
+
+namespace std {
+
+template <typename R, typename...> struct coroutine_traits {
+  using promise_type = typename R::promise_type;
+};
+
+template <typename Promise = void> struct coroutine_handle;
+
+template <> struct coroutine_handle<void> {
+  static coroutine_handle from_address(void *addr) noexcept {
+    coroutine_handle me;
+    me.ptr = addr;
+    return me;
+  }
+  void operator()() { resume(); }
+  void *address() const noexcept { return ptr; }
+  void resume() const {  }
+  void destroy() const { }
+  bool done() const { return true; }
+  coroutine_handle &operator=(decltype(nullptr)) {
+    ptr = nullptr;
+    return *this;
+  }
+  coroutine_handle(decltype(nullptr)) : ptr(nullptr) {}
+  coroutine_handle() : ptr(nullptr) {}
+  //  void reset() { ptr = nullptr; } // add to P0057?
+  explicit operator bool() const { return ptr; }
+
+protected:
+  void *ptr;
+};
+
+template <typename Promise> struct coroutine_handle : coroutine_handle<> {
+  using coroutine_handle<>::operator=;
+
+  static coroutine_handle from_address(void *addr) noexcept {
+    coroutine_handle me;
+    me.ptr = addr;
+    return me;
+  }
+
+  Promise &promise() const {
+    return *reinterpret_cast<Promise *>(
+        __builtin_coro_promise(ptr, alignof(Promise), false));
+  }
+  static coroutine_handle from_promise(Promise &promise) {
+    coroutine_handle p;
+    p.ptr = __builtin_coro_promise(&promise, alignof(Promise), true);
+    return p;
+  }
+};
+
+struct suspend_always {
+  bool await_ready() noexcept { return false; }
+  void await_suspend(std::coroutine_handle<>) noexcept {}
+  void await_resume() noexcept {}
+};
+
+} // namespace std
+
+struct ReturnObject {
+    struct promise_type {
+        ReturnObject get_return_object() { return {}; }
+        std::suspend_always initial_suspend() { return {}; }
+        std::suspend_always final_suspend() noexcept { return {}; }
+        void unhandled_exception() {}
+        std::suspend_always yield_value(int value) { return {}; }
+    };
+};
+
+struct A {
+  A(const A&);
+};
+
+ReturnObject evaluateModels(const A timeMachineId) {
+// No change for non-coroutine function expected because it is not safe.
+// CHECK-FIXES: ReturnObject evaluateModels(const A timeMachineId) {
+  co_return;
+}

[llvmbot]

@llvm/pr-subscribers-clang-tidy

Author: Dmitry Polukhin (dmpolukhin)

Changes

Summary:
Replacing by-value parameters with passing by-reference is not safe for coroutines because the caller may be executed in parallel with the callee, which increases the chances of resulting in dangling references and hard-to-find crashes.

Test Plan: check-clang-tools

---

Full diff: https://github.com/llvm/llvm-project/pull/140912.diff

2 Files Affected:

• (modified) clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp (+1)
• (added) clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param-coroutine.cpp (+81)

diff --git a/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp b/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
index a877f9a7ee912..52d4c70a83f65 100644
--- a/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
+++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
@@ -65,6 +65,7 @@ void UnnecessaryValueParamCheck::registerMatchers(MatchFinder *Finder) {
           TK_AsIs,
           functionDecl(hasBody(stmt()), isDefinition(), unless(isImplicit()),
                        unless(cxxMethodDecl(anyOf(isOverride(), isFinal()))),
+                       unless(hasBody(coroutineBodyStmt())),
                        has(typeLoc(forEach(ExpensiveValueParamDecl))),
                        decl().bind("functionDecl"))),
       this);
diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param-coroutine.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param-coroutine.cpp
new file mode 100644
index 0000000000000..f2fb477143578
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param-coroutine.cpp
@@ -0,0 +1,81 @@
+// RUN: %check_clang_tidy -std=c++20-or-later %s performance-unnecessary-value-param %t -- -fix-errors
+
+namespace std {
+
+template <typename R, typename...> struct coroutine_traits {
+  using promise_type = typename R::promise_type;
+};
+
+template <typename Promise = void> struct coroutine_handle;
+
+template <> struct coroutine_handle<void> {
+  static coroutine_handle from_address(void *addr) noexcept {
+    coroutine_handle me;
+    me.ptr = addr;
+    return me;
+  }
+  void operator()() { resume(); }
+  void *address() const noexcept { return ptr; }
+  void resume() const {  }
+  void destroy() const { }
+  bool done() const { return true; }
+  coroutine_handle &operator=(decltype(nullptr)) {
+    ptr = nullptr;
+    return *this;
+  }
+  coroutine_handle(decltype(nullptr)) : ptr(nullptr) {}
+  coroutine_handle() : ptr(nullptr) {}
+  //  void reset() { ptr = nullptr; } // add to P0057?
+  explicit operator bool() const { return ptr; }
+
+protected:
+  void *ptr;
+};
+
+template <typename Promise> struct coroutine_handle : coroutine_handle<> {
+  using coroutine_handle<>::operator=;
+
+  static coroutine_handle from_address(void *addr) noexcept {
+    coroutine_handle me;
+    me.ptr = addr;
+    return me;
+  }
+
+  Promise &promise() const {
+    return *reinterpret_cast<Promise *>(
+        __builtin_coro_promise(ptr, alignof(Promise), false));
+  }
+  static coroutine_handle from_promise(Promise &promise) {
+    coroutine_handle p;
+    p.ptr = __builtin_coro_promise(&promise, alignof(Promise), true);
+    return p;
+  }
+};
+
+struct suspend_always {
+  bool await_ready() noexcept { return false; }
+  void await_suspend(std::coroutine_handle<>) noexcept {}
+  void await_resume() noexcept {}
+};
+
+} // namespace std
+
+struct ReturnObject {
+    struct promise_type {
+        ReturnObject get_return_object() { return {}; }
+        std::suspend_always initial_suspend() { return {}; }
+        std::suspend_always final_suspend() noexcept { return {}; }
+        void unhandled_exception() {}
+        std::suspend_always yield_value(int value) { return {}; }
+    };
+};
+
+struct A {
+  A(const A&);
+};
+
+ReturnObject evaluateModels(const A timeMachineId) {
+// No change for non-coroutine function expected because it is not safe.
+// CHECK-FIXES: ReturnObject evaluateModels(const A timeMachineId) {
+  co_return;
+}

[vbvictor]

Add entry in ReleaseNotes.rst and the check docs about this new behavior

[dmpolukhin]

@HerrCai0907 @carlosgalvezp @PiotrZSL friendly ping, please take a look!

[dmpolukhin]

@HerrCai0907 @carlosgalvezp @PiotrZSL one more friendly ping. Please take a look, the change is benign and trivial one liner + test. I think it will really improve this check and avoid long hours for developers to figure out dangling references.

[vbvictor]

I have a mixed feeling about whether we should always exclude all coroutines by default or not.
When a coroutine has only one co-operator and nothing else should be safe to use a reference parameter? Am I wrong?

So I think to be "on a safe side" I'd suggest a new option "IngoreCoroutines" that will be true by default. If A user 100% knows that all references will outlive the coroutine he could set it to false. In the option description we could describe why it is not good to disable it as per cppcore-guidlines.
Generally speaking, I think it's not very good to implement recommendations of cppcore-guidlines as a must in non-cppcore checks, they should be under a flag.

I'd be happy to accept your changes with this option, without - I'd want a second opinion from a maintainer.

[dmpolukhin]

I have a mixed feeling about whether we should always exclude all coroutines by default or not. When a coroutine has only one co-operator and nothing else should be safe to use a reference parameter? Am I wrong?

No, it may not be safe even in case of single co_return because of initial coroutine suspend point before entering function body. So if the parameter passed by reference is used inside coroutine it is not safe see more here, except for the cases when the call is under co_await i.e. it is synchronous call.

So I think to be "on a safe side" I'd suggest a new option "IngoreCoroutines" that will be true by default. If A user 100% knows that all references will outlive the coroutine he could set it to false. In the option description we could describe why it is not good to disable it as per cppcore-guidlines. Generally speaking, I think it's not very good to implement recommendations of cppcore-guidlines as a must in non-cppcore checks, they should be under a flag.

I'd be happy to accept your changes with this option, without - I'd want a second opinion from a maintainer.

Let's wait a bit more for maintainer, if no response, I'll add an option and this this case we will have a question what should be the default value for the option.

[vbvictor]

Thank you for helpful docs.
I think the default behavior of the option should be to ignore all coroutines - that will be to make ordinary users safe.

[dmpolukhin]

@vbvictor and @HerrCai0907 @carlosgalvezp @PiotrZSL please take another look.

[dmpolukhin]

@vbvictor please take another look, all comments resolved.

[vbvictor]

LGTM, with minor suggestions

[carlosgalvezp]

Nit: typically we use the form IgnoreX for these types of things. I would then suggest naming the option IgnoreCoroutines and set it to true as default, i.e. revert the logic.