Use perfect forwarding for functions that use `apply__` functions #3215

SteveBronder · 2025-06-30T21:11:19Z

Summary

This fixes #3208 by using perfect forwarding for all functions that use our underlying apply family of functions for calling functions on containers and containers and containers. The issue was that the Holder class, when used in the apply functors, did not have enough type information to know which arguments it should take ownership of.

Consider the following function, where all types are passed in via constant reference.

template <typename T1, typename T2, require_any_container_t<T1, T2>* = nullptr>
inline auto gamma_p(const T1& a, const T2& b) {
  return apply_scalar_binary(
      [](const auto& c, const auto& d) { return gamma_p(c, d); }, a, b);
}

Calling this function with an Eigen expression that has a temporary in it would not give apply_scalar_binary and the Holder inside of apply_scalar_binary enough information to know that the Holder class should own any of the input arguments. As an example we can look at a simplified version of the code used in poisson_lccdf.hpp.

auto log_Pi = log(gamma_p(n_val + 1.0, lambda_val)));
double log_sum = sum(log_Pi);

gamma_p uses apply_scalar_binary and log uses apply_scalar_unary. We need to make sure the inputs and results of the gamma_p function do not fall out of scope by the time we go through log and then assign to log_Pi. Before this PR it would be possible for the expression n_val + 1.0 to fall out of scope as well as the result of gamma_p to go out of scope from log after log_Pi is assigned.

To combat this we now use perfect forwarding for all of the functions that use our internal apply family of functors. This should allow the Holder used internally by the apply functors to know which types need to be owned by it to make sure things do not fall out of scope.

Tests

There is no new tests for this. Since it is an isue on gcc I do wonder how we should test this in our CI/CD?

Side Effects

I'd like to think of some test we can write so that, in the future, developers do not accidentally write functions that use the apply family of functors that do not use perfect forwarding.

Release notes

Adds perfect forwarding to all functions that use the apply family of functors.

Checklist

Copyright holder: Steve Bronder

The copyright holder is typically you or your assignee, such as a university or company. By submitting this pull request, the copyright holder is agreeing to the license the submitted work under the following licenses:
- Code: BSD 3-clause (https://opensource.org/licenses/BSD-3-Clause)
- Documentation: CC-BY 4.0 (https://creativecommons.org/licenses/by/4.0/)
the basic tests are passing
- unit tests pass (to run, use: ./runTests.py test/unit)
- header checks pass, (make test-headers)
- dependencies checks pass, (make test-math-dependencies)
- docs build, (make doxygen)
- code passes the built in C++ standards checks (make cpplint)
the code is written in idiomatic C++ and changes are documented in the doxygen
the new changes are tested

andrjohns

Overall big fan of the changes! Just a couple of general queries before a full review

andrjohns · 2025-07-01T02:02:51Z

stan/math/fwd/fun/log_softmax.hpp

-          log_softmax_alpha(k).d_ += negative_alpha_m_d_times_softmax_alpha_t_m;
+inline auto log_softmax(T&& x) {
+  return apply_vector_unary<T>::apply(
+      std::forward<T>(x), [&](const auto& alpha) {


Suggested change

std::forward<T>(x), [&](const auto& alpha) {

std::forward<T>(x), [](auto&& alpha) {

The apply_vector_unary functors themselves should probably also perfect-forwarding, since they'll be passing their inputs to apply_* functions as well.

Also probably best to remove the reference-capture default while we're here

The apply_vector_unary functors themselves should probably also perfect-forwarding, since they'll be passing their inputs to apply_* functions as well.

Can you clarify? I'm not seeing in this function how much forwarding can be done. I do the perfect forwarding in the actual code for apply_vector_unary etc if that is was you mean.

Also probably best to remove the reference-capture default while we're here

Agree

andrjohns · 2025-07-01T02:06:51Z

stan/math/fwd/functor/apply_scalar_unary.hpp

+  static inline auto apply(const T2& x) {
+    return F::fun(x);
+  }


Suggested change

static inline auto apply(const T2& x) {

return F::fun(x);

}

static inline auto apply(T2&& x) {

return F::fun(std::forward<T2>(x));

}

Should this also be forwarding? As the downstream calls are forwarding their arguments to apply_scalar_unary<*>::apply()

andrjohns · 2025-07-01T02:09:08Z

stan/math/prim/constraint/cholesky_corr_constrain.hpp

-  return apply_vector_unary<T>::apply(
-      y, [K](auto&& v) { return cholesky_corr_constrain(v, K); });
+inline auto cholesky_corr_constrain(T&& y, int K) {
+  return apply_vector_unary<std::decay_t<T>>::apply(


Not something you necessarily need to change in this PR, but it might be a bit cleaner to move the std::decay_t<T> handling into apply_vector_unary itself

andrjohns · 2025-07-01T02:15:04Z

stan/math/prim/constraint/corr_constrain.hpp

@@ -43,7 +43,7 @@ inline plain_type_t<T> corr_constrain(const T& x) {
 * @param[in,out] lp log density accumulator
 */
 template <typename T_x, typename T_lp>
-inline auto corr_constrain(const T_x& x, T_lp& lp) {
+inline auto corr_constrain(T_x&& x, T_lp& lp) {
  plain_type_t<T_x> tanh_x = tanh(x);


Suggested change

plain_type_t<T_x> tanh_x = tanh(x);

plain_type_t<T_x> tanh_x = tanh(std::forward<T_x>(x));

andrjohns · 2025-07-01T02:19:24Z

stan/math/prim/constraint/lb_constrain.hpp

@@ -34,7 +34,7 @@ namespace math {
 */
 template <typename T, typename L, require_all_stan_scalar_t<T, L>* = nullptr,
          require_all_not_st_var<T, L>* = nullptr>
-inline auto lb_constrain(const T& x, const L& lb) {
+inline auto lb_constrain(T&& x, const L& lb) {


Looks like the the std::forward<T>(x) is missing from this function, and there are other container overloads in the file which probably need forwarding added as well

So for functions that just operate on scalars I don't think we need to worry about forwarding as much since those functions will immediately evaluate

andrjohns · 2025-07-01T02:21:51Z

stan/math/prim/constraint/offset_multiplier_free.hpp

          require_not_std_vector_t<M>* = nullptr>
-inline auto offset_multiplier_free(const std::vector<T>& x, const M& mu,
-                                   const std::vector<S>& sigma) {
+inline auto offset_multiplier_free(T&& x, const M& mu, S&& sigma) {


I think mu also needs to be forwarded here since it's passed to to_ref below

andrjohns · 2025-07-01T02:23:02Z

stan/math/prim/constraint/offset_multiplier_free.hpp

+      divide(subtract(std::forward<T>(y), std::forward<M>(mu_ref)),
+             std::forward<S>(sigma_ref)));


Suggested change

divide(subtract(std::forward<T>(y), std::forward<M>(mu_ref)),

std::forward<S>(sigma_ref)));

divide(subtract(std::forward<T>(y), std::forward<decltype(mu_ref)>(mu_ref)),

std::forward<decltype(sigma_ref)>(sigma_ref)));

andrjohns · 2025-07-01T02:23:45Z

stan/math/prim/constraint/offset_multiplier_free.hpp

+  auto&& mu_ref = to_ref(std::forward<M>(mu));
+  auto&& sigma_ref = to_ref(std::forward<S>(sigma));


Should these also be forwarded in the offset_multiplier_free call below?

andrjohns · 2025-07-01T02:25:17Z

stan/math/prim/constraint/prob_constrain.hpp

-  lp += log_inv_logit_x + log1m_inv_logit(x);
-  return exp(log_inv_logit_x);
+inline auto prob_constrain(T&& x, return_type_t<T>& lp) {
+  plain_type_t<T> log_inv_logit_x = log_inv_logit(x);


Suggested change

plain_type_t<T> log_inv_logit_x = log_inv_logit(x);

plain_type_t<T> log_inv_logit_x = log_inv_logit(std::forward<T>(x));

WardBrian · 2025-07-01T14:41:04Z

Re #3208, this branch fixes the issue in test/prob/poisson/poisson_ccdf_log_00000_generated_v_test, but I'm still getting ASAN failures in test/prob/loglogistic/loglogistic_cdf_00001_generated_ffv_test on line 116 of loglogistic_cdf.hpp

math/stan/math/prim/prob/loglogistic_cdf.hpp

Lines 115 to 116 in f7ccc01

    
               = -multiply_log(alpha_div_y_pow_beta, alpha_div_y) * prod_all_sq; 
        
           partials<2>(ops_partials) = beta_deriv * cdf_div_elt;

Could be related to #3147?

…arried through them

…d-apply

WardBrian · 2025-07-02T17:36:00Z

I do think we should weigh the odds that merging something of this scope during a release window would introduce more issues than it resolves (especially because the issues seem to have been present in the code for a couple years)

SteveBronder · 2025-07-02T18:23:22Z

I agree this is a large PR. Once I found the bug in one place I realized it was kind of systematic to all of our functions that can take in an expression and then output an expression. Let's talk irl. I agree this is risky before a release, but also think the bug is kind of large and can also happen in user space

WardBrian · 2025-07-02T18:41:01Z

Not denying it can happen to users, but as far as we know it hasn’t in the couple years it has been present. So, would it be that bad to hold the fix a couple months for the next release

Anyway, especially if @andrjohns thinks he can review it, I’m not opposed to merging, just wanted to ask in the name of due diligence

…::vector

SteveBronder · 2025-07-02T21:11:24Z

Yes I agree that it feels risky. But this is a very odd bug and imo I don't feel great about having this in a release when we know it is there.

SteveBronder and others added 2 commits June 13, 2025 16:33

perfect forward container math overloads

4eb5681

use perfect forwarding for functions that use apply

81bd2d0

SteveBronder marked this pull request as ready for review June 30, 2025 21:11

yashikno and others added 7 commits June 30, 2025 17:12

Merge commit 'bdd5b3ed4e54666590f5eb6fb6e822f1099da01b' into HEAD

561f772

[Jenkins] auto-formatting by clang-format version 10.0.0-4ubuntu1

e40bd1e

cleanup apply_scalar_unary

3c9462e

[Jenkins] auto-formatting by clang-format version 10.0.0-4ubuntu1

5dd0f62

update sum_to_zero

589895d

Merge commit '09542d0beef76139c8d3df580535cae80cfb0191' into HEAD

6301f3e

[Jenkins] auto-formatting by clang-format version 10.0.0-4ubuntu1

deec65a

andrjohns requested changes Jul 1, 2025

View reviewed changes

SteveBronder added 2 commits July 1, 2025 00:17

fix mu and sigma ref

8a824c9

update offset_multiplier_free requires

5fffe24

WardBrian mentioned this pull request Jul 1, 2025

Release CmdStan 2.37 stan-dev/cmdstan#1324

Open

26 tasks

SteveBronder and others added 7 commits July 1, 2025 15:36

update functions with holder

80dbe6c

make all recursive vector functions pf so that value information is c…

50052d9

…arried through them

Merge remote-tracking branch 'origin/develop' into fix/perfect-forwar…

939ed72

…d-apply

update

e63c866

[Jenkins] auto-formatting by clang-format version 10.0.0-4ubuntu1

67a15d5

fix functions that return expressions

ebb2dbd

[Jenkins] auto-formatting by clang-format version 10.0.0-4ubuntu1

92e4076

fix symmetrize

3e921d4

SteveBronder and others added 4 commits July 2, 2025 14:48

update logic for offset_multiply to direct to correct version for std…

43a2e0b

…::vector

[Jenkins] auto-formatting by clang-format version 10.0.0-4ubuntu1

2b9d261

fix categorical_logit test to use plain type for rep_matrix

1144241

[Jenkins] auto-formatting by clang-format version 10.0.0-4ubuntu1

3c42f90

SteveBronder and others added 2 commits July 2, 2025 17:09

add back tests for rep_matrix

0f1658d

[Jenkins] auto-formatting by clang-format version 10.0.0-4ubuntu1

7958b81

SteveBronder and others added 4 commits July 2, 2025 17:25

fix reverse

77646e6

Update reverse.hpp

8dc16ca

update block like functions for expressions

2b52498

[Jenkins] auto-formatting by clang-format version 10.0.0-4ubuntu1

b033319

	std::forward<T>(x), [&](const auto& alpha) {
	std::forward<T>(x), [](auto&& alpha) {

	plain_type_t<T_x> tanh_x = tanh(x);
	plain_type_t<T_x> tanh_x = tanh(std::forward<T_x>(x));

		divide(subtract(std::forward<T>(y), std::forward<M>(mu_ref)),
		std::forward<S>(sigma_ref)));

		auto&& mu_ref = to_ref(std::forward<M>(mu));
		auto&& sigma_ref = to_ref(std::forward<S>(sigma));

	plain_type_t<T> log_inv_logit_x = log_inv_logit(x);
	plain_type_t<T> log_inv_logit_x = log_inv_logit(std::forward<T>(x));

Uh oh!

Use perfect forwarding for functions that use apply_*_* functions #3215

Are you sure you want to change the base?

Use perfect forwarding for functions that use apply_*_* functions #3215

Uh oh!

Conversation

SteveBronder commented Jun 30, 2025

Summary

Tests

Side Effects

Release notes

Checklist

Uh oh!

andrjohns left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WardBrian commented Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WardBrian commented Jul 2, 2025

Uh oh!

SteveBronder commented Jul 2, 2025

Uh oh!

WardBrian commented Jul 2, 2025

Uh oh!

SteveBronder commented Jul 2, 2025

Uh oh!

Uh oh!

Use perfect forwarding for functions that use `apply__` functions #3215

Use perfect forwarding for functions that use `apply__` functions #3215

WardBrian commented Jul 1, 2025 •

edited

Loading