stan-dev / stanc3

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

Namespacing with external C++ and `--standalone-functions` causes issues #1424

Closed andrjohns closed 1 month ago

andrjohns commented 1 month ago

When calling --standalone-functions with undefined/external-c++, the generated functions expect the function definitions to be within the model namespace.

For example, with the make_odds example, the generated .hpp is:

// Code generated by stanc v2.35.0-rc1
#include <stan/model/model_header.hpp>
namespace external_model_namespace {
using stan::model::model_base_crtp;
using namespace stan::math;
stan::math::profile_map profiles__;
static constexpr std::array<const char*, 9> locations_array__ =
  {" (found before start of program)",
  " (in 'examples/external/external.stan', line 9, column 2 to column 31)",
  " (in 'examples/external/external.stan', line 16, column 2 to column 12)",
  " (in 'examples/external/external.stan', line 17, column 2 to column 26)",
  " (in 'examples/external/external.stan', line 12, column 2 to column 21)",
  " (in 'examples/external/external.stan', line 13, column 2 to column 23)",
  " (in 'examples/external/external.stan', line 5, column 2 to column 17)",
  " (in 'examples/external/external.stan', line 6, column 8 to column 9)",
  " (in 'examples/external/external.stan', line 6, column 2 to column 35)"};
}

// [[stan::function]]
double make_odds(const double& theta, std::ostream* pstream__ = nullptr) {
  return external_model_namespace::make_odds(theta, pstream__);
}

Compiling the generated hpp and including the user header fails, as the make_odds function wasn't declared in a namespace in the external user header.

This is a bit of an edge-case, so feel free to let me know if it would be more effort than it's worth to fix, and I can just workaround it in cmdstanr

WardBrian commented 1 month ago

Hm, this is an interesting edge case. It does seem like if you’re doing this, you’re doing something pretty weird, because you could just call your own header. If I was going to “fix” this I think they way I would want to do it is actually by just not emitting a definition at all in this case

andrjohns commented 1 month ago

An 'easy' fix could be to just emit the declaration for any undefined functions when standalone-functions is called. So the make_odds generation could just be:

// [[stan::function]]
double make_odds(const double& theta, std::ostream* pstream__);

Would that work?

WardBrian commented 1 month ago

That would break the compile of someone who needed their own forward decl, I think. For example, if make odds was recursive

It also would probably be an issue that the user header is included before the stanc generated code

andrjohns commented 1 month ago

Ah true. Yeah I'll just handle this internally in cmdstanr then