Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Warn about future reserved words #1048

Merged
merged 1 commit into from
Nov 19, 2021
Merged

Warn about future reserved words #1048

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/frontend/Ast.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,7 +296,9 @@ let rec id_of_lvalue {lval; _} =
token before the current statement and all the whitespace between two statements
appears as if it were part of the second statement.
get_first_loc tries to skip the leading whitespace and approximate the location
of the first token in the statement. *)
of the first token in the statement.
TODO: See if $sloc works better than $loc for this
*)
WardBrian marked this conversation as resolved.
Show resolved Hide resolved

let rec get_loc_expr (e : untyped_expression) =
match e.expr with
Expand Down
16 changes: 16 additions & 0 deletions src/frontend/Input_warnings.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,3 +19,19 @@ let deprecated token (pos, message) =
Middle.Location_span.of_positions_opt begin_pos end_pos
|> Option.value ~default:Middle.Location_span.empty in
add_warning span message

let drop_array_future () =
match !warnings with
| ( _
, "Variable name 'array' will be a reserved word starting in Stan 2.32.0. \
Please rename it!" )
:: tl ->
warnings := tl
| _ -> ()

let future_keyword kwrd version (pos1, pos2) =
add_warning
(Option.value ~default:Middle.Location_span.empty
(Middle.Location_span.of_positions_opt pos1 pos2) )
( "Variable name '" ^ kwrd ^ "' will be a reserved word starting in Stan "
^ version ^ ". Please rename it!" )
9 changes: 9 additions & 0 deletions src/frontend/Input_warnings.mli
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,3 +17,12 @@ val deprecated : string -> Lexing.position * string -> unit

val empty : string -> unit
(** Register that an empty file is being lexxed *)

val future_keyword :
string -> string -> Lexing.position * Lexing.position -> unit
(** Warn on a keyword which will be reserved in the future*)

val drop_array_future : unit -> unit
(** Hack: Remove the most recent warning about array as a future keyword.
Needed due to the {e other} hack of how we currently parse arrays.
*)
Comment on lines +25 to +28
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oof. Wouldn't it have been easier to detect the keyword identifiers in Deprecation_analysis.ml?

Copy link
Member Author

@WardBrian WardBrian Nov 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could. I like it in the parser for a variety of reasons - it really highlights the structure of the change (I think we should also move all the keywords we parse as identifiers into a separate nonterminal for the same organizational reason, and just not emit the warning for those). This will capture every identifier automatically, but in deprecation analysis we'd need to do it separately for Variables, FunDecls, FunApps, ConDistApps, ... etc. I think it's where it should live naturally, as it is something that is true at parsing time and doesn't need any type information or other structure (same reason get_lp and increment_log_prog are in the lexer).

It just really, really highlights how bad the hack for parsing array[] notation is at the moment. We're basically trying to parse a variable which is indexed and then asserting after the fact that the variable's "name" is array and treating it's index notation as a dimension declaration. The fact that we need to unwind one of the warnings we generated as a result is hardly the worst thing about it.

48 changes: 21 additions & 27 deletions src/frontend/parser.mly
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,11 @@ let rec iterate_n f x = function
| n -> iterate_n f (f x) (n - 1)
let nest_unsized_array basic_type n =
iterate_n (fun t -> UnsizedType.UArray t) basic_type n

(* $sloc and $symbolstartpos generates code using !=, which
Core_kernel considers to be an error.
*)
let (!=) = Stdlib.(!=)
%}

%token FUNCTIONBLOCK DATABLOCK TRANSFORMEDDATABLOCK PARAMETERSBLOCK
Expand Down Expand Up @@ -93,7 +98,7 @@ program:
let () =
match (ofb, odb, otdb, opb, otpb, omb, ogb) with
| None, None, None, None, None, None, None ->
Input_warnings.empty (fst $loc).pos_fname
Input_warnings.empty ($startpos).pos_fname
| _ -> ()
in
{ functionblock= ofb
Expand Down Expand Up @@ -160,11 +165,19 @@ generated_quantities_block:
identifier:
| id=IDENTIFIER { build_id id $loc }
| TRUNCATE { build_id "T" $loc}
| OFFSET { build_id "offset" $loc}
| MULTIPLIER { build_id "multiplier" $loc}
| LOWER { build_id "lower" $loc}
| UPPER { build_id "upper" $loc}
| ARRAY { build_id "array" $loc}
| id_and_v = future_keyword
{
let id, v = id_and_v in
Input_warnings.future_keyword id.name v $loc;
id
}

future_keyword:
| OFFSET { build_id "offset" $loc, "2.32.0" }
| MULTIPLIER { build_id "multiplier" $loc, "2.32.0" }
| LOWER { build_id "lower" $loc, "2.32.0" }
| UPPER { build_id "upper" $loc, "2.32.0" }
| ARRAY { build_id "array" $loc, "2.32.0" }

decl_identifier:
| id=identifier { id }
Expand Down Expand Up @@ -342,6 +355,7 @@ decl(type_rule, rhs):
in
let dims = match dims_opt with
| Some ({expr= Indexed ({expr= Variable {name="array"; _}; _}, ixs); _}) ->
Input_warnings.drop_array_future () ;
(match int_ixs ixs with
| Some sizes -> sizes
| None -> error "Dimensions should be expressions, not multiple or range indexing.")
Expand All @@ -358,27 +372,7 @@ decl(type_rule, rhs):
; is_global
}
; smeta= {
loc=
(* From the docs:
We remark that, if the current production has an empty right-hand side,
then $startpos and $endpos are equal, and (by convention) are the end
position of the most recently parsed symbol (that is, the symbol that
happens to be on top of the automaton’s stack when this production is
reduced). If the current production has a nonempty right-hand side,
then $startpos is the same as $startpos($1) and $endpos is the same
as $endpos($n), where n is the length of the right-hand side.


So when dims_opt is empty, it uses the preview token as its startpos,
but that makes the whole declaration think it starts at the previous
token. Sadly, $sloc and $symbolstartpos generates code using !=, which
Core_kernel considers to be an error.
*)
let startpos = match dims_opt with
| None -> $startpos(ty)
| Some _ -> $startpos
in
Location_span.of_positions_exn (startpos, $endpos)
loc= Location_span.of_positions_exn $sloc
}
})
)}
Expand Down
4 changes: 4 additions & 0 deletions test/integration/bad/lang/stanc.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -630,6 +630,8 @@ Syntax error in 'incomplete.stan', line 2, column 18 to column 19, parsing error

Found an incomplete binary expression - are you missing the right hand side?
$ ../../../../../install/default/bin/stanc incomplete2.stan
Warning in 'incomplete2.stan', line 2, column 9: Variable name 'upper' will
be a reserved word starting in Stan 2.32.0. Please rename it!
Syntax error in 'incomplete2.stan', line 2, column 17 to column 22, parsing error:
-------------------------------------------------
1: transformed data {
Expand Down Expand Up @@ -679,6 +681,8 @@ Syntax error in 'incomplete5.stan', line 3, column 0 to column 0, parsing error:

Ill-formed block. Expected a statement, variable declaration, or just "}".
$ ../../../../../install/default/bin/stanc incomplete6.stan
Warning in 'incomplete6.stan', line 2, column 8: Variable name 'upper' will
be a reserved word starting in Stan 2.32.0. Please rename it!
Syntax error in 'incomplete6.stan', line 3, column 16 to column 21, parsing error:
-------------------------------------------------
1: transformed data {
Expand Down
6 changes: 6 additions & 0 deletions test/integration/bad/new/stanc.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1198,6 +1198,9 @@ Syntax error in 'ill-formed-statement23.stan', line 1, column 35 to column 39, p

Ill-formed statement. Expected statement after "else".
$ ../../../../../install/default/bin/stanc ill-formed-statement24.stan
Warning in 'ill-formed-statement24.stan', line 1, column 30: Variable name
'upper' will be a reserved word starting in Stan 2.32.0. Please rename
it!
Syntax error in 'ill-formed-statement24.stan', line 1, column 35 to column 36, parsing error:
-------------------------------------------------
1: transformed data { if ( 1 ) ; upper}
Expand All @@ -1214,6 +1217,9 @@ Syntax error in 'ill-formed-statement25.stan', line 1, column 34 to column 38, p

Ill-formed statement. Expected statement after else.
$ ../../../../../install/default/bin/stanc ill-formed-statement26.stan
Warning in 'ill-formed-statement26.stan', line 1, column 29: Variable name
'upper' will be a reserved word starting in Stan 2.32.0. Please rename
it!
Syntax error in 'ill-formed-statement26.stan', line 1, column 34 to column 34, parsing error:
-------------------------------------------------
1: transformed data { if ( T) ; upper
Expand Down
22 changes: 22 additions & 0 deletions test/integration/good/pretty.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3023,6 +3023,11 @@ Warning in 'deprecated_syntax.stan', line 40, column 2: increment_log_prob(...);
Warning in 'deprecated_syntax.stan', line 42, column 11: get_lp() function is
deprecated. It will be removed in Stan 2.32.0. Use target() instead. This
can be done automatically with stanc --print-canonical
Warning in 'deprecated_syntax.stan', line 46, column 6: Variable name
'offset' will be a reserved word starting in Stan 2.32.0. Please rename
it!
Warning in 'deprecated_syntax.stan', line 47, column 6: Variable name 'array'
will be a reserved word starting in Stan 2.32.0. Please rename it!
Warning in 'deprecated_syntax.stan', line 51, column 2: Comments beginning
with # are deprecated and this syntax will be removed in Stan 2.32.0. Use
// to begin line comments; this can be done automatically using stanc
Expand Down Expand Up @@ -3942,6 +3947,14 @@ data {
int offset;
}

Warning in 'identifiers.stan', line 2, column 6: Variable name 'upper' will
be a reserved word starting in Stan 2.32.0. Please rename it!
Warning in 'identifiers.stan', line 3, column 6: Variable name 'lower' will
be a reserved word starting in Stan 2.32.0. Please rename it!
Warning in 'identifiers.stan', line 4, column 6: Variable name 'multiplier'
will be a reserved word starting in Stan 2.32.0. Please rename it!
Warning in 'identifiers.stan', line 5, column 6: Variable name 'offset' will
be a reserved word starting in Stan 2.32.0. Please rename it!
$ ../../../../install/default/bin/stanc --auto-format if-else-formatting.stan
generated quantities {
// make sure pp_recursive_ifthenelse properly places else
Expand Down Expand Up @@ -6289,6 +6302,15 @@ model {
array[1, 2, 3] real abc;
}

Warning in 'unreserved-array-keyword.stan', line 2, column 24: Variable name
'array' will be a reserved word starting in Stan 2.32.0. Please rename
it!
Warning in 'unreserved-array-keyword.stan', line 2, column 40: Variable name
'array' will be a reserved word starting in Stan 2.32.0. Please rename
it!
Warning in 'unreserved-array-keyword.stan', line 6, column 7: Variable name
'array' will be a reserved word starting in Stan 2.32.0. Please rename
it!
$ ../../../../install/default/bin/stanc --auto-format user-defined-lpdf-fun.stan
functions {
real bar_lpmf(int y, real z) {
Expand Down