Deprecate forward decls

[nhuurre]

Submission Checklist

• Run unit tests
• Documentation
  • If a user-facing facing change was made, the documentation PR is here: changes to forward declaration docs#612
  • OR, no user-facing changes were made

Closes #976

The backend simply generates a forward declaration for every function (even nonrecursive ones) which also simplifies functor struct generation a bit.
Also needed to do something smarter for potentially cyclic eigen templates (#1228 heuristic has been broken for a while now because simple recursive functions don't actually need forward decls)

And also I've allowed variables to shadow user defined functions and not just stan math functions. The reason is shown here:

functions {
  void foo() {
    xyz(); // function visible even without a forward declaration - yay
  }
  void bar() {
    real xyz; // name conflicts with the function `xyz` unless shadowing allowed - oh no
  }
  void xyz() {
  }
}

Release notes

Recursive user-defined functions no longer need forward declarations.

Copyright and Licensing

Copyright holder: Niko Huurre
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]

How would this interact with the external C++ functions feature?

I think a lot of this is good - generating forward decls for everything and removing any language from the documentation about the need for them is good. We can also make function declarations completely order-independent with this, if we’re willing to do 2 passes over the function block in typechecking, which I would like to see as part of a change like this.

That said, I think we probably want to pair this with something like an @external annotation (see stan-dev/design-docs#45) for their use with external C++, before any deprecation warning was emitted.

More specific review on the code to follow, those are just my thoughts based on the PR description

[nhuurre]

How would this interact with the external C++ functions feature?

Oh, yeah, I forgot to talk about that.
The deprecation here only applies to functions that have a definition. Undefined functions don't emit warnings.
I was thinking of requiring that external functions should be declared with

extern "C++" real foo(real);

in analogy with extern "C" you'd have in C++.

[WardBrian]

extern "C++" real foo(real); is an interesting suggestion. For what it's worth, I think "being similar to C++" isn't a strong reason for doing something in Stan, but historically that has been the case.

The thing I like about @extern is the potential to do something like this:

@extern("foo.hpp")
real foo(real);

This annotation does 3 things:

1. Disables the "declared but not defined" error for just this function
2. Disables all code-gen for this function (maybe functors could stay around, but IMO we don't want to generate anything since that means the user must write code which uses our templates if e.g. we generate a declaration)
3. Place the contents of foo.hpp where we would have generated any function code. This solves the "namespace issue" that currently makes external C++ a pain outside RStan.

I think an extern keyword could easily do 1 and 2, but I'm not sure how to do 3 with it.

[codecov]

Codecov Report

Merging #1277 (a9955a7) into master (975b213) will decrease coverage by 0.08%.
The diff coverage is 90.22%.

@@            Coverage Diff             @@
##           master    #1277      +/-   ##
==========================================
- Coverage   88.81%   88.73%   -0.09%     
==========================================
  Files          64       64              
  Lines        9668     9708      +40     
==========================================
+ Hits         8587     8614      +27     
- Misses       1081     1094      +13     

Impacted Files	Coverage Δ
src/middle/Mem_pattern.ml	66.66% <0.00%> (ø)
src/middle/UnsizedType.ml	72.80% <33.33%> (-0.47%)	⬇️
src/analysis_and_optimization/Optimize.ml	91.18% <85.00%> (-0.16%)	⬇️
src/stan_math_backend/Lower_expr.ml	92.37% <87.50%> (-0.25%)	⬇️
src/stan_math_backend/Transform_Mir.ml	95.05% <87.80%> (-1.57%)	⬇️
src/frontend/Typechecker.ml	89.65% <91.66%> (-0.03%)	⬇️
src/frontend/Deprecation_analysis.ml	91.75% <94.73%> (+0.39%)	⬆️
src/stan_math_backend/Lower_functions.ml	97.82% <94.82%> (-2.18%)	⬇️
src/frontend/Ast.ml	50.24% <100.00%> (+0.24%)	⬆️
src/frontend/Canonicalize.ml	96.62% <100.00%> (+0.19%)	⬆️
... and 4 more

[WardBrian]

Overall I think this is very nice. Most of my comments are stylistic or asking for more clarification

[WardBrian]

I think this wording should mention external C++ or just be very specific: "Foward declarations are not needed when the function is also defined and are deprecated in this case."

[WardBrian]

We also generally mention if the canonicalize flag can fix this, and if there is an expiration on the deprecation.

The latter requires a bit more discussion of intent. I don't think we need to settle what it looks like, but if the goal is that somewhere down the line there is a special syntax for external functions and the current forward decls are completely gone, that's worth discussing

[WardBrian]

I suspect that this + the unresolved comments above are probably the last substantial review this needs before approval.

Would you mind adding a test which shows function definitions being order-free (the snippet in the original PR text would suffice)? Besides that, I would also like for the PR to stan-dev/docs be written before merging this, since this requires some rather noticeable changes to the user-facing docs.

[WardBrian]

I'm going to hold off on a final review/approval until after the language meeting tomorrow in case anyone has some novel thoughts on extern functions which are worth considering before this change

[WardBrian]

During the language meeting we discussed the extern syntax. I'll be opening another issue here to discuss it more fully but long story short is it isn't a blocker for this. @bob-carpenter is very excited this change is finally being made

[WardBrian]

LGTM!