Support unboxed float literals #25
Merged
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
Author
|
This is not in the state I would like, and I have timed out. More next week. Closing for now. |
Author
|
This is now ready for review, but I would like some help with testing. Approach:
There is one test, and it works, but it is insufficient. One question: how do I make a test where the input is meant to be different from the output? Specifically, I'd like to make sure that e.g. |
Author
|
@ccasin is probably the best reviewer here |
ccasin
approved these changes
Jul 18, 2023
There was a problem hiding this comment.
This looks good. There is one minor question about negative literals we should resolve before merging.
You asked about how to add a test that demonstrates syntax changes. See the tests with a "ref" file - e.g., test/passing/tests/alignment.ml{.ref}.
For posterity: We spent a lot of time debating whether to go with the approach pioneered here. It has one major benefit, which is that we don't have to update the printing support code from the old Language_extension system to the new Jane_syntax system. On the other hand, I am pretty sad about it for a couple reasons:
- It makes each incremental ocamlformat update a bit more work to make and to review (it's no longer just copy and paste in the extended parser and parsetree - a little thought is required).
- This adds yet another approach for handling Jane Street features in ocamlformat, but the old ones are all still used simultaneously, bringing the total number to three or four:
- The older locals stuff uses the old manual encoding, and uses a gross hack to strip off the attributes during printing while staying under the radar of the subterm checks.
- Newer locals stuff and include functor use the old manual encoding but play nice with the subterm checks.
- Immutable arrays and comprehensions use the old "
Language_extension" version of what is now called Jane Syntax. - Unboxed types modify the parse tree in the extended parser, but bring the new Jane Syntax stuff into the standard parser.
This is pretty gross! In a perfect world, if Jane Syntax had stabilized, we should just move to that uniformly everywhere. But after giving it a try, I'm forced to agree with @goldfirere that it's not worth the effort to do that now, because in fact Jane Syntax keeps changing and the updates are indeed a lot of work. Thanks, Richard, for dealing with this.
In the future, we will be tempted to go back and take out the Language_extension stuff to use this approach. This should be considered low priority, but would be reasonable to do at some point. It might even ease a later move to Jane Syntax, should that ever stabilize. If we do so, though, we should probably rewrite history so it's as if we always did it that way. Otherwise, rebases on upstream will be an unnecessary nightmare.
Author
|
OK. I added some more testing, found a bug, and fixed it. @ccasin do you mind reviewing the last commit? Thanks! |
ccasin
reviewed
Jul 18, 2023
This just happened when I hit `dune build`. Don't really know what's going on. Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
Testing story not complete yet; want advice. Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
Signed-off-by: Richard Eisenberg <reisenberg@janestreet.com>
b5f4d13 to
ea1b3c8
Compare
ccasin
pushed a commit
that referenced
this pull request
Sep 19, 2023
* 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>
mdelvecchio-jsc
pushed a commit
that referenced
this pull request
Sep 20, 2023
* 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>
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
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.
I decided to just edit the parsetree, as this was so much easier than working with jane-syntax. To be discussed: whether it is wise in the long run.
This is a WIP: the tests are not yet complete, but I'd like to discuss with others how best to write tests.