Convert `-extension local` to use Jane syntax #1586

antalsz · 2023-07-21T23:05:54Z

Naturally, this comes with some Jane syntax changes, designed to make it easier to work with the attribute encoding without having to explicitly think about attributes.

This shouldn't change the user-facing syntax at all, but it removes the ability to explicitly write attributes (e.g., [@local]) or extension nodes (e.g., [%exclave]). Some things are left as attributes: [@tail] and [@nontail], because they really do want to be explicit modifiers on terms; and [@curry] in terms, although see below. The implicit [@curry] in types is left alone as an attribute, but its name is changed.

There is one outstanding question: I left the handling of [@curry] on fun terms alone, as an explicit attribute, but I wasn't sure if that was correct.

We leave %extension.escape alone because @ncik-roberts's forthcoming syntactic function arity parsing PR should eliminate it.

Reviewer: @ncik-roberts, I think!

EDIT: unboxed-primitive-args/test.ml is failing locally because it can't find caml/mlvalues.h; I haven't tried to debug this yet, but I can't see how it's related to these changes.

ncik-roberts

I've given it some light review but haven't engaged with the substantial Jane Syntax changes yet.

ocaml/parsing/jane_syntax.mli

ocaml/typing/typetexp.ml

ocaml/typing/typetexp.mli

ocaml/typing/typedecl.ml

ocaml/parsing/jane_syntax.mli

ocaml/parsing/builtin_attributes.mli

ocaml/parsing/builtin_attributes.ml

antalsz · 2023-07-26T16:55:58Z

This PR needs updating to handle locations on local_ x without encoding them as (); x; this is semantically idempotent, but is probably a weaker notion of erasability than we want. Open to suggestions that I ought to patch ocamlformat for this instead, though. This is now done.

antalsz · 2023-07-27T00:02:41Z

@ncik-roberts, I think the open conversations and new code need your review now, but I've addressed most of your comments!

ncik-roberts

Not completely done yet, but here are some more comments.

ocaml/parsing/ast_invariants.ml

ocaml/parsing/ast_iterator.mli

ocaml/parsing/ast_mapper.ml

ocaml/parsing/depend.ml

ocaml/parsing/jane_syntax.mli

ocaml/parsing/jane_syntax.ml

ncik-roberts · 2023-07-28T16:58:53Z

My most recent commit reworks the type of match_jane_syntax_piece to make it appear more clearly inverse to make_jane_syntax. This involves pushing an error message generation out to the clients in jane_syntax.ml, so there is a trade-off involved, but I feel like the most straightforward (IMO) code structure is a good trade. Dropping this commit is certainly on the table.

ncik-roberts

I just pushed a failing test highlighting some issues with how attributes are handled.

ocaml/parsing/jane_syntax_parsing.ml

ocaml/parsing/location.ml

ocaml/parsing/jane_syntax.ml

ncik-roberts

A little more review, a few more comments. Just typechecking left, and then this will be fully reviewed.

ocaml/parsing/location.mli

ocaml/parsing/pprintast.ml

ocaml/testsuite/tests/jane-modular-syntax/user_error3.compilers.reference

goldfirere

In a conversation with @antalsz, we agreed that this patch would have important effects on ppxs. Merge only with caution. To help us not forget this, I'm putting in a "Request changes". (I'm not sure the exact semantics of this, but I think it shows up in the block that appears above the merge button as a helpful reminder.)

ncik-roberts

Ok, I think I'm all done with this review attempt. Let me know when you'd like me to take a look after you've had a chance to look at my comments.

ocaml/typing/typecore.ml

antalsz · 2023-08-04T22:32:32Z

@ncik-roberts I made the location saving/restoring automatic, good call

ncik-roberts

The changes look good. The remaining things left:

I have a minor comment for trying to reduce the diff of this last piece.
Richard left a comment to mark the fact that this may affect ppxes.
There are a few threads to mark resolved if you agree that they are in fact resolved.

ocaml/parsing/parser.mly

ncik-roberts

Let's fix the test that's commented out in #1727 — currently, this program's parsetree doesn't round-trip with respect to Pprintast:

let local_ f x : int -> int = fun y -> x + y in
()

The output is something like:

let local_ f x = ((fun y -> x + y) : (int -> int)) in
()

But the parser treats the type annotation on the LHS of = specially for let local_ f: it inserts an Lexp_constrain_local.

Fixing this bug will allow us to uncomment the test in #1727.

ncik-roberts

Looks good to me. (Note Richard's unresolved comment — you were going to check that ppxes are ok before we merge this.)

antalsz · 2023-08-15T00:32:00Z

@ncik-roberts: Sorry about the force push; I needed to add some new tests to validate the issue in #1727. Luckily it actually only adds two commits for you to review:

c3aa354: Adds some new tests about printing of type annotations in bindings with and without local_, and adds some round trip checks; at this time, local_ fails those checks.
71d4b5c: Fix printing and fix the aforementioned tests.

It's worth noting that this uncovers an existing bug in pprintast's roundtripping:

let broken_coerce_to_lhs :> int = 42;;

becomes

let broken_coerce_to_lhs : int = (42 :> int);;

I haven't tried to fix that in this PR; unless it's very simple to do, it probably belongs elsewhere.

antalsz · 2023-08-15T00:32:31Z

Sorry, I should clarify: this does fix the printing issue highlighted in #1727!

ncik-roberts · 2023-08-15T13:10:42Z

I think these last two commits should be split out into another PR. This PR is already massive, and the last two commits were (a) already issues at the base, and (b) are semantically fairly separate from the reset of the work you did. I think that will make this PR easier to merge and those two commits easier to review in isolation.

(But thanks for fixing this problem — pprintast bugs are annoying.)

antalsz · 2023-08-21T22:20:08Z

Just caught a Pprintast bug and added a couple of commits to fix it

ncik-roberts

The most recent two commits (ending in "Fix printing of global_ in tupled constructors") look good to me -- I just have a minor comment that you can resolve upon addressing.

ocaml/parsing/pprintast.ml

antalsz · 2023-08-23T21:05:03Z

@ncik-roberts: I have split off the two commits you requested and force-pushed. The current range of commits is 85c202c..5738720. I will be squashing and rebasing this PR soon.

@A

This is the squashed form of commits 85c202c..5738720, whose individual commit messages are below, followed by rebasing onto `main`. I'm committing the conflicts to hopefully make it possible to review the resolution of the rebase. We'll see how well this works. ---------- Add matches on `Jane_syntax.Core_type.of_ast` Add the `local` extension, but don't use it yet Power `local_` with the modular syntax (but not other modal things) Power `global_` with the modular syntax machinery Update the parser to use Jane syntax for `local_`; buggy w/ `global_` Fix `Local.of_constr_arg`'s attribute handling Update ocamlprof Add constructor argument to `depend.ml` Make `local_` in types contain more attributes This lets us distinguish it from constructor arguments better Correctly update attributes when matching on types/constructor args Bootstrap `make alldepend` Fix printing bug for `local_` Refactor attribute handling This is a big change to `Jane_syntax_parsing` Update passing tests Add `Lexp_constrain_local` Fix `local_` in patterns & the location of `local_` in expressions Delete stray outdated comments Move `exclave_` over to Jane syntax Remove `extension.` options for `[@{non,}tail]` Move curry-tracking over to Jane syntax Reorder attribute addition for comprehensions Rewrite the missed uses of `[%exclave]` to `exclave_` Add some missing documentation Make `Jane_syntax_parsing` no longer depend on `Misc` Bootstrap Update ocamlprof Respond to simple parts of Nick's review Formatting (and content) update for `Jane_syntax` example comments Add test of attributes in `local_ (t[@A])`; currently handled wrong Update: `get_alloc_mode` → `unwrap_mode`, document it, and fix a bug Jane syntax constructor arguments don't come with attributes Add missing invariant documentation Handle locations for `local_` and `exclave_` without using `(); x` Add some missing uses of `Location.ghostify` Rework type of match_jane_syntax_piece Add regression test (currently failing) for attribute handling Respond to small pieces of @ncick-roberts's review Remove `~attr` arguments from individual `Jane_syntax` extensions More reponses to @ncick-roberts's review, including a bug fix Fix `pprintast`'s handling of attributes on `let x = local_ e` Fix bug with parenthesization of `local_` Handle saving/restoring locations with `loc_stack`s Minor review response Remove `Embedded_name.marker_attribute_handler` machinery Tweak `match_jane_syntax` (formerly `match_jane_syntax_piece`) Make `save_location` and `restore_location` automatic Reduce the parser diff Per @riaqn, adjust the type of `check_eq_mode`; also, rename it Added test demonstrating a bug in `Pprintast` Currently, `Pprintast` does not print `global_` on tupled constructor fields correctly, and instead crashes. ``` % cat test.ml; echo; _install/bin/ocamlc -dsource test.ml type t = C of global_ string type t = | C of File "test.ml", line 1, characters 14-21: 1 | type t = C of global_ string ^^^^^^^ Error: Tried to restore a saved location here, but none were found. ``` This is a regression from the previous broken behavior of printing the attribute literally: ``` % cat test.ml; echo; ocamlc -dsource test.ml type t = C of global_ string type t = | C of ((string)[@extension.global ]) ``` The next commit will fix `Pprintast` to have the correct behavior Fix printing of `global_` in tupled constructors Switch from `%(%)` to `%t`

goldfirere · 2024-02-21T20:33:35Z

In the end, we've decided to go a different direction here, keeping a lighter-weight encoding for local annotations. There is one piece of it all not yet complete, but is tracked by an internal ticket. Closing.

antalsz requested review from lpw25, ccasin, goldfirere, riaqn, ncik-roberts, stedolan, mshinwell, poechsel, gretay-js, xclerc and lukemaurer as code owners July 21, 2023 23:05

antalsz force-pushed the local-modular-extension branch from d6e433e to 2cdaed5 Compare July 21, 2023 23:26

ncik-roberts self-assigned this Jul 24, 2023

ncik-roberts reviewed Jul 24, 2023

View reviewed changes

antalsz force-pushed the local-modular-extension branch 2 times, most recently from 944ca6e to c862514 Compare July 26, 2023 23:52

ncik-roberts reviewed Jul 27, 2023

View reviewed changes

ncik-roberts force-pushed the local-modular-extension branch from e98bc19 to 9886a02 Compare July 28, 2023 18:46

ncik-roberts reviewed Jul 28, 2023

View reviewed changes

ocaml/parsing/jane_syntax_parsing.ml Show resolved Hide resolved

ocaml/parsing/jane_syntax_parsing.ml Outdated Show resolved Hide resolved

ocaml/parsing/location.ml Outdated Show resolved Hide resolved

ocaml/parsing/jane_syntax.ml Outdated Show resolved Hide resolved

ncik-roberts reviewed Jul 28, 2023

View reviewed changes

goldfirere requested changes Jul 28, 2023

View reviewed changes

ncik-roberts reviewed Jul 28, 2023

View reviewed changes

ncik-roberts approved these changes Aug 8, 2023

View reviewed changes

ocaml/parsing/parser.mly Show resolved Hide resolved

ncik-roberts requested changes Aug 10, 2023

View reviewed changes

antalsz mentioned this pull request Aug 11, 2023

Add Jane_syntax Pprintast tests #1727

Merged

ncik-roberts approved these changes Aug 14, 2023

View reviewed changes

antalsz force-pushed the local-modular-extension branch from ecba032 to 71d4b5c Compare August 15, 2023 00:27

ncik-roberts mentioned this pull request Aug 15, 2023

Syntactic function arity parsing #1548

Merged

ncik-roberts approved these changes Aug 22, 2023

View reviewed changes

ocaml/parsing/pprintast.ml Outdated Show resolved Hide resolved

antalsz force-pushed the local-modular-extension branch from 836a1ee to 5738720 Compare August 23, 2023 21:03

antalsz force-pushed the local-modular-extension branch from 5738720 to 5d807a3 Compare August 28, 2023 19:34

antalsz force-pushed the local-modular-extension branch from 5d807a3 to 8dca801 Compare August 28, 2023 19:37

antalsz added 5 commits August 29, 2023 18:47

Syntactic merge conflicts resolved; does not remotely compile

00e4134

Progress – can now run make promote-menhir (which fails)

0416474

Create unified Modes Jane syntax AST

b943b1f

Completed the rebase of the parser

9d8d2ba

Completed the rebase up through iterators

dcfbb5e

alanechang mentioned this pull request Nov 2, 2023

Add layout annotations janestreet/ocamlformat#42

Merged

goldfirere closed this Feb 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert `-extension local` to use Jane syntax #1586

Convert `-extension local` to use Jane syntax #1586

antalsz commented Jul 21, 2023 •

edited

Loading

ncik-roberts left a comment

antalsz commented Jul 26, 2023 •

edited

Loading

antalsz commented Jul 27, 2023

ncik-roberts left a comment

ncik-roberts commented Jul 28, 2023

ncik-roberts left a comment

ncik-roberts left a comment

goldfirere left a comment

ncik-roberts left a comment

antalsz commented Aug 4, 2023

ncik-roberts left a comment

ncik-roberts left a comment

ncik-roberts left a comment

antalsz commented Aug 15, 2023 •

edited

Loading

antalsz commented Aug 15, 2023

ncik-roberts commented Aug 15, 2023 •

edited

Loading

antalsz commented Aug 21, 2023

ncik-roberts left a comment

antalsz commented Aug 23, 2023

goldfirere commented Feb 21, 2024

Convert -extension local to use Jane syntax #1586

Convert -extension local to use Jane syntax #1586

Conversation

antalsz commented Jul 21, 2023 • edited Loading

ncik-roberts left a comment

Choose a reason for hiding this comment

antalsz commented Jul 26, 2023 • edited Loading

antalsz commented Jul 27, 2023

ncik-roberts left a comment

Choose a reason for hiding this comment

ncik-roberts commented Jul 28, 2023

ncik-roberts left a comment

Choose a reason for hiding this comment

ncik-roberts left a comment

Choose a reason for hiding this comment

goldfirere left a comment

Choose a reason for hiding this comment

ncik-roberts left a comment

Choose a reason for hiding this comment

antalsz commented Aug 4, 2023

ncik-roberts left a comment

Choose a reason for hiding this comment

ncik-roberts left a comment

Choose a reason for hiding this comment

ncik-roberts left a comment

Choose a reason for hiding this comment

antalsz commented Aug 15, 2023 • edited Loading

antalsz commented Aug 15, 2023

ncik-roberts commented Aug 15, 2023 • edited Loading

antalsz commented Aug 21, 2023

ncik-roberts left a comment

Choose a reason for hiding this comment

antalsz commented Aug 23, 2023

goldfirere commented Feb 21, 2024

Convert `-extension local` to use Jane syntax #1586

Convert `-extension local` to use Jane syntax #1586

antalsz commented Jul 21, 2023 •

edited

Loading

antalsz commented Jul 26, 2023 •

edited

Loading

antalsz commented Aug 15, 2023 •

edited

Loading

ncik-roberts commented Aug 15, 2023 •

edited

Loading