Skip to content

manual: ellipsis extension nodes for examples#1785

Closed
Octachron wants to merge 5 commits intoocaml:trunkfrom
Octachron:manual_extension_ellipsis
Closed

manual: ellipsis extension nodes for examples#1785
Octachron wants to merge 5 commits intoocaml:trunkfrom
Octachron:manual_extension_ellipsis

Conversation

@Octachron
Copy link
Copy Markdown
Member

As a follow-up to #1765, this PR replaces the [@ellipsis] attribute with [%ellipsis] extension nodes.

As a first step, the first commit in this PR replaces the separate toplevel process by the toplevel compiler-libraries. This was done mostly to have accurate location on the input phrase.
The other alternative would have been to launch the separate toplevel process with a custom ppx. However, taking direct control of the toplevel loop makes it possible to have finer separation of the different streams produced by the toplevel, so I judged the change worthwhile.
In particular, it becomes quite easy to underline the location of more than one error or warning messages, or to always print error and warning messages (see the fourth commit).

The second commit implements the ellipsis extension node themselves. All variants of extension nodes are supported. However, the syntax for some variants (the class related ones, the module expression and the module type extension nodes) is quite awkward, e.g.

module type s= [%ellipsis module type x = sig type t end];;
class c = [%ellipsis class x a b c = object end];; (* for class expressions *)
class c = object [%%ellipsis object method f = () end] end;; (* for class fields *)
class type c = [%ellipsis class type x = object end];; (* for simple class types *)

class type c = object [%%ellipsis class type x = object method f: int end] end;; 
(* for class type fields *)

module type w = sig
  class c: int -> [%ellipsis: class x: int -> object end] (* for class types *)
  module type X = [%ellipsis: module type x = sig end] (* alternative for module types *)
end;;

due to the mismatch between supported payloads and the corresponding parsetree nodes.
However, all extension nodes have a short variant [%ellipsis] which should
cover the most frequent use cases.

The third commit is a minor improvement of the error messages for intersecting text transforms that I wrote when debugging.

The last commit documents the new syntax, and in particular the replacement rule for the [%ellipsis] shorthand.

@damiendoligez
Copy link
Copy Markdown
Member

As a follow-up to #1765, this PR replaces the [@ellipsis] attribute with [%ellipsis] extension nodes.

I'm not sure that makes sense. What we have is some code that we want to annotate with something that says "don't print this part". Looks exactly like a job for attributes.

As far as I can tell, the switch to extension nodes is based on technical reasons (i.e. implementation details of the compiler) and I'm not convinced it's a good reason.

cc @gasche @alainfrisch

@gasche
Copy link
Copy Markdown
Member

gasche commented May 18, 2018

@damiendoligez: Well now you're making me feel bad for pushing for this route and having @Octachron do all the work!

There are pros and cons both ways:

  • attributes: natural syntax, but unreliable semantics due to lack of location in the parsetree. Can be fixed at the parsetree level, probably a good idea, but it's an invasive change that will break user code -- as any parsetree change.
  • extensions: ugly syntax, robust/reliable semantics

(One good news is that some of the improvements @Octachron did in this PR, on the way the examples are run, can be reused even if we don't go the extension route in the end.)

I would be happy to let other (@Octachron in particular, who did the work) voice their opinion on what their think is best. Personally, I wouldn't say that I like extensions more, but I know that I don't like having a mechanism implemented (ellipsis with attributes) if we know that the implementation is incorrect in many cases.

@Octachron
Copy link
Copy Markdown
Member Author

After writing the extension node variant, I still prefer the attributes version: attributes have reliable location in all contexts where extension nodes are allowed; and it would be possible to raise an error on attributes with no sensible locations, i.e.

type t = [ `A [@ellipsis] | `B ];;
type t = < a:int[@ellipsis]; b:int >;;

One good news is that some of the improvements @Octachron did in this PR, on the way the examples are run, can be reused even if we don't go the extension route in the end.)

I would say most of the work can be reused, since the implementation of the AST mapper itself is quite straigthforward (the most time consuming part was the error messages for invalid payloads, due to the sometime acrobatic syntax required).

@Octachron
Copy link
Copy Markdown
Member Author

Closed in favour of the attribute implementation.

@Octachron Octachron closed this Aug 27, 2018
@gasche
Copy link
Copy Markdown
Member

gasche commented Aug 27, 2018

... and sorry for this detour which consumed some of your time.

EmileTrotignon pushed a commit to EmileTrotignon/ocaml that referenced this pull request Jan 12, 2024
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.

3 participants