stan-dev / stanc3

The Stan transpiler (from Stan to C++ and beyond).
BSD 3-Clause "New" or "Revised" License
140 stars 44 forks source link

[BUG] `--standalone-functions` ignores undefined functions (external c++) #1297

Closed andrjohns closed 1 year ago

andrjohns commented 1 year ago

The --standalone-functions flag doesn't produce the same function code and // [[stan::function]] decorator if an external function is declared with the --allow-undefined flag.

For example, the following model:

functions {
  real retreal(real x) {
    return x;
  }
}

Parses to:

...

// [[stan::function]]
auto retreal(const double& x, std::ostream* pstream__ = nullptr)  
{
 return model_model_namespace::retreal(x, pstream__);
}

Whereas:

functions {
  real retreal(real x);
}

Parses to:

// Code generated by stanc v2.31.0
#include <stan/model/model_header.hpp>
namespace model_external_model_namespace {

using stan::model::model_base_crtp;
using namespace stan::math;

stan::math::profile_map profiles__;
static constexpr std::array<const char*, 1> locations_array__ = 
{" (found before start of program)"};

template <typename T0__,
          stan::require_all_t<stan::is_stan_scalar<T0__>>* = nullptr>
  stan::promote_args_t<T0__>
  retreal(const T0__& x, std::ostream* pstream__) ; 

}
;
andrjohns commented 1 year ago

I think that the only thing needed is the // [[stan::function]] decorator above the placeholder/generic definition, no other processing or definitions should be required

WardBrian commented 1 year ago

I think that the only thing needed is the // [[stan::function]] decorator above the placeholder/generic definition, no other processing or definitions should be required

The current standalone-functions functionality always exposes functions, not templates, and always puts them outside the model namespace, so adding a comment there would make this very different behavior.

The easiest "fix" for this is just making this match statement always equivalent to the current "Some" case: https://github.com/stan-dev/stanc3/blob/a490a0922b8d79702054a449eedba108ffbbeedb/src/stan_math_backend/Lower_functions.ml#L319-L321

That will introduce a new issue where

functions {
  real retreal(real x);
  real retreal(real x) {
    return x;
  }
}

would now output duplicate definitions, but that should be fixable. I think this oversight is simply because we don't want to generate two signatures if it was a "normal" (i.e. not for external C++) forward decl, and right now it is annoying to tell these two cases apart (one motivation for ideas in #1278) .

andrjohns commented 1 year ago

The current standalone-functions functionality always exposes functions, not templates, and always puts them outside the model namespace, so adding a comment there would make this very different behavior.

Ah gotcha. I've got no strong preference on a particular approach. It's for making external c++ compatible with expose_functions() in cmdstanr, so I'm happy with whatever works!

WardBrian commented 1 year ago

I think I have a reasonable fix for this which also fixes another known issue with external C++ (we don’t generate the necessary functors to call them in Higher order functions)