Add a special-case error message for old array syntax

[WardBrian]

Closes #1412. Surprisingly, this doesn't introduce any conflicts in the parser. If it ever does, we could remove it again, but for now it seems like a good quality of life feature.

One downside is printing a nice error message introduced a circular dependency between pretty printing and parsing. I resolved this by splitting the pretty printing module in two, but I'm not super happy with that.

Submission Checklist

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

Release notes

Added a better error message when a program attempts to use the removed array syntax.

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]

Examples of the new message:

Syntax error in 'declarations.stan', line 3, column 18 to column 18, parsing error:
   -------------------------------------------------
     1:  data {
     2:    int a0;
     3:    int<lower=0> a1[1];
                           ^
     4:    int a2[2,3];
     5:    array[1] int a3;
   -------------------------------------------------

";" expected after variable declaration.
It looks like you are trying to use the old array syntax.
Please use the new syntax:
array[...] int<lower=0> a1;

Syntax error in 'unsized-old-array.stan', line 2, column 18 to column 18, parsing error:
   -------------------------------------------------
     1:  functions{
     2:    real foo(  real[,] v){
                           ^
     3:      return sum(v);
     4:    }
   -------------------------------------------------

An identifier is expected after the type as a function argument name.
It looks like you are trying to use the old array syntax.
Please use the new syntax: 
array[...] real

(how does this look @andrjohns?)

[codecov]

Codecov Report

Attention: Patch coverage is 83.33333% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 89.87%. Comparing base (e162817) to head (02a1c64).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1413   +/-   ##
=======================================
  Coverage   89.87%   89.87%           
=======================================
  Files          63       64    +1     
  Lines       10529    10529           
=======================================
  Hits         9463     9463           
  Misses       1066     1066           

Files	Coverage Δ
src/frontend/Pretty_printing.ml	93.18% <ø> (+1.15%)	⬆️
src/stanc/stanc.ml	82.14% <100.00%> (ø)
src/frontend/Pretty_print_prog.ml	82.85% <82.85%> (ø)

[nhuurre]

I don't think this needs the generic "expected semicolon" part since the specific error is identified quite precisely.
Also, it wouldn't be too difficult to print the exact array dimensions instead of [...]:

Suggested change

	| ty=type_rule id=decl_identifier LBRACK {
	raise
	(Errors.SyntaxError
	(Errors.Parsing
	( Fmt.str
	"\";\" expected after variable declaration.@ It looks like you \
	are trying to use the old array syntax.@ Please use the new \
	syntax:@ @[<h>array[...] %a %s;@]@\n"
	| ty=type_rule id=decl_identifier LBRACK dims=separated_nonempty_list(COMMA, expression) RBRACK {
	let (ty, trans) = ty in
	let ty = List.fold_right ~f:(fun e ty -> SizedType.SArray (ty, e)) ~init:ty dims in
	let ty = (ty, trans) in
	raise
	(Errors.SyntaxError
	(Errors.Parsing
	( Fmt.str
	"Deprecated array syntax.@ Please use the new \
	syntax:@ @[<h>%a %s;@]@\n"

And actually, since this doesn't cause parsing ambiguities, maybe the canonicalizer could just accept the old array syntax.

[WardBrian]

I don't think this needs the generic "expected semicolon" part since the specific error is identified quite precisely

I left it in as a guard for there being a genuine mistake where the user didn't want an array at all but had a stray character which happened to be a [. It seemed odd that any other special character there would mention a ;, but this wouldn't.

maybe the canonicalizer could just accept the old array syntax.

It would need to be a bit more complicated; if you wanted to really allow that you'd also need to support the optional assignment (essentially go back to the parsing rule pre-#1388). I don't think this would cause any conflicts at the moment either.

I don't want to do that, mainly because as long as the canonicalizer would support it, some interfaces would just update their pipeline to canonicalize -> stanc -> c++ compile and therefore never actually update the Stan code. That would "work" unless/until we actually wanted to do something syntactically which was incompatible, at which point we're either back to where we are now, or our hands remain tied and the deprecation policy was not able to achieve the main goal of allowing language evolution.

[nhuurre]

a genuine mistake where the user didn't want an array at all but had a stray character

That seems unlikely, and I think that as long as the caret points to the errant bracket, the user can spot the mistake easily even if the error message is nonsense.
Also, this problem disappears if you go with my suggestion of parsing the whole dimension expression.

some interfaces would just update their pipeline to canonicalize -> stanc -> c++ compile and therefore never actually update the Stan code.

Is this a hypothetical, or do you know some interface that opposes our deprecation policy?

[WardBrian]

I know BRMS was using that set up until 2.34 forced them not to. I do not know of any others concretely, but I would be entirely unsurprised if some other downstream users of RStan have done this

I made the change to give a complete suggestion. It introduced a few spurious error cases in the messages file for situations where you have int foo[ followed by an invalid expression - I gave them all a generic message in that case.

[nhuurre]

note: these look off because a tab counts as one space when positioning the caret. Presumably this has always been an issue. Tabs are rarely used so maybe not worth fixing.

[WardBrian]

I was wondering about that but I'm not sure how one even would fix it, so I guess it is fine