Add a mode for rewriting [@local], [%local], [@ocaml.global], etc. to their keyword forms (local_, global_, etc.)
#38
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
[@local], [%local], [@ocaml.global], etc. in to their keyword forms (local_, global_, etc.)[@local], [%local], [@ocaml.global], etc. to their keyword forms (local_, global_, etc.)
goldfirere
requested changes
Aug 18, 2023
There was a problem hiding this comment.
Looks good!
I would think we would want this new mode on always, which would considerably simplify this PR (because there would be no need to pass the conf around), because I can't think of a reason not to be doing this rewriting. Though I think @ceastlund gets the final say here.
Thanks for the quick turnaround here!
Requesting changes only to stop us from merging before we're ready with public-release.
| (** [exposed cls exp] holds if there is a right-most subexpression of [exp] | ||
| which satisfies [Exp.mem_cls cls] and is not parenthesized. *) | ||
| let rec exposed_right_exp = | ||
| (* exponential without memoization *) |
There was a problem hiding this comment.
Line 1920: What about curry? The compiler accepts [@curry] and [@ocaml.curry]. Also see Fmt_ast.ml:785. But I find no occurrences of @curry in the tree, so maybe this isn't necessary.
Author
There was a problem hiding this comment.
That’s right – no uses of it, and we don’t advertise it in our user-facing documentation. Also, it doesn’t have a keyword form. I think it’s best to keep the AST-changing rewriting minimal, so I stuck with the things we advertise and that have keywords; we could even probably get away with omitting %exclave, as it’s new enough that it’s not used yet.
c3a3ea0 to
8ee2852
Compare
This is available under the option `--rewrite-old-style-jane-street-local-annotations` Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com>
Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com>
Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com>
Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com>
8ee2852 to
750122e
Compare
mdelvecchio-jsc
pushed a commit
that referenced
this pull request
Sep 20, 2023
…tc. to their keyword forms (`local_`, `global_`, etc.) (#38) * Add mode to rewrite `[@Local]`, etc., into `local_` This is available under the option `--rewrite-old-style-jane-street-local-annotations` Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com> * Add tests of rewriting `[@Local]` and fix the normalization check Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com> * Update `ocamlformat-help.txt` Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com> * Remove export of function that was for debugging Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com> --------- Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com>
mdelvecchio-jsc
pushed a commit
that referenced
this pull request
Oct 2, 2023
…tc. to their keyword forms (`local_`, `global_`, etc.) (#38) * Add mode to rewrite `[@Local]`, etc., into `local_` This is available under the option `--rewrite-old-style-jane-street-local-annotations` Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com> * Add tests of rewriting `[@Local]` and fix the normalization check Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com> * Update `ocamlformat-help.txt` Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com> * Remove export of function that was for debugging Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com> --------- Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com>
mdelvecchio-jsc
pushed a commit
that referenced
this pull request
Oct 12, 2023
(Note for future rebases: at this stage it is expected that `dune b
@test/passing/runtest` will give errors related to extra
`[@ocaml.curry]`s)
local: support local_ and curry in function types
Note for future rebases: it is expected that `dune b
@test/passing/runtest` will fail at this stage, in my case with
output:
File "test/passing/dune.inc", line 3594, characters 0-205:
3594 | (rule
3595 | (deps tests/.ocamlformat )
3596 | (package ocamlformat)
3597 | (action
3598 | (with-stdout-to local.ml.stdout
3599 | (with-stderr-to local.ml.stderr
3600 | (run %{bin:ocamlformat} --margin-check %{dep:tests/local.ml})))))
Command exited with code 1.
local: support 'fun (local_ x) ... -> '
local: nonlocal/global record field decls
For future reference during rebasing: `dune b @test/passing/runtest`
is not expected to pass on this commit. Some uses of `local_` are
turned into `[%extension.local]`.
local: return expressions 'fun ... -> local_ ...'
For reference during future rebases: `dune b @test/passing/runtest` is
expected to pass on this commit.
local: apply formatting
For future rebases: `make test` should pass at this stage. It may be
necessary to run `dune fmt` and `make regtests-promote` (but I checked
`dune b @test/passing/runtest` succeeded first)
Update to ocaml-jst a78975eb57
Autogenerated attributes are now 'extension.foo' instead of 'ocaml.foo'
change module types to export equalities
Signed-off-by: Cameron Wong <cwong@janestreet.com>
Support for "include functor" (#5)
* Initial support for the include_functor extension
* Add test for include functor
Signed-off-by: Chris Casinghino <ccasinghino@janestreet.com>
local: bugfix for punned named arguments with type annotations (#3)
A bug in the previous patch for locals causes ocamlformat to incorrectly
format local arguments which are punned with a label and have a type signature:
fun ~(local_ x : t) -> ...
by putting an extra invalid pair of parens around (x : t). This patch removes
the parens.
Signed-off-by: Stephen Dolan <sdolan@janestreet.com>
Ensure parentheses are placed around (local_ E) when necessary (#4)
Signed-off-by: Stephen Dolan <sdolan@janestreet.com>
Remove unnecessary parentheses around expressions with trailing [@attrs] (#6)
Signed-off-by: Stephen Dolan <sdolan@janestreet.com>
update parsers and printer for polymorphic parameters (with tests)
Signed-off-by: Chris Casinghino <ccasinghino@janestreet.com>
Support for comprehensions and immutable arrays
Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com>
Update to the improved language extension infrastructure
Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com>
add support for global constructor arguments
Signed-off-by: Zesen Qian <github@riaqn.org>
support for layout annotations on type parameters
Signed-off-by: Chris Casinghino <ccasinghino@janestreet.com>
Address review feedback.
Signed-off-by: Chris Casinghino <ccasinghino@janestreet.com>
Fix a test failure due to misformatted comment
Signed-off-by: Chris Casinghino <ccasinghino@janestreet.com>
Fix a (CI) test failure in help printing due to version mismatch
Signed-off-by: Chris Casinghino <ccasinghino@janestreet.com>
support exclave_ syntax
Signed-off-by: Zesen Qian <github@riaqn.org>
no fail fast (#24)
Signed-off-by: Zesen Qian <github@riaqn.org>
Don't desugar `let _ = local_ x` (#21)
* Don't desugar `let _ = local_ x`
The desugared syntax `let local_ _ = x` is invalid.
Signed-off-by: Jules Aguillon <jules@j3s.fr>
* Restrict `let local_ ...` rewrite even more
This restricts the desugaring of `let x = local_ y` to the cases where
`x` is an identifier or an identifier with a type annotation.
Signed-off-by: Jules Aguillon <jules@j3s.fr>
* Tests: Add a .ref file for local.ml
There are syntax rewrite on locals, it's important not to change the
test input.
* Fix local_ desugaring generating invalid syntax
These two items don't have the same AST and must be recognized:
let local_ x : string = "hi"
let (x : string) = local_ "hi"
* Preserve `local_` position when ptyp_poly constraint
These two items don't have the same AST:
let x : 'a . 'a -> 'a = local_ "hi"
let local_ x : 'a. 'a -> 'a = "hi"
---------
Signed-off-by: Jules Aguillon <jules@j3s.fr>
better exclave_ formatting (#28)
Signed-off-by: Zesen Qian <github@riaqn.org>
Support unboxed float literals (#25)
* Automatic changes to patch files
This just happened when I hit `dune build`. Don't really know what's
going on.
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
* Updated the standard parsetree for unboxed literals
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
* Support unboxed literals
Testing story not complete yet; want advice.
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
* Make parsetree changes separated
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
* Checkpoint
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
* Still not quite working, but done for tonight
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
* Vendor in jane-syntax into standard
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
* Separate out Jane Street stuff more
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
* More tests, and a bugfix
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
---------
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
Add instructions for how to update ocamlformat (#26)
* Write instructions
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
* Chris's comments
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
* Document testing
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
* Typo
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
* Follow on from Chris's edits
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
* Further advice about dune errors
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
* Comment about `make test`
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
---------
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
Co-authored-by: Chris Casinghino <ccasinghino@janestreet.com>
Support float# (#27)
* Update parser-standard for float#
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
* Add support for float#
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
* Add tests
But this doesn't work. I'm very confused about how to test here.
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
* Some more type tests
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
* Test float#
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
* Forward-proof printing of float#
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
* Update parser-standard/language_extension
This also removes the differences between ocamlformat's
language_extension and janestreet's. Instead, we now just
call enable_maximal. Seems simpler.
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
---------
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
Check formatting in CI (#29)
* Fix formatting
This required removing some of the "Jane Street extension" markers, as
ocamlformat was moving them to unhelpful places.
Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com>
* Check formatting in CI
Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com>
---------
Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com>
Capture a few notes from our working conventions (#30)
* Capture a few notes from Working Conventions
Merging this will allow me to stub out the section in Working
Conventions.
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
* Chris's suggestions
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
---------
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
Add a mode for erasing erasable jane-syntax (#32)
* Add (untested) support for erasing erasable Jane syntax
Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com>
* Make normalization ignore erasable jane syntax if requested
Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com>
* Add failing test of erasure
Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com>
* Update parser-standard to antalsz/flambda-backend 5b23940cfccd3047
Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com>
* Update parser-standard and location to antalsz/flambda-backend c056f5f2b0082711b596337f270a8eccf06d0b06
Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com>
* Update `--erase-jane-syntax` documentation per review
Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com>
* Documentation for `Erase_jane_syntax`
Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com>
* Only remove erasable `Jane_syntax` attributes from the old AST
Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com>
* Fix formatting
Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com>
---------
Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com>
Add support for float64 layout (#35)
Signed-off-by: Chris Casinghino <ccasinghino@janestreet.com>
Support explicit strengthening (#39)
* Support explicit strengthening
Support the syntax introduced in
oxcaml/oxcaml#1337.
Signed-off-by: Roman Leshchinskiy <rleshchinskiy@janestreet.com>
* Fix parens with attributes
Signed-off-by: Roman Leshchinskiy <rleshchinskiy@janestreet.com>
---------
Signed-off-by: Roman Leshchinskiy <rleshchinskiy@janestreet.com>
Co-authored-by: Roman Leshchinskiy <rleshchinskiy@janestreet.com>
Update build workflow to OCaml 4.14 (#33)
Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com>
In an if-then with no else, the body should have parens if local_ (#40)
Signed-off-by: Chris Casinghino <ccasinghino@janestreet.com>
Add a mode for rewriting `[@Local]`, `[%local]`, `[@ocaml.global]`, etc. to their keyword forms (`local_`, `global_`, etc.) (#38)
* Add mode to rewrite `[@Local]`, etc., into `local_`
This is available under the option
`--rewrite-old-style-jane-street-local-annotations`
Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com>
* Add tests of rewriting `[@Local]` and fix the normalization check
Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com>
* Update `ocamlformat-help.txt`
Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com>
* Remove export of function that was for debugging
Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com>
---------
Signed-off-by: Antal Spector-Zabusky <antal.b.sz@gmail.com>
Fix bug caused by comments before local_ at the beginning of bindings. (#41)
Signed-off-by: Thomas Del Vecchio <tdelvecchio@janestreet.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is enabled by the option
--rewrite-old-style-jane-street-local-annotations, which is off by default; @ceastlund, I defer to you on if we want to enable it by default or do so in some configurations or just do that as we need it internally.Also, be aware that this is on top of #37; we don't actually want to merge this until we've merged #37, but we can get it reviewed first. This means if you're reviewing this PR, skip the first commit (e6b2e26) and review changes from the rest; we've already reviewed up to the contents of e6b2e26 in #32.