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.

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

merged 1 commit into from
Nov 19, 2021

Conversation

WardBrian
Copy link
Member

Related to #953 and essentially the start of redoing stan-dev/stan#2712

Submission Checklist

Release notes

Warn about the following identifiers being reserved in a future version:
array, upper, lower, offset, multiplier.

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)

src/frontend/Ast.ml Show resolved Hide resolved
Comment on lines +25 to +28
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.
*)
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

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.

Copy link
Collaborator

@nhuurre nhuurre left a comment

Choose a reason for hiding this comment

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

Alright, makes sense.

@WardBrian WardBrian merged commit 146fee2 into master Nov 19, 2021
@WardBrian WardBrian deleted the future-keywords branch November 19, 2021 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants