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

Remove local rewrite flag and enable it by default #44

Merged
merged 13 commits into from
Nov 28, 2023

Conversation

alanechang
Copy link

@alanechang alanechang commented Nov 7, 2023

This PR removes the --rewrite-old-style-jane-street-local-annotations flag and enables rewriting with the new syntax by default.

A few consequences of this change are:

  • --erase-jane-syntax now erases the old style local annotation too. See local-erased.ml.ref for differences
  • In order for the AST equality checks to pass when we are modifying the tree deliberately, a new ignore_local_annot_differences option is added to ignore changes around local annotations in the standard AST. This option is set automatically when the legacy syntax is encountered. The alternative here is to convert old local syntax to the new or vice versa during normalization, which is non-trivial and will involve duplicating logic from jane syntax. The standard AST normalization is changed to convert old local syntax into the new syntax in order for the equality check to pass.

@alanechang alanechang force-pushed the aec/remove-local-rewrite-flag branch 2 times, most recently from df7ba4d to 0184a59 Compare November 7, 2023 19:16
lib/Conf.ml Outdated Show resolved Hide resolved
@alanechang
Copy link
Author

Removed the logic for erasing legacy local annotation syntax in Fmt_ast.ml and added a note about it in the test:

(* Note that this file cannot contain legacy local annotation syntax. If a
file with legacy local annotations wants to have its syntax erased, call
ocamlformat once without the --erase-jane-syntax flag to rewrite it into
the new syntax and then call ocamlformat a second time to erase the
syntax. *)

lib/Normalize_std_ast.ml Show resolved Hide resolved
lib/Normalize_std_ast.ml Show resolved Hide resolved
lib/Normalize_std_ast.ml Outdated Show resolved Hide resolved
lib/Normalize_std_ast.ml Outdated Show resolved Hide resolved
Signed-off-by: alanechang <alanechang@janestreet.com>
Signed-off-by: alanechang <alanechang@janestreet.com>
Signed-off-by: alanechang <alanechang@janestreet.com>
Signed-off-by: alanechang <alanechang@janestreet.com>
Signed-off-by: alanechang <alanechang@janestreet.com>
Signed-off-by: alanechang <alanechang@janestreet.com>
Signed-off-by: alanechang <alanechang@janestreet.com>
Signed-off-by: alanechang <alanechang@janestreet.com>
Signed-off-by: alanechang <alanechang@janestreet.com>
Signed-off-by: alanechang <alanechang@janestreet.com>
Signed-off-by: alanechang <alanechang@janestreet.com>
Signed-off-by: alanechang <alanechang@janestreet.com>
Signed-off-by: alanechang <alanechang@janestreet.com>
@alanechang
Copy link
Author

@goldfirere Added one more commit to handle the pattern case differently. This fixes an issue caught by the CI tests.

PR should be ready to merge if that commit looks good.

@goldfirere
Copy link

Took a look at that last commit. Looks fine. Let's merge.

@goldfirere goldfirere merged commit c03bb34 into janestreet:jane Nov 28, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants