MetaOCaml: support for .<e>. and .~e #11871

johnyob · 2023-01-05T16:28:08Z

A follow on PR from #10131, cc @Octachron.

This PR adds MetaOCaml brackets and escapes to the OCaml parser and Parsetree.

.<e>. is parsed as Pexp_bracket e
.~e is parsed as Pexp_escape e

In the type checker, these MetaOCaml nodes are handled as extensions, reporting a 'Uninterpreted MetaOCaml expression' error. For instance:

# let one = .<1>.;;
Error: Uninterpreted MetaOCaml expression.

Note: currently we handle .<e>. and .~e in the same way we handle extensions

Note: currently we just raise an exception (as we would for a uninterpreted extension)

nojb

Thanks! You can add a parsing test for the new constructions in testsuite/tests/parsetree/source.ml.

nojb · 2023-01-05T16:44:19Z

parsing/lexer.mll

@@ -340,6 +340,8 @@ let identchar_latin1 =

 let symbolchar =
  ['!' '$' '%' '&' '*' '+' '-' '.' '/' ':' '<' '=' '>' '?' '@' '^' '|' '~']
+let symbolcharnodot =
+  ['!' '$' '%' '&' '*' '+' '-'     '/' ':' '<' '=' '>' '?' '@' '^' '|' '~']


Can define

let symbolchar = symbolcharnodot | '.'

or

let symbolcharnodot = symbolchar # '.'

nojb · 2023-01-05T17:04:13Z

parsing/ast_helper.ml

@@ -228,6 +228,11 @@ module Exp = struct
      pbop_exp = exp;
      pbop_loc = loc;
    }
+
+  (* NNN begin *)


What does NNN stand for?

I don't believe it stands for anything. It seems to be a convention in the other MetaOCaml PRs (#10130, #10131, #10132) to prefix / annotate MetaOCaml specific changes with NNN.

I have no strong opinions regarding it, so I'm more than happy to remove the NNN comments.

nojb · 2023-01-05T17:24:35Z

parsing/pprintast.ml

+        (match e.pexp_desc with
+        | Pexp_ident li -> pp f ".~%a" longident_loc li
+        | _ -> pp f ".~%a" (paren true (expression ctxt)) e)


Can we use simple_expr here?

Suggested change

(match e.pexp_desc with

| Pexp_ident li -> pp f ".~%a" longident_loc li

| _ -> pp f ".~%a" (paren true (expression ctxt)) e)

pp f ".~%a" (simple_expr ctxt) e

nojb · 2023-01-05T17:26:29Z

parsing/pprintast.ml

@@ -754,6 +754,14 @@ and expression ctxt f x =
          (expression ctxt) body
    | Pexp_extension e -> extension ctxt f e
    | Pexp_unreachable -> pp f "."
+    (* NNN begin *)
+    | Pexp_bracket e ->
+        pp f "@[<hov2>.<@ %a @ >.@]" (expression ctxt) e


Is there a reason for the extra (breaking) space?

Suggested change

pp f "@[<hov2>.<@ %a @ >.@]" (expression ctxt) e

pp f "@[<hov2>.<%a>.@]" (expression ctxt) e

The spaces are useful for when e is a 'larger' expression. For instance: I find

.< let foo = bar () in baz foo foo >.

more readable compared to:

.<let foo = bar () in baz foo foo>.

But this is rather subjective, so I'm happy to implement the suggestion if you disagree.

nojb · 2023-01-05T17:28:05Z

parsing/pprintast.ml

@@ -754,6 +754,14 @@ and expression ctxt f x =
          (expression ctxt) body
    | Pexp_extension e -> extension ctxt f e
    | Pexp_unreachable -> pp f "."
+    (* NNN begin *)


These comments do not seem very useful.

I concur. See above comment regarding NNN.

hhugo · 2023-01-06T17:46:54Z

The comment from #10130 still holds #10130 (comment)

Kakadu · 2023-01-07T20:58:25Z

I looked a bit into metaocaml patches, and it seems that new syntax constructions are encoded using attibutes.
Maybe we shouldn't push changes in parser related to metaocaml? Maybe two extra parsetree nodes is already good enough?

johnyob · 2023-01-07T21:31:36Z

The comment from #10130 still holds #10130 (comment)

@hhugo once #10130 is merged (having implemented a satisfactory solution) I'll rebase on top of it

gasche · 2023-01-13T13:07:07Z

Semi-related: as far as we know, MetaOCaml has not been released upstream in version 4.14 and 5.0. This is something that people interested in contributing could consider helping with.

yallop · 2023-01-13T13:21:34Z

@gasche: what do you have in mind? MetaOCaml development doesn't take place in public, so I'm not sure what you're proposing.

Octachron · 2023-01-13T14:17:40Z

@yallop , would it not be possible for an external contributor to update the last released version of MetaOCaml to either 4.14 or 5.0 ? That might help people working on the upstreaming patches to build some intuitions of what changes are required to reduce MetaOCaml maintenance burden in its current incarnation.

yallop · 2023-01-13T16:17:42Z

@Octachron: each new release of BER MetaOCaml typically has various non-trivial changes.

Some changes arise from interactions with new features in the underlying OCaml release. For example, the detection of scope extrusion in BER MetaOCaml relies on various assumptions about the stack, as the notes in the source code explain:

We use a different method: we mark each piece of the generated code with the list of free variable the code contains. Each variable is associated with a `stackmark', which identifiers the region with which the variable is associated. All valid stackmarks form a total order. Alas, delimited control can reshuffle that order.

It may be that design and implementation decisions are needed to handle the changes to stacks in OCaml 5 (or perhaps the existing care taken to properly handle delimited control is already sufficient; I don't know offhand).

Other changes involve updates to the design or implementation of MetaOCaml itself. For example, the changelog for MetaOCaml 111 (based on OCaml 4.11) describes new features for let rec generation:

Added generation (mutually) recursive let. See more detailed explanation in PEPM 2019 and ML 2019. Examples in those papers work (see test/genletrec.ml)

In other words, MetaOCaml isn't just a fairly fixed patchset that needs to be updated/rebased when new OCaml releases are made; each new release has significant changes that require design and implementation decisions by the maintainer. Helping with the next MetaOCaml release consequently requires interacting with the maintainer.

gasche · 2023-08-05T16:43:53Z

It may be interesting to rebase this PR on top of #12470, which is our new attempt at upstream-friendly patches for lexer+parser MetaOCaml support. (But: we could also make a better job of providing @johnyob on feedback on the current PR, before we ask for more work on his side.)

Oleg Kiselyov and others added 12 commits January 5, 2023 16:41

metaocaml: modified lexing for >.

bccefb5

metaocaml: make .<x>. and .~x valid in vanilla OCaml

519199f

update tests

be6bd90

metaocaml: add .<e>. and .~e to the parsetree

5939ce8

metaocaml: update parser to use new parsetree constructs

02ae128

metaocaml: update ast_{helper, iterator, mapper} to handle .<e>. and .~e

693a86b

metaocaml: update preprocessing dependency resolution

0cd88ce

Note: currently we handle .<e>. and .~e in the same way we handle extensions

metaocaml: update prettyprinters

8e73872

metaocaml: update type checker to handle .<e>. and .~e

e074329

Note: currently we just raise an exception (as we would for a uninterpreted extension)

metaocaml: update ocamlprof

596931a

metaocaml: update error message from metaocaml to MetaOCaml

b758c44

bootstrap parser

975aa86

johnyob force-pushed the metaocaml_parser branch from 96b29d7 to 975aa86 Compare January 5, 2023 16:52

nojb reviewed Jan 5, 2023

View reviewed changes

hhugo mentioned this pull request Jan 6, 2023

Feature request: support BER MetaOCaml ocaml-ppx/ocamlformat#1211

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MetaOCaml: support for .<e>. and .~e #11871

MetaOCaml: support for .<e>. and .~e #11871

johnyob commented Jan 5, 2023

nojb left a comment •

edited

Loading

nojb Jan 5, 2023

nojb Jan 5, 2023

johnyob Jan 5, 2023

nojb Jan 5, 2023

nojb Jan 5, 2023

johnyob Jan 5, 2023

nojb Jan 5, 2023

johnyob Jan 5, 2023

hhugo commented Jan 6, 2023

Kakadu commented Jan 7, 2023 •

edited

Loading

johnyob commented Jan 7, 2023

gasche commented Jan 13, 2023

yallop commented Jan 13, 2023

Octachron commented Jan 13, 2023

yallop commented Jan 13, 2023

gasche commented Aug 5, 2023

	pp f "@[<hov2>.<@ %a @ >.@]" (expression ctxt) e
	pp f "@[<hov2>.<%a>.@]" (expression ctxt) e

MetaOCaml: support for .<e>. and .~e #11871

Are you sure you want to change the base?

MetaOCaml: support for .<e>. and .~e #11871

Conversation

johnyob commented Jan 5, 2023

nojb left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hhugo commented Jan 6, 2023

Kakadu commented Jan 7, 2023 • edited Loading

johnyob commented Jan 7, 2023

gasche commented Jan 13, 2023

yallop commented Jan 13, 2023

Octachron commented Jan 13, 2023

yallop commented Jan 13, 2023

gasche commented Aug 5, 2023

nojb left a comment •

edited

Loading

Kakadu commented Jan 7, 2023 •

edited

Loading