Fix issue with function shadowing

[WardBrian]

Closes #997.

This PR moves to qualifying all non-user functions with either stan::math or stan::model as appropriate. This means that the C++ will compile even if the user defines a variable of the same name.

Submission Checklist

• Run unit tests
• Documentation
  • OR, no user-facing changes were made

Release notes

Fixed an issue that arose during C++ compilation of models that used a variable with the same name as a Stan library function and called that function.

Copyright and Licensing

By submitting this pull request, the copyright holder is agreeing to
license the submitted work under the BSD 3-clause license (https://opensource.org/licenses/BSD-3-Clause)

[WardBrian]

@SteveBronder this seems mostly good except for a few functions which aren't fully in stan-math: pow seems like the main one we use. I tried this (based on ceil above it):

stanc3/src/stan_math_backend/Expression_gen.ml

Lines 294 to 308 in 4a87f0f

	| "pow" ->
	let std_prefix_data_scalar f = function
	| [ Expr.({ Fixed.meta=
	Typed.Meta.({adlevel= DataOnly; type_= UInt | UReal; _}); _
	})
	; Expr.({ Fixed.meta=
	Typed.Meta.({adlevel= DataOnly; type_= UInt | UReal; _}); _
	}) ] ->
	"std::" ^ f
	| _ -> "stan::math::" ^ f
	in
	Some
	(fun ppf es ->
	let f = std_prefix_data_scalar f es in
	pp_call ppf (f, pp_expr, es) )

But it still doesn't catch all cases

Any thoughts?

[WardBrian]

Seems like atan2 and fmod also have the same issue as pow. Could we just add dummy signatures for these to stan math that call out to std::? Otherwise we need to exactly recreate the resolution logic within the codegen

[rok-cesnovar]

Could we just add dummy signatures for these to stan math that call out to std::?

I think this is the way to go + a model in the tests somewhere with all the calls to these functions? If you can make an issue in Math with a list of the missing ones I can handle it tomorrow I think.

[WardBrian]

Thanks @rok-cesnovar, see here: stan-dev/math#2611

We currently have models in test/integration/function-signatures/math/functions which should test these. They're what is failing in the above tests for the most part

[rok-cesnovar]

Oh yeah, the tests are there. Should be ready by the time you start work tomorrow :)

[WardBrian]

I should note for posterity's sake that the alternative to this change (qualifying everything not in userspace with it's namespace) is to mangle everything from userspace, which could reuse the machinery from #962

I personally lean toward this solution as it leaves the C++ easier to read, IMO

[WardBrian]

I believe I have run down every compilation issue that isn't a result of the stan-math signatures @rok-cesnovar. When that PR gets merged this should be green, but we could also try to test before that to make sure

[rok-cesnovar]

Started a run with the Math PR branch: https://jenkins.mc-stan.org/blue/organizations/jenkins/stanc3/detail/PR-1011/39/pipeline/

[WardBrian]

@rok-cesnovar I don't understand the failures in that run. It claims that hearts.stan from the bugs example models generates code with a pow call that isn't qualified by stan::math but this simply isn't true

[WardBrian]

Okay, we define overloads of operators in stan::math, so my plan to completely remove the using namespace stan::math; directive won't work 100% (or we have to add using stan::math::operatorX; for 8 operators).

That doesn't seem like 100% of the issues but we will start there

[rok-cesnovar]

Oh man, that test isn't actually doing what we (or at least I) thought it should be doing. It was originally thought of as a comparison with stanc2. It's currently just comparing nightly with nightly. And its not trivial to force ./compare-compiler.sh to do what it should (compare nightly with PR stanc3 binary).

Do you mind if we remove the test and make an issue?

[WardBrian]

Sure, it seems like this PR has some other issues to sort out in the meantime anyway

[WardBrian]

@serban-nicusor-toptal - now getting a weird failure during verify changes https://jenkins.mc-stan.org/blue/organizations/jenkins/stanc3/detail/PR-1011/43/pipeline

[serban-nicusor-toptal]

Looks like a permission issue, checking.
Edit: Everything should be fine now.

[WardBrian]

@rok-cesnovar - still a problem with pow:

../../test/integration/good/code-gen/complex_scalar.hpp:1958:39: error: call to 'pow' is ambiguous
      stan::model::assign(gq_complex, stan::math::pow(z, y),
                                      ^~~~~~~~~~~~~~~
/mnt/nvme1n1/workspace/stanc3_PR-1011/performance-tests-cmdstan/cmdstan/stan/lib/stan_math/stan/math/rev/fun/pow.hpp:213:26: note: candidate function [with T = double, $1 = void]
inline std::complex<var> pow(const std::complex<var>& x,
                         ^
/mnt/nvme1n1/workspace/stanc3_PR-1011/performance-tests-cmdstan/cmdstan/stan/lib/stan_math/stan/math/rev/fun/pow.hpp:251:26: note: candidate function [with T = double, $1 = void]
inline std::complex<var> pow(std::complex<T> x, const std::complex<var>& y) {
                         ^
/mnt/nvme1n1/workspace/stanc3_PR-1011/performance-tests-cmdstan/cmdstan/stan/lib/stan_math/stan/math/rev/fun/pow.hpp:199:26: note: candidate function
inline std::complex<var> pow(const std::complex<var>& x,
                         ^
../../test/integration/good/code-gen/complex_scalar.hpp:1961:39: error: call to 'pow' is ambiguous
      stan::model::assign(gq_complex, stan::math::pow(z, gq_r),
                                      ^~~~~~~~~~~~~~~

[rok-cesnovar]

Hm, that would be complex pow. Ok, that i didnt actually account for in my PR.

[WardBrian]

@rok-cesnovar this still needs math changes right? #1067 isn't enough?

[rok-cesnovar]

Yeah, pow(complex) and friends are missing for primitive inputs. Sorry, was a busy week and havent gotten back to this.

[WardBrian]

No worries, just wanted to ask before trying to run the tests again!

[rok-cesnovar]

All models compiled 🎉 Finally.

[WardBrian]

@rok-cesnovar what's the necessary merge order between this and the stan-math PR?

[rok-cesnovar]

what's the necessary merge order between this and the stan-math PR?

stanc3 PR should be merged first and then the Math one should be merged as soon as possible. No other Math/Stan/Cmdstan PR will pass between merging the stanc3 PR and the Math PR.

The Math PR is ready, it won't pass the upstream tests until we merge this PR. The next step would be to get both PRs approved then I can handle the merging procedure.

[WardBrian]

@SteveBronder would you be able to review this? It's not a super complicated change on this end

[rok-cesnovar]

If Steve is not available or busy, happy to review this one. This is in my Ocaml/stanc3 comfort zone :)

[rok-cesnovar]

Looks good to me. One minor comment on the special casing operators.

[rok-cesnovar]

All good. Now we need the Math PR approved and we should be good to go.